test(release): bind lifecycle evidence to attested runs - #61
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if ( | ||
| !/^https:\/\/github\.com\/ahrav\/magic-context\/actions\/runs\/\d+$/.test(runUrl) || | ||
| repository !== "ahrav/magic-context" || |
There was a problem hiding this comment.
Minor nit: "ahrav/magic-context" is now hardcoded twice here (the runUrl regex and this repository check), in addition to the existing `https://github.com/${source.repository}` templating used elsewhere in attestationCertificateMatches. Consider hoisting it into a single EXPECTED_REPOSITORY constant next to QUALIFICATION_WORKFLOW_PATH so a future repo rename/fork can't leave one occurrence stale.
There was a problem hiding this comment.
Fixed in a0fe3e0. Hoisted EXPECTED_REPOSITORY next to QUALIFICATION_WORKFLOW_PATH.
There were in fact two occurrences, not one. The second is easy to miss because the regex escapes the slash (ahrav\/magic-context), so it does not match a plain grep for the repo string.
Rather than let the new constant appear twice, the run-URL check now reuses the claimedRunId parser, so the URL shape is defined in exactly one place:
repository !== EXPECTED_REPOSITORY ||
claimedRunId({ runUrl, repository, headSha, workflow }) === undefined ||That turned out to matter for more than drift. The old inline regex hardcoded the repository, but claimedRunId did not, and claimedRunId is the parser feeding the workflow API path. Details in the kilo anchoring threads on this file.
ReviewScope: Overall: this is a solid, well-scoped hardening of the release-evidence gate. Highlights:
Nits (posted inline where applicable):
Things I couldn't fully verify from this diff alone (not blocking, just flagging for awareness):
No correctness or security bugs found in the changed logic itself. |
eab2181 to
aeecf53
Compare
aeecf53 to
f06ce60
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f06ce60e7a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const demandStart = await runNativeLifecycle( | ||
| { kind: "retained-fd", fd: retained.fd }, | ||
| { | ||
| command: "start", | ||
| envelope: mergedEnvelope(opencodeEnvelope, piEnvelope), | ||
| deadlineMs: 60_000, |
There was a problem hiding this comment.
After the explicit stop, this calls runNativeLifecycle with command: "start" directly and even supplies the merged startup envelope itself. It therefore bypasses the actual managed-demand implementation in McHostLifecyclePolicy.demandStart and the module transport readiness/reconnect path; if that product path is broken, this release smoke still reports a successful demandStart and can provide false qualification evidence. Trigger the restart through a real managed consumer instead of invoking the native launcher directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one in this PR, but I want to be precise about what I verified rather than wave it off.
Your reading of the code is correct, and so is the symbol reference — I checked, because McHostLifecyclePolicy.demandStart does exist (packages/plugin/src/shared/mc-host-lifecycle/policy.ts:257) and it is not what this script calls. The smoke drives runNativeLifecycle with command: "start" and supplies the merged envelope itself, so it does bypass demandStart's coalescing of concurrent demands (shared = this.start(request.startupEnvelope)) and the createLazyManagedDemandStart path in module-transport.ts that builds the envelope via buildManagedStartupEnvelope.
Where I stopped short of your conclusion: the field that would carry the false claim, product_flows[].managed_demand_passed, has no producer wired to this script's demandStart result anywhere in the tree. It appears only in the verifier, the verifier's tests, and the template default of false. So as it stands this is an under-coverage gap in the canary, not a live path that mints false qualification evidence. That distinction is why I am not treating it as a blocker on this PR.
Why not simply fix it here: routing the restart through a real managed consumer means driving the policy through the retained-fd ownership model inside the owner subprocess, and I cannot validate that change in this environment — the script needs two live providers, real credentials, and a running daemon (MC_HOST_CANARY_BROCA_MODEL plus the Broca credential set). Landing an unvalidated rewrite of the canary's restart path would trade a known coverage gap for an unverified one, in the script whose entire job is to be trustworthy.
So I am tracking it separately as a coverage item, with this note attached: if managed_demand_passed is ever populated from this script's output, the gap becomes a correctness bug at that moment and has to be closed first. Happy to be pushed if you think it is worth blocking on now.
| const runInvocationUri = fields.runInvocationURI; | ||
| const attestedRunId = | ||
| typeof runInvocationUri === "string" | ||
| ? runInvocationUri.match(/\/actions\/runs\/(\d+)(?:\/attempts\/\d+)?$/)?.[1] | ||
| : undefined; |
There was a problem hiding this comment.
For a re-run of the same GitHub Actions workflow run, runInvocationURI distinguishes attempts using /attempts/<n>, but this regex discards that component and compares only the run ID. Consequently, proof artifacts and the installed evidence can come from different attempts—including artifacts from a failed attempt combined with the latest successful run returned by the API—while still satisfying the claimed single workflow source. Preserve the attempt number and verify every attestation and the workflow-run result against that exact attempt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in cd48362, which landed before this review round. The code you are quoting is the a57976b revision.
matchedAttestationAttempt now requires the attempt and returns it, and the run check resolves that specific attempt rather than the run:
- the certificate match requires
/actions/runs/<id>/attempts/<n>, and the attempt is the function's return value; - the API call is
GET repos/{repo}/actions/runs/{id}/attempts/{n}, assertingconclusion === "success",head_sha,path, andrun_attempt === n; - every qualified proof is forced to share one workflow source and one attempt, so proof artifacts cannot be mixed across attempts.
Your reasoning about the run-level conclusion reflecting only the latest attempt is exactly what that function's docstring now records.
One residual gap your comment implies did survive, and I fixed it in a0fe3e0: the attempt regex was anchored on the trailing path only, so it never checked that the invocation URI belonged to this repository. It is now anchored on the full expected prefix including the claimed run id. Regression test: "run urls and invocation uris are anchored to the expected repository".
| return ( | ||
| control.type === "harness_dispatch" && | ||
| control.run_id === runId && | ||
| control.harness === harness | ||
| ); |
There was a problem hiding this comment.
The smoke asks each real provider to return OK, but the replay assertion accepts any stream containing only the harness_dispatch control unit. If either adapter drops assistant text, or replay loses all response units while retaining dispatch metadata, run.status can still become completed and this production canary passes even though users receive no answer. Require the matching assistant_message for this run and validate its text before accepting the replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a0fe3e0.
The assertion did accept a stream whose only matching unit was the harness_dispatch control unit, so a replay that kept dispatch metadata while losing every response unit passed with run.status === "completed" and no answer for the user.
Added a second assertion requiring the assistant text for this specific run, walking the producer's actual envelope shape ({kind: "control", unit: {type: "assistant_message", run_id, message: {content: [{type: "text", text}]}}}, per crates/mc-host/src/broca/protocol.rs):
const assistantText = assistantTextForRun(replay, runId);
assert.ok(
assistantText.trim() !== "",
`${harness} replay carried no assistant text for run ${runId}`,
);One deliberate deviation from your suggestion. You asked to validate the text, and the prompt is "Reply with exactly OK." — but I assert non-empty text rather than equality with OK. Asserting the exact string against two live providers would make the canary flake on trailing punctuation or whitespace, and a canary that cries wolf gets muted, which costs more than the extra precision buys. Non-empty assistant text is what separates "the user got an answer" from "the user got nothing", which is the failure you identified.
Also factored the envelope narrowing into a shared controlUnit helper so both assertions agree on the shape.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| export function workflowRunApiPath(source: WorkflowSource): string | null { | ||
| const runId = source.runUrl.match(/\/actions\/runs\/(\d+)$/)?.[1]; |
There was a problem hiding this comment.
WARNING: workflowRunApiPath extracts run ID without validating repository prefix or supporting attempt suffixes
workflowRunApiPath matches //actions/runs/(\d+)$/ against source.runUrl without verifying that the URL origin and path begin with https://github.com/${source.repository}/actions/runs/. If passed a URL referencing a different repository or host, it discards the target origin and constructs a path under repos/${source.repository}/actions/runs/<id>. Additionally, if runUrl contains an /attempts/<num> suffix, the trailing regex fails to match and returns null.
Validate that source.runUrl starts with https://github.com/${source.repository}/actions/runs/ and permit optional /attempts/\d+ trailing segments.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in a0fe3e0. This was the more serious of your two sub-claims.
I verified the exploit shape concretely rather than reasoning about it. With the old parser:
workflowRunApiPath({
runUrl: "https://github.com/evil/fork/actions/runs/123456",
repository: "ahrav/magic-context",
...
})
// => "repos/ahrav/magic-context/actions/runs/123456"A run id belonging to another repository was silently requalified as ours, exactly as you describe: the parser matched only the trailing segment while the API path was rebuilt under source.repository. claimedRunId now requires the full https://github.com/${source.repository}/actions/runs/ prefix, and that is the single definition of the run-URL shape — the proof loop's duplicate inline regex is gone.
On the attempt suffix I am declining, deliberately. runUrl is the human-facing run URL and must not carry /attempts/<n>: the attempt is signed into the certificate's runInvocationURI, and accepting an attempt-suffixed runUrl would let evidence assert an attempt that nothing verified. The trailing $ is load-bearing there, so the new test asserts an attempt-suffixed runUrl returns null.
| const runInvocationUri = fields.runInvocationURI; | ||
| const attestedRunId = | ||
| typeof runInvocationUri === "string" | ||
| ? runInvocationUri.match(/\/actions\/runs\/(\d+)(?:\/attempts\/\d+)?$/)?.[1] |
There was a problem hiding this comment.
WARNING: runInvocationURI matching is unanchored, permitting cross-repository URI matching
runInvocationUri.match(/\/actions\/runs\/(\d+)(?:\/attempts\/\d+)?$/)?.[1] extracts the trailing run ID from any string without checking the repository or host prefix. While sourceRepositoryURI is checked separately, runInvocationURI in the attestation certificate should be strictly verified to start with https://github.com/${source.repository}/actions/runs/${claimedRunId} so that attestations referencing run IDs across foreign repositories cannot satisfy this check.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in a0fe3e0. I agree with the defense-in-depth argument, though I could not construct a live exploit.
Now anchored on the full expected prefix including the claimed run id, instead of matching the trailing path:
const invocationPrefix =
`https://github.com/${source.repository}/actions/runs/${runId}/attempts/`;Why it was not exploitable in practice: sourceRepositoryURI and buildConfigURI are both checked against source.repository within the same certificate, and GitHub's issuer will not mint a certificate whose sourceRepositoryURI is our repository while runInvocationURI names another. But that argument rests on the issuer's behaviour rather than on anything this code enforces, which is a poor thing for a release gate to depend on — so it is now enforced here. The regression test covers a foreign-repository runInvocationURI carrying a matching trailing /attempts/1.
| !/^https:\/\/github\.com\/ahrav\/magic-context\/actions\/runs\/\d+$/.test(runUrl) || | ||
| repository !== "ahrav/magic-context" || | ||
| !/^[0-9a-f]{40}$/.test(headSha) || | ||
| headSha !== expectedHeadSha || |
There was a problem hiding this comment.
WARNING: Unconditional headSha !== expectedHeadSha check causes non-qualified validation to fail
In validateInstalledReleaseEvidenceAgainstArtifacts, the requirement that expectedHeadSha be a valid 40-character SHA is conditioned on requireQualified (line 585). However, inside the proof artifact verification loop, headSha !== expectedHeadSha is evaluated unconditionally. When validating schema only (--check-schema) or running in an environment without the qualified checkout commit, expectedHeadSha is "" or differs from the proof artifact commit, causing --check-schema to throw an error.
Condition the commit match check on requireQualified: (requireQualified && headSha !== expectedHeadSha).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed empirically and fixed in a0fe3e0.
I reproduced this rather than taking the reading on faith: I wrote the regression test first, then reverted just the one-line gate. Without the fix, schema-only validation over evidence that carries proof artifacts fails with
mc-host installed release evidence: proof artifact
registry_package:@cortexkit/mc-host-darwin-arm64 has no immutable workflow run
Your diagnosis is exact. The SHA-validity requirement is gated on requireQualified while the proof-loop comparison was not, so --check-schema at any commit other than the one the proofs name failed closed. Applied your suggested shape:
(requireQualified && headSha !== expectedHeadSha) ||Two notes on scope. It was latent rather than live: the committed template ships proof_artifacts: [], so CI's --check-schema never entered the loop — it only bites on real evidence. And it is not a weakening of the gate: --check still binds proofs to the checkout commit, and the pre-existing test "qualified evidence is bound to the release checkout commit" still proves a mismatched commit cannot gate GA.
New test: "schema-only validation accepts proofs from another release commit". Worth noting no test previously exercised requireQualified = false with proof artifacts present, which is how this got through.
| method: "run.status", | ||
| params: { run_id: runId }, | ||
| }, | ||
| { timeoutMs: Math.max(1, deadline - Date.now()) }, |
There was a problem hiding this comment.
WARNING: Polling loop timeout math triggers spurious 1ms timeout errors upon deadline expiration
In verifyBrocaRoutes, the run.status polling loop computes timeout with { timeoutMs: Math.max(1, deadline - Date.now()) }. When the deadline is reached or exceeded, Math.max(1, ...) clamps the timeout to 1 ms. The request is dispatched with a 1 ms timeout and fails with a client timeout error, throwing an exception before the deadline check assert.ok(Date.now() < deadline, ...) at line 261 is evaluated.
Check Date.now() < deadline before making the request or assert that remaining time is strictly positive before dispatching client.request.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in a0fe3e0. Traced the loop body order:
client.request(..., { timeoutMs: Math.max(1, deadline - Date.now()) })— throws here on expiryif (status.state === "completed") break;assert.ok(state === "queued" || state === "running", ...)assert.ok(Date.now() < deadline, "did not complete before deadline")— unreachable
So an over-deadline run surfaced an opaque 1 ms transport timeout instead of the deadline failure. For a release-qualification canary that is worse than the raw flake, because it misattributes why qualification failed.
Took your first option, checking before dispatching, since it also removes the clamp entirely:
const remainingMs = deadline - Date.now();
assert.ok(remainingMs > 0, `${harness} did not complete before deadline`);The trailing assertion is now redundant — the loop re-enters through the same check after the 50 ms sleep — so it was removed.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summaries (5 snapshots, latest commit 618119e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 618119e)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit a0fe3e0)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit cd48362)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit a57976b)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit f06ce60)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Reviewed by gemini-3.7-flash · Input: 76.7K · Output: 7.1K · Cached: 199.9K |
f06ce60 to
a57976b
Compare
The target test report binding derived its expected observations from the observations it was checking, so a proof that cited `test_report_path` and `test_report_sha256` as null produced an expected object identical to itself and cleared the GA gate with no report at all. The citation is now mandatory for a target proof, confined to tmp/mc-host-test-reports/, distinct per target, and its bytes must parse as a passing report for that target, so an unrelated repository file can no longer stand in. The injected verifier seams used `??`, which treats a verifier that declines with null the same as an absent override and fell through to the real `gh attestation verify`. The negative test therefore passed only because the subprocess failed, and the unit suite shelled out to a credentialed, network-dependent binary. Presence of the option now selects the seam. A run-level conclusion reflects only the latest attempt, so re-running a failed run blessed artifacts signed by the attempt that failed. The attestation now yields the signing attempt, every proof and the installed evidence must share it, and the attempt endpoint confirms that attempt succeeded. Verification is memoized per attempt, replacing one identical gh api call per proof, and a failed check reports the gh exit status and stderr so a missing actions: read scope is not misread as tampering. An unreadable cited report degrades to a domain failure instead of a raw ENOENT, and the GA proof matrix asserts against the schema entry point it actually exercises rather than staging artifacts and stubs that the earlier schema rejection never consulted.
Address review findings on the release-evidence gate and the cross-harness canary. Anchor run identity to the expected repository. `claimedRunId` matched a bare trailing `/actions/runs/<id>`, so a run id borrowed from another repository or host parsed successfully and was then rebuilt as a path under `source.repository` - silently requalifying a foreign run as one of ours. The run URL and the certificate's `runInvocationURI` are now both matched against the full expected origin, repository, and run prefix. The proof loop reuses that single parser instead of repeating the pattern inline, which also removes the duplicated `ahrav/magic-context` literal in favour of one `EXPECTED_REPOSITORY` constant. Scope the release-commit binding to the GA gate. The proof loop compared `head_sha` against the checked-out commit unconditionally, while the requirement that the commit even be a valid SHA is gated on `requireQualified`. Schema-only validation of real evidence therefore failed closed whenever it ran at any other commit, which contradicts what `--check-schema` promises. The comparison is now gated on `requireQualified`; `--check` keeps the binding, and the existing test still proves a mismatched commit cannot gate GA. In the canary, prove the polling deadline before dispatching. An expired budget was clamped to 1 ms, so the request failed as a client timeout and threw before the deadline assertion could name the real cause. Also require the replayed assistant text for the run: the previous assertion accepted a stream carrying only the `harness_dispatch` control unit, so a replay that kept dispatch metadata but lost every response unit still passed while the user received no answer.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0fe3e0c10
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `https://github.com/${source.repository}/${source.workflow}` && | ||
| artifactMatches | ||
| ) { | ||
| return attempt; |
There was a problem hiding this comment.
When the same workflow run is re-run, the proof bytes remain identical because their source URL deliberately omits the attempt, so a digest can have attestations from both a failed attempt and a later successful one. This returns the first matching array entry and then verifies only that attempt; therefore a successful rerun is rejected whenever the failed attestation appears first, or when different artifacts return the same attempts in different orders. The inspected gh attestation verify --help only promises one JSON-array entry per verified attestation and specifies no ordering, so collect the matching attempts and choose a common successful attempt rather than depending on array order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 618119e. Good catch — and the mechanism you identified is a direct consequence of a decision I defended earlier in this same review round, which makes it worth spelling out.
In the kilo thread on workflowRunApiPath I argued that runUrl must not carry /attempts/<n>, because the attempt belongs in the certificate. That is exactly what makes your scenario reachable: because the proof's run_url omits the attempt, a re-run produces byte-identical proof bytes, so one subject digest ends up attested in the failed attempt and again in the successful one. The two facts are the same fact.
Both failure modes you named were real:
- the failed attempt appearing first was chosen and then rejected by the run check;
- two artifacts whose arrays arrived in different orders selected different attempts and tripped "must share one workflow run attempt", appearing to disagree about their source.
You are also right about the contract: gh attestation verify promises one array entry per verified attestation and specifies no ordering, so depending on position was unfounded.
matchedAttestationAttempts now returns every matching attempt, and the gate intersects those sets across all proof artifacts and the installed evidence, then requires one shared attempt to have concluded successfully. Candidates are tried in attempt order rather than arrival order.
That forced the run check to move after the proof loop, which I think is the more honest structure anyway: no single artifact can tell which of several shared attempts is the one that succeeded, so that decision does not belong in a per-artifact step. It still runs once per distinct attempt via the existing cache — the "verified once for one shared attempt" test still holds at exactly one call — and still reports workflow run is unverified when no shared attempt passes.
Regression test: "a re-run qualifies on the successful attempt whatever the attestation order". It feeds both ["1","2"] and ["2","1"] with only attempt 2 succeeding, and asserts qualification succeeds either way while the run check is consulted for attempt 1 first. I verified it fails against the old first-match behaviour:
attested workflow run is unverified (injected verifier declined)
One scope note for the record: both symptoms fail closed, so this blocked good releases rather than admitting bad evidence. It was still worth fixing at P1, because it would have surfaced as an unreproducible qualification failure right after a re-run — the worst possible time to be debugging the gate.
A digest can legitimately be attested in more than one run attempt. Re-running a workflow leaves the proof bytes unchanged, because a proof's `run_url` deliberately omits the attempt, so the same subject digest is signed in the attempt that failed and again in the attempt that succeeded. `matchedAttestationAttempt` returned the first matching array entry and the gate then verified only that attempt. Since `gh attestation verify` promises one entry per verified attestation and documents no ordering, qualification depended on array order: a successful re-run was rejected whenever the failed attempt happened to appear first, and two artifacts whose arrays disagreed on order looked like they disagreed about their workflow source. Both outcomes fail closed, so this blocked good releases rather than admitting bad ones, but it did so unreproducibly. Collect every matching attempt instead, intersect the sets across all proof artifacts and the installed evidence, and then require one shared attempt to have concluded successfully. Candidates are tried in attempt order rather than in the order the attestation array happened to arrive. The run check therefore moves after the proof loop: no single artifact can tell which of several shared attempts is the one that succeeded. It still runs once per distinct attempt through the existing cache, and it still reports `workflow run is unverified` when no shared attempt passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 618119ec47
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| if ( | ||
| observed.head_sha !== source.headSha || | ||
| observed.path !== source.workflow || |
There was a problem hiding this comment.
When --check uses the real gh api response rather than the test override, this exact comparison rejects legitimate workflow attempts: GitHub's Get a workflow run attempt response represents path with its ref suffix (for example, .github/workflows/main.yaml@refs/heads/main), while source.workflow is constrained to the bare .github/workflows/mc-host-release-qualification.yml. Consequently every otherwise successful attested attempt reaches does not match the claimed source, leaving the GA evidence gate unable to pass; compare the path component before @ while separately retaining the existing commit binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 75a4b3c. Your conclusion is right and the fix is applied, but the evidence is more interesting than a straight confirmation, so I want to record what I actually found.
I checked the live API before changing anything, because the failure you describe would be total — the gate could never pass — and that is a strong claim. Against this repository, both endpoints return the bare path:
$ gh api repos/ahrav/magic-context/actions/runs/33235772296/attempts/1 --jq .path
.github/workflows/ci.yml
Across roughly a thousand recent runs spanning three workflows and both pull_request and schedule events, not one path contains @. So the symptom is not occurring today.
But your citation is substantively correct, and that is what settles it. GitHub's documented example response for "Get a workflow run attempt" is:
"path": ".github/workflows/build.yml@main"(@main rather than @refs/heads/main, but the same shape.) So the documented contract and the observed behaviour disagree, and the gate was written to accept exactly one of them.
That is decisive here for a reason specific to this PR: this comparison has never executed against a real response. QUALIFICATION_WORKFLOW_PATH does not exist yet, so no gh api result has ever reached it, and every test replaces verifyWorkflowRun wholesale. A fail-closed gate with an unexercised equality check straddling a documented/observed discrepancy is exactly the thing that should be normalized before it ever runs in anger — the failure mode is a release that cannot be qualified, diagnosed under time pressure.
Applied your suggested shape, comparing the component before @ and keeping the commit binding separate and exact. Two things worth noting:
- This makes the file self-consistent.
buildConfigURIwas already normalized the same way (.split("@", 1)[0]) when the certificate is checked, so the two places that compare a workflow path now agree. - Nothing else is relaxed. The ref is a mutable label; the immutable binding is
head_sha, still compared exactly, alongsiderun_attempt. Dropping the ref costs no real strength.
I also pulled the comparison out into workflowRunAttemptMatchesSource so it is reachable from a test at all — it previously sat behind the spawnSync call to gh. New test "a run attempt matches whether or not its path carries a ref suffix" accepts the bare, @main, and @refs/heads/main forms and still rejects a different workflow path, a mismatched head_sha, and a mismatched attempt. Verified it fails against the old exact comparison.
GitHub's documented example response for "Get a workflow run attempt" reports `path` with a ref appended (`.github/workflows/build.yml@main`), while observed responses for a directly triggered workflow in this repository return the bare path. The attempt check compared `path` exactly, so it accepted only one of the two documented shapes. That matters because the gate fails closed and this path has never been exercised end to end: the qualification workflow does not exist yet, so no real `gh api` response has ever reached the comparison. Had GitHub returned the documented suffixed form, every otherwise valid attempt would have been rejected as not matching the claimed source and the GA gate could never pass. Compare the path component before `@`, which is how `buildConfigURI` is already normalized when the certificate is checked. Nothing else is relaxed: the ref is a mutable label, and the immutable binding remains `head_sha`, still compared exactly, alongside the attempt number. The comparison moves into `workflowRunAttemptMatchesSource` so it can be tested directly. It previously sat behind a `spawnSync` call to `gh`, which the test override replaces wholesale, so no test could reach it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75a4b3cb25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const PAYLOAD_INDEX_PATH = "release/mc-host-payload-index.json"; | ||
| const STOP_PROVENANCE_PATH = "release/mc-host-n-minus-one-stop.json"; | ||
| const SHA256_RE = /^[0-9a-f]{64}$/; | ||
| const QUALIFICATION_WORKFLOW_PATH = ".github/workflows/mc-host-release-qualification.yml"; |
There was a problem hiding this comment.
The reviewed tree does not contain .github/workflows/mc-host-release-qualification.yml: git ls-tree -r 60a2cfd lists only the four existing workflows, and a repo-wide search finds this path only in the verifier and its tests. Because qualified proofs must name this exact path and gh attestation verify is invoked with it as --signer-workflow, no workflow at the commit being qualified can produce an acceptable attestation, so the GA evidence gate cannot pass. Add the workflow that emits these proofs or bind this constant to an existing committed signer workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct on the facts, and intentional. The tree really does not contain .github/workflows/mc-host-release-qualification.yml — only ci.yml, claude-code-review.yml, retrieval-benchmark.yml, and shm-hardening-optin.yml — and the path appears nowhere outside the verifier and its tests. Your inference follows: with QUALIFICATION_WORKFLOW_PATH required on every qualified proof and passed as --signer-workflow, no attestation produced at this commit can satisfy the gate.
That is the declared state of this PR rather than an oversight. From the description, under "Remaining Qualification":
The gate stays fail-closed until the protected qualification workflow, exact Linux/macOS campaigns, installed registry flows, and production Synapse evidence exist.
So the gate cannot pass yet by construction, and that is the intended posture: the verifier lands first, fail-closed, and the workflow that emits the proofs follows. claude[bot] reached the same reading independently earlier in this round.
Declining both suggested remedies, for the same reason. Adding the workflow is a separate change with its own review surface — it needs branch protection, OIDC permissions, and the campaign definitions, none of which belong in a verifier PR. And binding the constant to an existing committed signer workflow would be worse than the current state: it would make the gate passable by attestations from ci.yml, which is not a protected release-qualification workflow, so the gate would report qualified on evidence no release campaign produced. A gate that cannot pass yet is safe; a gate that passes for the wrong signer is not.
Worth noting the one thing this does cost, which your comment implicitly identifies: the verifyWorkflowRunAttempt path has never executed against a real API response, because nothing has ever produced a run for this workflow. That is exactly how the ref-suffix mismatch you flagged separately (#3885687028) survived to review, and it is now fixed. If you spot other checks on that same unexercised path, they are worth raising for the same reason.
| if (key === "evidence") { | ||
| continue; |
There was a problem hiding this comment.
When a production-qualified lock is accompanied by an evidence file whose artifact citations and production_qualified value still match but whose test_only or unqualified fields are stale or tampered, this unconditional skip makes release:qualify:check accept it. The later loadReleaseContext check validates only schema/release, the qualification boolean, and artifact citations, while the installed-release verifier merely hashes this file, so the full release:check can qualify evidence that requireQualificationEvidence would explicitly reject. Continue allowing the file to be absent where intended, but compare and validate it whenever it is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I verified this end to end and your mechanism is real. I am not applying the fix unilaterally, because doing so reverses a deliberate decision this PR made explicitly — so this needs an author call rather than a bot-driven flip. Everything needed to make that call is below.
The chain holds. This PR changed the --check loop from "evidence may be absent, but must match when present" to "evidence is never compared":
if (options.check) {
+ if (key === "evidence") {
+ continue;
+ }
if (!existsSync(path)) {
- if (key !== "evidence") drift.push(`${relative}: missing`);
+ drift.push(`${relative}: missing`);
} else if (readFileSync(path, "utf8") !== expected) {And your reading of the downstream coverage is exact. In build-mc-host-payload.ts, the present-file branch validates only schema, release_contract_sha256, release.id/release.version, lock.production_qualified === evidence.production_qualified, and then consumes evidence.artifacts. It never looks at test_only or unqualified. The installed-release verifier only hashes the file into qualification_sha256. So with the skip in place, nothing compares those two fields against the canonical derivation, and a stale or edited file that keeps its citations and production_qualified intact passes the full release:check.
That matters specifically because those are the fields the script's own header names as the rejection signal:
entries release engineering has not qualified yet are explicitly
qualified: false, which propagates to non-production evidence thatrequireQualificationEvidence(the U2/U6 build gate) rejects
I also confirmed the comparison is feasible: evidenceText is derived inside the same generate() call from the contract, manifest, and digests, so expected is exactly what that invocation would write. A mismatch means stale, deterministically — this is not machine-dependent. (The unrelated closure source node size changed drift comes from closureCatalog, a different output.)
Why I stopped. The PR does not merely skip the file, it added a test asserting the skip:
test("check mode ignores local evidence whether absent or present", ...)
writeFileSync(join(root, OUTPUT_PATHS.evidence), '{"stale":true}\n');
expect(generate(root, { check: true }).drift).toEqual([]);
So "a present-but-stale evidence file is not drift" is a stated intent with a test behind it, not an accident. Your fix cannot land without rewriting that test, and I am not willing to silently invert an intentional decision on a release-security gate. I did apply and then revert the change locally to confirm it is a clean one-line restoration.
The decision. These two cannot both hold:
--checktolerates a stale localtmp/evidence file, so a developer's leftover artifact never fails the check (the new test's intent); or--checkis the only thing comparing that file'stest_only/unqualifiedagainst the canonical derivation, so skipping it leaves those fields unvalidated everywhere (your finding).
If the goal was only to stop a developer's leftover file failing the check, your formulation gets both — absence stays tolerated, drift is caught — and the test should become "absent is not drift, stale is". If instead the file is genuinely expected to diverge legitimately in some flow I have not found, then the skip is right and the gap should be closed where those fields are actually consumed, by validating test_only/unqualified in loadReleaseContext rather than by hashing alone.
I lean toward your version plus the amended test, but it is the author's call. Flagging rather than deciding.
1 participant
Summary
Release checks now bind proof bytes, target reports, the installed evidence record, checkout commit, signer workflow, and workflow run into one attested chain. Cross-harness smoke exercises two owners, stop plus demand-start, real request completion, replay, and cleanup.
Verification
bun run test:release: 135 tests passedRemaining Qualification
The gate stays fail-closed until the protected qualification workflow, exact Linux/macOS campaigns, installed registry flows, and production Synapse evidence exist. npm publication remains intentionally out of scope.
Stack