Repository navigation
feat(*): let a failed turn's row open to its whole error - #978
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
I reviewed the full github/main...HEAD diff and found one narrow, nonblocking UI-label regression. The runtime-to-RPC-to-Web path otherwise preserves the bounded text, carries the scrubbed/capped long form consistently live and on replay, and keeps the kernel boundary intact.
Coverage included AGENTS.md/CLAUDE.md and the Runtime/Web CONTEXT terms, the changed code and tests, relevant callers and history, backward compatibility for older markers, test-strength changes, generated contracts, and architecture constraints. I found no weakened tests.
Verification:
- Python 3.12 with the
devextra: 808 focused runtime, schema, and kernel tests passed. - Web UI: 217 focused tests passed; RPC generation check and TypeScript type-check passed.
- Import-linter: 10 contracts kept, 0 broken.
- Source-language, large-file, commit-message, and
git diff --checkgates passed (the three repository scripts were invoked directly becausemakeis unavailable). - The first default Python run produced 808 setup errors because the fresh default environment lacked
raven_everos; the documented all-extras sync then could not builddatriebecause this host has no GCC. Syncing the repository'sdevextra under Python 3.12 supplied the required test plugins and produced the passing run above.
|
Windows compatibility review of head No actionable Windows regression found in the inspected 35-file change. Code inspection covered exception/body extraction, scrubbing and caps, runner/lane/RPC propagation, UTF-8 session persistence, both generated clients, web live/replay rendering, tests and docs. The continuation design remains documentation; this PR implements full failure text. Native Windows verification (Python 3.13.12, isolated source/environment/home):
Broader Python selections stopped on three existing tests: the two image-window tests below counted 5 provider calls instead of 4 after Raven's permission judge intercepted their fake tool; the tool-event test returned an approval error because its fake provider lacks Exact focused commands and baseline controlsAll pytest invocations appended Focused head command ( uv run pytest `
tests/test_agent_loop_llm_error_patience.py::test_a_call_the_ladder_gave_up_on_keeps_the_body_its_record_kept `
tests/test_agent_loop_run_emit.py::test_run_turn_hands_the_lane_the_long_account_of_a_crash `
tests/test_rpc_spine.py::test_outlet_emit_error_includes_the_long_account_when_present `
tests/test_rpc_spine.py::test_a_failed_turn_sends_the_runners_long_account `
tests/test_spine_scheduler_lane.py::test_a_runners_long_account_rides_the_failure `
tests/test_spine_scheduler_lane.py::test_an_answerless_turn_carries_its_long_account `
tests/test_spine_scheduler_lane.py::test_a_crash_with_no_long_account_carries_none `
tests/test_spine_scheduler_lane.py::test_only_a_non_empty_string_counts_as_a_long_account `
tests/test_spine_events.py tests/test_rpc_schema_match.py `
-n 0 -x --tb=short --basetemp "$check/pytest" --junitxml "$check/junit.xml"Native probe ( Base controls, each run separately with the same flags (each failed as at head): uv run pytest tests/test_agent_loop_llm_error_patience.py::test_a_size_refusal_closes_the_window_for_the_rest_of_the_turn
uv run pytest tests/test_agent_loop_llm_error_patience.py::test_the_configured_budget_collapses_the_window_and_zero_leaves_it_open
uv run pytest tests/test_agent_loop_run_emit.py::test_run_tool_call_emits_tool_events_and_noticeThe control suite directories were Coverage limits: no live vendor call, browser end-to-end session or full-suite pass claimed. Git fetch/local archive problems required full-SHA GitHub source archives; all 35 changed head-file blobs were verified against API metadata. |
bd90480 to
3df58d3
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the revision delta and range-diff from bd9048019937, including the new failure-detail cache, descriptor-safe transport, truncated-secret handling, call-record integration, neutral notice label, and their callers/tests. The prior UI-label follow-up is fixed for live and replayed notices. I found no new correctness, compatibility, security, test-strength, or architecture issue.
Coverage included the repository rules and Runtime/Web context, the changed implementation and callers, history/rebase separation, older-record compatibility, test assertions, generated localization, and the kernel/import boundaries. Tests were strengthened rather than weakened.
Verification:
- 175 focused Python security/failure-path tests passed.
- 497 kernel, RPC-schema, and architecture tests passed.
- 187 transcript tests passed.
- Web TypeScript type-check and RPC generation check passed.
- TUI i18n generation check passed after installing its locked dependencies.
- Import-linter kept all 10 contracts.
- Source-language, large-file, commit-message, and
git diff --checkgates passed against the current PR base.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
I reviewed the delta from 3df58d385b80, its callers, security trade-offs, tests, history, compatibility, repository rules/context, and architecture boundaries. The CPU-cost optimization is effective and the added tests strengthen coverage, but its fixed 64-character pre-run window creates one narrow redaction gap when a known-cut credential label and short value have at least 64 whitespace characters between them. This is a low-reachability follow-up, not a merge blocker under the fourth-round bar.
Verification:
- 179 focused security, failure-path, call-record, persistence, and lane tests passed.
- Import-linter kept all 10 contracts.
- Source-language, large-file, commit-message, and
git diff --checkgates passed for the revision delta. - A direct public-API reproduction confirmed the named follow-up.
A failed turn showed one line, cut at 140 characters of a text the runtime had already bounded to 200, so a provider's own account (which pool refused, what to do about it) never reached the page. The runner now builds a long account beside the bounded text: the exception's message, then the body its HTTP client kept on it (litellm reduces an OpenAI-style error body to one line and keeps the rest on exc.body), scrubbed with the call record's body scrubber and then raven.security.redact, read within a bounded window and capped at the call record's 8,000-character body ceiling. It is set on the exception the runner lets out, since the kernel imports nothing to scrub with, and only when it says more than the bounded text. The lane copies it onto TurnFailed.full_detail, and the error event and the turn's session marker carry it as an optional full_detail field. The page's failed row gains a toggle that opens the account in a wrapped, scrolling block, live and when a session is replayed. Every 200-character text is unchanged.
The full-failure-text plan named its worktree and the main checkout by their absolute paths on one machine. A reader elsewhere cannot follow them, and they carried a local directory name into the public tree. The plan now names the two once, as $WT and $MAIN, and its commands use those.
…ries The comment above the lane's crash arm said the error event carries only the exception's class and message. The event can now carry the long account a runner attached as well, so the comment says that, and keeps the point it was making: the traceback never leaves the process, so this log line is its only record.
The long account runs two scrubbers. Every credential test used a vendor-shaped key, which raven.security.redact catches alone, so dropping the call record's body scrubber failed only the scan-window test, whose spy replaces that scrubber. A provider body that echoes a request's Authorization header as JSON, with a token of no vendor shape, is caught by the body scrubber alone; the new test holds that, and fails on its own when the body scrubber is dropped.
Two single-construct mutants of the full failure text left every test
green: a cap that cut an account of exactly MAX_BODY_CHARS characters,
and an HTTP error whose parsed body is {} or [] read as that body. Each
now has a test that fails on its own mutant: an account at the ceiling
is kept whole, and an empty parsed body is no body, so the call record
falls back to the exception's message.
…e and copy Three mutants of the page's half left the suite green. The test that compares a live row with a replayed one drew the live row with mount.note, past noteRow, so dropping noteRow's full option went unseen; it now draws through noteRow, the call the session's error stage makes. An opened row keeping its tooltip, and Copy carrying the short line instead of the whole account, are now pinned in the toggle test and in a new copy test.
ui-web/CONTEXT.md retires "page" for the whole document, and the turn-continuation spec, the full-failure-text plan and one test docstring used it for the web client. They now say "web client". The spec also qualifies two words it took from neighbouring entries, "an older Raven gateway" and wsCurrentTurn() for the workspace's turn count, and a stale row now sends a go-on message rather than injecting one.
Scrub incomplete private-key blocks before they reach failure records, avoid backtracking on authorization whitespace and repeated JWT prefixes, and reuse each exception's scrubbed account. Read and write the base exception storage so foreign descriptors cannot publish raw data or suppress failure notification. Give quiet notices a neutral expansion label and refresh the specification and plan. Co-authored-by: Codex <noreply@openai.com>
The tail patterns that remove a credential at a known cut end at the end of the input, and re.sub retries them at every position. A credential-shaped run that ended before other text therefore made each retry walk the rest of the run again, so the cost grew with the square of the run's length: 32,000 characters of "--token=" followed by a space cost 0.86 seconds of CPU on the failure path, past the 0.5-second pin, and a 262,000-character provider body cost 9 to 36 seconds on the event loop inside the call record. A tail match can only lie in the text's last run of non-whitespace characters and the label in front of it, so the patterns now read that region. A long final run is still read whole, which keeps a truncated token hidden. Over 48 inputs the output is identical to scanning the whole text. Both shapes join the timing test.
raven.security.redact scrubs every ACP surface as well as failure records, so two choices this branch made reach tool titles, result previews, permission prompts, replay and error messages too. A JWT is matched only where a token can start, which keeps a body of "eyJ" runs linear and leaves a token glued to a word, such as "/verify-eyJ...", unredacted. A private-key header with no END replaces everything after it, so a preview that only quotes the header loses the lines below it. Pin both with the cases each still catches, and name what the JWT boundary gives up in the design document.
The tail patterns' region was found with a regex search for the last whitespace, which retries every position: 31,000 spaces before the last two words of a failure cost 32 seconds on the event loop. The region also began only 64 characters before the final run, so a label with a wider gap before a short cut value was missed, and a text with no whitespace in its last stretch kept the whole-text cost: 64,000 characters of "pwd=" ending in a comma took 10.8 seconds, and the scan cap turns the same pass on for any text over 256 KiB on every ACP surface. A tail pattern's value runs to the end of the text and is made of one class of characters, so its match lies in the run of that class that ends the text, behind a label whose gap, scheme word and name come before it. Each search now starts behind those, found by matching the reversed text step by step, which reads a gap of any width once. A property test checks the result against the whole-text search on 3,000 generated texts and one long value per pattern, and 12,008 other texts with gaps up to 5,000 characters give the same output as the whole-text version. The reported shapes, a label gap and three label runs join the timing tests.
e307bc8 to
e72634a
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The latest redaction change fixes the previously reported unbounded-gap miss while retaining the bounded failure-path cost. I found no new actionable defect.
Covered the refreshed github/main...HEAD diff and the latest commit delta; applicable AGENTS/CLAUDE/CONTEXT rules; redaction callers and commit history; backward compatibility against the former whole-text behavior; test changes for weakening or overfitting; and the existing agent/lane/RPC/UI architecture boundaries. The propagation and rendering portions are unchanged from the earlier rounds.
Verification: 190 focused Python tests passed; 50,000 independently generated cut-tail inputs matched the whole-text reference; the original 64-space case and a 10,000-space case redact correctly; long-run stress took 0.166 s CPU; all 10 import contracts passed; source-language and large-file policy checks passed.
|
Windows compatibility follow-up at No new actionable Windows issue found in the inspected 37-file scope. Range-diff confirms the seven previously reviewed commits replay unchanged. Inspected the added bounded redaction/cache and exception-storage changes, their callers/tests, the quiet-notice label, documentation and the upstream stylesheet overlap. Native Windows / Python 3.13.12 verification:
Python commands
Suite uv run pytest tests/test_agent_loop_failure_text.py tests/test_llm_call_record.py tests/test_agent_loop_broken_turn.py tests/test_spine_scheduler_lane.py tests/test_spine_events.py tests/test_rpc_spine.py tests/test_rpc_schema_match.py tests/test_acp_redact.py tests/test_agent_loop_run_emit.py::test_run_turn_hands_the_lane_the_long_account_of_a_crash tests/test_agent_loop_llm_error_patience.py::test_a_call_the_ladder_gave_up_on_keeps_the_body_its_record_keptSuite Setup: Limits: no live provider, browser end-to-end or full-suite pass claimed; only the named cases from the broader run-emit and error-patience files were rerun. Tests used isolated exact-SHA source/home/environment, with every changed blob verified. |
3 participants
Summary
A failed turn's Web row can expand to the provider's full diagnostic text, up to 8,000 characters, while the existing bounded reason stays unchanged. The optional
full_detailfield travels through the runner, lane, RPC event and session marker, so live display and replay expose the same account. Copy includes the expanded account. Quiet runtime notices use "Show details" instead of the error label.Failure details and call-record bodies remove known credential shapes before the final cap. An unterminated private-key block is removed through the end of the input, including when an earlier scan or call record cut away its closing armour. Known cut boundaries also remove incomplete JWTs, vendor tokens, URL passwords and labelled credential tails; each of those patterns is searched only from behind the run of its own value's characters that ends the input and the label in front of it, so the scan stays linear whatever the text holds and still reads a label gap of any width. The authorization and JWT patterns avoid repeated backtracking, and the turn path and outer runner reuse the computed detail. Attachment and lane reads use the base exception's instance storage, so a foreign read-only property or throwing getter cannot replace the scrubbed value or swallow the terminal failure event.
The optional fields are mirrored in the RPC schema and both generated clients. The design document separates this delivery from the subsequent Turn Continuation feature; Retry still starts an ordinary turn.
Type
Verification
The automated suites and static/build checks below were re-run after synchronizing with main, using the existing local dependencies. The independent behavioral and timing probes were measured before that synchronization. The last three commits change only
raven/security/redact.py, two test files and the design document; on that final tree the Python suite, the ui-web tests, the type, generation and lint checks,ty, the import contracts, the range gates and pre-commit were run again, and the ui-web build checks were not, since none of their inputs changed.uv run --no-sync pytest -q tests/test_agent_loop_failure_text.py tests/test_llm_call_record.py tests/test_agent_loop_broken_turn.py tests/test_agent_loop_llm_error_patience.py tests/test_agent_loop_run_emit.py tests/test_spine_scheduler_lane.py tests/test_spine_events.py tests/test_rpc_spine.py tests/test_rpc_schema_match.py tests/test_kernel_budget.py tests/test_acp_redact.py tests/test_security_untrusted_context.py -x --tb=short --show-capture=no: 904 passed.From
ui-web:npm test -- --no-file-parallelism src/features/transcript/TranscriptPage.test.tsx src/state/session/pipeline.events.test.ts scripts/gates/i18n-keys.test.mjs: 225 passed.From
ui-web:npm run type-check,npm run gen:check,node ../ui-tui/scripts/gen-i18n.mjs --check,node ../ui-tui/scripts/gen-rpc-types.mjs --check, andnode node_modules/eslint/bin/eslint.js src/features/transcript/TranscriptPage.tsx src/features/transcript/TranscriptPage.test.tsx: passed.uv run --no-sync lint-imports: 10 contracts kept.uv run --no-sync ty check raven evolver agents plugins-dist scripts: passed. Ruff check and format check passed on the 20 Python files this PR changes.From
ui-web:npm run build,uv run --no-sync python build.py,node scripts/check-page.mjs,node scripts/check-css.mjs, andnode scripts/check-class-namespace.mjs: passed, including both boot snapshots.UV_NO_SYNC=1 make check-large-files check-source-language COMMIT_RANGE=origin/main..HEAD: passed.PYTHONPATH=. uv run --no-sync python scripts/check_commit_messages.py origin/main..HEAD, commitlint over the same range andgit diff --check origin/main...HEAD: passed.uv run --no-sync pre-commit run --from-ref origin/main --to-ref HEAD: all applicable hooks passed except the whole-TUI Prettier hook, which reportsnewInstancePicker.tsxandsubagentsHub.tsx. Both files and the formatter config are unchanged from main; an isolated main-file control with the same locked Prettier version reproduces both failures.node ui-tui/node_modules/prettier/bin/prettier.cjs --check ui-tui/src/i18n/messages.generated.ts: passed.Regression coverage includes truncated private keys, incomplete JWT and credential tails, terminal-event delivery with foreign exception attributes, scan reuse and CPU cost, the cut-tail scan's cost on label-shaped runs and long gaps, its agreement with a whole-text search, a label gap of any width, the JWT boundary and private-key END rules, and quiet-notice expansion. The five independent boundary probes passed before the base synchronization; the committed regression suite above passed again afterwards.
On the same 32,267-character synthetic crash input, the queued event-loop callback delay fell from 15.351 seconds to 0.00951 seconds. The regression checks use thread CPU time with a 0.5-second ceiling.
Cut-tail scan: a failed turn whose crash text holds 31,000 spaces before its last two words took 32.6 seconds while the region came from a regex search for the last whitespace, and 0.016 seconds now; 64,000 characters of
pwd=ending in a comma took 10.8 seconds through the whole-text tail search and 0.003 seconds now, and a 300,000-character call-record body of either shape takes under 0.3 seconds. These and the figures below were measured with the machine's load average between 36 and 132. Across 3,728 timed texts, every label or prefix unit repeated to 64,000 characters behind a cut and to 300,000 past the scan cap, the slowest measured alone took about 0.2 seconds against about 0.1 seconds on the base revision, and a text past the cap takes the same time at twice the size. Searching the whole text again turns six timing cases red; a smaller name room, a skipped gap or scheme step, or any value class narrowed below its pattern's turns the property or gap test red, as does removing the JWT boundary or restoring the END-only private-key rule. On 12,008 generated texts with gaps up to 5,000 characters, the output equals the whole-text version's.Chromium with synthetic local data: full text retained and HTML escaped, expanded tooltip removed, internal scrolling and Retry preserved, neutral notice label displayed, no page errors. No live vendor call or native Windows rerun is claimed for these fixes.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
raven.security.redactis also the scrubber for every ACP surface (tool titles, result previews, permission prompts, replay, error messages) and for the outbound client's permission log, so two of its rules reach those surfaces too: a private-key header without END hides the rest of that text, and a JWT glued to a preceding word character, such as/verify-eyJ..., is left unredacted. Tests pin both.Related Issues
N/A