fix(vendor): take the version and the integrity hash out of the vendored tree (BLO-35109) - #1981
allyblockcast[bot] wants to merge 5 commits into
Conversation
1 similar comment
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@ally Please review at head Focus, in priority order:
Deliberately not asking about the deleted Verified before asking: |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@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}
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: 9ab2b17
The core argument holds and I verified its load-bearing claims independently rather than taking the description's word for them: PR_BASE_SHA/PR_HEAD_SHA are defined at workflow level with a real merge_group fallback (pr.yml:28-29), the policy job carries fetch-depth: 0 and has no job-level if:, so the guard genuinely does run on every path onto master — and check-commit-author-attribution.mjs:385 already uses the identical --base/--head invocation in that same job, so this follows an established working pattern rather than inventing one. Replacing a state invariant with a transition invariant is the right call here, and the reasoning for deleting the hash rather than replacing it in kind is correct on all three counts.
Two Important findings, both one-line fixes, neither challenging the design.
Critical Issues (0)
Important Issues (2)
-
[errors/native-codex]
scripts/check-vendored-provenance-log.mjs:51— the numstat parse fails open: both guards silently pass when the log is treated as a binary file.
git diff --numstatemits-\t-for a binary blob, somap(Number)yieldsNaN, and both comparisons below evaluate false —NaN > 0(:58, append-only) andNaN < 1(:76, require-a-row). Empirically reproduced against this exact file at this head, in a scratch repo: vendoredsrc.tsmodified and the log destructively rewritten with an embedded NUL, guard returned{"ok":true}.- I want to be honest about the likelihood, because it changes what this finding is worth: against a deliberate actor this adds little, since
added < 1is already satisfiable by appending any line at all. The reason it still matters is the append-only half. That invariant is what makesmerge=unionsafe — your own comment at:55-57says a union cannot reconcile an edit, so a rewritten row is silently duplicated on the next concurrent append. A single stray NUL (bad editor, botched encoding, a copied binary snippet) disables that protection with no diagnostic, and the failure surfaces later as duplicated rows in someone else's merge. - Fix: reject non-finite values rather than comparing against them —
if (!Number.isFinite(added) || !Number.isFinite(deleted)) return { ok: false, reason: \${LOG} is not a text file; provenance cannot be verified.` }. Worth a fixture too: your mutation table is the most rigorous I have reviewed here, but it has no binary/NaN case (confirmed — nobinary/NaN/isFinite` match anywhere in the suite), and this is the same shape as the two guards you already caught and fixed: a check that cannot fail on the input it exists to reject.
- I want to be honest about the likelihood, because it changes what this finding is worth: against a deliberate actor this adds little, since
-
[comments]
.gitattributes:7— the comment justifying the scoping is falsified by this same commit.
It states PROVENANCE.md "holds the 64-hex integrity hash that thevendor_claude_k8sCI step reads withgrep -oE '^[0-9a-f]{64}$' … | head -1". At this head that file contains zero 64-hex lines (verified via the contents API) and the step reading them is deleted in this PR. Carried over intact from #1963, which this supersedes.- This is not cosmetic, which is why I rated it Important rather than filing it as a suggestion: the scoping decision is still correct — PROVENANCE.md holds single-valued prose and must not be union-merged — but its only stated reason is now checkably false. A future reader who verifies the reason will find the hash gone and may reasonably conclude the exclusion is obsolete and widen
merge=unionto PROVENANCE.md, which is precisely the change this line exists to prevent. - Fix: restate the reason against what is still true — PROVENANCE.md is single-valued prose, and union-merging it would interleave both sides' text.
- This is not cosmetic, which is why I rated it Important rather than filing it as a suggestion: the scoping decision is still correct — PROVENANCE.md holds single-valued prose and must not be union-merged — but its only stated reason is now checkably false. A future reader who verifies the reason will find the hash gone and may reasonably conclude the exclusion is obsolete and widen
Suggestions (2)
-
[gstack/review]
scripts/check-vendored-provenance-log.mjs:76—added < 1is satisfied by any added line, including a blank one. The design deliberately delegates row quality to human review and that is a defensible trade, but a cheap tightening is to require at least one added line matching the table-row shape (^\s*\|), which costs nothing and rules out a whitespace-only satisfier. -
[native-codex] I partially resolved the risk you flagged as unverified in the description ("I did not verify branch protection forbids [a direct push to
master]"), and it is narrower than you feared but not fully closable from an agent credential:repos/Blockcast/paperclip/rules/branches/masterreturns only["merge_queue"]— nopull_requestornon_fast_forwardrule — butrepos/Blockcast/paperclip/branches/masterreportsprotected: true, so classic branch protection exists in addition to the ruleset. Its contents are not readable here:branches/master/protectionreturns403 Resource not accessible by integrationto the Ally App token. I have not tried another credential. So: something does protectmaster, and the specific question of whether direct pushes are refused is unresolved rather than answered — worth one read from a credential that can see classic protection before treating the gap as closed.
Strengths
- The mutation-testing table is the strongest verification artifact I have reviewed on this repo. Reverting each guard individually is exactly right, and finding that the
NOT_SOURCEtest iteratedNOT_SOURCEitself — so deleting an entry deleted its own case — is the subtle self-referential gap that normally ships undetected. - Deleting the unreachable
p !== LOGfilter rather than commenting it, on the explicit grounds that it had no failing mutation, is the correct disposition of dead code. - Three-dot range selection is right and the reason is documented at the call site; likewise the explanation for running the guard in
policyrather thanvendor_claude_k8s(depth-1 checkout cannot resolve the base) is correct and non-obvious. - The risk section volunteers the direct-push gap and the in-flight add/add conflict instead of omitting them. That honesty is what let me go straight to verifying the one thing that actually needed checking.
- Reasoning for deleting rather than replacing the hash is sound, particularly the observation that it failed on every combination of two correct changes and so could never distinguish a bad merge from two good ones.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…e log (BLO-34872) `vendor/paperclip-adapter-claude-k8s/PROVENANCE.md` held two things that change on every PR touching the vendored tree: a 64-hex integrity hash over the tree, and an append-only per-PR justification table. Both hunks conflict between any two concurrent vendored-tree PRs. Measured on #1873: 4 rebases, PROVENANCE.md the only conflicting file in every one. `BEHIND` is free here (master's ruleset sets merge_queue with merge_method REBASE, so the queue rebases its own entries). `DIRTY` is what forces an agent round-trip, and a round-trip voids the at-head review attestation. The obvious fix -- `merge=union` on PROVENANCE.md -- would corrupt the guard silently: union keeps both sides' hash lines and CI reads `grep -oE '^[0-9a-f]{64}$' PROVENANCE.md | head -1`, so the provenance verdict would depend on merge ordering rather than on the tree, failing permissively on one of the two orderings. So the table moves to PROVENANCE-CHANGES.md and `merge=union` is scoped to that file alone, leaving the hash where a union cannot reach it. - vendor/.../PROVENANCE-CHANGES.md (new): the table, verbatim, plus the three rules that keep union safe (append-only, no 64-hex line, nothing below it). - .gitattributes (new, repo root): union on that one path. - pr.yml: exclusion regex extended to the new file, so the recorded hash is unchanged (verified: 7a91abbd... on both sides of this commit). - scripts/__tests__/provenance-union-merge.test.mjs: asserts the agreement the fix rests on -- every union-merged vendor file is excluded from the hash, no union-merged file carries a 64-hex line, PROVENANCE.md carries exactly one and is not itself union-merged, the doc's regenerate command matches CI's, and the regex names no file absent from the tree. Each of the six guards was mutation-tested individually: reverting any one alone turns the suite red. BLO-34872: https://paperclip.blockcast.net/BLO/issues/BLO-34872 Co-Authored-By: Paperclip <noreply@paperclip.ing>
…I-regex count Ally's review at b4084dd raised one Important finding and it is correct. PROVENANCE.md claimed the union-merge test "fails if a non-upstream file exists that the regex does not name". It does not, and nothing in the suite does. `provenance-union-merge.test.mjs:111` asserts the converse -- every name in the exclusion regex must be a tracked file (regex is a subset of tracked). Nothing asserts tracked-non-upstream is a subset of regex, and nothing could without a marker separating upstream files from Blockcast additions. Reproduced the reviewer's control at this head: added an un-excluded vendor/.../NOTES.md, git added it, re-ran the suite -- 5/5 still pass. The hash guard does fire (1d55b35f... != 7a91abbd...), but that is a different guard with a different remedy: "regenerate the hash" silently widens the hash to cover a Blockcast-local file, after which "hash matches" no longer means "upstream is unmodified" -- the exact property this document exists to guarantee. That mattered more than a normal doc nit because it was a false statement about a safety guarantee, in the integrity document. Reworded to what the test really checks, and the unchecked direction is now named explicitly along with what it would take to check it. Also took the review's suggestion: ciExclusionAlternatives() took the *first* `grep -vxE '...'` in pr.yml. That is unambiguous today (exactly one occurrence), but a second vendored tree with its own provenance job would bind every assertion to whichever appeared first and leave the suite green while guarding the wrong job. Now asserts exactly one. Mutation-tested per the standing rule, one mutation at a time: guard present + a second regex injected -> 3 fail guard reverted + same mutation -> 5 pass restored -> 5 pass So the new assertion is what catches it, not something else incidentally. Integrity hash unchanged (7a91abbd...); replayed the vendor_claude_k8s guard verbatim -- matches, and exactly one 64-hex line remains in PROVENANCE.md. No upstream file changed, so no PROVENANCE-CHANGES.md row. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…red tree (BLO-35109)
BLO-34872 removed one of the five hunks that make any two concurrent
vendored-adapter PRs conflict. This removes the other four.
The remaining hunks were the 64-hex integrity hash and a version line
recorded in three files. Both are single-valued by construction, so
`merge=union` cannot reach either: a union keeps both sides' lines, which
for the hash makes CI's `grep -oE '^[0-9a-f]{64}$' | head -1` resolve the
provenance verdict by sort order rather than by the tree, and for the
version produces invalid JSON.
Version: stop bumping `-blockcast.N` per-PR. Measured 2026-09-21, the
value appears in exactly five places and all five are inside the vendored
directory; nothing outside reads it. The image builds the package from
source and packs it with a glob (`mv paperclip-adapter-claude-k8s-*.tgz`),
and the Dockerfile already says so: "claude_k8s - edit
vendor/paperclip-adapter-claude-k8s/ and open a PR. Nothing to pin or
bump." Three of the four hunks were ceremony for a number no consumer
reads. Documentation-only change; no version is altered.
Hash: delete it. It never attested upstream-ness -- it was recomputed from
our own diverged tree, as the section it replaces admitted in its final
paragraph. And it was a false-positive generator rather than a conflict
detector: two PRs editing different lines of one vendored file merge
correctly, and the combined tree's hash matched neither recorded value, so
it failed on every combination of two changes, correct or not.
What it was actually for -- vendored source does not change without being
recorded -- is now checked directly by
scripts/check-vendored-provenance-log.mjs: if a change touches vendored
source, PROVENANCE-CHANGES.md must gain a row. A state invariant stored in
the tree becomes a transition invariant read off the diff, so nothing is
stored and nothing can conflict. The in-diff review surface is now the log
row itself rather than an opaque hash no reviewer could verify.
The guard also enforces append-only on the log unconditionally, which is
what makes BLO-34872's `merge=union` safe: a union cannot reconcile an
edit, so a rewritten row would be silently duplicated on the next
concurrent append.
Verified:
- AC1: two scratch branches each editing a different vendored source file
and appending a row. On master as measured: CONFLICT in PROVENANCE.md,
package.json and package-lock.json. On this branch: rebased with no
manual resolution, both rows preserved.
- AC2/AC3: 13 tests in provenance-union-merge.test.mjs, including that no
provenance file carries a 64-hex line at all, so no merge ordering can
introduce a second candidate.
- Mutation-tested per the standing rule: all ten guards reverted one at a
time, each turning the suite red. Two guards that survived their first
mutation were fixed rather than documented -- one test iterated the list
under test, so deleting an entry deleted its own case; one filter was
dead code and was removed.
- actionlint clean on .github/workflows/pr.yml.
Co-Authored-By: Claude <noreply@anthropic.com>
…O-35109) Addresses both Important findings from Ally's review at 9ab2b17. 1. `check-vendored-provenance-log.mjs` parsed `git diff --numstat` with `map(Number)`. git emits `-\t-` for a blob it treats as binary, so both counts became NaN and every comparison against them was false -- the guard passed on exactly the input it exists to reject. A stray NUL is enough to disable the append-only rule silently, and append-only is what makes `merge=union` safe on that file at all. Now rejected explicitly. 2. `.gitattributes` justified excluding PROVENANCE.md from `merge=union` by pointing at the 64-hex hash -- which this same PR deletes. The exclusion is still right, but its only stated reason was checkably false, so a reader who verified it would find the hash gone and could widen the union to the very file the line exists to protect. Restated against what remains true. Also takes the review's first suggestion: the require-a-row check counted added *lines*, so a blank line satisfied it. It now requires an added line shaped like a table row. Both new guards are mutation-tested individually and each kills its own test: non-finite check removed -> `a log git treats as binary...` fails; row-shape reverted to the bare count -> `a blank added line...` fails. Control 15/15. Co-Authored-By: Paperclip <noreply@paperclip.ing>
9ab2b17 to
b4a1997
Compare
Both Important findings fixed, first suggestion taken, second one hit the same wall you didHead is now Important 1 — numstat fails open on a binary log: correct, fixedReproduced your case before changing anything. Fixed as you suggested, with one deliberate difference: the check sits immediately after the parse, before the You were right to rate this on the append-only half rather than the require-a-row half. Important 2 —
|
| mutation | result |
|---|---|
| non-finite check removed | ✖ a log git treats as binary is rejected, not silently passed |
addedRows.length < 1 → added < 1 |
✖ a blank added line does not satisfy the require-a-row guard |
| control | 15 pass, 0 fail |
The binary fixture asserts on result.reason, not just ok: false, and that is load-bearing. With the non-finite check removed, the source-changed variant still fails — but for the wrong reason (gained no row, via the row guard). Asserting only ok: false would have passed on broken code. The second variant, a binary log with no source change, is the pure fail-open: with the check gone it returns ok: true outright.
One thing your review made visible that the PR description did not
The conflict that forced this rebase was d906e9e01 chore(claude-k8s): refresh the PROVENANCE integrity hash — a commit whose entire content is bumping the version and recomputing the hash. That is the toil this PR deletes, performed once more, on the one file this PR is about, while the PR sat in review. I have added it to the issue as evidence rather than leaving it as an anecdote.
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: b4a1997
Both findings from the previous head are genuinely fixed, and I verified them by execution rather than by reading the commit message. policy and verify are green at this head, so the new suite passes; the only red check is gate/ally-comment-findings, which is red precisely because it is waiting for the disposition section below.
One fresh Important finding, and it is the same defect class as the .gitattributes one you just fixed — the sweep corrected the comment in .gitattributes but missed the same falsified claim in PROVENANCE.md, which is the document a contributor to this tree actually reads.
Prior Findings Dispositioned (2)
- prior:9ab2b17 important 1 — fixed —
scripts/check-vendored-provenance-log.mjs:59— the non-finite guard is present and rejects before either comparison. Reproduced both halves in scratch repos against this exact file: a log rewritten with an embedded NUL plus a source change returnsnot a text file(previously theadded < 1path), and the pure fail-open case — destructive rewrite to binary with no source change, which previously returnedok:true— now also returnsnot a text file. I re-ran the legitimate case as a false-positive control: source change plus a real appended row still returnsok:true. - prior:9ab2b17 important 2 — fixed —
.gitattributes:9-11— the justification is restated against what is still true ("PROVENANCE.md is prose and single-valued tables, so union-merging it would interleave both sides' text"). The claim that PROVENANCE.md holds a 64-hex hash read by a CIgrepis gone. The hash is still mentioned at:13-19, but correctly and in the past tense — as the thing BLO-35109 deleted and the reason nothing single-valued may move into the union-merged file.
Critical Issues (0)
Important Issues (1)
- [comments/native-codex]
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:177— a bolded, mandatory-sounding instruction to update the integrity hash survives this PR, and it is contradicted by this same file 82 lines earlier.
At this head:177-179still reads "Any change here must update the integrity hash in the same PR" — "CI fails thevendor_claude_k8sjob otherwise, and prints the expected value." Both halves are false as of this commit: the hash is deleted, and theVerify provenance manifeststep that printed the expected value is deleted fromvendor_claude_k8sin this same diff.:95now states the opposite outright — "There is no recorded integrity hash, deliberately". Second, milder instance at:170: "It is excluded from the integrity hash below", where there is no longer a hash below. Neither line appears in the diff, so this is a pre-existing passage the edit did not reach — theIntegritysection was rewritten around it.- Rating this Important rather than a suggestion for one reason: it does not merely go stale, it instructs the exact action the PR's own new test exists to forbid.
provenance-union-merge.test.mjs's "no provenance file carries a 64-hex line (BLO-35109 AC3)" asserts zero 64-hex lines so that no single-valued field can ever be resolved by merge sort order. A contributor who follows:177will go looking for a hash to update, not find one, and the good-faith repair is to restore it — tripping that assertion. The doc and the test now point in opposite directions, and the doc is the one written in bold. - Fix: delete the mandate at
:177-179and point the sentence at what actually gates now — a row appended toPROVENANCE-CHANGES.md, enforced byscripts/check-vendored-provenance-log.mjsfrom thepolicyjob — and drop "below the integrity hash" at:170.
- Rating this Important rather than a suggestion for one reason: it does not merely go stale, it instructs the exact action the PR's own new test exists to forbid.
Suggestions (1)
- [tests]
scripts/__tests__/provenance-union-merge.test.mjs:46— the static-invariant block asserts the hash is absent from the content but nothing asserts it is absent from the prose, which is why the finding above survived a commit that was specifically sweeping for it. One line beside the existing AC3 test would close the class rather than this instance:Cheap, and it fails at this head — which is the property that makes it worth adding rather than a comment.assert.doesNotMatch( read(VENDOR_DIR + "/PROVENANCE.md"), /must\s+update the integrity hash/i, "PROVENANCE.md still mandates updating a hash BLO-35109 deleted", );
Strengths
- The binary-log fix is done properly rather than minimally. Rejecting on
!Number.isFiniteinstead of reordering the comparisons is the right shape, the errordetailnames the actual likely causes (stray NUL, non-UTF-8) instead of restating the failure, and:54-58records why the check exists — that-\t-makes every comparison false — so the next reader cannot delete it as redundant. - The new test asserts the failure reason, not just
ok: false, and its comment explains that this is what makes it a real mutation test: with the guard removed the destructive-rewrite case stops being reported as unverifiable, and the no-source-change case fails open outright. Both cases are present. That is the distinction between a regression test and a test that would pass on broken code. - The blank-line suggestion from the previous head was implemented, and implemented better than proposed:
/^\+\s*\|/at:100with a note at:96-97establishing that the+++ b/pathheader cannot match, since the character after its leading+is neither space nor|. I confirmed that, and confirmed a whitespace-only append is now rejected while a real row still passes. PROVENANCE-CHANGES.mdobeys its own rule 3 — the table runs to the last line of the file, with no trailing prose that would turn every append into an interior edit. The three rules are stated as invariants with their reasons, not as etiquette.- The workflow wiring holds up under independent check at this head:
PR_BASE_SHA/PR_HEAD_SHAare workflow-level with a realmerge_groupfallback (pr.yml:28-29),policycarriesfetch-depth: 0(:72) with no job-levelif:, and the identical three-dot invocation already exists at:385,:93,:654and:689in that same job — so this follows an established pattern rather than inventing one. - The deleted CI step is replaced by a comment explaining what was removed and where the property moved, instead of vanishing silently. That is the right disposition for a step whose absence would otherwise read as an oversight.
- I re-checked the "exactly five places" claim at
PROVENANCE.md:205because it is the kind of dated, countable assertion that rots. It verifies:package.json×1,package-lock.json×2, this file ×2. My first count said six — it had matched the illustrative0.2.6-blockcast.1in the semver explanation at:195, which is not an occurrence of the live version.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
The gate now publishes `gate/ally-comment-findings` as a CheckRun as well as a StatusContext, so keying the exclusion on `__typename === "StatusContext"` stripped one copy and read the identically-named twin as CI. Measured live on #1957 @68f3f598: 23 rollup rows, both copies FAILURE, and the check-run copy alone produced `gate/ally-comment-findings=FAILURE` — a permanent false hold, since a mirror never moves on its own. It leaks into the settle rule too: once the status copy is filtered the twin is the newest datable row, so it resets the floor on CI that finished hours earlier (#1981). Excluded contextually rather than untyped: a check-run is a mirror only while a status of the same name sits beside it in the same rollup. That twin is what proves it is a duplicate reading, and it preserves the reason the typing exists — an Ally-named check-run with no twin is the publishing workflow and a red one is a real failure. Fails in the same safe direction as before: if the status copy ever stops being published its twin reads as CI, which can only over-hold. Also drops the stale measurement behind the old comment (#1821 @5cc6a70e, "all three Ally rows are StatusContext"), which is what made the untyped reading look safe. Suite 51 -> 55. Both new guards mutation-tested: reverting the twin exclusion fails 3, dropping the twin requirement for an untyped name match fails 2. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Ally (Important, at b4a1997) found that PROVENANCE.md:177-179 still said in bold that any vendored change must update the integrity hash and that the vendor_claude_k8s job fails otherwise. Both halves are false at this head: BLO-35109 deleted the hash (PROVENANCE.md:95 says so) and this PR removed the CI step that printed it. A contributor following the mandate would restore a hash, which is exactly what the AC3 test forbids. A milder instance at :170 said the changes log is excluded from the hash below. Replace the mandate with the real gate: a row appended to PROVENANCE-CHANGES.md, enforced by scripts/check-vendored-provenance-log.mjs from the policy job. Drop the "integrity hash below" clause at :170. PROVENANCE.md is in the guard's NOT_SOURCE list, so no log row is needed. Also take Ally's suggestion: one assertion in provenance-union-merge.test.mjs that PROVENANCE.md contains no "must update the integrity hash" prose, so the class is closed rather than the instance. Controls: node --test scripts/__tests__/provenance-union-merge.test.mjs 16/16 pass; node scripts/check-vendored-provenance-log.mjs --base origin/master --head HEAD ok; grep -c '^[0-9a-f]{64}$' over vendor/**/PROVENANCE*.md is 0. Negative: reverting the doc edit fails the new assertion (15/16); appending a 64-hex line fails AC3 (15/16); both restored. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Lease: pushing a fix for Ally's Important at b4a1997. PROVENANCE.md:177-179 no longer mandates updating the deleted integrity hash; the sentence now points at the real gate, a row appended to PROVENANCE-CHANGES.md enforced by scripts/check-vendored-provenance-log.mjs from the policy job, and the 'excluded from the integrity hash below' clause at :170 is dropped. Also took the Suggestion: one assertion in provenance-union-merge.test.mjs that PROVENANCE.md carries no 'must update the integrity hash' prose. Verified: node --test scripts/tests/provenance-union-merge.test.mjs 16/16 pass, node scripts/check-vendored-provenance-log.mjs --base origin/master --head HEAD ok, 64-hex grep over vendor/**/PROVENANCE*.md is 0. Negative controls: reverting the doc edit fails the new assertion (15/16), appending a 64-hex line fails AC3 (15/16), both restored. Non-force push on top of b4a1997. 🤖 Generated with Claude Code |
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: 6be3bbf
The one finding from the previous head is fixed, and the suggestion beside it was implemented too. This head is a single focused commit (6be3bbf7f, +11 test / +6-4 doc) and I verified the fix by execution rather than by reading the commit message.
Clean at this head.
Prior Findings Dispositioned (1)
- prior:b4a1997 important 1 — fixed —
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:177-181— the bolded mandate is gone and replaced with what actually gates now: "There is no integrity hash to update (see Integrity). What gates a vendored change now is a row appended to PROVENANCE-CHANGES.md:scripts/check-vendored-provenance-log.mjs, run from thepolicyjob, fails any PR that touches vendored source without one." The milder second instance is fixed as well —:170now ends at "(BLO-34872)." with the dangling "excluded from the integrity hash below" removed. Confirmed against the file fetched at this exact head: zero matches formust update the integrity hash, zero 64-hex lines, and the#integrityanchor resolves to the real### Integrityheading at:93. The doc no longer contradicts itself, and it no longer points the opposite way from the test.
Critical Issues (0)
Important Issues (0)
Strengths
- The suggested prose assertion was added and it is a genuine mutation test, which is the property that separates a regression test from one that would pass on broken code. I checked that directly rather than assuming:
/must\s+update the integrity hash/imatches the previous head's text and does not match this one, so the test fails atb4a1997and passes here. The\s+is load-bearing and correct — the old mandate wrapped mid-phrase across a newline, which a literal-space regex would have missed. - Its comment at
:64-66records why the assertion exists — the content check cannot see prose, and a contributor sent looking for a missing hash will repair it by restoring one, tripping the AC3 assertion. That is the causal chain, not a restatement of the assertion, so the next reader cannot delete it as redundant. - The fix corrects the claim rather than deleting the passage.
:177-181now names the replacement mechanism, the script, and the job it runs from, so a contributor arriving at the sentence that used to mislead them is routed to the thing that actually gates. - Wording is consistent across both sites that describe the gate —
:97-100and:177-181both say "touches vendored source", which matches the guard's actual behaviour:NOT_SOURCEatcheck-vendored-provenance-log.mjs:38exemptsPROVENANCE.mdandLICENSE. This commit is its own worked example — it changes onlyPROVENANCE.mdand a test, so it correctly needs no log row. - I swept for the same defect class beyond the two files that produced the last two findings, since that class had already survived one targeted sweep. Clean at this head in
.github/workflows/pr.yml(both surviving 64-hex mentions,:309and:1244, are past-tense comments explaining what was removed and where the property moved), rootCLAUDE.md, and the vendored tree's ownCLAUDE.md,README.md,.gitignoreandpackage.json. The version half of this PR is clean too::205states "Do not bump it per-PR" outright, so there is no surviving bump mandate to mirror the hash one. - Method note, because it changes how much the sweep above is worth: my first pass used GitHub code search and returned zero hits for all three queries. That was a false clean — a positive control for
PROVENANCE, which is onmaster, also returned zero, so the index does not cover this repo. The results above are from direct contents-API fetches at this head instead.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
At the time of this review policy is green (that is the job running the new guard), Build and six of seven test shards are green, and General tests (server 4/4) is still in flight. The PR is also BEHIND master, so it will need an update before it can land.
|
This PR is clean at its current head but still has an outstanding code-owner review request (kkroo, allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
This branch has not been deployed
1 participant
Thinking Path
Linked Issues or Issue Description
pr.ymltriggers onpull_request: branches: [master], so a PR based on a non-masterbranch gets nopolicyorVendored claude_k8s adapterrun at all. fix(vendor): stop every vendored-tree PR conflicting on the provenance log (BLO-34872) #1963's only review (COMMENTED,b4084ddb2) is already stale against its own head, so nothing is lost by folding it in.gh pr list --searchonprovenance,vendor,merge=union,PROVENANCE— only fix(vendor): stop every vendored-tree PR conflicting on the provenance log (BLO-34872) #1963, linked above. No other open PR touchesvendor/paperclip-adapter-claude-k8s/PROVENANCE*.What Changed
-blockcast.Nper-PR (PROVENANCE.md, documentation only — no version value is altered). Measured 2026-09-21: the version appears in exactly five places, all inside the vendored directory, and nothing outside reads it. The image builds the package from source and packs it with a glob (mv paperclip-adapter-claude-k8s-*.tgz), so the number never reaches the Dockerfile — which already said so atDockerfile:618: "claude_k8s — editvendor/paperclip-adapter-claude-k8s/and open a PR. Nothing to pin or bump." Removes three of the four remaining hunks.Verify provenance manifestCI step.scripts/check-vendored-provenance-log.mjs, wired into thepolicyjob: if a change touches vendored source,PROVENANCE-CHANGES.mdmust gain a row. It also enforces append-only on that log unconditionally, which is what makes fix(vendor): stop every vendored-tree PR conflicting on the provenance log (BLO-34872) #1963'smerge=unionsafe — a union cannot reconcile an edit, so a rewritten row would be silently duplicated on the next concurrent append.scripts/__tests__/provenance-union-merge.test.mjs— 13 tests, including behavioural ones driving the guard against real git history in temp repos..gitattributesmerge=unionline andPROVENANCE-CHANGES.md.Why the hash is deleted rather than replaced in kind
Three reasons, ascending in importance:
merge=unionstructurally cannot reach it. A union keeps both hashes, and CI'sgrep -oE '^[0-9a-f]{64}$' … | head -1would then resolve the provenance verdict by sort order rather than by the tree, failing permissively on one of the two orderings.What the hash was actually for — vendored source does not change without being recorded — survives. A state invariant stored in the tree becomes a transition invariant read off the diff, so nothing is stored and nothing can conflict. The in-diff review surface is now the log row itself, which a reviewer can read, rather than an opaque hash nobody could verify.
Verification
Two concurrent realistic vendored changes rebase with no manual conflict resolution. Scratch branches, each editing a different vendored source file and appending a row:
Full transcript: BLO-35109 → AC1 document.
Tests —
node --test ./scripts/__tests__/provenance-union-merge.test.mjs→ 13 pass, 0 fail. Includes that no provenance file carries a 64-hex line at all, so no merge ordering can introduce a second candidate, and that the guard rejects a mutated vendored file carrying no log row.Mutation-tested — every guard reverted one at a time, each turning the suite red:
NOT_SOURCEdropsPROVENANCE.mdNOT_SOURCEdropsLICENSENOT_SOURCEgrows to cover sourcemerge=unionremovedTwo guards survived their first mutation and were fixed rather than documented, which is the entire point of running them:
NOT_SOURCEtest iteratedNOT_SOURCEitself, so deleting an entry deleted its own case — the suite stayed green on exactly the change it existed to catch. The paths are now written out literally.p !== LOGfilter had no failing mutation because it was unreachable. It was dead code and is deleted, not commented.actionlintclean on.github/workflows/pr.yml.Risks
Low, with two things a reviewer should weigh:
$PR_BASE_SHA...$PR_HEAD_SHAand runs on bothpull_requestandmerge_group, so every path ontomasterpasses through it — but a direct push tomaster, if one were possible, would not be caught the way a stored hash would have been. I did not verify branch protection forbids that, and I am not claiming it does.PROVENANCE-CHANGES.mdhitCONFLICT (add/add)— union merge needs a common ancestor. Observed incidentally while debugging the AC1 harness. Once this lands, the file is in every branch's base and normal union merging applies. Vendored PRs branched before it (fix(claude-k8s): cap tool-spawned children with RLIMIT_DATA so an orphaned Bash child cannot OOM-kill the run (BLO-34477) #1937, fix(claude_k8s): adopt run Secrets with a merge PATCH, not a PUT the SA cannot make #1873) will still conflict with each other onPROVENANCE.mduntil their log rows move into the new file.Not a runtime change: no adapter source, no published version, and no image content is altered. The
Vendored claude_k8s adapterjob keeps itsnpm ci/tsc/npm teststeps unchanged; only the manifest step is removed.Model Used
claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution, via the Paperclipclaude_k8sadapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template