refactor(adapter): record the bare-line trust as structural, and name the edits that void it (BLO-31955) - #1662
Open
allyblockcast[bot] wants to merge 4 commits into
Open
Conversation
…d allowlist
`isClaudeSkillNotFoundStartupFailure` guarded its raw-transcript scan with a
blocklist of conversation event types (`assistant|user`). `parseClaudeStreamJson`
branches on exactly three types and ignores every other one, so each newly
appearing event shape slipped that blocklist by default -- which is how the
guard had to be widened twice for one cause, `assistant` then `user`.
Inverted to an allowlist of harness-authored types, so an unrecognised type
fails closed. Membership criterion is stated: the payload must be entirely
harness-authored scalars. `{system, rate_limit_event}` qualify; `result` is
deliberately excluded because a *truncated* one can reach the scan carrying the
model's final message.
The hazard was one config edit from live, not hypothetical. `job-manifest.ts`
appends `config.extraArgs` to the CLI argv verbatim (`:1256`, sourced at
`:1125` from an agent's `adapterConfig`), so `--include-partial-messages` on any
single agent re-opens the guard with no code change, no diff and no review --
and the failure is permanent retry suppression (`skill_not_found` is in
`NON_RETRYABLE_CONTINUATION_ERROR_CODES` and excluded from the zero-token
reset), not a visible error. Measured on the CLI this adapter runs (v2.1.210):
that flag emits 9 `stream_event`s for a two-word prompt, each wrapping model
prose in `event.delta.text_delta`.
Detection is unchanged. The four existing guard cases pass unmodified, plus new
cases for a production-shaped `init` line, `system:status` (which v2.1.210
emits pre-turn), and `rate_limit_event` (the FAR-32 repro in execute.test.ts).
Verified as a negative control: the new `stream_event` case fails against the
previous blocklist while all 12 detection-preserving cases pass, so it
discriminates the fix rather than merely passing alongside it.
Per-line scoping reads only the first `"type"` per line so a nested type cannot
veto its own line. Measured as defence-in-depth rather than a live fix: a real
1717-byte `init` line carries exactly ONE `"type"` (its `mcp_servers` entries
are `{name, status}`, `output_style` a bare string), so a whole-transcript
assertion would pass today too. What per-line buys is removing the dependency
on that staying true, and keeping the `rate_limit_event` detection.
Also repairs two provenance obligations #1525 left unmet: it updated the
integrity hash but added no Local-modifications row and did not bump
`-blockcast.N`. Adds rows for both #1525 and this change, bumps to
`0.2.6-blockcast.2`, and recomputes the hash.
Refs BLO-31794. Follow-up to #1525 (BLO-7991 AC3).
Co-Authored-By: Claude <noreply@anthropic.com>
Ally review follow-up on #1650. `system` is a multiplexer, so admitting the type wholesale reproduced the defect this PR fixes one level down: a new subtype admitted by default, exactly as a new top-level type was admitted by the old blocklist. The membership criterion is stated per payload, so it has to be applied per (type, subtype) wherever a type demultiplexes. Measured against the v2.1.210 binary the adapter runs, `system` carries at least `init`, `status`, `compact_boundary`, `hook_response` and `mcp_status`. Only the first two are admitted. `hook_response` is why this is a live hole rather than future-proofing: the binary builds it as `{type:"system",subtype:"hook_response",...,output,stdout,stderr}`, embedding a hook process's raw stdout — operator-configured, not harness-authored, and reachable through `--settings` via the same `config.extraArgs` channel that motivated inverting this guard at all. (`hook_error`, named in the review, is in no v2.1.210 string table and is not a subtype at this version.) A `system` line with no readable subtype fails closed. Both new cases were run as negative controls and fail without the gate while the other 78 pass. The hook fixture is single-quoted deliberately: JSON.stringify escapes `"` to `\"` and the phrase regex does not match across the backslash, so a double-quoted fixture would have passed vacuously and proved nothing. Also from the review: - Run the phrase test before the transcript walk. Semantically identical (both predicates pure, neither regex `/g`), but it skips an eager split of the entire pod log on the common phrase-absent failure. - PROVENANCE.md: delete the blank line that terminated the GFM table, so the #1525 and BLO-31794 rows render as rows rather than literal text. - parse.test.ts: drop the claim that a hardcoded fixture detects CLI drift, and condition `system:status` on `--include-partial-messages`. Integrity hash recomputed. Refs BLO-31794. Co-Authored-By: Claude <noreply@anthropic.com>
…ranscript The subtype gate in f22f4d1 was right to exclude `system:hook_response`, but pairing it with a whole-transcript veto silently disabled detection on the large majority of production runs. Paperclip provisions a SessionStart hook itself, so `hook_started` / `hook_response` open the transcript BEFORE `init`. Measured on this instance's pod logs: 6510 of 8036 `init`-carrying logs contain `hook_started` (81%), and in a 399-log sample carrying both, the hook line preceded `init` 399/399 times. A veto requiring EVERY line to be harness-authored therefore returned false on all of them — no raw scan, no `skill_not_found` — while the suite stayed green, because its only positive fixture was a synthetic two-line shape no production run has. That is exactly the "fix the false positive by disabling detection entirely" failure mode BLO-31794's acceptance criteria warn about. Attribute the phrase to its line instead: it counts only when it sits on a line the harness authored (an allowlisted event, or a bare non-event line, which in stream-json mode is the CLI speaking outside the protocol — 0 of 6893 sampled production lines are bare). Every false positive in this family is the phrase INSIDE an event payload, so this is the more faithful invariant, and unknown types still fail closed. The classification logic is unchanged — lifted verbatim into `claudeLineIsHarnessAuthored`. Only the quantifier moved, from "every line is harness-authored" to "the phrase sits on a harness-authored line". So `hook_response` stays excluded (adding it, or `hook_started`, would admit operator text) and detection survives the preamble regardless. Corrects three claims the review found inaccurate: the subtype enumeration omitted `hook_started` (a sixth, and the one that trips first); PROVENANCE.md said "Detection is unchanged" when on the production shape it was not; and the loss was described as forward-looking when it was live and fleet-wide. One deliberate narrowing, recorded in source: the phrase regex's `\s+` matches a newline, so a phrase straddling two lines no longer matches. The CLI emits it on one line, and the direction is safe — a lost classification degrades to the retryable `buildPartialRunError`, whereas a false positive is permanent. Negative control: the four new detection cases FAIL against the whole- transcript veto while every false-positive case still passes, so they discriminate the fix rather than passing beside it. 828/828 green (was 824). Refs BLO-31794. PROVENANCE integrity hash recomputed. Co-Authored-By: Claude <noreply@anthropic.com>
Author
Author
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
allyblockcast
Bot
changed the base branch from
fix/blo-31794-harness-authored-allowlist
to
master
September 5, 2026 07:34
… what voids it Comment-only; no behavioural change. Titled `refactor` rather than `docs` because the change lands in a source file and the repository's review gate suppresses the source-code gates (including the vendored-adapter check) on a `docs:` prefix — and that check is this change's verifying signal. `claudeLineIsHarnessAuthored` trusts a line carrying no `"type"` as harness-authored. That is load-bearing — it is what lets the CLI's own bare `Error: Skill "<name>" not found` be detected at all — but the justification rested on the CLI's protocol discipline, corroborated by a 6893-line sample. The property is in fact structural. The surface the predicate reads has exactly one writer: the `tee` in the pipeline job-manifest.ts builds at :1725, which carries no `2>&1` on any stage. So the file receives Claude's stdout and nothing else; hook stderr, MCP-server stderr and the fail-fast `[wrapper]` line (written to /dev/stderr at :1597, and downstream of the `tee` regardless) bypass it by construction. Operator- and MCP-authored text cannot reach the predicate as a bare line at all. Verified end to end rather than taken from review: `stdout` is assigned only from podLogPath (execute.ts:1941 tail via fs.open, :1954 on-disk), and the one reader of the *merged* container-log stream (readPodContainerLogTail, :646, via readNamespacedPodLog) is confined to diagnostics and never reaches the parse surface. That yields a second invalidating edit the review did not name. Both are now recorded at the call site, because every prior iteration in this family (BLO-7991 -> #1525 -> BLO-31794) was an invisible widening: a change with no diff on the guard itself. Naming them converts a fifth into a reviewable diff. Also corrects two comments describing `stdout` as "the entire pod log" — it is the pod log's stdout stream, and the overstatement is what made the bare-line trust read as riskier than it is. No predicate, regex, allowlist or test expectation changed; parse.test.ts is untouched. PROVENANCE.md integrity hash recomputed and its BLO-31794 row reconciled with the structural claim. BLO-31955
allyblockcast
Bot
force-pushed
the
fix/blo-31955-structural-invariant
branch
from
September 5, 2026 07:36
a140fc8 to
d101499
Compare
allyblockcast
Bot
changed the title
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d101499
The substance of this change verifies. I checked all four structural citations against the tree at this head, and every one is exact:
| Claim | Verified |
|---|---|
job-manifest.ts:1725 pipeline, no 2>&1 on any stage |
✅ cat /tmp/prompt/prompt.txt | claude … | tee <podLogPath> | <failFastFilter> > /dev/null |
job-manifest.ts:1597 [wrapper] → /dev/stderr, downstream of tee |
✅ inside failFastFilter, > "/dev/stderr" |
execute.ts:646 readPodContainerLogTail confined to diagnostics |
✅ one caller (:2014) → containerLogTail → appendPodCause/buildPartialRunError message only; never stdout |
stdout assigned only from podLogPath (:1941 tail, :1954 on-disk) |
✅ those are the only two assignments besides let stdout = "" at :1780 |
Single-writer also holds: podLogPath appears in job-manifest.ts only at :1724 (mkdir) and :1725 (the tee). So the "structural, not a sample" claim is earned, not asserted.
I also verified the row's own self-description. Against current master (d81c5f499), parse.test.ts and package.json are byte-identical, and the parse.ts delta is purely comment text. The "comment-only: no predicate, regex, allowlist or test expectation changed, and parse.test.ts is untouched" claim is true.
Critical Issues (1)
- [gstack/review]
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:169— This PR cannot be merged at this head:mergeable_state: dirty, 38 commits behindmaster. #1650 was squash-merged into master asd81c5f499at 08:24:59Z, so master already carries this branch's first three commits by content under a different SHA — including the allowlist, the subtype gate, and0.2.6-blockcast.2. The branch's own-blockcast.1 → .2bump and the same PROVENANCE table rows therefore now collide with master's copy of them. Polled twice, 3s apart, to rule out a coldunknown.- Rebase onto
master. The real increment surviving that rebase is small and already correct: the 28-line structural-trust block inparse.ts, twoentire pod log→pod log's stdout streamwording fixes, and the BLO-31955 text on theBLO-31794row. Drop the version andpackage-lockhunks — master is already at.2. - CI is also red at this head and shares the same remedy.
General tests (server 4/4)fails on 4 tests acrosssrc/__tests__/docker-opencode-runtime-pin.test.ts("slow image pull outside the short capability budget", "force-removes the probe container after a timeout") andsrc/__tests__/graceful-shutdown-exit.test.ts("flushes the final breadcrumb through pressured piped stderr", "flushes a fatal record when the crash starts after the final shutdown breadcrumb");verifyis only the roll-up ("Fail if any split verify lane failed"). Not attributable to this diff — both are timing/environment-sensitive@paperclipai/serversuites, they touch no adapter file, the vendored-adapter lane is green, and the functionally identical code was green on1c092cfe. But under the standing CI-gate rule the PR must not land until the gate issuccessat its own head, so re-run after the rebase rather than merging past it.
- Rebase onto
Important Issues (1)
- [native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/parse.ts:433— The justification for admittingsystem:statusis stale under the attribution model, and the hole it papers over is a false positive on a retry-killing code. The comment reasons:statusmay carrycompact_result/compact_error"derived from model output", but "compaction cannot occur before the first turn, so any transcript reaching it also contains anassistantline, which this guard rejects independently." That was true under the round-2 whole-transcript veto. Round 3 replaced it with per-line attribution, so anassistantline elsewhere in the transcript now rejects nothing. The only surviving whole-transcript rejection isinput.assistantContentSeenat:668— precisely the signal BLO-7991 established as unreliable, and whose three failure shapes (truncatedassistant, completeassistantwith no usage, pre-assistantuser) are why this raw guard exists at all.- Concretely: a
system:statusline whosecompact_resultquotes the trigger phrase is now attributed to the harness and classifies asskill_not_found— which is inNON_RETRYABLE_CONTINUATION_ERROR_CODESand excluded from the zero-token reset, so the cost is permanent retry suppression, the exact asymmetry the surrounding comments are careful about everywhere else. - This is the one place in the file where a round-2 argument was carried into round 3 without being re-derived, so it fits the PR's own stated purpose rather than sitting outside it. Cheapest fix consistent with the design: gate
statuson the absence ofcompact_result/compact_erroron the line (same shape as the existing subtype read), or dropstatusfrom the subtype allowlist and keep theinit → status → deathcase working via the bare error line. Either way the comment needs to stop citing a rejection that no longer happens.
- Concretely: a
Suggestions (2)
- [pr-review-toolkit]
vendor/paperclip-adapter-claude-k8s/src/server/parse.ts:507— The stated goal of naming the two voiding edits is to "convert a fifth one into a reviewable diff". A comment names them; it does not force anything, and both edits land in a different file where nobody is reading this block. Voiding edit #1 is directly assertable — ajob-manifest.test.tscase that builds the manifest and asserts theclaudeInvocationstring contains no2>&1would redden the lane in the PR that introduces it, naming the invariant. That is the same forcing-function pattern this tree already uses forENV_NAME_CLASSIFICATION(BLO-29804), so it is idiomatic here rather than novel. - [pr-review-toolkit]
vendor/paperclip-adapter-claude-k8s/src/server/parse.ts:495— All five line-number citations are correct today (I verified each), but line numbers are the most perishable form of reference and this block is explicitly written to outlive future edits. Every citation does carry a symbol or literal anchor alongside it (failFastFilter,[wrapper],readPodContainerLogTail, the quoted pipeline), so a reader can re-locate — worth keeping that discipline if the block is edited again, since the anchors are what will still work in six months.
Strengths
- Replacing a 6893-line empirical sample with a single-writer argument over the actual pipeline is a real strengthening, not a restatement — a sample can only ever bound what was observed, whereas "the
teehas no2>&1" bounds what is possible. Demoting the sample to corroboration rather than deleting it is the right call. - The self-description is accurate and checkable, which is rarer than it should be. "Comment-only,
parse.test.tsuntouched" verified byte-identical against master. - Naming the two edits that would void the invariant, and noting that neither shows a diff at this call site, correctly identifies the actual failure mode of this family (BLO-7991 → #1525 → BLO-31794): every prior break was an invisible widening from elsewhere.
- The
hook_responsereasoning is grounded in a real pod log carrying an nginx 503 HTML page inside asystemevent, andhook_erroris explicitly recorded as not a v2.1.210 subtype rather than being defensively enumerated. Both are the honest form.
Recommended Action
- Rebase onto
master(d81c5f499) — the branch is unmergeable and its first three commits are already landed by content. Drop the version/lockfile hunks. - Re-derive the
system:statusjustification atparse.ts:433against the attribution model, and close or re-argue thecompact_resultpath. - Re-run CI on the rebased head; do not merge past the red gate even though the four failures are unrelated to this diff.
- Consider the
2>&1assertion injob-manifest.test.tsopportunistically.
13 tasks
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
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.
Thinking Path
Linked Issues or Issue Description
Follow-up from Ally's review of #1650 (Suggestions 1 and 2), both rated non-blocking and deliberately not folded into that PR.
What Changed
Comment-only. Zero behavioural change. Titled
refactorrather thandocsbecause the change lands in a source file, and the review gate suppresses source-code gates — includingVendored claude_k8s adapter, this change's verifying signal — on adocs:prefix.parse.ts— theclaudeLineIsHarnessAuthoreddoc comment now states the bare-line trust as structural, citingjob-manifest.ts:1725, and names the two edits that would void it.parse.ts:557,:646— corrected "the entire pod log" to "the pod log's stdout stream". The overstatement is what made the bare-line trust read as riskier than it is.PROVENANCE.md— integrity hash recomputed (8f77b1f5…→00a123ab…); theBLO-31794row's empirical phrasing reconciled with the structural claim as a third review round.The claim, verified rather than transcribed
podLogPathteeatjob-manifest.ts:17252>&1on that pipeline2>&1in the file are>/dev/null 2>&1onccrotate/git plumbing (:1581,:1660,:1700)[wrapper]line/dev/stderr(:1597) and downstream of theteeregardlessstdouthas 3 assignments (execute.ts:1780/1941/1954); both real sources readpodLogPath, viafs.openreadPodContainerLogTail(execute.ts:646,readNamespacedPodLog) — container logs do interleave both streams, but it feeds diagnostics only and neverstdoutSo the file receives Claude's stdout and nothing else. Hook stderr, MCP-server stderr, the
[wrapper]line and the prompt all bypass it by construction — operator- and MCP-authored text cannot reach the predicate as a bare line at all. Strictly stronger than a sample; the sample is retained as corroboration, not as the basis.A second invalidating edit, beyond the review. The issue names one — adding
2>&1before thetee. Check 5 surfaces another of the same shape: routing a merged container-log read into the parse surface voids the invariant identically. Both are now named at the call site.Verification
Vendored claude_k8s adaptergate, run locally invendor/paperclip-adapter-claude-k8s:npm test→ 828 tests / 14 files passnpx tsc --noEmit→ cleanpr.yml:900→actual == recordedparse.test.tsis untouched, so the suite is a genuine negative control: any predicate change surfaces as a failure rather than as a silently-adjusted expectation.Risks
Low. Comments and one hash line; no executable change.
The one real risk is merge sequencing, and it is not low — see below.
claudeLineIsHarnessAuthoreddoes not exist onmaster; fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650 introduces it. This branch is based on fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650 at1c092cf, so the diff currently shows fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650's three commits plus mine, and merging it would land fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650's work under this PR's review. It should land on its own merits.pr.ymltriggers only onpull_request.branches: [master]— a stacked PR gets no CI matrix at all, including the adapter gate that is this change's verifying signal. Retargeted tomasterfor that reason; the noisier diff is the deliberate trade.delete_branch_on_merge: falseon this repo, so nothing auto-retargets. After fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650 merges:git rebase origin/master(drops fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650's commits by patch-id, leaving one), force-push, and re-verify the provenance hash — a rebase changes it if fix(adapter): invert the skill-guard event check to a harness-authored allowlist #1650's final state differs from1c092cf.Model Used
Claude Opus 4.5 (
claude-opus-4-5), 1M context, extended thinking, with tool use (repository inspection, local test execution, GitHub API).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateparse.test.tsis untouched on purpose so it acts as the negative control for the zero-behavioural-change claim; adding tests to a comment-only diff would weaken that signal.