fix(ci): fence the forward-roll baseline to an ancestor of the change under test (#4447) - #4604
fix(ci): fence the forward-roll baseline to an ancestor of the change under test (#4447)#4604bytelazy wants to merge 1 commit into
Conversation
… under test (apache#4447) resolveRegistryNightlyPredecessor reads whatever nightly tarball the npm registry is currently advertising. CI does not check that this tarball was published from a commit reachable from the workspace under test, so the forward roll can run backwards: a newer main-branch nightly writes durable state that the older workspace code then reads, producing a false failure (the new record fails to decode) or, worse, a silent no-op (when both sides happen to speak the same vocabulary). npm version metadata carries gitHead for packages published from a repo, and the CI checkout already uses fetch-depth: 0, so we have the information without an extra fetch. resolveRegistryNightlyPredecessor now accepts an optional fencedAncestorHead and, when set, fails loudly when gitHead is missing or not an ancestor of that ref. The two CI lanes that resolve the forward-roll baseline pass HEAD as the fence, and the public CLI signature carries the resolved gitHead so the assertion can match it later if it ever needs to. Generated-by: Claude (Claude Code)
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
This fence cannot ship as written: it throws unconditionally against the real registry, and the job it throws in is the only required check.
[P0] gitHead does not exist on this package, so the fence fails closed on every run
release-cli-publication.mjs:268-274:
const gitHead = versionMetadata.gitHead;
if (fencedAncestorHead !== undefined) {
if (!gitHead) {
throw new Error(`Registry Nightly ${version} is missing gitHead metadata required for ancestor fencing (#4447)`);I queried the registry directly. Every nightly is missing it:
dist-tags.nightly = 0.2.0-dev.15.20260902
0.2.0-dev.9.20260831 … 0.2.0-dev.15.20260902 : gitHead = null (all six)
This is structural, not intermittent. npm-publication.yml:167 publishes with npm publish "$NIGHTLY_TARBALL", and npm only injects gitHead when publishing from a git working directory. A tarball publish never carries it.
The blast radius is the whole gate. ci.yml:297 runs resolve-nightly-predecessor whenever state_root_compat is true, and .asf.yaml:63 names test as the single required status check. cli-package-validation.yml:359 runs it too, and that workflow is called from npm-publication.yml, so nightly publication breaks with it.
#4447 asserts that "npm version metadata carries gitHead for packages published from a repository." That premise is false for this package, and the whole change rests on it. @Flandern1211 asked about provenance in that thread and has not been answered; that question turns out to be the crux.
The alternative authority does exist. npm's provenance attestation carries the source commit:
provenance/v1 -> eacfb46aa7ec93273bf468335f4270bba62d35a5
git+https://github.com/apache/maka@refs/heads/main
So the fence is buildable, just not on this field.
[P0] The second lane cannot answer the ancestor question either
cli-package-validation.yml:333 checks out with only ref: and persist-credentials:, so fetch-depth defaults to 1. #4447 says CI already uses fetch-depth: 0, which is true of ci.yml and not of this one. In a depth-1 clone, merge-base --is-ancestor on a genuine ancestor exits 128 with fatal: Not a valid commit name, and :229-241 catches everything and reports "is not an ancestor". So the moment the first P0 is fixed, this one fires. They need fixing together.
That catch {} is worth a separate look regardless: stdio already pipes stderr and nothing reads it, so a 128 and a 1 produce the same message. Distinguishing "not an ancestor" from "cannot tell" would have made both of these visible immediately.
Why this PR self-tests green
It changes scripts/, which is not in DURABLE_STATE_DECODER_FILES, so state_root_compat is not selected and test never reaches the fenced step on this branch. The check rollup here has only PR effort label. Worth knowing before the next iteration, since a green run on this PR proves nothing about the fence.
Is the fence worth having at all?
I want to put this question on the table rather than assume the answer, because it changes what the next step should be.
Under pull_request, ci.yml:52 checks out with no ref:, so actions/checkout takes refs/pull/N/merge, the PR head merged with the base tip. In #4447's main scenario, a branch that has not rebased and is well behind main, the nightly's source commit is therefore already in the tree under test, and the fence neither fires nor needs to. What it actually catches is narrower: a nightly published to main after the run started. That is a real window, but a small one, and the failure it prevents is a CI-visible false failure rather than a wrong artifact.
Against that, the fence adds a dependency on registry metadata that this publication path does not produce, in the single required check for the whole repo. That trade does not look right to me as it stands.
Two ways forward, and I think either is better than iterating on the current shape:
- Rebuild it on provenance. The commit is there, verified above. The cost is a second registry endpoint, attestation parsing, and
fetch-depth: 0on the validation lane. - Close this and reopen #4447 with the corrected premise.
gitHeadis unavailable, provenance is the candidate, and the window is narrower than the issue implies. Someone should decide whether that window justifies the machinery before more is built on it.
I lean toward saying so in #4447 first, since the issue's premise is what needs correcting, and this PR is a faithful implementation of it.
Evidence boundary: the registry queries, the provenance lookup and the fence's throw against real data were executed by me. The workflow reachability comes from reading ci.yml, cli-package-validation.yml, npm-publication.yml and .asf.yaml at this head. I did not run the PR's test suite, and the actions/checkout merge-ref behavior is stated from its documented default rather than observed in a run.
AI-assisted review: drafted with Maka. The registry and provenance checks are mine.
|
Closing — @Astro-Han is right on all counts, and I verified each before responding. The premise this PR implements is false for this package. I queried the registry: 's nightly ( and the five before it) all carry , because publishes a tarball and npm only injects from a git working directory. As written, this fence would throw on every run — the single required check. His alternative authority checks out too: the SLSA provenance attestation carries the source commit ( against ), so a fence is buildable on provenance, not on . That needs a second registry endpoint, attestation parsing, and on the validation lane. His point about the conflating exit 128 (shallow clone, not a valid commit) with exit 1 (genuinely not an ancestor) is also correct — that distinction would have surfaced both P0s immediately. Per his lean, I'll take the corrected premise back to #4447 rather than iterate this shape here. Apologies for the churn; I should have verified the registry metadata before implementing, not after review. |
2 participants
Summary
The released-artifact forward roll qualifies durable state by writing it with
a published build and reading it with the build under test.
resolveRegistryNightlyPredecessorinscripts/release-cli-publication.mjsresolved whatever
dist-tags.nightlythe registry advertised without checkingwhether the commit it was published from was reachable from the checkout under
test.
When
mainadvanced and published a newer Nightly while a PR run was inflight, the forward roll ran backwards: the newer Nightly wrote state that the
older workspace then tried to read, producing either a false failure (a record
it cannot decode) or a silent no-op (passing without exercising a version
boundary).
This change adds ancestor fencing to
resolveRegistryNightlyPredecessor:assertCommitIsAncestorusesgit merge-base --is-ancestor <gitHead> <head>to prove the predecessor commit precedes the ref under test.
resolveRegistryNightlyPredecessoraccepts an optionalfencedAncestorHeadparameter and, when given, requires
gitHeadto be present on the versionmetadata and asserts it is an ancestor of
fencedAncestorHead.resolve-nightly-predecessoraccepts an optional secondargument
[fencedAncestorHead]and emitsgit_headin its outputs..github/workflows/ci.ymland.github/workflows/cli-package-validation.ymlpass
HEADto fence their forward-roll baseline resolution.Fixes #4447
Verification
node --test scripts/release-cli-publication.test.mjs(Node v24.20.0):all 19 unit tests pass, including the new 4-scenario suite for
fences the predecessor commit to an ancestor of the change under test (#4447):gitHeadis a verified ancestor.gitHeadis not an ancestor.gitHeadis missing and a fence was requested.node --test scripts/ci-workflow-policy.test.mjs: all 40 workflow policychecks pass.
AI use
Tool(s) and scope: Claude Code — designed the fence, updated the script,
workflows, and test suite, and authored the commit. The commit carries a
Generated-by: Claude (Claude Code)trailer.Checklist
Does this PR entail a change in behavior?