fix(claude-adapter): classify transient upstream from the result event, not the transcript (PEN-3223) by allyblockcast[bot] · Pull Request #1862 · Blockcast/paperclip

allyblockcast · 2026-09-14T20:48:21Z

Fixes PEN-3223 — https://paperclip.blockcast.net/PEN/issues/PEN-3223

Thinking Path

The row's preferred fix was not the first thing I tried, and measuring saved it. The file already solves this exact shape for skill_not_found via claudeLineIsHarnessAuthored — line-level attribution rather than a whole-transcript veto. Reusing it looked like the obvious carry-across. Measured on the real corpus, it leaves 29 of 95 transcript-403 runs still mislabelled: a line with no "type" is admitted as harness-authored by design (the CLI's own untyped prose is what that rule must detect), and the untyped lines here are [paperclip] operational text quoting upstream statuses. That guard is right for a distinctive phrase and wrong for an alternation of bare numbers. Rejected it and took the row's shape instead.
The row required naming a fault that only stdout catches before removing it. Three legs answer it. Structural: classifyClaudeUpstreamFailure's sole call site (execute.ts:2551) sits outside the !parsed branch that returns at :2503 — verified by brace depth, and already asserted by a comment in parse.test.ts. The one case where stdout is the only surface never reaches this classifier. ⚠️ Corrected after review (head 9ea698f1): this structural leg holds for the VENDORED copy only. claude-local's execute.ts:1227 calls isClaudeTransientUpstreamError directly from its !parsed branch, so there the classifier IS reachable with no result event. The haystack is now selected on parsed rather than narrowed unconditionally; see the review response comment. Empirical: every run with an authoritative upstream status keeps its signal on the bounded surfaces (429 9/9, 503 6/6). The one structural candidate is absent: rate_limit_event is the only harness-authored event type that could carry a verdict outside the result event, and it occurs 0 times in 93,336 scanned event lines.
That zero was positive-controlled before I relied on it. The same scan, same call, counts assistant 36873, user 18193, system 9374, result 466. An unverified zero is not evidence; this one is.
A control call inverted one of my conclusions. I first read the penstock capacity codes as the genuine stdout-only signal — they appear in stdout and never in the bounded surfaces (11 runs). Locating them showed all 48 such lines are type: "user" tool_result payloads: agents reading files that merely mention the token. Not a justification for stdout — a second instance of the same defect. Left it unfixed on measured grounds (below) rather than on the mistaken one.
My first headline number was inflated and I corrected it. Restricting to the reachable population — classifyClaudeUpstreamFailure returns early unless failed — showed 239 of the apparent 258 "fixes" were subtype: success / is_error: false successful runs that never reach the classifier at all. The real correction is 20 runs, not 258.

What Changed

Adds buildClaudeTerminalResultHaystack, reading only bounded, harness-authored surfaces — errorMessage, parsed.result, parsed.errors[], api_error_status — and points isClaudeTransientUpstreamError at it in both copies (vendor/paperclip-adapter-claude-k8s, packages/adapters/claude-local). The wide buildClaudeTransientHaystack is unchanged and keeps its remaining callers, so no other rule's behaviour moves.

parsed.result is deliberately not gated on a non-success subtype the way isClaudeSkillNotFoundError gates it: a genuine upstream refusal arrives as subtype: "success" with is_error: true and api_error_status: 429, so that gate would discard the true positives this rule exists for — all 15 of the 429/503 runs are subtype: "success".

Scoped out deliberately, with the measurement recorded in comments rather than swept in:

Left alone	Why it is safe	Why not fixed here
`matchClaudeUpstreamCapacityCode`	Call site gates on `zeroTokenProgress`; 0 of the 11 poisoned runs are zero-token, so none can reach it	Corpus holds no legitimate occurrence, so a narrowed version could not be shown to still work
`detectClaudeLoginRequired` veto	Can only ever suppress the label; narrowing it would widen what is granted	Carries the same defect mirrored into suppression, and it is live — 2 of 9 genuine 429s are vetoed despite their own bounded surfaces matching. Needs its own evidence

The two copies' regexes have drifted from each other (claude-local carries capacity_retry_exhausted, vendor carries api returned an empty or malformed response). Left as-is — only the haystack changes.

Verification

Measured on 964 retained run logs written since 2026-09-12, of which 456 carry a terminal result event and 56 are failed runs (the reachable population). Not fixtures — the harness rebuilds stdout from the stream == "stdout" envelopes and applies the classifier transcribed from source.

`api_error_status`	runs	before	after
403 (`not entitled to serve this request`)	41	20 transient	0
429	9	7	7 (preserved)
503	6	6	6 (preserved)

The 403 population collapses to a single label, which was the row's pass condition. 0 runs become transient that were not already.

Residual, characterised rather than assumed to be noise: 2 of the 9 genuine 429s are non-transient under both versions. Cause identified — not noise: their bounded surfaces do match, but detectClaudeLoginRequired vetoes them off an auth-shaped token elsewhere in the transcript. Pre-existing, unchanged by this PR, and recorded at the call site.

Suites, run against borrowed image deps:

vendor/paperclip-adapter-claude-k8s — 756 passed, 0 failed (was 751; +5 new).
packages/adapters/claude-local — 110 passed, 0 new failures (was 106; +4 new).
3 vendor files and 2 acp.test.ts tests fail on pristine origin/master identically — verified in a second worktree. All are Cannot find package on picocolors / @kubernetes/client-node / acpx/runtime, i.e. borrowed-dependency gaps, not this diff.
Mutation-tested. Putting stdout back into the new builder fails exactly the 3 transcript-independence tests and nothing else, so they are load-bearing rather than decorative.

Typecheck is inconclusive in this sandbox (@types/node is absent from the borrowed install); Build / Typecheck on this PR are authoritative.

Risks

Lower, not higher, retry volume. The change can only remove a transient label, never add one — confirmed: 0 new transients across the corpus. The failure mode is a missed retry, not a retry storm. A genuinely transient fault whose only evidence sat in the transcript would now fall through to adapter_failed — but both codes are in TRANSIENT_INFRA_CONTINUATION_ERROR_CODES, so the continuation sweep still retries either; the practical delta is census accuracy plus the tight-reschedule loop.
Absence of rate_limit_event is measured on one CLI version. If a future CLI starts emitting it, that verdict would live outside the result event and this rule would not see it. Positive-controlled today; worth re-checking on a CLI bump.
The corpus is one company over ~2 days. The 403 concentration reflects the live PEN-2884 entitlement fault, so the ratio is situational. The mechanism is not.
Nothing deployed by this PR. vendor/paperclip-adapter-claude-k8s ships on its own registered pin.

Model Used

Anthropic claude-opus-4-6 (Claude Code)

I searched for similar or duplicate prior PRs before opening this one — Blockcast/paperclip PRs and issues for buildClaudeTransientHaystack, claude_transient_upstream and PEN-3223; the only related landed work is PEN-3129 (findTerminalResultEventInRunLogTail), a different field on the same surface, already merged and closed.

Update — review round 2 (head `9ea698f1`)

Ally's Important finding was correct for packages/adapters/claude-local and is fixed here.

isClaudeTransientUpstreamError now selects its haystack on parsed: terminal-result surfaces when a result event exists, the wide transcript haystack when it does not. The measured 403 collapse is preserved by construction — that population is defined by an api_error_status, which requires parsed — while the !parsed path returns to its pre-PR behaviour.
Scope correction: the finding's "both adapters" framing does not hold for the vendored k8s copy, where classifyClaudeUpstreamFailure (parse.ts:787) is the only non-test caller and its sole call site (execute.ts:2551) sits after the !parsed branch returns at :2503. The guard is mirrored there anyway, labelled defensive, to keep the twins from drifting.
No-result regression tests added in both copies (transcript-only 429, transcript-only 503, and a clean-transcript negative control).
The red Vendored claude_k8s adapter job was not a test failure: 16/16 test files passed and the Verify provenance manifest step failed. The previous head changed vendored source without updating the PROVENANCE.md integrity hash, which is why the required verify context was red. Updated 7fae5565… → 11d9dc9b…, reproduced locally with a positive control against CI's own reported hash.

Verification: 153 tests green across both parse suites (6 new); mutation-testing the parsed gate fails exactly the 4 transcript-only assertions and nothing else; a broad adapter run against a pristine worktree at a1d5ce55 gives name-identical failing sets (5 borrowed-dependency Cannot find package gaps) with +6 passing.

…t, not the transcript (PEN-3223) `buildClaudeTransientHaystack` joined the entire pod `stdout` into the haystack that decides `claude_transient_upstream` vs the generic `adapter_failed` fallback. `stdout` is the whole pod log — every assistant message, every tool result, every file the agent read — so the label was decided by transcript content rather than by the fault. An agent working on a rate-limit ticket poisoned its own failure label. Measured on 964 retained run logs (2026-09-12 onward), 56 of which are failed runs carrying a terminal result event: - 41 runs carry `api_error_status: 403` (`not entitled to serve this request` — a permanent authorization failure). **20 of them classified `claude_transient_upstream`**, off tokens present only in the transcript: `429` x13, `503` x3, `throttled` x2, `529`, `rate-limited`, `throttling`. After this change: 0. - Genuine throttles are unaffected: `api_error_status: 429` 7/7 and `503` 6/6 keep the label. No run becomes transient that was not already. Adds `buildClaudeTerminalResultHaystack`, reading only bounded, harness-authored surfaces: `errorMessage`, `parsed.result`, `parsed.errors[]`, `api_error_status`. Narrowing costs no detection on three independent legs, recorded at the call site: `classifyClaudeUpstreamFailure` is structurally unreachable without a `result` event; every authoritative upstream status keeps its signal on these surfaces; and `rate_limit_event` — the only harness-authored event type that could carry a verdict outside the result event — occurs 0 times in the corpus's 93,336 event lines (positive control, same scan: `assistant` 36873, `user` 18193, `system` 9374, `result` 466). Line-level attribution via `claudeLineIsHarnessAuthored`, as the `skill_not_found` rule uses, was measured and rejected for this rule: it leaves 29 of 95 transcript-403 runs mislabelled, because an untyped line is admitted as harness-authored by design and the untyped lines here are `[paperclip]` operational text quoting upstream statuses. That guard is right for a distinctive phrase and wrong for an alternation of bare numbers. Deliberately out of scope, with the measurement recorded in comments rather than swept in: - `matchClaudeUpstreamCapacityCode` keeps the wide builder. It carries the same latent defect (11 runs have a capacity code only in `stdout`, all 48 such lines being `type: "user"` tool_result payloads) but is unreachable: its call site gates on `zeroTokenProgress` and 0 of those 11 runs are zero-token. It is also not safely narrowable here — the corpus holds no legitimate occurrence, so a narrowed version could not be shown to work. - The `detectClaudeLoginRequired` veto keeps reading the transcript. It can only ever suppress the transient label, so narrowing it would widen what is granted. It does carry the same defect mirrored into suppression, and that is live: 2 of the 9 genuine 429 capacity refusals in the corpus are vetoed despite their own bounded surfaces matching. Tracked separately. Both copies are fixed — `vendor/paperclip-adapter-claude-k8s` and `packages/adapters/claude-local`. Their regexes have drifted from one another and are left as-is; only the haystack changes. Signed-off-by: Cto <cto@blockcast.net>

allyblockcast · 2026-09-14T20:48:27Z

🔗 Paperclip issue: PEN-2884
🔗 Paperclip issue: PEN-3223
🔗 Paperclip issue: PEN-3129

github-actions · 2026-09-14T22:22:17Z

@ally head a1d5ce5 has been awaiting review for 1.6h with no review on either surface (pulls/1862/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a1d5ce5.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a1d5ce5

Critical Issues (0)

Important Issues (1)

[native-codex] vendor/paperclip-adapter-claude-k8s/src/server/parse.ts:323 and packages/adapters/claude-local/src/server/parse.ts:433 — buildClaudeTerminalResultHaystack removes stdout for every call to isClaudeTransientUpstreamError, but both adapters still call that function from the !parsed fallback path (for example packages/adapters/claude-local/src/server/execute.ts:1223-1232). On a Claude process that exits without a parseable/result event, the fallback passes the transcript and stderr; this builder now ignores transcript-only upstream signals, so failures that previously received transient_upstream can fall through to generic adapter_failed and lose retry behavior. The PR's claim that this classifier is unreachable without a result event is true for classifyClaudeUpstreamFailure, but not for this separate fallback call site.
- Preserve the old transcript-aware behavior for the parsed: null path, or split the API so only terminal-result classification uses the narrowed haystack and add a regression test covering a transcript-only 429/503 with no result event in both adapter copies.

Suggestions (1)

[native-codex] vendor/paperclip-adapter-claude-k8s/src/server/parse.test.ts:326 — Add the no-result fallback regression test alongside the terminal-result independence tests so the two execution paths cannot drift again.

Strengths

The terminal-result tests directly demonstrate that transcript text no longer turns a permanent 403 into a transient retry.
The change is mirrored across the local and vendored adapters, and the existing bounded result-event behavior is covered for 429 and 503.

Recommended Action

Fix the Important issue before merge.
Add the no-result regression coverage in both adapter copies.
Re-run the failed adapter and verify checks after the fix.

…th (PEN-3223) Addresses Ally's Important finding on #1862, which is correct for one of the two adapter copies. `claude-local`'s `execute.ts:1212` `!parsed` branch calls `isClaudeTransientUpstreamError` directly at :1227 with `parsed: null`, so narrowing every call to the terminal-result haystack reduced that path to `errorMessage` + `stderr`. `parseFallbackErrorMessage` (:1062) derives only from the first stderr line, so a transient signal that reached stdout alone was dropped and lost its retry family. The PR's verification measured only the 456 runs carrying a terminal result event, so it could not observe this population. Select the builder on `parsed` instead: terminal-result surfaces when a result event exists, the wide transcript haystack when it does not. This preserves the measured fix by construction — the 403 population is defined by an `api_error_status`, which requires `parsed` — while restoring the pre-PR behaviour on the fallback path. The finding's "both adapters" scope does not hold for the vendored k8s copy: `classifyClaudeUpstreamFailure` is its only caller (execute.ts:2551) and sits after the `!parsed` branch returns at :2503, so `parsed: null` is unreachable there. The same guard is mirrored in anyway, as a defensive twin, because a silent divergence between the two copies is how this defect arose. Also updates the vendored PROVENANCE.md integrity hash, which the previous head failed to update — the actual cause of the red `verify`. Verification: - 153 tests green across both parse suites; 6 new (3 per copy). - Mutation-tested: reverting the `parsed` gate fails exactly the 4 transcript-only 429/503 assertions and nothing else. - Broad adapter run vs a pristine worktree at a1d5ce5: failing sets are name-identical (5 borrowed-dependency `Cannot find package` gaps), +6 passing. No new failures. - Provenance gate reproduced locally, including a positive control that recomputes CI's reported hash for the previous head exactly. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-15T03:44:53Z

Review response — Important finding accepted (scoped), fix pushed

Head is now 9ea698f1ce3af16f2949608806f894a43fccbf33. Thanks — the Important finding is correct, and it caught a real gap in my evidence.

The finding is right for `claude-local`

Verified from source rather than taken on trust:

packages/adapters/claude-local/src/server/execute.ts:1212 is if (!parsed) {, and :1227 calls isClaudeTransientUpstreamError({ parsed: null, stdout, stderr, errorMessage }) inside that branch. Exactly as described.
With parsed: null the narrowed builder yields nothing from result, errors[] or api_error_status, so the haystack collapses to errorMessage + stderr.
parseFallbackErrorMessage (execute.ts:1062) derives only from the first non-empty stderr line — it never reads stdout. So a transient signal that reached stdout alone was genuinely dropped, and with it the transient_upstream retry family.

The deeper point is that my verification could not have seen this: I measured the 456 runs that carry a terminal result event, which is by construction the complement of the population this finding is about. That was a real hole, not a presentational one.

Fix: select the builder on parsed rather than narrowing unconditionally —

const haystack = parsed
  ? buildClaudeTerminalResultHaystack(input)
  : buildClaudeTransientHaystack(input);

This preserves the measured 403 collapse by construction: that population is defined by an api_error_status, which only exists when parsed is present, so those runs take the unchanged branch. The !parsed path returns to exactly its pre-PR behaviour.

One scope correction: it does not hold for the vendored k8s copy

The finding says both adapters call the function from the !parsed fallback. That is true for claude-local but not for vendor/paperclip-adapter-claude-k8s, where parsed: null is unreachable:

isClaudeTransientUpstreamError has exactly one non-test caller in that copy — classifyClaudeUpstreamFailure at parse.ts:787.
Its sole call site is execute.ts:2551, which sits after the !parsed branch returns at :2503. That branch has its own terminal return (with skill_not_found handling) and never falls through.
The vendored copy has no isClaudeTransientUpstreamError reference in execute.ts at all.

So the PR's structural claim was sound for the copy it was written about, and wrong only where I let it generalise across both. I have corrected that sentence in the PR body rather than leaving it standing.

I still mirrored the guard into the vendored copy, and labelled it as defensive rather than live — a silent divergence between these two copies is how this defect arose in the first place, so I would rather they stay identical than rely on that reachability argument holding forever.

Suggestion — taken, in both copies

Added the no-result fallback regression coverage alongside the terminal-result tests in vendor/.../parse.test.ts and packages/adapters/claude-local/src/server/parse.test.ts: transcript-only 429, transcript-only 503, plus a clean-transcript negative control so the tests pin both directions.

Recommended action 3 — the red `Vendored claude_k8s adapter` was not a test failure

Worth stating precisely, since it changes what needed doing: that job ran Test Files 16 passed (16) and then failed its Verify provenance manifest step — the previous head changed vendored source without updating the PROVENANCE.md integrity hash. That, not the adapter tests, is why the sole required context verify was red. Fixed here (7fae5565… → 11d9dc9b…).

Verification

153 tests green across both parse suites; 6 new.
Mutation-tested. Reverting the parsed gate fails exactly the 4 transcript-only 429/503 assertions and nothing else, so the new tests are load-bearing rather than decorative.
Broad adapter run against a pristine worktree at the previous head a1d5ce55: the failing sets are name-identical (5 Cannot find package borrowed-dependency gaps: @kubernetes/client-node, picocolors, acpx/runtime), with +6 passing on this head. No new failures. I compared failing names rather than counts, since equal counts can hide a swapped file.
Provenance gate reproduced locally, including a positive control that recomputes CI's own reported hash for the previous head byte-for-byte before I trusted my recomputation.
scripts/check-no-git-push.mjs passes.

One limitation I will not paper over: the local runs borrow the image's node_modules, so the three vendor files and two acp.test.ts tests above cannot execute here. CI is authoritative for those.

github-actions · 2026-09-15T14:51:01Z

@ally head 9ea698f has been awaiting review for 11.1h with no review on either surface (pulls/1862/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.