test(release): authenticate qualification evidence - #64
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 |
ReviewReviewed
Both fixes are well covered by tests, and I traced the existing test fixtures ( Minor observations (non-blocking)
No security or correctness issues found beyond what's already fixed by this PR; no additional bugs identified. |
33c34e1 to
2b72aef
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (4 snapshots, latest commit a4344f6)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a4344f6)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit fe8ee1d)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous review (commit bc17fa3)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous review (commit 2b72aef)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by gemini-3.7-flash · Input: 105.7K · Output: 4.1K · Cached: 371.4K |
![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
Reject leading-zero semantic versions
When an authenticated daemon or catalog entry reports a version such as 00.01.000, this regex accepts it and parseInt normalizes it to [0, 1, 0], so evaluateDaemonCompatibility("mc-host/00.01.000") currently returns { ok: true }. Numeric semantic-version identifiers with leading zeroes are not canonical, despite this function and its compatibility errors promising canonical X.Y.Z values, so a malformed peer version can pass the fail-closed compatibility gate; reject leading zeroes except for the single digit 0.
ℹ️ 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".
| // that does not carry it can never have produced (or reproduce) the | ||
| // attested evidence. Failing here names the gap directly instead of | ||
| // surfacing it later as an opaque per-proof attestation mismatch. | ||
| if (requireQualified && !existsSync(join(rootDir, QUALIFICATION_WORKFLOW_PATH))) { |
There was a problem hiding this comment.
When release:evidence:check validates qualified evidence, this condition always rejects the current repository because .github/workflows/mc-host-release-qualification.yml is absent. A repo-wide search of .github/workflows and workflow references found only the four existing workflow files and no generator or renamed equivalent, so no qualified release can pass this newly added gate until the referenced signer workflow is included or the constant names an existing workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified the factual core: the file is genuinely absent, and this gate is added by this PR, not inherited from the base. ls .github/workflows shows only ci.yml, claude-code-review.yml, retrieval-benchmark.yml, and shm-hardening-optin.yml, and the only references to .github/workflows/mc-host-release-qualification.yml are this script and its test. The other mc-host-release-qualification hits in the tree are tmp/mc-host-release-qualification.json, a different artifact that happens to share the substring.
Where I disagree is the consequence. This gate is not what makes a qualified release unpassable, because two pre-existing checks already require that workflow to have run:
// line 900, already on the base branch
workflow !== QUALIFICATION_WORKFLOW_PATHEvery qualified proof's cited source.workflow must equal that same constant, and each proof's attestation is then verified with --signer-workflow ${source.repository}/${source.workflow}. GitHub can only produce such an attestation if that workflow actually ran in this repository, which requires the file to exist. So with the workflow absent, qualified evidence fails at the per-proof attestation step whether or not this existsSync check is present. Removing it would not let a GA release through; it would only move the failure later and make it read as an opaque attestation mismatch, which is what the comment above it is there to avoid.
For what it's worth, release:evidence:check currently fails earlier than either check, at readJson with ENOENT, because the installed evidence file does not exist in the checkout yet. That is true on the base branch too, so this PR does not change the outcome of that command.
So I am keeping the gate as a fail-closed assertion and treating the missing workflow as separate release work: it has to land for GA regardless, since it is the workflow that mints the attestations. Happy to drop the check if you would rather the branch carry no reference to a workflow that does not exist yet, but that trades a clear error for a vaguer one without unblocking anything.
| // re-issuing `host.status`. The observation is single-use and only a | ||
| // terminal state short-circuits; a `starting` observation still runs | ||
| // the polling probe so it can wait out startup within its own budget. | ||
| let observedStorage: "ready" | "starting" | "unavailable" | null = null; |
There was a problem hiding this comment.
When managed demands overlap, this closure-level slot is shared by every compatibilityProbe invocation, including both magic-context and synapse demands, while each Magic Context waiter later consumes whichever value was written last. A slower probe can therefore overwrite another waiter's observation, causing that waiter to report ready or unavailable from a different request or daemon generation; in the ready case, module traffic can proceed even though its own authenticated status reported storage as starting. Carry the storage observation alongside the corresponding snapshot or key it to the request and daemon identity instead of using one shared slot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in fe8ee1db. You identified a hazard this branch created while merging two independent hardenings, and the write-side reasoning is right on every point.
What I verified before changing anything:
sharedCompatibilitydedupes per data root, and its key isrootResolution.ok ? rootResolution.root : "\u0000no-root"while the closure always probesroot.root. So a real-root and a no-root key can be in flight at once, giving two concurrent writers to one slot.- A demand whose capability is not
magic-contextreturns before the storage probe, so it writes an observation it never consumes. - The reuse path never saw
expectedDaemonId; the fence I had threaded only guarded theprobeManagedStoragefallback. So untagged reuse was exactly the "different request or daemon generation" case you describe.
The slot now carries the incarnation whose host.status produced it, and is consumed only when the demanding waiter certified that same incarnation:
let observedStorage: {
daemonId: Uint8Array;
state: "ready" | "starting" | "unavailable";
} | null = null;Reuse requires expectedDaemonId !== undefined && sameDaemonId(observed.daemonId, expectedDaemonId); anything else falls through to the polling probe, which already carries the fence. I reused the existing sameDaemonId from mc-host-client/types rather than adding a comparison helper.
I took the tagging option rather than carrying the observation on the snapshot because it keeps the change inside the two closures; the snapshot is a shared single-flight value, so attaching per-waiter state to it would have widened the surface.
One limitation worth stating plainly: this is not covered by a new unit test. defaultStorageProbe and defaultCompatibilityProbe are closures internal to createManagedLifecyclePolicy that call the module-level probes directly, and passing options.storageProbe bypasses the defaults entirely, so observing them would mean restructuring production code to inject a seam. The change is a strict narrowing of when reuse is allowed, so it cannot produce a ready the old code would not also have produced. It is covered indirectly by the 124 lifecycle tests and the full plugin suite.
| command === "restart" && | ||
| record.ok && | ||
| (effects?.start_committed !== true || state !== "running" || reason !== "started") |
There was a problem hiding this comment.
When a malformed native restart result combines a pre-start failure with start_committed: true, this narrowed condition now accepts it as long as ok is false. For example, {state:"unavailable", reason:"no_data_dir", effects:{stop_committed:false,start_committed:true}} parses successfully, even though the native start_phase sets start_committed:false for every failure before spawn and authenticated publication. This lets the CLI report a start commit that provably never occurred; retain the newly supported post-commit failure outcomes while rejecting reasons and states that can only occur before the start commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one is now outdated rather than declined. It was raised against bc17fa3d, the pre-rebase head, and contract.ts is no longer part of this PR.
The branch has since been rebased onto the current base, which had already absorbed #59 through #62. Six of the eight commits here were duplicates of work that had landed, and the parseDaemonResult change was one of them, so the base's own version now applies:
git diff --quiet origin/feat/mc-host-daemon-lifecycle HEAD -- \
packages/plugin/src/shared/mc-host-lifecycle/contract.ts # identical
contract.ts does not appear in this PR's diff at all now, which is why GitHub left this comment pinned to the old commit while re-anchoring the other two.
On the substance, though, your reading looks correct against the code that is on the base branch. The condition there is
command === "restart" &&
record.ok &&
(effects?.start_committed !== true || state !== "running" || reason !== "started")so gating on record.ok does mean a result with ok: false and start_committed: true no longer trips it, where the previous effects?.start_committed === true && (!record.ok || ...) form would have. Your example of {state:"unavailable", reason:"no_data_dir", effects:{start_committed:true}} parses under that.
I have not changed it here, because doing so would pull base code into a PR whose diff no longer touches that file, and the fix wants its own test for the pre-commit reasons and states. It is worth raising against the base branch as its own change; flagging it here so it does not get lost when this comment is marked resolved.
- Fail closed and re-validate in the Synapse provider when a module restart invalidates the compatible daemon identity, so later pages never ride an incarnation that skipped compatibility validation. - Drop the discarded evaluateCompatibility call in the managed probe; applyCompatibility is the single enforcement point. - Stop fabricating storage/synapse readiness on probe short-circuit paths; only observed components are reported. - Reuse the compatibility probe's host.status storage observation so demand-start no longer opens a second connection in the common case. - Derive the compatibility stage set, order, and check ids from one exported descriptor list. - Require the qualification workflow to exist in the checkout for GA-qualified verification, and verify the shared workflow run once instead of per proof. - Share the U9 evidence identity gate between the build context loader and requireQualificationEvidence. - Restore macOS coverage for the native lifecycle binary and CLI contract in the relocated mc-host-lifecycle job. - Exercise the schema-level validator directly in the GA proof table test instead of installing unreachable artifacts.
bc17fa3 to
8d7c12d
Compare
|
Rebased onto the current Why the history was rewritten. The base had absorbed #59/#60/#61/#62 since this branch forked, and 6 of the 8 commits here were already in it: 2 were byte-identical ( On the two observations above; both are resolved by deletion, not by a comment.
const attestationResult =
options.verifyAttestation !== undefined
? options.verifyAttestation(proofPath, proof, workflowSource)
: ghAttestationJson(rootDir, proofPath, workflowSource);That keys off the property being Base additionally narrows attestations to a shared workflow-run attempt ( What this branch still contributes. Base's I dropped this branch's One resolution was a genuine merge of both sides rather than a pick: base threaded Gates on |
The managed lifecycle policy caches the storage state carried by the compatibility probe's `host.status` reply so the demand path can reuse it instead of opening a second connection. That cache was a single untagged slot, and more than one probe can write it: `sharedCompatibility` dedupes per data root, so a real-root and a no-root key can be in flight together, and a demand for a capability other than `magic-context` writes an observation it returns without ever consuming. A waiter could therefore consume a state observed on a different request or daemon incarnation than the one it certified, and a stale `ready` would let module traffic publish even though that waiter's own authenticated status reported storage as starting. The reuse path also bypassed the `expectedDaemonId` fence, which only guarded the fallback probe. Tag the observation with the incarnation whose `host.status` produced it and reuse it only for a demand that certified the same incarnation. Anything else falls through to the polling probe, which already carries the fence.
`pipe2` is a Linux extension that the Darwin libc does not provide, so the
`libc::pipe2` call failed to resolve and broke the macOS build of the
`ck-mc-host` binary outright:
error[E0425]: cannot find function `pipe2` in crate `libc`
Keep the atomic close-on-exec creation on Linux and fall back to `pipe`
followed by an explicit `FD_CLOEXEC` on Darwin. Both descriptors are wrapped
in `OwnedFd` before the flag is set so a failing `fcntl` closes them instead
of leaking a pair.
The fallback is not atomic with creation the way `pipe2` is, which matters
only if another thread execs between the two calls. The child spawned below
is the only exec this binary performs, it runs after both calls, and it keeps
the read end deliberately by dup2-ing it onto stdin while closing every
descriptor above 3.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4344f6ed2
ℹ️ 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".
| #[cfg(target_os = "macos")] | ||
| // SAFETY: pipe writes exactly two descriptors into the array. | ||
| cvt( | ||
| unsafe { libc::pipe(pipe_fds.as_mut_ptr()) }, | ||
| "envelope pipe creation failed", |
There was a problem hiding this comment.
When ck-mc-host starts on macOS with any standard descriptor closed, resolve_generation_launcher or open_log can reuse fd 0, 1, or 2 before this newly enabled pipe path runs. The child remaps stdin/stdout/stderr before copying the retained executable to fd 3, so that remapping can overwrite the launcher descriptor; a same-fd dup2 also leaves the newly applied FD_CLOEXEC flag intact. Service managers that launch the CLI with closed stdio can therefore make daemon startup time out or lose its log, so duplicate all retained descriptors above the standard-fd range before performing the child remaps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I implemented this one before I could defend it, then reverted it. The reasoning about the child is correct, but the premise it rests on cannot hold in this binary, so the fix was guarding a state that does not occur.
Everything you say about the child is accurate, and I want to be clear that I checked it rather than waving it off:
- Nothing in this repository sanitizes stdio. There is no
/dev/nullguard and noF_GETFDsweep anywhere undercrates/mc-module/src/bin/ck_mc_host/. - The ordering hazard is real if a retained descriptor is low.
dup2(log_fd, 1)would overwrite a launcher at fd 1, and the laterdup2(exe_fd, 3)would then pin the log, leavingfexecve(3)pointed at the log file. - Your close-on-exec point is the sharper half and it is also correct. A same-fd
dup2succeeds as a no-op and does not clearFD_CLOEXEC, so a read end at fd 0 or a log at fd 1 would be closed at exec rather than inherited. Worth noting this half is not Darwin-specific: Linux setsO_CLOEXECat creation viapipe2, andopen_logpassesO_CLOEXECon both platforms, so a same-fddup2would strand the descriptor there too.
Where it breaks down is the first step: ck-mc-host cannot reach resolve_generation_launcher or open_log with fd 0, 1, or 2 free, because Rust's standard library reopens closed standard descriptors before main runs. std::sys::pal::unix::init calls sanitize_standard_fds(), which points any closed standard descriptor at /dev/null, and aborts the process if it cannot.
Measured on this toolchain (1.98.0), launching a Rust binary from a parent that closed fd 0 with exec 0<&-:
open fds at main(): ["0->/dev/null", "1->socket:[...]", "2->socket:[...]", "3->/proc/.../fd"]
first open() got fd 3
The same parent running ls instead shows 0 -> /proc/<pid>/fd, so fd 0 was genuinely free — the difference is the Rust runtime, not the launcher.
Since you scoped this to macOS, that is the case I checked most carefully in the std source rather than assuming it generalizes. The poll fast path is in fact excluded there:
// The poll on Darwin doesn't set POLLNVAL for closed fds.
target_vendor = "apple",But the fcntl fallback immediately below it is not excluded for Darwin — its list is emscripten, fuchsia, vxworks, l4re, horizon, vita — and it does the same repair:
for fd in 0..3 {
if libc::fcntl(fd, libc::F_GETFD) == -1 && errno() == libc::EBADF {
open_devnull();
}
}So on darwin-arm64 and darwin-x64, the two targets this PR adds to CI, a closed standard descriptor is reopened before any of this code runs.
I also checked the one route that would bypass startup repair, a descriptor freed later at runtime. The only stdin use in the binary is std::io::stdin().lock() in read_envelope and read_launcher_envelope, which borrows the shared handle and never closes fd 0. Nothing else touches fds 0-2.
For completeness on what I discarded: I had move_above_stdio hoisting the log, launcher, and pipe read end above the standard range with F_DUPFD_CLOEXEC, plus a lifecycle_cli test that stages a real launcher and runs start from a child with fd 0 closed. That test is what settled it — it passes with the hoist removed, because the launcher lands at fd 3 either way. I would rather carry no test than one that cannot fail, and adding unsafe and a new SpawnError path to guard an unreachable state is the kind of defensive code this repository asks me not to accrete.
Happy to reconsider if you can point at a path where a standard descriptor is free after main — that would make this reachable and I would take the hoist back.
| // A `module_restarted` failure on an earlier page invalidated | ||
| // the compatible daemon identity; re-validate before this page | ||
| // so it never rides an unverified incarnation. | ||
| if (!this.initialized && !(await this.initialize(signal))) { |
There was a problem hiding this comment.
When the caller's AbortSignal fires while this newly added revalidation is awaiting initialize(signal), initialize catches the aborted raceSignal and returns false. This branch then records the current page as a retryable transport failure even though the immediately preceding check classified the same signal as cancelled; later pages are also reported as cancelled. Check the signal again after initialization so an aborted multi-page request does not misreport one page as an infrastructure failure and invite an unintended retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 348a1f84. Your read of the mechanism is exactly right, including the detail that makes it easy to miss: the abort arrives as a return value, not as an exception.
What I verified before changing anything:
initializeswallows the abort. The tail istry { return await raceSignal(initialization, signal); } catch { return false; }, so araceSignalrejection from the abort listener is folded into a plainfalsewith no way for the caller to tell it apart from a real initialization failure.- The managed path folds it the same way one level up:
const outcome = signal ? await raceSignal(demand, signal) : await demand;sits inside atrywhosecatchlogs and returnsfalse. - The disagreement you describe is real. The branch hard-coded
code: "transport"anddisposition: "retryable", while the check at the top of the same loop classifies the identical signal ascancelled. So one page of a cancelled multi-page request was recorded as an infrastructure failure and the rest as cancelled.
The signal is now re-read after initialization, with permanentFailure keeping precedence so the branch matches the ordering of the check above it:
const aborted = signal?.aborted === true;The consequence you flagged is the one I care about most: retryable on a withdrawn request invites a retry of work the caller already abandoned.
On coverage, the test is reports a page cancelled when the abort lands inside its re-validation, and I confirmed it is not vacuous — with the aborted read forced to false it fails with Expected: "cancelled" / Received: "transport", which is the exact symptom you reported.
Getting it to land in that branch took some care, and the reason is worth recording. In the injected-client shape the branch is reachable but not abortable: getSharedClient memoizes per factory, and the constructor pre-seeds metadata from fingerprint/tableEpoch/dims, so a second initialize has no suspension point and returns true before any signal can fire. The managed path does have a real await, but recertifyForRestart re-initializes eagerly, so a page that restarts leaves initialized true again for the next page. The test therefore installs the branch's documented precondition directly — managed origin, identity cleared from the first page's own response — the same way the existing tests in this file install post-rotation state through internals. That keeps the second page's signal.aborted check ahead of the abort, so the abort can only be observed inside the re-validation itself. It asserts one demand, one receipt for the first group, and that the cancelled page never reached the wire.
I left embedItems alone. It has the same !this.initialized && !(await this.initialize(signal)) shape, but it breaks without recording a failure, so there is no classification to get wrong.
…tion
Two independent gate defects found in review, each with a test that fails
without its fix.
`parseSemverTriple` matched each part with `\d+`, so a version carrying
leading zeroes parsed. `Number.parseInt` then normalized it, and
`evaluateDaemonCompatibility("mc-host/00.01.000")` returned `{ ok: true }`
because `[0, 1, 0]` falls inside the supported half-open range. The verdict
this function emits promises a canonical `X.Y.Z` value, so the check was
admitting a shape it reports as canonical. Each part is now a semver numeric
identifier: a single `0`, or a non-zero digit followed by any digits. The
only other callers are the contract's own range bounds and a catalog entry's
module version; the generated contract's bounds are canonical, so the
narrowing rejects a non-canonical peer and nothing else.
`embedItemsDetailed` classified a failed per-page re-validation as a
retryable `transport` failure. `initialize` folds an abort raised during its
own await into a plain `false` rather than a rejection, so a request the
caller cancelled mid-re-validation recorded that page as an infrastructure
failure — inviting a retry of work the caller had withdrawn — while every
later page reported `cancelled` from the check at the top of the loop. The
signal is re-read after initialization so one page can no longer disagree
with its siblings about why the request stopped.
Leading-zero semantic versions — fixed in
|
1 participant
Summary
Installed-release qualification now binds proof bytes, target reports, the checkout commit, workflow run, signer workflow, and the top-level evidence record into one attested chain. Target reports must name the expected target and carry an exact passing schema; injected verification failures remain hermetic and never fall through to ambient GitHub state.
Verification
bun test scripts/verify-mc-host-release-evidence.test.ts: 23 passedbun run typecheckbun run lintRemaining Qualification
The protected qualification workflow and real registry/platform evidence remain external. npm publication is intentionally skipped.
Stack
Layer 6 of 7 above #46. Parent: #62. Next: #65.