Skip to content

fix(ci): fence nightly forward-roll baseline to proven ancestor - #5021

Open
Flandern1211 wants to merge 2 commits into
apache:mainfrom
Flandern1211:fix/forward-roll-provenance-ancestor
Open

Flandern1211 wants to merge 2 commits into
apache:mainfrom
Flandern1211:fix/forward-roll-provenance-ancestor

Conversation

@Flandern1211

@Flandern1211 Flandern1211 commented Sep 8, 2026

Copy link
Copy Markdown

Summary

The forward-roll baseline was selected from the live npm nightly tag without proving that the published build came from a commit reachable from the checkout under test. This could make the qualification run against a future or otherwise unverifiable baseline.

This change:

  • reads the Nightly source commit from npm's SLSA provenance attestation, because tarball publication leaves gitHead unset;
  • verifies the source commit with git merge-base --is-ancestor for PR and Nightly forward-roll validation;
  • fails closed when provenance is missing, the commit is invalid, or Git history is unavailable, while distinguishing a genuine non-ancestor result;
  • makes the CLI qualification checkout full-history and adds the release script/workflow paths to the trigger list;
  • carries the proven source commit through predecessor evidence and revalidates it before release mutations;
  • adds an explicit fence_predecessor lane mode: formal release and ASF tagged-candidate validation keep provenance identity checks but do not require a live Nightly to be an ancestor of a frozen tag;
  • rejects legacy CLI argument arities that could silently bypass the fence;
  • adds deterministic provenance, ancestry, revalidation, mode, CLI-arity, dependency-selection, and workflow-policy coverage.

Fixes #4447

Review focus

The earlier #4604 implementation used gitHead, which is null for current maka-agent Nightly tarball publications, and assumed full Git history in a validation lane that used the checkout default. This PR uses the live SLSA endpoint and explicitly sets fetch-depth: 0 in the affected qualification job.

The formal release lane shares the reusable qualification workflow but validates a frozen product tag. Its caller now explicitly opts out of ancestor fencing while still requiring the signed Nightly source identity to remain current. PR and Nightly forward-roll callers keep the fence enabled.

Verification

  • node --test --test-concurrency=1 scripts/release-cli-publication.test.mjs scripts/release-cli-workflow-policy.test.mjs scripts/asf-npm-workflow-policy.test.mjs — 40 passed, 0 failed after rebase onto upstream/main a49ba754
  • npm exec biome check scripts/release-cli-publication.mjs scripts/release-cli-publication.test.mjs scripts/release-cli-workflow-policy.test.mjs scripts/asf-npm-workflow-policy.test.mjs — passed
  • npm exec biome lint scripts/release-cli-publication.mjs scripts/release-cli-publication.test.mjs scripts/release-cli-workflow-policy.test.mjs scripts/asf-npm-workflow-policy.test.mjs — passed
  • npm run format:check — passed
  • npm run lint — passed
  • YAML parsing for all affected workflows — passed
  • git diff --check — passed
  • npm --workspace @maka/runtime-host run build after the sequential workspace build — passed
  • Live resolver checks in both fence and unfenced modes — passed against the current npm Nightly; source commit 11b93d111a796f79a91635b00d6acd105ecea435 resolved correctly
  • npm run check:asf-npm, npm run check:product-release-identity, and npm run check:model-metadata — passed

npm run build was attempted after the rebase. Core, Storage, MCP, Runtime, Runtime Host, Computer Use, Eval, and CLI built successfully; the repository then failed in the untouched UI workspace on existing API/type mismatches involving settledText, autoScroll, trailingAction, WorkHub types, and related current-main exports. The failure is outside this PR's changed files.

Historical review

Successful related release/qualification PRs reviewed: #4313, #3481, #4131, and #3192. They bind release evidence to exact immutable identities, preserve existing release boundaries, and add deterministic policy tests.

Unsuccessful or superseded related PRs reviewed: #4604 (CLOSED, invalid gitHead premise and shallow-history assumption), #4418 (CLOSED, superseded by #4412), and #3241 (CLOSED, redirected to broader release-verifier work). This revision addresses #4604's metadata and formal-lane scope failures.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex investigated #4447, implemented the provenance/ancestry fence and lane-mode correction, added tests, ran verification, and reviewed the diff. An independent review pass found and fixed the missing source-commit binding in the final revalidation step.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally; targeted lint/format/build checks pass, while the full repository build remains blocked by the unrelated UI errors described above

Does this PR entail a change in behavior?

  • Yes — forward-roll validation now fences the Nightly source, while formal/tagged candidate lanes explicitly retain provenance identity without applying the incompatible ancestor relation
  • No

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review (Command Code) — not an approval

The mechanism is right and I verified the parser against the live registry payload (dist-tags.nightly → SLSA slsa.dev/provenance/v1 bundle → resolvedDependencies[0].digest.gitCommit), and git merge-base --is-ancestor exit codes behave as assertCommitIsAncestor assumes (0 ancestor / 1 not-ancestor / 128 unusable). The problem is where the fence is applied, not how.

P1 (Must-Fix) — the fence is unconditional in a job the formal release lane shares, and there it can never hold: it blocks the release.

release-cli-stage.yml checks out the frozen product tag and passes it in:

# .github/workflows/release-cli-stage.yml:59-64, 73-76
- uses: actions/checkout@... 
  with:
    ref: v${{ inputs.version }}
    path: product-source
...
source_commit="$(git -C product-source rev-parse HEAD)"
...
# :86-91
uses: ./.github/workflows/cli-package-validation.yml
with:
  source_commit: ${{ needs.authorize.outputs.source_commit }}

Inside the reusable workflow, the qualification job (the one that resolves the predecessor) checks out that same commit — ref: ${{ inputs.source_commit || github.sha }} (cli-package-validation.yml:350-354) — and resolves with HEAD as the fence head:

# :375-377
- name: Resolve the current npm Nightly as immutable evidence
  id: predecessor
  run: node scripts/release-cli-publication.mjs resolve-nightly-predecessor "$GITHUB_OUTPUT" HEAD

fencedAncestorHead = 'HEAD' flows into the new check, which is unconditional:

// scripts/release-cli-publication.mjs:336-343
if (fencedAncestorHead !== undefined) {
  assertCommitIsAncestor({ commit: sourceCommit, head: fencedAncestorHead, ... });
}

and fails hard on exit 1 (:243-248).

Why the formal lane can never satisfy it: the product tag is created at the approved candidate commit, and the npm vote window is ≥72 h (ASF_NPM_RELEASE.md, RELEASE_CHECKLIST.md), while npm-publication.yml keeps publishing a Nightly from main on a daily cron. After the vote window, the current Nightly's source commit is a descendant of the tag commit, so git merge-base --is-ancestor <nightly> <tagCommit> exits 1 and validation fails. The pre-PR design deliberately accepted either direction for exactly this transition (cli-package-validation.yml passes --expect-epoch-relation any), so this is a behavior change for the formal lane, and it blocks a protected publication path before any npm mutation.

(A second, independent way it fails there: the tag checkout is fetch-depth: 1 in authorize (:64), and a checkout of a tag does not bring descendants into history — so even the mechanism can't be evaluated against a newer Nightly.)

Smallest sound fix: make the fence a lane decision rather than an unconditional default — add a workflow_call boolean input (default true) and pass false from release-cli-stage.yml, keeping it on for the ci.yml forward-roll and Nightly lanes. If the fence must apply to the formal lane too, the resolver has to select the newest published Nightly whose source commit is an ancestor of HEAD, rather than rejecting the only relation that lane can produce.

P2 (Should-Fix) — the pre-PR argument arities still exist and silently disable the fence.

// :736
if (command === 'resolve-nightly-predecessor' && (args.length === 1 || args.length === 2)) {
  const [output, fencedAncestorHead] = args;
// :747
if (command === 'assert-nightly-predecessor' && (args.length === 3 || args.length === 4)) {

With the 1-arg form, fencedAncestorHead is undefinedincludeSourceCommit defaults to false (:311) → no provenance fetch, no ancestry check, and no source_commit written to the outputs. The fence disappears silently instead of failing closed, which contradicts the PR's "fails closed" claim. The same applies to the 3-arg assert-nightly-predecessor: expectedSourceCommit === undefinedincludeSourceCommit: false (:366) → no provenance revalidation at publish time, and the comparison current.sourceCommit !== expectedSourceCommit becomes undefined !== undefined. No in-repo caller uses the short forms, so they exist only as doors past the new guard.

Related footgun: an empty head is not "unfenced" — '' !== undefined, so the check runs and merge-base --is-ancestor <sha> "" exits 128, producing a misleading "history or commit is unavailable" error rather than skipping. Mode selection by arity/emptiness is ambiguous in both directions; a named mode (UNFENCED or an explicit boolean) would be unambiguous.

P3 (Nice-to-have) — extraction predicate does not match the admitting predicate. :280-289 admits a statement only when some dependency matches the URI and a 40-hex digest, but then extracts with resolvedDependencies.find(d => d.uri === expectedSourceUri) without the digest check. With two entries for the same URI the first malformed one wins. It fails closed downstream, so this is cosmetic, but the returned value should come from the same predicate that admitted it.

Review-relevant risks. This is CI/release tooling, a protected area: the change can block or affect the formal npm release. Under CONTRIBUTING.md this needs independent human review. I verified the workflow guards that were preserved (apache/maka + refs/heads/main assertion, environment: npm-publication, the nightly dist-tag assertion before publish, no floating tags) — those are intact.

Required conclusion.

  1. Optimal for the actual problem? No. The mechanism is right, but an unconditional fence in a resolver shared by two lanes with opposite ancestor direction is the wrong placement.
  2. Production code that can be deleted? The legacy arities at :736 (1-arg) and :747 (3-arg), and with them the derived default at :311.
  3. Low-quality tests to delete or replace? none identified to delete — the new unit tests are substantive and use the real payload shape. One gap to close: no workflow-policy assertion pins the 4th positional argument, so deleting "$PREDECESSOR_SOURCE_COMMIT" from all three call sites would leave every assertion green while the 3-arg fail-open disables revalidation at publish time.
  4. Deeper refactor required? Not large, but the predecessor resolution should take an explicit fence mode rather than inferring it from optional parameters.
  5. Ready to merge? No — P1 is a release-blocking regression in a protected path; P2 is a two-line change that makes the fence real.
  6. Residual risks / verification gaps: I could not execute the release lane, so P1 is derived from the code path plus the documented release process rather than an observed failing run. The repo has no prior formal maka-agent release (dist-tags.latest is still the alpha), so this would first bite at the first real formal release. In PR lanes the fence is largely inert (the PR merge ref normally has the current main as a parent), so CI may pass while the formal lane breaks.

Approval boundary. This is automated review; it is not an approval, and it does not count as the independent human review that release/CI changes require. No approve was submitted.

@Flandern1211
Flandern1211 force-pushed the fix/forward-roll-provenance-ancestor branch from af8f2c0 to dabec6e Compare September 11, 2026 02:51
@Flandern1211
Flandern1211 force-pushed the fix/forward-roll-provenance-ancestor branch from dabec6e to 362c3e7 Compare September 11, 2026 02:55
@Flandern1211

Copy link
Copy Markdown
Author

Addressed the review findings in the rebased head 362c3e73.

  • The reusable validation workflow now has an explicit fence_predecessor boolean. PR and npm Nightly forward-roll validation use fence; formal release-cli-stage and tagged ASF candidate validation use unfenced, retaining signed provenance identity/revalidation without requiring a Nightly descendant/ancestor relation that would block a frozen tag release.
  • Legacy one-argument resolver and three-argument assertion forms are rejected. The CLI now requires an explicit fence|unfenced mode and a proven source commit for predecessor revalidation.
  • Provenance extraction now returns the same URI+40-hex dependency that admitted the statement.
  • Added policy coverage for all caller modes, explicit arguments, and the final source-commit revalidation binding.

After rebasing onto upstream/main@a49ba754, the focused release/workflow suites pass 40/40; full format and lint pass; Runtime Host build passes after the sequential workspace build; YAML and diff checks pass. The full repository build still reaches and fails only on existing UI type/API mismatches outside this PR.

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up (Command Code) — verified resolved

I re-checked the head after your push.

The P1 is fixed. The predecessor mode is now explicit — parseNightlyPredecessorMode accepts fence or unfenced and throws on anything else — and the lane wiring matches what I recommended: the qualification job fences on pull requests, while release-cli-stage.yml passes fence_predecessor: false for the formal release lane, where the frozen tag can never have a newer Nightly as its ancestor. The release-blocking path is closed.

The P2 is fixed as well — replacing the arity-based dispatch with a named mode removes the fail-open short forms and the empty-string ambiguity, and the version bump/history checks are unchanged at publish time.

Thanks — this is exactly the shape the two lanes needed. (Automated review; not an approval; release tooling still needs independent human review.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forward-roll baseline is not fenced to an ancestor of the change under test

2 participants