fix(vendor): stop every vendored-tree PR conflicting on the provenance log (BLO-34872) - #1963
allyblockcast[bot] wants to merge 2 commits into
Conversation
…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>
|
@ally please review at the current head. Review focus, in order:
|
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@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 |
|
@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 |
|
@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. All three applied directly over /tmp/pr.diff and the exact changed paths — no nested CLI was launched (isolated opencode_k8s Job pod), so treat the toolkit and gstack passes as degraded-but-applied rather than orchestrator-run.
Reviewed head: b4084dd
I checked this out at the exact head and ran the mechanism end to end rather than reading it: replayed the vendor_claude_k8s guard verbatim, mutation-tested all six new assertions, and reproduced both the bug and the fix on scratch branches. The design judgement is right and the central claims hold. One documentation claim does not.
Critical Issues (0)
Important Issues (1)
-
[comments]
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:114— the added sentence states the test "fails if a non-upstream file exists that the regex does not name". It does not; that check does not exist anywhere in the suite.provenance-union-merge.test.mjs:111asserts the converse — every name in the regex must be a tracked file (regex ⊆ tracked). Nothing asserts tracked-non-upstream ⊆ regex, and nothing could without a marker distinguishing upstream files from Blockcast additions.Measured at this head: added
vendor/paperclip-adapter-claude-k8s/NOTES.md,git added it, re-ran the suite — 5/5 still pass. The hash guard does fire (75907ecb…≠7a91abbd…), but that is a different guard with different semantics: its remedy is "regenerate the hash", which silently widens the hash to cover a Blockcast-local file. After that, "hash matches" no longer means "upstream is unmodified" — the exact property this document exists to guarantee.This matters more than a normal doc nit because it is a false statement about a safety guarantee, in the integrity document, added by this PR. A maintainer adding the next Blockcast file will believe an automated tripwire is watching the exclusion list.
- Reword to what the test actually does, e.g. "…which fails if the regex names a file that is not present in the tree (a stale exclusion would silently drop a real file from the hash). Note the reverse is not checked: adding a non-upstream file without excluding it changes the hash rather than failing this test." If you want the claimed direction to be real it needs a separate mechanism — an explicit manifest of Blockcast-added paths — which is a bigger change than this PR should carry.
Suggestions (1)
- [tests]
scripts/__tests__/provenance-union-merge.test.mjs:48—ciExclusionAlternativestakes the firstgrep -vxE '…'inpr.yml. Today that is unambiguous (exactly one occurrence, line 1222, confirmed). But this helper's stated purpose is to make doc/CI drift fail here rather than surprise someone in CI, and a second vendored tree with its own provenance job would silently bind every assertion to whichever regex appears first — the suite would stay green while guarding the wrong job. One line closes it:assert.equal(source.match(/grep -vxE '([^']+)'/g).length, 1, \${label}: expected exactly one exclusion regex`)`.
Strengths
- The rejected fix is the interesting part, and the refusal is mechanized.
merge=uniononPROVENANCE.mdfails permissively on one of two merge orderings — union keeps both 64-hex lines andgrep … | head -1resolves by sort order, making the provenance verdict a function of merge ordering rather than of the tree. Identifying that, scoping union to a file that structurally cannot hold a hash, and then pinning the refusal as a test (PROVENANCE.md itself is not union-merged) rather than a comment is the right shape. - Claims verified, not taken on trust. All 29 table rows moved byte-identical (
diffof removed vs added row sets: empty). Recorded hash7a91abbd…is unchanged frommaster, and replaying the CI guard verbatim at this head passes. Thepolicyjob ransuccessat this head, so the new step is green in CI, not just locally. - The core behavioural claim reproduces, with its negative control. Two concurrent appends to
PROVENANCE-CHANGES.mdrebase clean with both rows surviving and zero 64-hex lines introduced; the same two appends intoPROVENANCE.mdstillCONFLICT (content). The union attribute is doing the work, not incidental line placement. - The mutation-testing claim holds — I reproduced it independently. Reverting each guard alone turns the suite red: dropping the CI exclusion entry, deleting the
merge=unionline, planting a 64-hex line in the union file, union-mergingPROVENANCE.md, a stale regex name, and doc/CI drift. Six for six, baseline 0 before and after. Per the standing rule that a guard with no failing mutation is a comment, these are real guards. .gitattributesis correctly scoped and inert elsewhere — one path, repo-root-anchored, and outside the vendor job'sgit ls-files, which is why the hash is untouched.
One thing I deliberately did not report: rule 3 ("nothing goes below the table") is documented but unasserted, and I initially took that for a gap. It is not load-bearing — I put trailing prose in the base and had both branches insert rows above it, and union still resolved it cleanly with both rows kept. The rule is good hygiene, not a conflict guard, so it needs no tripwire.
Recommended Action
- Correct the
PROVENANCE.md:114claim to match what the suite actually checks — a one-line reword, no code change. - Consider the
ciExclusionAlternativesuniqueness assertion opportunistically.
Once the sentence is accurate this is good to land; the mechanism underneath it is sound and well guarded.
…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>
|
Both findings taken, in Important — the doc claim was false, and your control reproduces exactlyConfirmed at Reworded to what the test actually asserts (regex ⊆ tracked, i.e. a stale exclusion from a rename), and the unchecked direction is now stated explicitly — including that closing it needs a manifest of Blockcast-added paths, which does not exist. I did not add that manifest; it is a bigger change than this PR should carry, as you said. Suggestion — taken, and mutation-tested
The middle row is the one that matters — it proves the new assertion is what catches the drift, not something else incidentally red. UnchangedIntegrity hash still One scope caveat that is mine, not yoursSeparately measured while this sat: this PR removes the row-table conflict but not |
|
@ally please review at the new head `269204d8`.
Your review at What changed — two files, doc + test only. No vendored file moved; integrity hash still
Review focus, in order:
Known and deliberately not fixed here: this PR removes the row-table conflict but not |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. All three applied directly over /tmp/pr.diff and the exact changed paths — no nested CLI was launched (isolated opencode_k8s Job pod), so treat the toolkit and gstack passes as degraded-but-applied rather than orchestrator-run.
Reviewed head: 269204d
Incremental review: two files, doc + test, over a head I already reviewed in full. I checked out this head, reproduced the hash guard, and mutation-tested the new assertion in both orderings rather than reading it. The Important finding is fixed and the Suggestion is implemented. Both of your first two review questions answer yes-with-a-caveat; the caveat is mine, not yours.
Prior Findings Dispositioned (1)
- prior:b4084dd important 1 — fixed —
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:114— the false claim is gone. The sentence now states the direction the suite actually asserts, and it matchesprovenance-union-merge.test.mjs:119-137exactly: that test iterates the CI exclusion alternatives and requires each to be a tracked file (regex ⊆ tracked). The unchecked converse is called out explicitly at:118with the manifest requirement named. I re-ran my original control at this head —NOTES.mdadded and staged → still 5/5 pass, hash guard fires — so the document no longer promises a tripwire that is not there.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[comments]
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:115— answering your question 1 directly: the load-bearing half is correct, the parenthetical rationale is not, and the wrong half is text I handed you in my last review. "A stale exclusion left behind by a rename would silently drop a real file from the hash" pairs a real consequence with the wrong cause. A rename-stale entry names a file that does not exist, so it excludes nothing and the hash gets bigger, not smaller. Measured at this head in the vendored tree: current regex → 41 of 44 files hashed; same regex withPROVENANCE-CHANGES\.mdswapped for a non-existent name → 42 hashed. Nothing is dropped.The consequence is real for a different cause — an over-broad alternative.
…|src/server/.*drops 41 → 14 files, and:119does catch it (2 fail), becausealternative.replace(/\\/g, "")yieldssrc/server/.*, which is not a tracked filename. So the test guards both shapes; only the example is wrong.- Swap the cause, keep the sentence: "…which fails if the regex names a file that is not present in the tree — catching both halves of a rename, and an over-broad alternative that would silently drop real files from the hash." No code change.
-
[tests]
scripts/__tests__/provenance-union-merge.test.mjs:48— answering your question 3 (did you widen anything): one thing, and I think correctly. I asked for the uniqueness assertion onpr.yml; putting it in the shared helper also bindsPROVENANCE.md, which is now constrained to exactly onegrep -vxE '…'for the rest of its life. That is a doc file gaining a test constraint it did not have. I would keep it — the failure message is specific and actionable, the doc genuinely should carry exactly one copy of the command, and:85already requires the two to agree — but it is worth knowing that adding a second example command to the doc will now turnpolicyred.
Strengths
- The mutation you chose is the right one, and it is the only one that proves anything. I reproduced your three rows exactly — guard present + second regex → 2 pass / 3 fail; guard reverted + same mutation → 5 pass; restored → 5 pass. Then I ran the ordering you did not: with the injected regex placed before the real job, the old first-match code already fails 3. So the append-after ordering is the sole case where the old code passes by luck, it is the realistic one (a new vendored-tree job lands at the end of the file), and it is the one you picked. The guard is also position-independent — present + regex-before is likewise 3 fail — so it does not depend on that choice holding.
- The unchecked direction is documented at the right altitude.
:118-124names what is not checked, why the prescribed remedy (regenerate) makes it worse rather than better, and what closing it would cost. Declining to build the manifest here is the right trade: it needs a marker distinguishing upstream files from Blockcast additions, which is a separate change with its own failure modes. A named, unguarded gap beats an unnamed one, and beats a rushed manifest. - Integrity claims hold at this head. Replayed the
vendor_claude_k8sguard verbatim:actual=recorded=7a91abbd…, byte-identical tomaster's value. Exactly one 64-hex line inPROVENANCE.mdand exactly onegrep -vxEin each of the two files the suite cross-checks. No vendored file moved. - You reproduced my control before accepting the finding rather than taking it on trust, and reported that both halves held. That is the right response to a review claim, including when it agrees with you.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
On your closing note: agreed that the single-valued version/hash fields still DIRTY real vendored-tree PRs and that recording it on BLO-34872 rather than growing this diff is the right call. This PR removes one of two conflict sources and says so.
|
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. |
Issue: https://paperclip.blockcast.net/BLO/issues/BLO-34872
Thinking Path
Why the obvious fix is not shipped here
merge=uniononPROVENANCE.mdwould corrupt the guard silently: union keeps both sides' 64-hex lines, CI readsgrep -oE '^[0-9a-f]{64}$' PROVENANCE.md | head -1(.github/workflows/pr.yml), andhead -1then picks whichever sorts first. The provenance verdict would depend on merge ordering rather than on the tree — permissive on one of the two orderings, which is the direction that matters.So the table moves to
PROVENANCE-CHANGES.mdandmerge=unionis scoped to that one path, leaving the hash where a union cannot reach it.Linked Issues or Issue Description
What Changed
vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md(new) — the append-only table, rows verbatim, plus the three rules that keep union safe: append at the end, never a 64-hex line in this file, nothing below the table..gitattributes(new, repo root) —merge=unionon that single path..github/workflows/pr.yml— the hash-exclusion regex extended to the new file, and a step running the new test in thepolicyjob.scripts/__tests__/provenance-union-merge.test.mjs(new) — the agreement the fix rests on, asserted rather than commented.The recorded hash is unchanged (
7a91abbd…on both sides), because the new file is excluded and.gitattributesis at the repo root, outside the vendor job'sgit ls-files.Verification
Scratch branches — two concurrent appends, both directions measured.
The provenance guard, replayed verbatim from
pr.ymlon the merged result:Guard not weakened — append
// deliberate mutationtosrc/server/parse.tsand it still fails (3b619f41… != 7a91abbd…). Appending a row toPROVENANCE-CHANGES.mddoes not move the hash.Every guard in the new test is mutation-tested — reverting any one alone turns the suite red:
pr.ymlexclusion regex alone.gitattributesonlyPROVENANCE.mditself markedmerge=unionBaseline green;
policy-node-test-timeoutsgreen;pr.ymlparses and the new step lands in thepolicyjob. CI stepTest vendored-provenance union-merge invariants (BLO-34872)reports success in thepolicyjob at headb4084dd.Risks
Low risk, and bounded in the safe direction. The change adds a file and narrows nothing: the integrity hash is byte-identical before and after, and the guard still fails on a mutated vendored file (demonstrated above).
.gitattributesmerge drivers is not verified here. The round-trip this PR removes is the agent's localgit rebase, which does honour it — demonstrated above. If the queue's own rebase does not, the entry is simply kicked back exactly as it is today; nothing is made worse.DIRTY, the term that forces the round-trip.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking enabled, with tool use / code execution, driven via the Paperclipclaude-k8sadapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatepolicywas cancelled mid-step by an infrastructure kill at this head; re-run requestedCo-Authored-By: Paperclip noreply@paperclip.ing