fix(agent-server): never surface Claude Code's [ede_diagnostic] header as the error cause (#1849) - #1938
Merged
Conversation
|
Resolve by running |
AndriiPasternak31
marked this pull request as ready for review
August 2, 2026 11:56
AndriiPasternak31
left a comment
AndriiPasternak31
left a comment
Contributor
There was a problem hiding this comment.
Review — /review + /validate-pr, second pass
Read-only against origin/dev @ 8e924526. Head 5472ce27 unchanged since the first pass, so the earlier content analysis stands; this pass re-derives the two decisions the PR routes to the reviewer, plus the merge-order question.
Verdict: APPROVE-grade diff. No code change requested. Everything below is process or framing. Posting as a comment rather than an approval only because three mechanical gates are still open — see the checklist.
Re-verified this pass and holding: both errors[] read sites fixed (stream_parser.py:381 and :411; a branch-wide grep finds no third — remaining get("errors") hits are cleanup_service.py:2589 stats and skill_service.py:1261,1449 finalize). Input is credential-sanitized at both producers before the parser sees it (headless_executor.py:651-654, claude_code.py:366-367) and again at egress (:1029), and _entry_text's " ".join(str(raw).split()) removes a CRLF log-forging vector that previously reached logger.warning raw. Invariant #5 untouched — failure_classifier.py is byte-identical across src/backend/services/ and src/scheduler/ on dev (verified by diff), and correctly left alone here.
Decision 1 — accept the SUB-003 exposure now, or land \b401\b/\b403\b anchoring first?
Recommendation: ship now. File the anchoring as its own P2. Don't gate this P1 on it.
I reproduced the disclosed figure exactly — 1 in 57.6 over 2,000,000 UUID4s (1.69% for the full realistic message). The number is right. Two further measurements change what to do with it.
The anchoring works in isolation. Over 300,000 resume-not-found messages it takes the UUID false-positive rate from 1.78% to exactly zero, and loses zero true positives across 15 realistic auth strings — including tight forms (401), [401], code:401,, status=401, got 403.. Only no-separator forms (HTTP401, error401) regress, and those aren't real API error shapes. Clean standalone change.
But it buys much less than the framing implies. The disclosure scopes the exposure to the UUID sub-case. The actual change is that all real error text now reaches is_auth_failure's bare-substring table on this path, where previously only the marker did — and the marker trips nothing. Against 10 realistic execution_error texts:
plain anchored
ENOENT ... open '.config/credentials.json' AUTH AUTH (credentials)
Tool 'Bash' failed: ... /etc/forbidden-path AUTH AUTH (forbidden)
net::ERR_ABORTED at .../oauth/callback AUTH AUTH (oauth)
KeyError: 'authentication_mode' AUTH AUTH (authentication)
git push rejected: ... denied (403) AUTH AUTH (403)
Task stopped after 403 turns AUTH AUTH (403)
AssertionError: expected 403 but got 200 AUTH AUTH (403)
MCP server returned 401 during handshake AUTH AUTH (401)
No conversation found ... 8f2401ab-... AUTH ok <- fixed
File not found: .../401k-report.csv AUTH ok <- fixed
false positives before anchoring: 10/10
false positives after anchoring: 8/10
Anchoring removes 2/10 — the embedded-digit subset only. Every delimited numeric still matches (correctly, by its own rule) and so does every word indicator. It's a ~20% mitigation of the exposure, not a fix for it. Sequencing a P1 behind that is the wrong trade.
And the misfire is cost, not incorrectness — traced rather than assumed. Pre-raise switch (task_execution_service.py:1397) retries with the same stale resume_session_id, so it deterministically fails identically; the except handler (:1683-1699) passes error_msg into the TerminalEnvelope unchanged with error_code=None (gated strictly on 503, so the dispatch breaker is untouched); routers/sessions.py:651 still matches → clear_cached_claude_session_id → cold retry → succeeds. test_resume_fallback_survives_a_403_bearing_uuid's docstring claim holds.
The baseline is the clincher: pre-fix this path had a 0% misfire rate because it was 100% broken. You can't keep the 0% without keeping the bug.
Ask: reword the "~1.73%" disclosure to name the general classifier exposure rather than the UUID sub-case, and file the anchoring with the 2/10 finding attached so it isn't oversold as the fix for it.
Decision 2 — does the err[:300] residual block closing #1849?
Recommendation: no. Close #1849 on merge; file the truncation as a follow-up.
- Pre-existing and untouched.
headless_executor.py:1029,1032applieserr[:300]ondevtoday, to every 502 detail. This PR changes what enters the budget, never the budget itself. Not a regression it introduces. - Practically unreachable on the path that matters. Truncation only defeats self-healing when a >283-char real error precedes the resume marker in the same filtered array.
--resumeagainst a missing JSONL fails at startup, before turn work can accumulate other errors — which is exactly why #1673 observederrors == ["No conversation found with session ID: <uuid>"], a single ~60-char entry. Post-filter that's still one entry, well inside 300.test_multi_error_truncation_limit_is_knownhas to synthesize"A" * 400to reach the residual at all. - Pinned loudly and correctly. The known-bad pin states in its docstring that a failure means the residual was fixed, gives the exact edit for that moment, and blocks vacuous passing with
assert _CLAUDE_MSG not in detail.
#1849's impact #3 was itself flagged in the issue as "inferred from the emitter above — not reproduced against 2.1.215". Closing the inferred defect and pinning the narrow residual satisfies it.
Still open
- Follow-ups: none filed. Re-checked today —
AUTH_INDICATORSandede_diagnosticsearches return no follow-up issues, and everything created since 2026-08-01 (#1931–#1933) is unrelated. A PR body isn't a tracker; these evaporate at merge. Minimum bar: theAUTH_INDICATORSanchoring and theerr[:300]budget. docs/memory/feature-flows.md"Recent Updates" row missing. Two flows updated, index untouched. Not mandated by the skill (no new flow) but it's the convention (#1450, #1445, #1444 all carry one).- #1849 carries both
status-in-progressandstatus-ready— contradictory, drop the latter. - No real-agent runtime coverage. Release gate, not merge gate — the PR is honest about it.
stream_parser.pyis baked intotrinity-agent-base; CI never runs it in a container andverify-localran--skip-agent. Worth carrying into release notes: nothing changes on a running agent untilbuild-base-image.sh+ recreate, and a revert needs one too.
Merge order
Confirmed by git merge-tree, not inferred: this PR merges clean into current dev. #1911, #1913 and #1940 all append at the same docs/memory/learnings.md EOF hunk (@@ -230,3 @@), so whichever lands first, the other three conflict — in every ordering, not just this one. The conflict is confined to that single file and both sides are disjoint appends to an append-only ledger, so resolution is concatenate, keep both — no semantic decision. Only real hazard is a careless --ours/--theirs silently dropping an entry; verify after each with:
git show dev:docs/memory/learnings.md | grep -c '^## 20'Recommend merging this one first — it's the P1 and it's the only one of the four that's currently conflict-free.
Pre-merge checklist
- File the follow-ups (min:
AUTH_INDICATORSanchoring,err[:300]budget) - Add the
docs/memory/feature-flows.md"Recent Updates" row - Reword the "~1.73%" disclosure to name the general classifier exposure
- Drop the contradictory
status-readylabel on #1849 - Real-agent smoke on a rebuilt
trinity-agent-basebefore release - Second reviewer per SOC 2 (this is a comment, not an approval)
Full working notes, including the reproduction scripts behind every figure above, are in my local review record. Happy to paste any of them into the thread.
… header (#1849) TDD red. 24 cases (19 test functions, one parametrized 5 ways) driving the real parser, the real 502 finalizer, and the REAL backend predicate (routers/sessions.py::_is_resume_not_found, loaded via spec_from_file_location rather than `from routers.sessions import ...` — the package import raises ImportError under CI's randomized seeds). 18 fail against unmodified source, all behaviourally: - errors[0] is Claude Code's own [ede_diagnostic] header, so the marker wins over the real cause at errors[1..]; - the acceptance test proves impact #3 (resume self-healing defeated) is real, not inferred: _is_resume_not_found(detail) is False today; - a malformed `errors` shape RAISES inside the parser (KeyError: 0 for a dict, TypeError for an int/bool) — the swallowed raise lands as HTTP 200 success; - a bare-string errors is indexed character-by-character ("boom" -> "b"). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r as the error cause (#1849) Mirror the CLI's own consumer: drop `[ede_diagnostic]` entries from `result.errors`, join the rest. Applied at BOTH `errors[0]` reads — the pre-existing max_turns branch (#361) reads the same array from the same emitter and has the same defect shape. When a result carries no real cause at all, the diagnostic's key=value payload is kept as clearly-labelled context (minus the `[ede_diagnostic]` token, which is unsearchable against this repo and misdirects investigators) rather than dropped — nothing else on this path is recorded. It also stays in the existing WARNING log; not DEBUG, since the agent server is pinned to INFO and DEBUG would emit nothing. The `#1673:` comment block is EXTENDED, not replaced — a sibling test asserts only `"#1673" in src`. Also fixes four latent defects in the same two expressions: - a malformed `errors` shape (a dict, an int, a bool) made `errors[0]` RAISE inside the parser, and the swallowed raise fell through to the #160 `context: fork` placeholder — HTTP 200, status=success; - a bare-string `errors` was indexed character-by-character ("boom" -> "b"); - a blank/None entry could join to "" and re-introduce the empty error_message #1673 fixed; - a leading newline slipped a marker past the prefix test. No new imports; no change to headless_executor / jsonl_recovery / claude_code (#1870 / #1853 / PR #1718 territory) or to the backend predicate, which is the acceptance criterion rather than the fix site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#1849) - tests/registry.json: entry for tests/unit/test_1849_ede_diagnostic_filtered.py. - learnings.md: new entry for the durable class — a cross-surface contract carried by third-party FREE TEXT has no schema and breaks silently when the vendor reformats. Broken twice in six weeks (#1673 -> #1849), green CI both times, because no test drove producer -> transforms -> the real consumer predicate. Also covers the "raise inside a swallowing parser is a false success" and "mirror a vendor's own consumer-side filter" corollaries. - parallel-headless-execution.md: revision-history row, AND a fix to the stale "How Errors Are Classified -> Source 1" section, which quoted pre-#1673 code and attributed it to claude_code.py — a file that has not owned this since the #122 extraction. Appending a row above a wrong snippet would leave the doc self-contradictory, and that snippet is exactly what a future contributor reads before "simplifying" the helper. - session-tab.md: hazard clause on the resume-fallback step. The contract is restored, not changed — but it was documented without noting that it depends on an agent-side free-text string surviving filter -> join -> sanitize -> [:300], which is why it has silently broken twice. No requirements/ or architecture.md change: this is a bug fix (Rule #4), and architecture.md documents nothing about result.errors parsing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… to ~1.73% (#1849) The plan cited 1.09% "measured over 200,000 UUIDs". Re-measured here over 500,000 `uuid4()`s: 1.727% contain a bare "401" or "403" (0.879% / 0.854% individually) — ~1 in 58, not ~1 in 92. uuid4 pins '4' at string index 14, which lifts the rate above the naive 30-position estimate. The fixed prose around the UUID ("Execution error: No conversation found with session ID: ") contributes no match, so the UUID is the only source. Material because this number is the whole cost argument for shipping #1849 before the `\b401\b` word-boundary hardening — it belongs in the PR body and release notes correct. Also: assert the WHOLE diagnostic payload survives, not the "stop_reason=null" fragment (uses the previously-unreferenced _MARKER_PAYLOAD constant). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ns (#1849) Four review corrections, no behaviour change. 1. Disclosure defect (the substantive one). The plan justified joining at the max_turns branch with "verified safe: max_turns text lands only in an informational 422 and is substring-matched by nothing". That is wrong: `classify_switch_failure` returns "auth" for ANY >=400 body that trips `is_auth_failure`, and the `except httpx.HTTPError` handler calls `is_auth_failure(error_msg)` independently of status code — so the 422 detail is matched at the same two sites as the 502. The R1 exposure therefore covers BOTH branches. Practically inert (a max_turns result with real `errors[]` entries is unobserved; marker-only/absent still yields the fixed "Task stopped after N turns"), but a spurious switch there re-runs a turn that legitimately hit its limit, so the misfire costs tokens rather than a fast-failing duplicate. Recorded in the feature-flow row, since the whole R1 disposition rests on the exposure being measured and disclosed. 2. `errors[0]` did NOT raise on all four malformed shapes. Verified against the pre-fix expression: dict/int/bool raise; an explicit `null` falls through the `if errors` guard to the old bare literal, and a nested list assigns a raw `list` into the str-typed `error_message` field. The commit message and the flow row already said "a dict, an int, a bool"; the test file's docstrings said "a nested list" and "on a non-sequence raises". Corrected so the artifact matches what was measured. 3. The casefold/fixed-length-slice interaction is now stated as an invariant instead of left to be re-derived: full case folding never shortens a string, so the 16 folded chars came from at most 16 original ones and the slice starts at or after the real payload — the token cannot leak. Worst case, confirmed with a "st"-ligature marker, is one dropped payload character on input Claude Code cannot emit. 4. `test_multi_error_truncation_limit_is_known` pins a KNOWN-BAD residual, and its failure message read as if the failure were the regression. It now says outright that a failure means truncation was FIXED, names the flip, and asserts the marker really is truncated away so the pin can't pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AndriiPasternak31
force-pushed
the
vybe/issue-1849
branch
from
August 2, 2026 13:57
5472ce2 to
dbb1d19
Compare
AndriiPasternak31
approved these changes
Aug 2, 2026
AndriiPasternak31
left a comment
AndriiPasternak31
left a comment
Contributor
There was a problem hiding this comment.
Approving — dbb1d192 (rebased)
My first pass called this APPROVE-grade, no code change requested, and nothing in the diff has
changed since. Converting to an approval.
I rebased this branch onto dev myself (5472ce27 → dbb1d192). That was our doing, not an
oversight of yours: dev moved four times today (#1935 609f8dff, #1934 f5a21624, #1940
d632c3a5, #1936 c4c83f4a) and #1940 took the docs/memory/learnings.md anchor first, so this
branch inherited a conflict it didn't have this morning.
The resolution was mechanical — an append-only ledger, two disjoint entries, keep-both in merge
order. Proof rather than assertion:
- Entry count
59 → 60(grep -c '^## 20'), i.e. exactly one entry added and none lost - File set identical before and after the rebase (6 files)
- Of those 6, only
learnings.md's diff changed; the other five are byte-identical - All 6 commits still authored by @vybe
test_1849_ede_diagnostic_filtered.py→ 23 passed on the rebased tree
One gate I am deliberately not closing myself: the docs/memory/feature-flows.md "Recent
Updates" row is still missing (the PR body doesn't claim it either). Writing it would mean
authoring prose about your fix and then approving the PR containing it — the wrong side of SOC 2
separation of duties. Filed separately instead; see the linked follow-up.
Remaining from the first pass, none blocking, none code:
- Follow-ups unfiled:
AUTH_INDICATORSanchoring, theerr[:300]budget - The "~1.73%" disclosure still reads narrower than the general classifier exposure
- #1849 carries both
status-in-progressandstatus-ready - Real-agent smoke on a rebuilt
trinity-agent-basebefore release — release-time, not merge-time,
but it does need to happen since this ships in the agent image
Recovery handle if the rebase ever needs undoing: 5472ce27.
3 tasks
vybe
pushed a commit
that referenced
this pull request
Aug 4, 2026
…execution (#1870) (#1944) * test(#1870): commit a redacted REAL captured Claude Code tail as fixture The plan's first draft asserted, from the issue's rendered summary table, that both trailing records are string-content. Measured over 1,075 real transcripts that is wrong: `[Request interrupted by user...]` is LIST content (261 list / 2 str), `<task-notification>` is str (999 / 41). This fixture is extracted from a real CC 2.1.220 transcript, not typed. Structure is byte-preserved (every key, every block shape, the real non-message record interleaving); only free-text values that could carry third-party data are replaced. It carries the E1 shape that matters most: the final message is TWO assistant records sharing one message.id -- `thinking` then `text` -- and BOTH carry stop_reason=end_turn. 40.6% of real end_turn markers are thinking-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#1870): RED — 69 tests + call-site scaffolding, recovery stubbed to None Red-first. The recovery function is a stub returning None, so behaviour is byte-for-byte today's: the 502 still fires. 29 of the 69 tests fail, and the headline reproduction fails with the issue's exact error: E fastapi.exceptions.HTTPException: 502: Execution error: [ede_diagnostic] result_type=user last_content_type=n/a stop_reason=null Landed with the tests so the gate is exercised behaviourally rather than dying at import: - models.py recovered_terminal (C1), deliberately separate from recovered_from_jsonl - headless_executor.py _RECOVERY_NOTICE (C2) + _try_recover_completed_turn + the nested `if` at the execution_error branch. The raise body is character-identical, re-indented one level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1870): GREEN — recover a completed turn reported as error_during_execution Implements _recover_completed_turn_from_jsonl. Three independent gates, all required: main-thread only x turn-scoped x finished. The load-bearing property: the recovered answer is the marker message's message.id GROUP, fail-closed when that group holds no text -- NOT a text window ending at the marker. A thinking-enabled final message is two records sharing one message.id, and BOTH carry stop_reason=end_turn. Measured here over 1,075 real transcripts / 6,663 main-thread markers: 40.6% are thinking-only, and message.id grouping yields text for 6,660 of them. _read_jsonl_records drops the final partial line on an interrupted write -- and #1870 IS the interrupted-tail case -- so a window rule would return the turn's narration WITHOUT the answer as a 200 SUCCESS, stored, never retried. That is strictly worse than the bug being fixed. Grouping is also the right artifact: a normal success stores only result_text, whereas a window stores intermediate narration (measured window/final ratio p90 2.19x, max 79.5x). Other gates: - _is_main_thread (isSidechain/isMeta) on marker selection, the boundary walk AND text collection. A subagent end_turn + its string-content prompt satisfy both the marker and boundary tests; ungated, a crashed main thread returns 200 with a subagent's internal thought. - The LAST qualifying record must ITSELF be end_turn -- stricter than "the last end_turn in scope", so an interrupted-mid-tool thread cannot be rescued by an earlier marker. - Marker timestamp bounded on BOTH ends. Unparseable since_iso fails closed. - since filter applied to collected records on every path (after a 10MB seek the real failure is a wrong boundary, not a missing one). Observability (both directions, per plan §8): a hit logs completed_turn_recovered_from_jsonl; EVERY decline logs completed_turn_recovery_declined with a specific reason. A fail-closed gate that silently stops firing is otherwise indistinguishable from "the bug never happened". 69/69 new tests green; 180 green across the 9 focus files. The #1673 raise is character-identical (re-indented one level) -- verified mechanically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#1673): pin the JSONL precondition and the shapes that would re-open it #1870 added a recovery step inside this branch, so the #1673 pins now depend on "no JSONL on disk" -- which held only because /home/developer/... does not exist on a test host. Make it an asserted precondition via an autouse _JSONL_PROJECTS_DIR fixture instead of an environmental accident (no CI job runs tests/unit in a container, so the premise is true but implicit). Three new negative tests: - JSONL present but no end_turn => still 502. - The shape that would actually re-open #1673: a PRIOR turn's end_turn in the JSONL AND partial stdout in response_parts => still 502. Exercises the staleness guard and the never-recover-from-stdout invariant together. (The first draft of this test covered the safe direction only.) - Recovery is not attempted for rate_limit / max_turns / authentication_failed even with a perfectly recoverable transcript. Nothing pinned that before. 16 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#1870): requirement 10.4.3, flow-doc section, registry entry - requirements/scheduling.md: new 10.4.3 after 10.4.2 (precedent: 10.4.1 is a headless_executor error-classification requirement in the same file). States the <=600s coverage bound as its own bolded clause with the "raise the agent's timeout" lever named -- it is permanent and has NO fallback, because stream-json carries no completion signal at all. - feature-flows/parallel-headless-execution.md, per the plan's binding conflict rule with PR #1938: * revision-history row at the top anchor (same anchor #1938 uses -- an unavoidable, trivially-resolved collision: keep both, newest first) * a NEW section placed AFTER the error-classification region rather than editing lines ~350-400, keeping the conflict surface minimal * FIXED the stale `"execution_error" | 503 | falls through...` table row. It has been wrong since #1673 gave the case a dedicated 502 branch and #1938 does not fix it; leaving a doc permanently self-contradicting about one field to dodge one mechanical conflict is the worse trade. - architecture.md deliberately NOT edited: its only mention (JSONL reaping) stays true and no new cross-cutting surface is added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#1870): record where each recovery signal actually lands (traced) Scrutiny item S3/E11 asked whether metadata.error_type is really PERSISTED on the recovered 200 row. Traced: it is NOT -- and neither is recovered_terminal. apply_result's success branch cherry-picks exactly six metadata keys (cache_read/cache_creation/input_tokens, context_window, cost_usd, session_id, compact_events) and drops the rest; schedule_executions has no metadata column; the #1083 async callback converges on the same applier. Consequences, now written down rather than left as an assumption: - R8's "error_type is the audit trail" is false at the DB level. - recovered_terminal (C1) is agent-side + on-the-wire only, exactly as the plan's own C1 rationale anticipated ("a new backend can start reading it later without coordination") -- but that means it is not yet an operator- visible record. - The only signals that actually persist onto the execution row are the recovery NOTICE (it rides inside the stored response text) and the agent log line. So C2 is currently the sole persisted operator-facing signal, not a secondary nicety -- material if anyone later proposes reducing it to a footnote. Follow-up (flagged, not filed): teach apply_result's success branch to read recovered_terminal. That branch is the single chokepoint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1870): close the window fallback's narration-as-answer hole (review) Three review findings on the #1870 branch, all in the new agent-side recovery surface. No behaviour change on the paths measured in the corpus. 1. CRITICAL — the `message.id`-less window fallback re-opened R13. Verified red: a thinking-only marker (the modal shape after a truncated write, 40.6% of real markers) with no `message.id` made the `(boundary, marker]` walk collect the turn's EARLIER narration — answer entirely absent — and return it as a recovered 200 SUCCESS. That is the exact silent partial-deliverable regression the `message.id` rule was written to prevent, live on the one path that would ever run if Claude Code stopped emitting the field. The marker record must now carry text ITSELF before the window is consulted; a multi-record final message still joins in full, since its last record has text. Pinned by two new tests, one of them the red repro. 2. Decline reasons were conflated, which is how a fail-closed gate rots. A marker with an unparseable/absent timestamp reported as `stale_marker` — so a future CLI moving the `timestamp` field would decline EVERY recovery with a reason an operator reads as "working as designed" and never investigates. Reasons are now split by the action they imply: `marker_no_timestamp` / `malformed_message` (format moved), `future_marker` (clock wrong), `stale_marker` / `sub_thread_only` (guard working), and `not_finished` — the expected steady-state decline, previously indistinguishable from "no assistant records at all" — which carries the observed `stop_reason` as a shape-validated token. 3. Log-forging primitive in the new decline lines. `resume_session_id` reaches `ctx.claude_session_uuid` straight from the /task request body, the agent server logs plain text (`logging.basicConfig`), and Vector splits records on newlines. The new lines interpolated it raw; they now use `!r`, matching what `_read_jsonl_records` already does on its own reject path. The three pre-existing #678 sites with the same shape are left alone and flagged. Also records that `_MAX_FUTURE_CLOCK_SKEW_S` compares two clocks that are both the agent container's own, so it is slop against a corrupt timestamp rather than a tuned cross-host skew budget. tests/unit/test_1870_completed_turn_recovery.py 81 passed (was 80 -> +12 cases across 4 new tests); focused suite 197 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: trinity-ability <trinity-ability@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
# Conflicts: # docs/memory/feature-flows/parallel-headless-execution.md # docs/memory/learnings.md # tests/registry.json
Contributor
Author
|
Rebased onto
Merging on AndriiPasternak31's existing approval. |
vybe
enabled auto-merge (squash)
August 6, 2026 14:03
# Conflicts: # tests/registry.json
AndriiPasternak31
added a commit
that referenced
this pull request
Aug 16, 2026
…cution 502 and timeout 504 bodies (#1853) The execution_error 502 raised a bare detail="Execution error: <msg>", discarding ctx.metadata (session_id/cost/context) and ctx.raw_messages (the full stream-json transcript) that were in scope. New _execution_error_502_detail mirrors _timeout_504_detail: {message, metadata, execution_log}. The message text is byte-identical (preserves #1938 + the backend resume-not-found self-heal). _timeout_504_detail gains the same validated session_id fallback + execution_log, so the 504 path persists them for real too. _valid_session_id UUID-shape-validates the session_id fallback before it is persisted — ctx.claude_session_uuid can be an untrusted resume_session_id (log-forging vector; sanitize_dict does not strip newlines) (FI-1). Also corrects the stale "reaps after 24h" comment (real: 6h sweep / 1h age guard = 1-7h effective). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
Aug 16, 2026
…#1853) New tests/unit/test_1853_error_telemetry_salvage.py (21 tests): - _valid_session_id UUID guard incl. the FI-1 newline-injection case - _execution_error_502_detail / _timeout_504_detail carry transcript + validated session_id; the structured body does NOT trip _is_reader_race_signature (ENG#2) - _finalize_headless_result error branch raises a 502 dict with an UNCHANGED message (#1938) + transcript + session id (ENG#10) - _extract_agent_error 3-tuple; resume-not-found message preserved (#1673/#1849) - apply_result FAILED branch persists the transcript with the embedded secret REDACTED, a #1741 tool_calls summary, the validated session id, salvaged cost; won-gated close/emit unchanged on a lost CAS (#1578/#1804) - real-sqlite readback proving the column names (ENG#11) Refines the now-misleading test_1083 comment (a FAILED write CAN carry the transcript when the envelope has one). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
Aug 16, 2026
…tured shape (#1853) #1853 deliberately changed the error_during_execution 502 body from a bare string to a structured {message, metadata, execution_log} dict. Two #1673 contract tests asserted the old string shape and began failing: - test_502_detail_carries_the_resume_marker_backend_matches_on: the resume- not-found marker the backend substring-matches now lives in detail["message"] (carried verbatim, preserving #1938 + the self-heal). Assert the dict shape. - test_execution_error_not_a_reader_race_dict_body -> _does_not_trip_reader_race: "not a dict" is no longer the discriminator (the body IS a dict now). Assert the real contract: _is_reader_race_signature(detail) is False (it keys on recovery_attempted, which the #1853 body omits). Intent preserved on both; behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Aug 16, 2026
…ution/504 row (#1853) (#2224) * docs(scheduling): requirements §10.4.4 — telemetry+transcript on failing error_during_execution/504 row (#1853) Trinity Rule #1 (requirements before implementation). Documents Approach B: the FAILED applier mirrors the SUCCESS branch — persists sanitized execution_log + tool_calls summary + UUID-validated claude_session_id + salvaged cost/context on the error_during_execution (502) and timeout (504) FAILED row. Names the residuals (_write_terminal_and_gate + standalone scheduler still land bare). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(agent-server): carry telemetry+transcript in the error_during_execution 502 and timeout 504 bodies (#1853) The execution_error 502 raised a bare detail="Execution error: <msg>", discarding ctx.metadata (session_id/cost/context) and ctx.raw_messages (the full stream-json transcript) that were in scope. New _execution_error_502_detail mirrors _timeout_504_detail: {message, metadata, execution_log}. The message text is byte-identical (preserves #1938 + the backend resume-not-found self-heal). _timeout_504_detail gains the same validated session_id fallback + execution_log, so the 504 path persists them for real too. _valid_session_id UUID-shape-validates the session_id fallback before it is persisted — ctx.claude_session_uuid can be an untrusted resume_session_id (log-forging vector; sanitize_dict does not strip newlines) (FI-1). Also corrects the stale "reaps after 24h" comment (real: 6h sweep / 1h age guard = 1-7h effective). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(execution): persist transcript+session_id on the FAILED applier row (#1853) _extract_agent_error now returns the agent execution_log transcript too; the httpx handler threads it + the validated session_id onto the existing TerminalEnvelope fields. apply_result FAILED branch mirrors the SUCCESS branch: sanitize_execution_log(json.dumps(transcript)) + the #1741 tool_calls summary + claude_session_id, passed into the EXISTING db.update_execution_status call. cost/context already salvaged from metadata. Single terminal applier (#1483) preserved — payload widened, no new CAS writer, _write_terminal_and_gate untouched. All side-effects stay gated on the existing won bool (#1578/#1804) — the new columns are added above the gate, no predicate widened. Bare-string old-image bodies leave the columns null (graceful mixed-fleet degrade). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(agent-server): import re for the #1853 session-id UUID validator _SESSION_ID_UUID_RE / _valid_session_id use re.compile; headless_executor did not import re. Completes the previous agent-side commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(execution): telemetry+transcript salvage on failing 502/504 rows (#1853) New tests/unit/test_1853_error_telemetry_salvage.py (21 tests): - _valid_session_id UUID guard incl. the FI-1 newline-injection case - _execution_error_502_detail / _timeout_504_detail carry transcript + validated session_id; the structured body does NOT trip _is_reader_race_signature (ENG#2) - _finalize_headless_result error branch raises a 502 dict with an UNCHANGED message (#1938) + transcript + session id (ENG#10) - _extract_agent_error 3-tuple; resume-not-found message preserved (#1673/#1849) - apply_result FAILED branch persists the transcript with the embedded secret REDACTED, a #1741 tool_calls summary, the validated session id, salvaged cost; won-gated close/emit unchanged on a lost CAS (#1578/#1804) - real-sqlite readback proving the column names (ENG#11) Refines the now-misleading test_1083 comment (a FAILED write CAN carry the transcript when the envelope has one). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(architecture): FAILED applier persists transcript+session_id (#1853) task_execution_service bullet: the FAILED branch of apply_result mirrors the SUCCESS telemetry (execution_log/tool_calls/claude_session_id) above the won gate; _extract_agent_error 3-tuple + the _timeout_504_detail transcript/session extension. Names the residual bare-terminal writers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(feature-flows): telemetry+transcript on failing 502/504 rows (#1853) - task-execution-service.md: FAILED applier mirrors SUCCESS telemetry; _extract_agent_error 3-tuple - parallel-headless-execution.md: structured 502/504 body + _execution_error_502_detail + validated session_id fallback; stale 24h->1-7h comment note - feature-flows.md: Recent Updates index row Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(execution): live-capture E2E salvage on the #1853 FAILED row (#1853) Extends the #1853 suite with the #1944 §4.4 live-capture pattern: a REAL error tail written into the agent's ~/.claude/projects/-home-developer/ drives the REAL _finalize_headless_result (genuine #1870 recovery decline, not monkeypatched), so the structured 502 is proven to carry the transcript + validated session id on the exact failure path the fleet takes — and a #1870-recoverable tail is NOT re-failed by #1853. Adds the end-to-end chain (real error tail -> real finalize 502 body -> backend _extract_agent_error -> apply_result -> a persisted, redacted, session-tagged FAILED row in real sqlite). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(execution): migrate #1673 502-body assertions to the #1853 structured shape (#1853) #1853 deliberately changed the error_during_execution 502 body from a bare string to a structured {message, metadata, execution_log} dict. Two #1673 contract tests asserted the old string shape and began failing: - test_502_detail_carries_the_resume_marker_backend_matches_on: the resume- not-found marker the backend substring-matches now lives in detail["message"] (carried verbatim, preserving #1938 + the self-heal). Assert the dict shape. - test_execution_error_not_a_reader_race_dict_body -> _does_not_trip_reader_race: "not a dict" is no longer the discriminator (the body IS a dict now). Assert the real contract: _is_reader_race_signature(detail) is False (it keys on recovery_attempted, which the #1853 body omits). Intent preserved on both; behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(execution): migrate #1849 502-detail assertions to the #1853 structured shape (#1853) `_detail_for` asserted the error_during_execution 502 `detail` was a plain string, but #1853 made it a structured body (message + metadata + execution_log). Mirror the real consumer (`_extract_agent_error` derives `result.error` from `detail["message"]`) — the same migration already applied to the sibling test_1673. Fixes the 3 regression-diff failures in test_1849 (test_resume_fallback_fires_through_the_502_detail, test_resume_fallback_survives_a_403_bearing_uuid, test_multi_error_truncation_limit_is_known). Also fixes a pre-existing test-isolation leak surfaced by this PR's new test file reordering: test_audit_chain_edges::test_enabling_the_hash_chain_survives_a_restart (added by #2026) writes `audit_hash_chain_enabled='true'` to the real global system_settings and never restores it, so a later `platform_audit_service.log()` silently takes the chained writer — flaking test_1966's test_real_audit_service_swallows_its_own_failures. Add an autouse snapshot/restore fixture (mirrors the #762 restore in tests/test_audit_log_unit.py). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
3 participants
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.
Fixes #1849
Agent containers need a base-image rebuild to pick this up. The only product change is in
docker/base-image/agent_server/services/stream_parser.py, which is baked intotrinity-agent-base. Merging this PR changes nothing on any already-running agent until./scripts/deploy/build-base-image.shruns and agents are recreated — same constraint as commit7a179429. A revert needs one too. Please flag this in the release notes.What's wrong
#1673 correctly found that
error_during_executioncarries its text inmsg["errors"], notmsg["result"], and fixedstream_parser.pyto readerrors[0]. On current Claude Code (observed 2.1.215),errors[0]is never a real error — the CLI builds the array as[<marker>, ...realErrors]where the marker is its own internal diagnostic header:…and the CLI's own consumer filters every
[ede_diagnostic]-prefixed entry out before showing anything to a user. A second variant reads[ede_diagnostic] turn aborted (…) stop_reason=…. Upstream:anthropics/claude-code#82235.Three consequences, all reproduced end to end (the issue called #3 "inferred, not reproduced" — it is now demonstrated by driving the real parser → the real 502 detail → the real backend predicate and measuring
_is_resume_not_found == False):errors[1..]were dropped.context: forkplaceholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673's own self-healing was defeated.routers/sessions.py::_is_resume_not_foundsubstring-matches"no conversation found"against the 502 detail. With the marker in front, the match fails,clear_cached_claude_session_id()never runs, and the session re-locks into exactly the permanently-broken state bug: agent-server masks error_during_execution as a successfulcontext: forkplaceholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673 was filed to fix.The fix
Two private helpers in
stream_parser.py, applied at botherrors[0]reads:_entry_text(entry)— whitespace-normalized, dict-unwrapping best-effort text for one entry._split_claude_errors(raw)— returns(real_errors, diagnostics), mirroring the CLI's own filter (so it is correct on any CLI version — no version gate).Applied at the
execution_errorbranch and the pre-existingmax_turnsbranch (#361), which reads the same array from the same emitter and has the same defect. The issue names only the first; the second was found during planning. Both now", ".join(...)the surviving real errors instead of taking[0], so a multi-error result is preserved.Degenerate all-diagnostics case — the modal production shape; all four observed rows were marker-only. The diagnostic's
key=valuepayload is kept as clearly-labelled context,Claude Code reported no error detail (diagnostic: …), with the unsearchable token stripped and the tail capped at_DIAGNOSTIC_MAX=180. Nothing else on this path is recorded (cost,tool_calls,response,claude_session_idall empty — see #1853), so discarding it would be a net telemetry loss. The fallback literal is deliberately not the bare"Execution error", whichheadless_executor'sf"Execution error: {…}"would render as"Execution error: Execution error". The word choice is load-bearing: it must trip none ofis_auth_failure/_is_resume_not_found/_is_rate_limit_message, and that is test-pinned.The diagnostic stays in the existing WARNING log at both sites — not DEBUG as the issue suggested, because
agent_server/main.pypinslogging.basicConfig(level=logging.INFO)and a DEBUG record would emit nothing at all.Four latent defects fixed in the same two expressions
errorsshape (dict, int, bool) madeerrors[0]raise inside the parser. The raise is swallowed by the per-lineexcept Exception, and sincecost_usd/duration_msare already set the execution fell through to the bug: Headless task returns empty response for skills with context: fork #160context: forkplaceholder → HTTP 200,status=success. That is bug: agent-server masks error_during_execution as a successfulcontext: forkplaceholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673's own bug in a new shape.errorswas indexed character-by-character ("boom"→'b').Noneentry could join to"", re-introducing the emptyerror_messagebug bug: agent-server masks error_during_execution as a successfulcontext: forkplaceholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673 fixed. (Note: the issue's own suggested", ".join(errors)has exactly this hole.)casefold()-insensitive — deliberately more lenient than the CLI's own case-sensitive filter, since no legitimate cause begins with this token in any case.Unclaimed security win
_entry_text's whitespace normalization means neither the joined message nor the diagnostic tail can carry a newline. Pre-fix,errors[0]reachedlogger.warningraw — so this closes a CRLF log-forging vector on a path fed by third-party text. Not the reason for the change, but worth recording.Real error text now reaches the backend's
is_auth_failuresubstring classifier, which the inert marker previously suppressed. So anexecution_errorcan now trigger a SUB-003 subscription switch and, via the pre-raiseclassify_switch_failure, one same-execution_idretry.This covers both branches, not just
execution_error. The plan asserted themax_turnstext was "substring-matched by nothing" — that claim was refuted at review.classify_switch_failure(task_execution_service.py:408-433) returns"auth"for any>= 400response body that tripsis_auth_failure— a 422 included — and theexcept httpx.HTTPErrorhandler (:1701) callsis_auth_failure(error_msg)independent of status code. So themax_turns422 detail is matched at the same two sites.In practice the
max_turnsbranch stays inert: amax_turnsresult whoseerrors[]carries real entries is unobserved, and a marker-only/absent array still yields the fixedTask stopped after N turnsstring. The difference fromexecution_erroris the cost shape — a spurious switch there re-runs a turn that legitimately hit its turn limit, so the misfire costs tokens rather than a fast-failing duplicate.The misfire rate:
AUTH_INDICATORScontains bare"401"/"403", and a session UUID trips it ~1.73% of the time — ~1 in 58. Measured 1.727% over 500,000uuid4()s; independently re-measured at review as 1.738% (8688/500,000, ~1 in 57.6), 0.6σ apart. (uuid4pins'4'at string index 14, which lifts the rate above a naive estimate.)This is cost, not incorrectness. The duplicate turn fails fast (~2s, $0, per #1673 evidence) and the resume fallback still fires — pinned by
test_resume_fallback_survives_a_403_bearing_uuid, so the claim is tested rather than asserted. The dispatch circuit breaker is unaffected: it countserror_code == AUTH, gated on HTTP 503 on the sync path and mapped toerror_code=Nonefor 502 byresult_callback._STATUS_MAPon the async #1083 path.Recommended follow-up before merge (not filed — this is a request to the reviewer)
Anchor the auth indicators with word boundaries —
\b401\b/\b403\b— in:src/backend/services/failure_classifier.py:9(AUTH_INDICATORS), and its byte-identical vendored mirrorsrc/scheduler/failure_classifier.py, which is parity-tested bytests/unit/test_904_sigkill_no_false_auth.py::TestBackendSchedulerParity— both copies must change together or the parity test reds.task_execution_service.py:1397and:1695.This was deliberately sequenced after this PR, not skipped: it changes auth classification on every path in the fleet, a strictly larger blast radius than this P1 parser fix, and hardening first would serialize the whole batch behind a PR that is not one of the claimed issues. Shipping this as a draft means the reviewer sees the recommendation before anything merges.
Verification
Tests added:
tests/unit/test_1849_ede_diagnostic_filtered.py— 19 test functions / 23 collected cases.The acceptance test drives a marker-first
errors[]through the real parser and_finalize_headless_resultinto the realrouters/sessions.py::_is_resume_not_found(loaded viaspec_from_file_location, notfrom routers.sessions import …, which raisesImportErrorunder CI's randomized seeds). A unit test assertingmetadata.error_message == Xwould have stayed green through both #1673 and #1849 — that is why the pin spans the whole chain.TDD red is behavioural, not an ImportError: pre-fix → 18 failed, 5 passed, failing on
_is_resume_not_found(detail) is False,KeyError: 0,TypeError,assert 'b' == 'boom'. Re-proven at review by revertingstream_parser.pytoorigin/dev(18 failed / 5 passed), then restoring clean.Run in this ship stage (affected-file neighbourhood only — the full suite is cited, not re-run):
Full suite (WAVE-4 verify-local, cited not re-run): unit
1 failed, 6054 passed, 18 skipped, 1 xfailed. Docker stages all passed: build + import-smoke, boot + health, integration70 passed.The single red is
tests/unit/test_1069_voip_call_path_param.py::TestVoipCallPathParam::test_flat_path_params_are_agent_name_not_nameand it is proven pre-existing, not assumed: a pristinegit archive origin/dev @ 8e924526was extracted into a temp tree and run with the same venv via the same harness invocation → identicalImportError: cannot import name 'get_flat_dependant'. The file is byte-identical across dev and this branch. Root cause is repo-level and independent of this PR:fastapi>=0.115.0is a floor, not a pin, so any fresh venv now resolves 0.141.1 whereget_flat_dependantno longer exists. This will red for every contributor building a clean env and deserves its own issue — please do not attribute it here.Verification honesty — what was NOT proven
verify-localran with--skip-agentfor a host-environment reason, not because the agent stage passed. The operator's live dev stack owns the globaltrinity-agent-network, andorigin/devis in global mode, so the agent precheck hard-refuses.--skip-agentremoves exactly stage 3 (import agent_serverinside the built base image) and stage 5 (real agent/health) — the two stages that would cover this PR's only product change.Compensations, verified rather than asserted:
cd docker/base-image && python3 -c "import agent_server.services.stream_parser"→ OK.Mon an existing file, with no new imports and no new module underdocker/base-image/.Residual: no real-agent runtime coverage. Of the PRs in this batch, this is the one that most deserves a real-agent smoke before merge.
docs/memory/learnings.mdwill conflict with the PR for #1831. Both patch the identical hunk@@ -230,3 +230,7 @@of a 232-line file with the same three trailing context lines — both append a new## 2026-08-01 — pitfall — …block at EOF. Git's 3-way merge cannot auto-resolve two insertions at one anchor.Merge this PR first (it is the P1); #1831 then rebases and keeps both blocks. Resolution is mechanical — keep both appended sections, either order — but must be done by hand. Do not let auto-merge attempt this file.
Known residuals — pinned and deliberate
headless_executorapplieserr[:300]after the join, so a long first error can push a later resume marker past the budget. Pinned bytest_multi_error_truncation_limit_is_known.test_multi_error_truncation_limit_is_known/ T17 must be flipped deliberately — its failure message says outright that a failure means truncation was FIXED, and an addedassert _CLAUDE_MSG not in detailstops the pin passing vacuously._entry_text's dict unwrap only partially defends against a hypothetical nested marker shape. Guessing at a non-existent API is worse than an honest gap.Uncommitted by design
.claude/agents/test-runner.md(the/update-testscatalog) is intentionally not updated —.claudeis a private submodule, so committing it would put a gitlink to an unpushed commit into this PR. The in-repo indextests/registry.jsonis updated. Suggested catalog row for whoever lands it intrinity-dev:Follow-ups — listed here, deliberately not filed
\b401\bword-boundary anchoring inAUTH_INDICATORS(details above). Pre-merge recommendation.ExecutionMetadata.error_codeto retire substring matching. The field already exists and its own comment says it awaits exactly this. This cross-surface contract has now broken twice in six weeks with green CI (bug: agent-server masks error_during_execution as a successfulcontext: forkplaceholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673, bug: #1673's errors[0] picks Claude Code's internal [ede_diagnostic] header — real error text dropped, resume-not-found self-healing defeated #1849) — the strongest argument in this PR for doing it.err[:300]budget inheadless_executor(or truncate per-error before the join).max_turns422 detail has no length cap at all — belongs inheadless_executor.py, out of this fence.errors[]with an emptyresult_textmisroutes to 502, so neither the 429 path nor SUB-003'srate_limitswitch fires.terminal_reason="empty_result".anthropics/claude-code#82235.Out of scope
#1870 and #1853 are the same error-handling family, but their files overlap open PR #1718 (
headless_executor.py/claude_code.py/codex_runtime.py/subprocess_*). This branch touches neither —stream_parser.pyis not in #1718's diff, so there is zero conflict surface here.Files changed (6)
docker/base-image/agent_server/services/stream_parser.pyerrors[0]readstests/unit/test_1849_ede_diagnostic_filtered.pytests/registry.jsondocs/memory/feature-flows/parallel-headless-execution.md:363-372snippet, which quoted pre-#1673 code and misattributed it toclaude_code.pydocs/memory/feature-flows/session-tab.mddocs/memory/learnings.mdPer CLAUDE.md Rule #4 a bug fix needs a descriptive commit message only;
architecture.mdandrequirements/*were checked and mention neitherresult.errorsparsing norstream_parser, so neither needed an edit.🤖 Generated with Claude Code