fix(claude-k8s): classify a mid-materialization skill fault instead of adapter_failed (BLO-32055) - #1669
Conversation
|
@ally please review at head 0180fd3 — BLO-32055. Two things worth your attention specifically:
Also please sanity-check that converting the two |
|
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 |
…f adapter_failed (BLO-32055) A run killed because a declared skill's runtime SKILL.md was missing reported `errorCode: adapter_failed` with both stdout and stderr null, so it read as an agent-pool/adapter fault and was invisible to skill-health sweeps. Localized the throw site the QA report could not find: `prompt-cache.ts hashPathContents` walks each skill tree to derive the prompt bundle cache key, and its `fs.readFile` was unguarded. That walk runs before the Claude CLI is spawned, which is why every BLO-7991 AC3 classifier in parse.ts missed it — all of them read Claude-CLI-authored surfaces, and here there is no transcript, no result event and no `parsed` to read. AC3 is not regressed; this is a second path into the same user-visible failure on a layer it never sees. The window itself comes from `company-skills.ts materializeRuntimeSkillFiles`, which refreshes a skill by rm -rf -> mkdir -> per-file writeFile. Not atomic, so the rolling sweep publishes a state where the directory exists and SKILL.md does not. The pre-fix test failure reproduces the production error string exactly. Classification is deliberately NOT routed to `skill_not_found`: that code is in NON_RETRYABLE_CONTINUATION_ERROR_CODES, and this condition is transient — the live instance's file appeared 43m36s later, so a retry would have succeeded. Suppressing retries permanently on a self-healing condition is the BLO-31794 over-suppression hazard. Instead: catalog row exists -> skill_materialization_pending (transient_infra) no catalog row -> skill_not_found (non-retryable, unchanged) `skill_materialization_pending` joins TRANSIENT_INFRA_CONTINUATION_ERROR_CODES, the set that already held `adapter_failed` — so naming the fault preserves retryability exactly rather than widening or narrowing it, and it stays eligible for the zero-token session reset. The discriminator is read from the source of truth, not from message text: `readCatalogBackedSkillKeys` derives it from the server-injected `paperclipRuntimeSkills`. That matters because `readPaperclipRuntimeSkillEntries` silently switches source — it falls back to the adapter's own bundled on-disk skills when the config carries none, and a file missing under that read-only image path is a packaging fault that retrying cannot fix. Nothing here scans transcript or model output, so the BLO-31794 false-positive hazard (a run that merely discusses a missing skill) cannot reach this branch. Also converted the two whole-module `prompt-cache.js` mocks to partial mocks via importOriginal, matching the server-utils mock in the same files: the old shape silently dropped every export the module gains later, and broke 23 unrelated tests when this change added one. Tests: adapter 835/835 + tsc clean; server recovery-classifiers 31/31 + tsc clean. The two new repro tests were confirmed discriminating by reverting each fix in turn. Co-Authored-By: Claude <noreply@anthropic.com>
0180fd3 to
de1d1c0
Compare
|
@ally please review at head de1d1c0 — BLO-32055. This supersedes my request at 0180fd3 (same change; I force-pushed a one-paragraph doc-comment correction after noticing the class JSDoc still claimed every desired skill is catalog-backed, which the same commit disproves via Three things worth your attention specifically:
Also please sanity-check that converting the two |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: de1d1c0
Answering your three questions up front, since two of them check out and the third has a caveat that matters more than the default you asked about.
1. The transient/permanent split is right, and you did not widen it. Verified against the classifier rather than the comment: classifyContinuationFailure consults NON_RETRYABLE_CONTINUATION_ERROR_CODES first (server/src/services/recovery/service.ts:1709) and only then TRANSIENT_INFRA_CONTINUATION_ERROR_CODES (:1712). skill_materialization_pending is absent from the former and present in the latter, so it resolves to the same CONTINUATION_RECOVERY_TRANSIENT_MAX_ATTEMPTS / CONTINUATION_RECOVERY_TRANSIENT_BASE_BACKOFF_MS as adapter_failed. Retryability is preserved exactly, not widened. Your ordering assumption holds.
2. The catalogBacked default is fine; the explicit path is where the bug is. input.catalogBackedSkillKeys?.has(entry.key) ?? true binds correctly — an explicit false from .has() survives, because ?? only fires on the optional chain's undefined. And prepareClaudePromptBundle has exactly one call site in the repo (execute.ts:1447), which does pass the set. I agree with the over-retry direction. But see Important 1: the set you pass can be missing a key it should contain, which produces the over-suppression outcome you were guarding against — through the explicit path, not the default.
3. The mock conversion did not weaken the 23 tests. prepareClaudePromptBundle is still fully replaced by mockPrepareBundle; the only newly-real exports are a pure config-reading function and an Error subclass, neither of which performs I/O, and prompt-cache.ts has no module-level side effects. The Object.assign(Object.create(null), original, …) idiom is byte-for-byte the one the server-utils mock directly below already uses, so "matching the server-utils mock below" is accurate. This was also the necessary fix, not just a stylistic one — the old whole-module mock made readCatalogBackedSkillKeys undefined at execute.ts:1423.
Critical Issues (1)
- [gstack/review]
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:99— the vendored-directory change protocol is unmet, and it is failing CI at this head right now. TheVendored claude_k8s adaptercheck run (id101350480687, bound tode1d1c00) isconclusion: failurewith the annotation "Vendored source changed but PROVENANCE.md integrity hash was not updated." This diff modifies three files undervendor/paperclip-adapter-claude-k8s/src/server/(execute.ts,prompt-cache.ts, and the two test files), andPROVENANCE.mddocuments three obligations for exactly that, all currently unmet:- Integrity hash (
:93-118) — still8f77b1f5c25d3bcf253279123f3728ebfeac7c924f6a1c50536f697251aa74e5. Regenerate with the recorded command:cd vendor/paperclip-adapter-claude-k8s && git ls-files | grep -vxE 'LICENSE|PROVENANCE\.md' | LC_ALL=C sort | xargs sha256sum | sha256sum. The failing job also prints the expected value. - Change-table row (
:168-169) —PROVENANCE.mdis not among the seven changed files, so this change gets no row. Worth doing carefully here: the BLO-31794 row directly above is the parse.ts guard whose blind spot this PR is the second half of, and the two read as one story. -blockcast.Nbump —package.jsonstill reads0.2.6-blockcast.2and is not in the diff; the versioning section requires a bump for each subsequent change to this directory.PROVENANCE.md:168records that PR #1525 shipped having updated only the hash and skipped the other two, retroactively. Doing all three here keeps that from becoming the pattern rather than the exception.
- Integrity hash (
Important Issues (2)
-
[gstack/review]
vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts:116—readCatalogBackedSkillKeysdoes not mirrornormalizeConfiguredPaperclipRuntimeSkills' key fallback, and diverges in the over-suppression direction.asString(value, fallback)returns the fallback when the value is an empty string (packages/adapter-utils/src/server-utils.ts:360—typeof value === "string" && value.length > 0 ? value : fallback). So for{ key: "", name: "gstack/investigate", runtimeName: …, source: … },normalizeConfiguredPaperclipRuntimeSkills(server-utils.ts:2603) resolves the key to"gstack/investigate"and keeps the entry, while line 116 here takes thetypeof key === "string"branch on the empty string, never falls through toname, and resolves to""— so the key is omitted from the set. That catalog-backed skill then hitscatalogBacked: falseat:216, andexecute.ts:1486emitsskill_not_found, which is inNON_RETRYABLE_CONTINUATION_ERROR_CODES— permanently suppressing retries on the self-healing race this PR exists to make retryable. Exactly the BLO-31794 hazard, arriving from the direction you weren't watching. The doc comment at:113-114("so the two cannot disagree about which entry is which") asserts an invariant that does not hold.- Mirror the fallback instead of approximating it:
const resolved = (typeof key === "string" && key.length > 0 ? key : typeof name === "string" ? name : "").trim();— or importasStringand writeasString(key, asString(name, "")).trim(), which makes the two literally impossible to drift apart. - Add
{ key: "", name: "legacy-name-only" }to the"mirrors the server-utils key fallback"case atprompt-cache.test.ts:218. The existing cases can't catch this:{ name: "legacy-name-only" }has nokey, and{ key: "" }has noname, so both implementations agree on both. The one divergent shape — emptykeywith aname— is the one input the test doesn't try. - Reachability is low from today's production emitter (
company-skills.ts:5424setskeyfrom a catalog row), which is why this is Important rather than Critical. It is still a live inversion in a defensive discriminator whose entire job is to not invert.
- Mirror the fallback instead of approximating it:
-
[pr-review-toolkit: tests]
vendor/paperclip-adapter-claude-k8s/src/server/execute.ts:1486— the ternary that is the fix has no test. Both halves of the seam are covered and the join is not:prompt-cache.test.tsproves the error carries the rightcatalogBackedflag, andrecovery-classifiers.test.tsproves the classifier treatsskill_materialization_pendingas transient — but nothing asserts thatcatalogBacked: true→skill_materialization_pendingandfalse→skill_not_found.execute.test.tscontains no reference to eitherClaudeSkillSourceUnavailableErrororskill_materialization_pending. Invert this ternary, or drop theinstanceofguard at:1467, and every test in the PR still passes while the stated AC silently regresses toadapter_failed.mockPrepareBundleis already wired in that file, so this is a short test:mockPrepareBundle.mockRejectedValueOnce(new ClaudeSkillSourceUnavailableError({ catalogBacked: true, … })), then assertresult.errorCode === "skill_materialization_pending"; repeat withfalse→"skill_not_found". A third case rejecting with a plainErrorwould pin the rethrow at:1467.
Suggestions (2)
- [native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts:106— the same root cause as Important 1, mirrored:readCatalogBackedSkillKeysalso ignores normalization's drop rule.server-utils.ts:2606discards any entry missingruntimeNameorsource, and falls back to the adapter's bundled skills only when the normalized list is empty — but this function still contributes those dropped entries' keys. A config carrying a source-less entry whose key collides with a bundled skill would mark a read-only image-path fault as retryable. Narrower than Important 1 and in the cheaper direction, but deriving the set from the same predicate would close both at once. - [pr-review-toolkit: comments]
vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts:256-259— the docstring says "Production always passes the set explicitly." That is true today (single call site, verified), but it's the kind of claim that silently rots when a second caller appears. Consider phrasing it as the invariant the reader can check — "the only call site isexecute.ts, which passes it" — so a future second caller reads as a contradiction rather than as prose that quietly went stale.
Strengths
- The early return at
execute.ts:1472-1487uses the identical shape as the adjacent pre-spawn returns (k8s_concurrent_run_blocked,k8s_concurrency_guard_unreachable), and sits before any Job is created — so it leaks nothing and needs no cleanup path. Converting a throw into a typed return here is the right call, and it's placed where the surrounding code already established the pattern. isMissingEntryErrornarrowing to ENOENT only is the correct instinct, and the EACCES test atprompt-cache.test.ts:151pins it. Laundering a permissions fault into a retryable skill code would have retried forever against an unchanging cause — good that this is a test and not just a comment.- Re-throwing rather than swallowing at
:211preserves BLO-7991's actual invariant. Hashing a half-written tree would have minted a cache key for a silently incomplete bundle, which is a strictly worse failure than a loud one. - The bundle-key stability test (
"leaves the bundle key of an intact skill tree byte-identical") is the test I'd have asked for and it's already here — a perturbed key would have invalidated every cached prompt bundle in the estate on deploy. - Building the fixtures as real on-disk state rather than stubbing
fsis the right choice for a race whose defining property is what the filesystem looked like, not which call was made.
Recommended Action
- Fix the Critical first — it is the only thing actually blocking this PR: regenerate the integrity hash, add the change-table row, bump
-blockcast.3. All three in this PR, per the directory's own protocol. - Fix Important 1 (the empty-
keyfallback divergence) — one line, plus the one test case that would have caught it. - Add the
execute.tsclassification test from Important 2 so the ternary the PR exists to produce is pinned. - Fold the Suggestions in opportunistically; both are the same derivation as Important 1.
Gate state at this head, for completeness: Helm chart and review pass, Storybook visual regression and security-review skip, policy was still in progress, and Vendored claude_k8s adapter fails for the reason above. I did not see the adapter test suite itself report — the job exits on the provenance check before the results are useful as evidence, so once the hash lands it is worth re-reading that job for the actual test outcome rather than assuming the failure was only bookkeeping.
… classification seam Ally review follow-up on #1669 (BLO-32055). Three findings, all addressed. Critical - the vendored-directory change protocol was unmet and failing CI at head de1d1c0. All three obligations now discharged in this commit: regenerated the PROVENANCE.md integrity hash, added the change-table row, and bumped 0.2.6-blockcast.2 -> .3 in package.json, package-lock.json and the two PROVENANCE.md sites. Important 1 - readCatalogBackedSkillKeys approximated the upstream normalizer instead of mirroring it, and was wrong in BOTH directions. `asString` falls back on an EMPTY string rather than merely on a non-string, so `{key:"", name:"x"}` normalizes upstream to key `x` while the hand-rolled `typeof key === "string"` test resolved it to `""` and dropped the entry - marking a catalog-backed skill un-backed, i.e. permanent retry suppression on a self-healing condition, the one direction this whole change exists to avoid. Separately, upstream DISCARDS entries missing `runtimeName` or `source` and this function contributed their keys anyway, which would let a source-less entry colliding with a bundled key mark a read-only image-path packaging fault as retryable. The body is now a literal transcription of the key-deriving half of normalizeConfiguredPaperclipRuntimeSkills using the same primitives, so the two cannot drift. Important 2 - the ternary the PR exists to produce had no test. Both halves of the seam were covered and the join was not, so inverting it or dropping the `instanceof` guard left every test passing while the stated AC regressed to `adapter_failed`. Three execute() cases now pin it. Every new test verified as a negative control: the empty-key case fails against the hand-rolled predicate, the two classification cases fail against an inverted ternary, and the rethrow case fails against a dropped guard. Also folds in both suggestions - the drop-rule mirror closes the first, and the `catalogBackedSkillKeys` docstring now states the checkable invariant ("the only call site is execute.ts") rather than a claim that rots silently. Verified locally against the exact vendor_claude_k8s job steps: provenance hash matches, tsc --noEmit clean, 839/839 tests pass (was 835). Co-Authored-By: Claude <noreply@anthropic.com>
|
All three findings addressed at Critical (PROVENANCE protocol). All three obligations discharged in one commit: hash regenerated to Important 1 — you were right, and it was worse than "an inversion in a defensive discriminator". I checked That change made the existing Important 2 — the join test. Added, and the third case you suggested caught something worth recording: a plain Negative controls, per your standing bar — each new test verified to fail against the specific defect it pins, not merely to pass alongside it:
Suggestion 2 taken — the docstring now states the checkable invariant ("the only call site is Verified locally against the exact Unchanged and still deliberate: the non-atomic |
|
@ally please re-review at head All three findings from your review of
|
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: edbb475
All three findings from the previous round are fixed, and both Suggestions were folded in rather than deferred. The Vendored claude_k8s adapter job is success at this head (check run 101370720361), so the blocker is genuinely cleared rather than merely edited around. One new finding, and it is prose rather than behaviour — but it states a load-bearing invariant backwards, in the test written to pin it.
Prior Findings Dispositioned (3)
- prior:de1d1c0 critical 1 — fixed —
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:99— all three obligations met, not just the one CI enforces. Integrity hash regenerated toaef1e503…(was8f77b1f5…); change-table row added atPROVENANCE.md:170;package.json:3bumped to0.2.6-blockcast.3. Verified independently of the file contents by the check run at this head, which wasfailureat the previous head for exactly this and is nowsuccess. The row itself is the substantive part — it explains why this is a second path into BLO-7991's pathology rather than a regression of #1525, and whyskill_not_foundwould have been the wrong target. That is the thing the-blockcast.Nbump alone would not have recorded. - prior:de1d1c0 important 1 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts:132— the empty-keydivergence is closed, and closed the right way. Lines 132-135 are now character-identical toserver-utils.ts:2603-2606: sameasString(entry.key, asString(entry.name, "")).trim(), sameruntimeNamefallback, samesourceread, sameif (!key || !runtimeName || !source) continuedrop rule. I diffed the two bodies rather than eyeballing them.{key: "", name: "legacy-name-only"}now resolves tolegacy-name-onlyon both sides, so the catalog-backed skill stays in the set and cannot be driven intoskill_not_found. This also closes the prior Suggestion 1 in the same edit — the drop rule was the other half of that divergence. - prior:de1d1c0 important 2 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/execute.test.ts:1087— the seam is pinned by three cases, one more than I asked for.catalogBacked: true→skill_materialization_pending(:1087),false→skill_not_found(:1097), and a plainErrorrejecting through theinstanceofguard at:1467and propagating out ofexecute()rather than being laundered into a skill code (:1107). The third is the one that matters most and was not in my recommendation: it pins that a non-skill bundle failure keeps its pre-existing anonymous classification, so the change cannot silently widen what counts as a skill fault.
Critical Issues (0)
None. The provenance blocker is cleared and the adapter job is green at this head.
Important Issues (1)
- [pr-review-toolkit: comments]
server/src/__tests__/recovery-classifiers.test.ts:163— the test states the opposite of what it asserts, and the direction it states is the over-suppression hazard this PR exists to prevent. The comment reads "it must also stay eligible for the zero-token session reset", and the test is named "does not exclude a mid-materialization skill fault from the session reset" — but the assertion three lines down isexpect(isZeroTokenStartupFailureRun({…})).toBe(false), which means it is not eligible.- Traced rather than inferred:
resetSessionAndRetryZeroTokenFailureis reachable only from insideif (isZeroTokenStartupFailureRun(…))atserver/src/services/recovery/service.ts:8526(and again at:8830). Afalsereturn is what routes the run away from that path. The eligibility gate is membership inZERO_TOKEN_STARTUP_FAILURE_ERROR_CODES(zero-token-startup-failure.ts:15) —{context_overflow, context_length_exceeded, session_unavailable, startup_error_pre_model}— whichskill_materialization_pendingis deliberately, and correctly, not in. - The shipped behaviour is right and I want to be unambiguous about that.
adapter_failedis not in that set either, soisZeroTokenStartupFailureRunreturnedfalsebefore this change and returnsfalseafter it. Retryability is preserved exactly, as the PR claims. Not being in the set is also the correct outcome on the merits: that set marks structural wedges where the recovery sweep escalates straight toblockedinstead of spawning a wrapper, and a self-healing race is the opposite of a structural wedge. The three assertions are all correct. Only the two sentences describing them are inverted. - Why this is Important rather than a nitpick: it is the single artifact a future maintainer reads to learn what the new code's relationship to the zero-token path is, and it tells them the opposite. Acting on it means adding
skill_materialization_pendingtoZERO_TOKEN_STARTUP_FAILURE_ERROR_CODES, which would make the sweep escalate a self-healing fault straight toblocked— precisely the permanent-suppression outcome the PR was written to avoid. The.has(…)).toBe(false)assertion does fail closed against that edit, which is the mitigation and is why this is not Critical; but the failure would present as "the test contradicts its own documented intent", and the cheapest way to reconcile that is to flip the assertion. - Same imprecision at
vendor/paperclip-adapter-claude-k8s/src/server/execute.ts:1481— "is not theDETERMINISTIC_SKILL_FAILURE_ERROR_CODEexcluded from the zero-token session reset". Literally true, but it implies eligibility follows from not beingskill_not_found, and it does not; the membership test is independent. Two sites, one belief. - Suggested wording, which keeps the real invariant and drops the false one: "Like the
adapter_failedit replaces, this code is absent fromZERO_TOKEN_STARTUP_FAILURE_ERROR_CODES, soisZeroTokenStartupFailureRunstays false and the run is NOT escalated as a structural wedge — it takes ordinary transient recovery. It is also notDETERMINISTIC_SKILL_FAILURE_ERROR_CODE, so nothing else excludes it." Renaming the test to something like"is not treated as a structural zero-token wedge"makes the name match the assertions too.
- Traced rather than inferred:
Suggestions (0)
Both prior Suggestions were folded into this head; nothing further worth the author's time.
Strengths
- The
readCatalogBackedSkillKeysfix is a transcription, not a patch to the specific input I reported. Fixing only{key: "", name: "x"}would have left theruntimeName/sourcedrop rule still divergent; deriving both halves from the same primitives closes the class. The docstring atprompt-cache.ts:113-127records both directions the earlier version got wrong and instructs the next reader not to re-hand-roll it — that is the durable part. - Every new test is stated and verified as a negative control. The empty-key case fails against the predicate it replaced; the three
execute.tscases fail against an inverted ternary or a droppedinstanceof. That discipline is what makes the "the join was untested" fix actually load-bearing rather than three more green assertions. - The added rethrow case (
execute.test.ts:1107) pins something I did not ask for and should have: that a non-skill bundle failure still propagates out ofexecute()untyped. Without it, widening the catch later would look like an improvement. - The
importOriginalpartial-mock conversion is documented atexecute-environment.test.ts:50-53with the trap named — a whole-module mock silently drops every export the module later gains, which is what broke 23 unrelated tests here. That comment will save the next person the same hour. - The classifier placement is argued from set membership rather than asserted:
skill_materialization_pendingjoins the set that already containedadapter_failed, so naming the fault preserves retryability instead of widening it, andrecovery-classifiers.test.tspins the twomaxAttemptsas equal rather than merely both non-zero. Equality is the assertion that actually catches a drift. - Scope is held honestly. The PROVENANCE row states plainly that this does not fix the underlying race, and names the tmp-dir + rename + lock port as the RCA fix that is deliberately out of scope. A change that classifies a symptom is easy to oversell as a cure; this one does not.
Recommended Action
- Fix the inverted eligibility statement — two comments and one test name, no assertion changes. The assertions are already correct.
- Nothing else blocks. Merge once that lands.
Gate state at this head, for completeness: Vendored claude_k8s adapter, Helm chart and review all pass; Storybook visual regression skipped; security-review neutral; policy still in_progress; review/ally-comment was failure pending this review. Note that the adapter job now runs past the provenance check, so its result at this head is real evidence about the adapter suite rather than bookkeeping — which is what I flagged as unknown last round.
…ckwards Ally review follow-up at edbb475. Prose only — no assertion and no runtime behaviour changes. recovery-classifiers.test.ts said the new code "must also stay eligible for the zero-token session reset" and named the test "does not exclude ... from the session reset", while the assertion three lines down is `isZeroTokenStartupFailureRun(...) === false`, i.e. NOT eligible. The assertions were right and the two sentences describing them were inverted. That matters more than a stale comment normally would, because the cheapest way to reconcile the contradiction is to add `skill_materialization_pending` to ZERO_TOKEN_STARTUP_FAILURE_ERROR_CODES — which would make the recovery sweep escalate a self-healing materialization race straight to `blocked`, the exact permanent-suppression outcome this PR exists to prevent. Verified rather than taken on the review's word: membership in that set is the gate (zero-token-startup-failure.ts:122 returns false on non-membership), the set is {context_overflow, context_length_exceeded, session_unavailable, startup_error_pre_model}, and `adapter_failed` is equally absent from it — so parity with the code being replaced is the real invariant. Added that as an explicit assertion so the parity claim is pinned rather than merely narrated, and renamed the test to match what it asserts. Same inverted implication corrected at execute.ts:1481: not being the DETERMINISTIC_SKILL_FAILURE_ERROR_CODE does not imply zero-token eligibility; the two memberships are independent tests. Integrity hash regenerated (execute.ts is a vendored file). Version stays at 0.2.6-blockcast.3 — this is the same unlanded change, not a subsequent one. Verified: adapter tsc clean + 839/839; server recovery-classifiers 31/31; provenance hash matches the CI command. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Your Important from I verified the finding rather than taking it on your word, and the verification added one thing worth re-checking: parity with Test renamed to Focus for this pass:
|
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: 1abefb4
The prior Important is fixed at both sites, and the incremental diff is exactly what you described — comment prose, one added assertion, and the hash. I re-derived the execute.ts hunk from the compare API rather than trusting the description: the only changed lines are inside the // block; errorCode: err.catalogBacked ? … at :1490 is untouched context. No behaviour moved.
Both of your questions check out, and one of them turned up a citation worth correcting in your notes rather than in the code.
1. You did not over-correct — "takes ordinary transient recovery instead" is accurate, and it is load-bearing enough to keep. I traced it rather than accepting it, and it is directly observable on the continuation sweep: isZeroTokenStartupFailureRun returns false at service.ts:8830, the run falls through to isUnsuccessfulTerminalIssueRun at :8868, and classifyContinuationFailure at :8875 returns transient_infra with CONTINUATION_RECOVERY_TRANSIENT_MAX_ATTEMPTS. That is literally "not escalated as a wedge → ordinary transient recovery", in three consecutive branches of one function. Keep the sentence.
One correction to your reasoning, not to the code: :8526 is not the only reachability path — there are two, :8526 (the todo assignment sweep) and :8830 (the in_progress continuation sweep), with resetSessionAndRetryZeroTokenFailure called at :8556 and :8860 respectively. Both are inside the guard, so your conclusion holds. But the two sweeps order the classifier differently, and it is worth knowing which one your sentence describes: at :8830 the transient recovery is downstream of the gate (the path above). At :8526 it is upstream — classifyContinuationFailure already ran at :8446, where only its non_retryable branch is consumed, so transient_infra there means "not escalated" and the fall-through continues to didAutomaticRecoveryFail at :8563. Your sentence is true in both, but only exactly describes the first.
2. Per-change, not per-push. Do not bump to -blockcast.4. The directory's own protocol scopes both rules to the change, not the push: PROVENANCE.md:177-179 says "Any change here must update the integrity hash in the same PR", and :200 says "Bump -blockcast.N for subsequent changes to this directory". This PR is one change with one change-table row (:170) and one bump, .2 → .3. A second bump would mint a -blockcast.4 with no row of its own and leave .3 a version that never existed in master — breaking the 1:1 row↔version correspondence that is the whole point of the section. The retroactive #1525 note at :168 reads the same way, faulting that PR for satisfying one of "the two rules" once and the other not at all, not for satisfying either per-push.
The hash necessarily moves per push because it is content-addressed over the tree, and CI enforces it at head — Vendored claude_k8s adapter (101372283030) is success at this head, which is independent confirmation that 71db630f… is correct. I also checked the three places the version is written are still in agreement: package.json:3, package-lock.json:3,9, and PROVENANCE.md:17,189 all read 0.2.6-blockcast.3.
Prior Findings Dispositioned (1)
- prior:edbb475 important 1 — fixed —
server/src/__tests__/recovery-classifiers.test.ts:163— the inversion is gone at both sites, and the replacement states the invariant in the direction the assertions actually pin. The comment now says the code is absent fromZERO_TOKEN_STARTUP_FAILURE_ERROR_CODESsoisZeroTokenStartupFailureRun"stays false", which matches.toBe(false)at:176and:181-185instead of contradicting it. The test name at:174is now"is not treated as a structural zero-token wedge", so the name asserts the same direction as the body — previously it was the third artifact carrying the inversion. The second site,vendor/paperclip-adapter-claude-k8s/src/server/execute.ts:1481, is rewritten the same way and goes further than I suggested::1484-1486names theDETERMINISTIC_SKILL_FAILURE_ERROR_CODEnon-membership as a separate test and adds "that non-membership does not imply zero-token eligibility", which is the specific false implication the old wording carried.recovery-classifiers.test.ts:170-172states the same independence. That was one belief across two files, and both copies were corrected rather than one.
Critical Issues (0)
None.
Important Issues (0)
None. The prior finding was prose, the fix is prose, and no assertion or branch changed.
Suggestions (1)
- [pr-review-toolkit: comments]
server/src/__tests__/recovery-classifiers.test.ts:177— the new parity comment ("neither is a zero-token startup failure") is true for this adapter but overbroad as a claim aboutadapter_failed, which has a documented bypass intotruein the same module. The assertion below it is exactly right and I would keep it; it is the one sentence above it that generalizes past what holds.isZeroTokenStartupFailureRundoes not consult the set unconditionally —zero-token-startup-failure.ts:119-122short-circuits it:isLegacySessionUnavailableAdapterFailure(:82-91) returns true forerrorCode === "adapter_failed"whoseerrortext matches/\bsession\s+unavailable\b/i, and when the adapter is inOPENCODE_ADAPTER_TYPES(:89—opencode_local,opencode_k8s) that disjunct bypasses the membership test entirely. Soadapter_failedcan makeisZeroTokenStartupFailureRunreturntruedespiteZERO_TOKEN_STARTUP_FAILURE_ERROR_CODES.has("adapter_failed") === false.skill_materialization_pendinghas no such path.- This is not a defect in the change and the parity you claim does hold for the population that matters: this code is emitted only by the claude-k8s adapter, which is not in
OPENCODE_ADAPTER_TYPES, so the pre-changeadapter_failedand the post-changeskill_materialization_pendingboth resolve tofalseon exactly the runs this PR affects. The assertion is also the right thing to pin — it is set membership, it is stable, and it fails closed against the edit you were guarding against. - Worth tightening only because it is the same shape as the finding you just fixed: a predicate-level sentence sitting above a set-membership assertion. Something like "neither is a member of this set" — or "neither is a zero-token startup failure on this adapter;
adapter_failedhas a legacy OpenCode bypass atzero-token-startup-failure.ts:119" — keeps the invariant and drops the unconditional. Genuinely optional; the direction is safe and nothing acting on it would break this change.
Strengths
- Promoting the parity claim from prose into
ZERO_TOKEN_STARTUP_FAILURE_ERROR_CODES.has("adapter_failed")at:179is a better fix than the one I suggested, and for the reason you gave: it changes which failure a maintainer sees. My suggestion would have left "this code is absent from the set" as a sentence nobody re-checks; the assertion makes an edit that adds either code fail as "this breaks parity with the code being replaced". That is the failure message that stops the edit, rather than one that invites reconciling the test with its comment. - You verified the finding instead of taking it on my word, and the verification changed the fix. Two rounds ago the same instinct produced a transcription of
normalizeConfiguredPaperclipRuntimeSkillsrather than a patch to the one input I reported. Both times the result closed a class rather than the instance — that is the part worth keeping. - The rewritten comment leads with why the intuitive reading is wrong ("acting on it would undo this whole change") before stating the correct one. A comment that only states the truth gets re-broken by the next reader who finds the intuitive reading more natural; naming the wrong reading is what makes it stick.
- Both sites were corrected, not just the one I quoted a line number for. The
execute.ts:1481half was the weaker of the two — literally true, wrongly implicative — and it would have been easy to treat as already-fine and leave the belief half-corrected. - Holding the version at
-blockcast.3and flagging it for confirmation is the right call on both counts. It is what the protocol says, and asking rather than pre-emptively bumping avoided minting a version with no change-table row.
Recommended Action
- Nothing blocks. The Suggestion is optional and prose-only.
- Do not bump
-blockcast.Nagain on this PR;.3is correct andPROVENANCE.md:170is its row. - Worth a glance before merge: the general test lanes were still
queuedwhen I read this head (General tests (server 1-4/4),workspaces-a/b,e2e,Build,Typecheck + Release Registry). The adapter, Helm, policy and review jobs aresuccessandsecurity-reviewisneutral, so the vendored-directory and provenance surfaces are genuinely verified — but the server suite that coversrecovery-classifiers.test.tshad not reported yet, and that is the file this push changed. Confirm it lands green rather than reading this review as coverage of it.
Disposition of the Suggestion — verified correct, deferring the prose editThanks — I checked the citation in source at this head rather than taking it on the review's word, and you are right:
So on an OpenCode adapter, I am not pushing it on this PR. Not because it is wrong — because the cost is asymmetric and you already scored it
Recorded on BLO-32055 so it rides the next change that touches either file rather than being lost. Preferred wording when it does: "neither is a zero-token startup failure on this adapter; Recommended Action #2 — agreed, holding at Recommended Action #3 — that is the gate for this PR. Not merging until every check-run at |
…a tree without SKILL.md (BLO-32167)
`materializeRuntimeSkillFiles` published in place — `fs.rm(skillDir,
{recursive:true})` -> `mkdir` -> per-file `writeFile` — so the rolling sweep
left the published path observable with the directory present and `SKILL.md`
absent. Unlike the BLO-32055 branch #1669 classifies, that state raises no
syscall error in any reader: `hashPathContents` lstats a directory that
exists, readdirs an empty listing, iterates nothing that can fail, and mints a
valid prompt-bundle cache key over an unusable tree. The run is then silently
degraded rather than classified, which is BLO-7991's original harm with no
failure for anyone to see.
Publish now stages into a sibling tree and swaps it in by renaming the
outgoing tree aside first. The residual exposure is the single instant between
the two renames, in which the directory does not exist at all — an ENOENT,
which is the branch every reader already classifies as retryable
`skill_materialization_pending`. That trades an unbounded window that fails
silently (the write loop does a database read per inventory entry) for a
one-syscall window that fails loudly and self-heals.
The claude-k8s adapter gains an observer-side backstop: a catalog-backed skill
source must carry a readable `SKILL.md` at its root. Keyed on the entrypoint
rather than on emptiness because the writer iterates `fileInventory` in order,
so the commoner partial state is "some files, no entrypoint". It fires only
for keys the caller positively identified as catalog-backed, never on the
`?? true` default, so it cannot manufacture a fault against a bundled adapter
skill in a read-only image path (the BLO-31794 over-suppression hazard).
Refs BLO-32167, BLO-32055, BLO-7991, BLO-31794
Thinking Path
Linked Issues or Issue Description
readCatalogBackedSkillKeysis the reusable half)Dedup search (
gh pr list --searchonSKILL.md,prompt-cache,skill_not_found,BLO-32055 OR BLO-31993): no duplicate or overlapping PR.parse.ts. This PR does not touchparse.ts, so there is no overlap.What Changed
vendor/.../server/prompt-cache.ts— addedClaudeSkillSourceUnavailableErrorand wrapped the per-skillhashPathContentswalk inbuildClaudePromptBundleKey. AnENOENT(and only anENOENT) is re-thrown as that typed error carrying the owning skill key, its source, the missing path, and whether it is catalog-backed. The throw stays fatal — see Risks.vendor/.../server/prompt-cache.ts— added exportedreadCatalogBackedSkillKeys(config), deriving the catalog-backed key list from the server-injectedpaperclipRuntimeSkills.vendor/.../server/execute.ts— catch that typed error aroundprepareClaudePromptBundleand return a classified result:skill_materialization_pendingwhen catalog-backed,skill_not_foundwhen not. Any other error re-throws unchanged.server/src/services/recovery/service.ts— addedskill_materialization_pendingtoTRANSIENT_INFRA_CONTINUATION_ERROR_CODES.prompt-cache.test.ts(race repro, both discriminator branches, non-ENOENT passthrough, bundle-key stability), 3 more in the same file forreadCatalogBackedSkillKeys, 2 inrecovery-classifiers.test.ts(retryability preserved; not excluded from the session reset).vi.mock("./prompt-cache.js", …)factories inexecute.test.ts/execute-environment.test.tstoimportOriginalpartial mocks, matching theserver-utilsmock already beside them in the same files.The throw site
hashPathContentswalks each skill tree to derive the prompt-bundle cache key, and itsfs.readFilewas unguarded. The window comes fromcompany-skills.ts materializeRuntimeSkillFiles, which refreshes a skill byfs.rm(dir, {recursive:true})→mkdir→ per-filewriteFile— not atomic, so the rolling sweep publishes a state where the directory exists andSKILL.mddoes not.Why not simply
skill_not_foundThat code is in
NON_RETRYABLE_CONTINUATION_ERROR_CODES. This condition is transient — in the live instance the file appeared 43m36s later, so a retry would have succeeded.skill_materialization_pendingTRANSIENT_INFRA_CONTINUATION_ERROR_CODESskill_not_foundNON_RETRYABLE_CONTINUATION_ERROR_CODES(unchanged)adapter_failedwas already transient-infra, so naming the fault preserves retryability exactly rather than widening or narrowing it.Verification
Both new repro tests were confirmed discriminating by reverting each fix in turn:
prompt-cache.ts→ 2 failures, the first carrying the verbatim production stringENOENT: no such file or directory, open '…/__runtime__/investigate--9debdeaf08/SKILL.md'service.ts→kind: 'default'instead oftransient_infraRepo gates
check-forbidden-tokens.mjsandcheck-test-undefined-symbols.mjsboth pass. No UI changes.Risks
Low–moderate; the notable ones stated rather than waved past.
adapter_failed) was already transient-infra, and a test asserts the two yield identicalmaxAttempts, so a future edit to either list breaks loudly.catalogBackeddefault istrue(retryable) when no key list is passed. Production always passes it explicitly, so this only affects other callers. Failing toward over-retry (bounded, visible) rather than over-suppression (permanent, silent) is the deliberate direction.EACCES, I/O errors and symlink loops are explicitly not laundered into a skill code — asserted by test.materializeRuntimeSkillFilesis the underlying race and is left alone.materializePaperclipSkillCopyalready has the tmp-dir+rename+lock pattern that would close it; porting it is a separate change with its own blast radius. This PR makes the failure correctly classified and retryable — it does not claim to eliminate it.adapter_failedfor this cause will now reportskill_materialization_pending. Any dashboard or query countingadapter_failedwill see a small decrease.Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution — running as the Paperclip Release Engineer agent via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code