Repository navigation
feat: Prax's self-diagnosed bugs fixed, an honest prax_doctor with log health, and credential secrets kept out of traces - #259
Merged
Conversation
… wrapped tools once on_tool_end receives a ToolMessage for every ToolCall invoke (every ToolNode call), not a str: - OTelToolCallback sliced it, raising TypeError after the span was popped, so span.end() never ran and every successful tool span was lost. The preview now goes through tool_output_text and end() runs in a finally (on_tool_error too). - GraphCallbackHandler stored str(ToolMessage), a pydantic repr, as every tool node's summary and live preview. It now stores the content, and a tool that handled its own failure (an error ToolMessage, which ends via on_tool_end) completes its node as "failed". Governance and context binding wrap a tool in same-named StructuredTools that each fire on_tool_start as a child of the layer above, so OTelToolCallback counted TOOL_CALLS and opened a span once per layer. Inner layers are now recognised by parent run + name and skipped. tool_output_text (prax/agent/message_text.py) is the shared, never-raising flattener for tool results of any shape.
… stop crashing The browser and content spokes posted to #browser and #content, which neither TeamWork's project defaults nor Prax's startup ever created, so every such post was dropped with "Unknown channel". The spoke role agents were never registered either, so their posts were unattributed and their status updates were no-ops. - prax/services/teamwork_channels.py: one import-free registry of the channels Prax posts to (PRAX_CHANNELS) and the role agents it posts as (PRAX_ROLE_AGENTS). - teamwork_hooks: ensure_prax_channels() ensures the whole registry at startup (ensure_mirror_channels kept as an alias); register_role_agents() registers every role, one failure not costing the rest; reset_all_idle covers them all. - TeamWorkClient: a registry channel the project lacks is ensured once on first post (recovers a failed startup ensure or an older project); unknown channels warn once per name, then count at DEBUG; the same lookup now backs forward_external_message. ensure_channels failures and names missing from its response log at WARNING. - ensure_branch_channel read self._enabled, which the client never assigns, so it raised AttributeError on every real instance; it now uses the enabled property, and post_branch_update keeps the whole operation inside its try and reports an undelivered post as False. - The legacy coding-agent interjection handler imported bridge helpers that were removed with the coding-agent CLIs; it now falls through to Prax instead of replying with a generic error. - tests/test_channel_registry.py: AST drift guard over channel and role literals, plus lazy-ensure, warning and startup-hook tests. The branch channel stub now goes through the real constructor.
prax_doctor read TEAMWORK_URL, provider keys and FLASK_SECRET_KEY from os.environ, but pydantic loads .env without exporting it, so a host-process deployment was told "TeamWork: not configured" and failed the LLM check while both were set. Every check now reads the settings object: - TeamWork uses settings.teamwork_active and says why it is off (empty URL, or the deprecated TEAMWORK_ENABLED=false overriding a set URL, a warning). - LLM builds the model for every enabled tier through build_llm and FAILs with the factory's error. Construction makes no network call (tested), and the probes run inside llm_factory.probe_builds() so they are not recorded as tier choices in the turn's trace. - Plugins use the loader's real state through a new PluginLoader.health_report(), which system_status shares (both probed methods that do not exist, so failures were never reported and the count was "?"). It reports blocked plugins, plugins whose load raised (kept in a separate _load_failures, since load_errors is shown as "blocked"), and loaded plugins with failure_count > 0. A registry rolled_back status is not checked: rollback() reloads at once and the reload re-activates it. - The spoke check is gone: build_all_spoke_tools() imports every registered spoke unguarded while building the tool list prax_doctor is in, so it could never fail while the doctor could run. - The health monitor is read through a new get_check(max_age_s=300), the staleness rule get_health_status already used, instead of get_last_check(), which is None for the first nine turns. The tool error rate was 0% or 100%: governed_tool recorded TOOL_ERROR on the hub but never TOOL_SUCCESS, so the denominator held only errors. Hub successes are now recorded (spoke layers record neither, so a delegation is one call). New, behind LOG_HEALTH_ENABLED (default off): prax/services/log_health.py, a root logging handler installed at app startup that counts WARNING+ records by (level, logger, file:line). It never stores the formatted message or exception text; it keeps the %-template only when the record has args, and caps the table at 500 sites. prax_doctor warns when one site reaches LOG_HEALTH_WARN_COUNT and lists the top LOG_HEALTH_TOP_N, without reading the log file.
…hat ran Tool spans gain requested_args_sha256 (from the model's response, matched by tool_call_id) and args_sha256 (from the innermost tool start, so a wrapper that rewrites arguments shows up). Hashes only, in the wire record's canonical form. check_wire_record now matches on them and reports ARGS DIFFER and CHANGED BEFORE RUNNING; older traces still match on name and time.
Registering a role switches on TeamWork's activity log, live output and status for it: TeamWorkClient skips those calls for unregistered agents. Registering the spoke roles therefore streamed spoke tool output, including browser_login results, into TeamWork's persistent activity log, and grew reset_all_idle from 5 to 16 synchronous PATCHes on every turn's critical path, flipping concurrently running turns' spokes to idle. - PRAX_ROLE_AGENTS is back to the four core roles, so startup registers exactly what it did before (the orchestrator plus Planner, Researcher, Executor, Auditor) and reset_all_idle patches the original five. The per-agent try/except stays, so one failed create_agent no longer aborts TeamWork startup. - The channel registry, ensure_prax_channels, the lazy ensure and the once-per-name warning are unchanged. - TeamWorkClient: the single lock was held across the lazy ensure request (15 s timeout), so posts to unrelated channel names and the drop counter waited behind it, on Discord's event loop for forwarded messages. Each registry name now has its own ensure lock, built once in __init__, and the drop bookkeeping has a separate lock that is never held across I/O. - Tests: the role drift guard now pins the core roles and fails if any spoke role is registered, through the registry or a literal create_agent call in the package or app.py. A new test checks that a slow lazy ensure of one channel does not stall posts to other names; it fails on the old single lock. - README lists the channels from the registry and the actual role agents. - check_layers allowlist: the grandfathered teamwork_hooks import moved to line 66.
…ial outputs withheld Tool spans were deduplicated by tool NAME, so two parallel calls to the same tool folded into one span (one call's hashes, the other call missing). Wrapper layers are now recognised by run chain: a start whose parent run is an open run of the same tool maps to that span; anything else opens its own. "Changed before running" compared the model's raw-argument hash with the hash of the innermost layer's inputs, which LangChain has validated (defaults filled, values coerced, expected_observation popped), so it fired on ordinary calls. The model's arguments are now kept in memory for the life of the call and compared loosely with every layer's inputs; a missing or different requested value sets a new span flag, args_changed, which check_wire_record.py reports. args_sha256 stays, documented as post-validation and not comparable. Credential tools (hard_floors.CREDENTIAL_TOOLS, public, independent of HARD_FLOORS_ENABLED) return passwords in plain text. Their output is withheld and their argument values masked in the OTel span attributes, trace summaries, TeamWork live output and activity, and the spoke runner's tool log.
…scrub log health Health telemetry: tool successes are now counted in memory (a deque of timestamps pruned to the 60-minute stats window), never stored as events. At tool-call rate they evicted the rare alerts (LLM_ERROR, TURN_TIMEOUT, SPOKE_FAILURE) from the 2000-event cap, so the health check read healthy, flooded TeamWork's recent-events feed, and grew the JSONL file without bound. record_event(TOOL_SUCCESS) is routed to the counter too. Because successes are not persisted, the error rate covers only events since the process started, so reloaded errors no longer read as 100% after a restart; the stats carry the rate's own counts and the alert quotes them. The file is bounded: rewritten atomically on load when rows were dropped, and by record_event past twice the memory cap. Plugin health: a non-builtin plugin's key, manifest and exception text are plugin-authored and reached prax_doctor and system_status, which carry no untrusted-content banner. Those plugins now appear as a sanitised, capped key plus a fixed category pointing at plugin_list. Builtin plugins keep their message. Log health: templates are kept only for Prax's own call sites (library templates such as werkzeug's carry the client IP and a timestamp); locations never name a user (<workspace>/..., <external>/<basename>); uninstall() detaches from the logger install() used and does not forget a handler that is still installed elsewhere. Tests: the app-startup test no longer reloads app (it left the flag-on settings behind, so later create_app() calls installed a handler nobody removed); conftest pins TEAMWORK_URL="" so create_app() does not wait ~30 s for a TeamWork that is not running; the doctor's "checked Ns ago" assertion tolerates a slow clock.
…e matching is two-pass, system_status stops quoting log lines - plugin_list gains a "Failed to load" section from the loader's new get_load_failures() accessor: health_report sends the model there for a withheld plugin's crash detail, and no tool showed it. Labelled as plugin-reported text, each entry flattened to one line and capped at 300 characters. It also no longer returns early when nothing is loaded but plugins were blocked or crashed. - The Health Monitor is registered at startup again, so its log_activity alerts are no longer dropped by TeamWorkClient. RoleAgent.has_status keeps it out of reset_all_idle, which stays at the orchestrator plus the four core roles (5 PATCHes); a drift guard fails if a status-less role is ever given a status or live output. - check_wire_record matches in passes: every exact (name + hash) match first, then hashless legacy spans, then reports the rest, so an ARGS DIFFER can no longer consume a parallel sibling's exact span and misreport it too. - system_status reports recent errors as a count from the last 256 KB of the log (no full readlines), plus log health's busiest ERROR call sites (location and template only) when it is on, and points at read_logs. Raw [ERROR] lines, which can carry plugin-written text, are no longer quoted.
…back as arguments - log_action (every audit entry -> workspace trace.log + debug log): a credential tool's argument values are masked and its own output withheld; governance verdicts on it are kept (from_tool=False). Errors from a credential tool record the exception type only. - New prax/agent/turn_secrets.py: when a CREDENTIAL_TOOLS call returns, the values of its secret-named key=value / key: value lines are registered on the turn's TurnGovernanceState (empty at begin_turn, cleared at drain, never persisted, values < 4 chars ignored, repr/JSON-escaped forms included). browser_login, browser_credentials and browser_fill_login also register the stored values they read. scrub() masks them in the OTel input/output/error previews, the trace running node, output and error summaries, the spoke and sub-agent tool logs, governance's INFO logs and approval summaries, audit args/results, and every entry of the orchestrator's trace.log. - arguments_differ: bools compare through pydantic's own lax bool parsing; integers compare exactly before any float fallback. - Spoke tool log: the failure heuristic reads the real output; only the display is redacted. The sub-agent reuses this logger instead of its old unredacted copy.
…thout blow-up, audit keeps login outcomes - The turn's secret set is no longer cleared when the audit log drains: a timed-out or stopped turn's worker can still run tools on that state. It goes away with the state, which begin_turn replaces; never in a repr. - arguments_differ compares numbers as Decimals (a model-sent "1e999999999" used to become a billion-digit int and freeze the process) and as floats when validation produced a float (not a rewrite). - The audit withholds output only for tools that return a secret (browser_login, browser_credentials); the other credential tools keep their outcomes, and the domain stays in the args, all scrubbed. - Spoke and sub-agent answers are scrubbed at their sinks (logs, span summary, live output, channel post); a spoke failure keeps its traceback, scrubbed. - log_health locations keep only path characters (a workspace plugin's directory name is plugin-authored).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prax diagnosed three issues in itself and asked for them to be fixed. This PR fixes them, along with what the same classes of bug turned up. Every finding was adversarially verified before it was fixed, and the fixes were reviewed and re-verified twice.
0. Recovered: argument hashes in traces (missed #250)
The argument-hash commit was pushed to
feat/wire-record-checkafter #250 had already been squash-merged, so it never reachedmain. It is cherry-picked here. Fixes on top:requested_args_sha256(the model's raw arguments, matching the wire record) stays. "Changed before running" is now anargs_changedflag from a loose comparison done in memory. It dropsexpected_observation, ignores filled-in defaults, and tolerates validation's conversions ("5"→5,"yes"→True, int→float)."1e999999999"can't freeze the process. Numbers are compared as Decimals.1. Prax's self-diagnosis, fixed properly
OTelToolCallback.on_tool_endTypeError on every tool callstr(ToolMessage)).span.end()always runs: previously every successful tool span was lost. Nested wrapper layers no longer triple-countTOOL_CALLS. The samestr(ToolMessage)bug in the trace recorder is why trace reports showedcontent='…'. Unit tests use realToolMessages through a realToolCallUnknown channel: browser#contentis missing too, and core channels existed only because TeamWork created them once. Now there is one registry of every channel Prax posts to (teamwork_channels.PRAX_CHANNELS): ensured at startup, ensured lazily once per name, with per-name locks. An AST drift guard fails CI if a call site posts to an unregistered name. The warning is kept, not downgraded to debug as proposed (it was the only signal), but emitted once per nameensure_branch_channelusesself._enabledenabledproperty. The test stub bypassed the real constructor, which is why tests passed. It now uses the constructor, and a real-client regression test was added#claude-codeinterjection handler raised ImportError; it now falls through cleanly2.
prax_doctorreports correctlyos.environ, which pydantic never fills, so the doctor said "TeamWork: not configured" and FAILed the LLM check. It now reads settings. The LLM check builds each enabled tier through the real factory (verified to make no network call).plugin_listshows load failures.LOG_HEALTH_ENABLED, default off). An in-process handler groups WARNING+ records by code location. It keeps templates only for Prax's own call sites, never raw messages (logs carry user data). Workspace paths are scrubbed and locations sanitised.system_statusno longer quotes raw log lines and reads only a bounded tail.3. Credential redaction (found by review)
BROWSER_SECRETS_OUT_OF_CONTEXT=false,browser_loginreturnsusername=…\npassword=…. That text reached:_write_tracewrites into the workspacetrace.log(committed to git, searchable, in TeamWork's file browser).The password also came back as
browser_fill(text=…)arguments.trace.log, and spoke/sub-agent answers.reset_all_idle.BROWSER_SECRETS_OUT_OF_CONTEXT=truein production, so the model never sees the password at all.Tests
make cigreen: 4342 passed (up from 4069 onmain). No secrets-proxy requests during the run.test_tool_callback_outputs,test_channel_registry,test_doctor,test_log_health,test_turn_secrets,test_credential_output_redaction,test_system_status,test_coding_channel_interjection, plus args-hash and wire-check cases.tests/conftest.pypinsTEAMWORK_URL="". Localmake cino longer waits about 30 s percreate_appfor a TeamWork that isn't running: 23 min → 15 min.Not in this PR (follow-ups)
read_logsstill reads the whole log file.plugin_listprints raw text.GET/DELETE /teamwork/claude-code/sessionsimport removed helpers..envsetsHEALTH_MONITOR_ENABLED=false.After merge
Deploy. Optionally set
LOG_HEALTH_ENABLED=trueandBROWSER_SECRETS_OUT_OF_CONTEXT=truein production.