fix(mc-host): bind runtime execution to verified identity - #59
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 |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
4a2d65c to
07af908
Compare
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (5 snapshots, latest commit f160a91)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f160a91)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit b998c56)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit 8b499a8)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit c438f4a)Status: No Issues Found | Recommendation: Merge Files Reviewed (20 files)
Previous review (commit 07af908)Status: No Issues Found | Recommendation: Merge Files Reviewed (19 files)
Reviewed by gemini-3.7-flash · Input: 65.6K · Output: 4.8K · Cached: 126.4K |
Descriptor-rooted paths assumed Linux `/proc/self/fd` magic-symlink
semantics. On darwin, a shipped supported target, `open("/dev/fd/N")` is
equivalent to `fcntl(N, F_DUPFD, 0)` and the entry is not a symlink, so
verification left the handed-out descriptor at EOF and interpreted module
arguments lost the closure tree their sibling imports resolve against.
- Rewind a resolved node descriptor after verification hashes it to EOF.
- Give interpreted module arguments a path the loader can walk back to the
closure, and inherit a module descriptor only where that path is
descriptor-rooted.
- Append `harness_dispatch` from the reserved terminal headroom like
`run_started`, so aggregate pressure can no longer fail a run the
supervisor already admitted before it spawns, and the per-run
assistant-text budget stays whole.
- Inherit only the descriptors a child's own arguments name, dropping the
closure directory descriptor from both harness adapters.
- Collapse the duplicated descriptor-root selection into one helper and
drop the now-unused `resolve_node` and `ValidatedGeneration::path`.
- Drop an unreachable disjunct in `merge_selection`.
![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: c438f4a22c
ℹ️ 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".
| BackendEvent::HarnessDispatch { harness } => { | ||
| return append_reserved(run, protocol::harness_dispatch_unit(&run.run_id, harness)); |
There was a problem hiding this comment.
A custom LlmExecutionBackend can emit HarnessDispatch repeatedly, and every occurrence is appended without charging retained bytes, incrementing replay_bytes, or recording that the one reserved dispatch slot was already used. Because the public backend contract permits emitting events synchronously, an erroneous backend can loop on this variant and grow a run's replay without either configured memory cap until the host exhausts memory; reject duplicate dispatches or track and enforce a single reserved dispatch per run.
Useful? React with 👍 / 👎.
Two public-CI jobs assumed the private ../commons cortexkit crates were available. They are not: provision-rust-ci-stubs.sh creates metadata-only stubs with an empty lib.rs, which satisfies the manifest graph but resolves no types. The shared-memory source build compiled mc-module, whose mc-core and mc-store dependencies import real cortexkit items, so the stub lib.rs produced unresolved imports and 35 cascading errors in mc-store. Drop the ck-mc-host binary build and the lifecycle_cli contract from the public matrix; the local release gate runs the whole workspace against the real sibling sources, and mc-host itself does not depend on mc-store, so the transport, host, and addon coverage this job exists for is unchanged. Check (plugin) had the opposite gap. Its `bun run test` ends in test:release, whose dependency-boundary check shells out to `cargo tree -p mc-host`, and cargo refuses to load the workspace until every path-dep manifest exists. That job never provisioned the stubs, so the check failed on a missing cortexkit-cache-core manifest rather than on the dependency edge it asserts. Provision the stubs there too; nothing in the job compiles them.
Two build breaks that public CI could not see. The shared-memory matrix stopped at an earlier failure before reaching the macOS test step, and no job compiles examples at all. `hostile_shapes_at_the_lock_names_fail_closed` planted its FIFO with `rustix::fs::mkfifoat`, which rustix gates away from Apple targets, so the test compiled only on Linux even though it carries no cfg gate. Plant the FIFO with the POSIX `mkfifo` utility instead, matching the same decision already recorded in `connection_file.rs`: a lock name must classify as a hostile shape on every platform, so the test stays compiled everywhere rather than being cfg'd out on the platform whose absence let the break through. `mkfifo` honours the umask, so the mode is set explicitly after. The `synapse_perf` example still built `LaneInfo` without the `execution_provider` field added when lanes began publishing their provider. Its inline engine computes on the CPU, so it advertises the same value a bundle-backed CPU lane does. This break also fails `cargo nextest run --workspace`, which is the local release gate.
The seven `shm-hardening-*` drills landed in shapes the evidence loader could not read, so `validate:incident-verifiers:ci` failed on the first one it reached. The job had been skipped behind a failing upstream check, so this never surfaced. The verifier resolver understood only `mc-module` and paths rooted at the e2e package. Resolve Rust verifiers through cargo's target layout instead: `--test <target>` names an integration target under `tests/`, and a `::`-qualified `--lib` filter names an inline `#[cfg(test)]` module, so the verifier is the module's own source file. A `src/`-rooted TS path is recorded relative to a package root the command never names, so it resolves against every package and requires exactly one hit rather than attributing the mutation to whichever package sorts first. Every resolved path is already required to exist, so an unhandled layout fails loudly instead of pointing at a plausible wrong file. A deferred record states that no mutation was applied and names the reason. It binds no verifier and carries no replay, so it is not evidence and does not become an evidence record; the hardening-matrix gate is what keeps the unproven claim blocked. An artifact whose records are all deferred therefore has no run command to require, and the empty-artifact guard now counts what the artifact declared rather than what survived, so it still rejects an artifact that declares nothing. The accepted snapshot moves to 20 artifacts and 27 records. The three inventory-completeness assertions still fail: `source-inventory` carries no items for these drills, and each entry needs a disposition and rationale per claim that only their author can decide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20312f1368
ℹ️ 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".
| } else { | ||
| "/proc/self/fd/" | ||
| }; | ||
| assert!(args[0].starts_with(descriptor_root)); |
There was a problem hiding this comment.
On macOS, ResolvedHarnessNode::module_path() deliberately returns closure_path because DESCRIPTOR_PATHS_ARE_FILE_LIKE is false, so the Pi entrypoint and provider-extension arguments will not start with /dev/fd/. Consequently, cargo test -p mc-host --test broca_subprocess always fails pi_argv_privacy_contract on the supported macOS targets after the fixture successfully runs; make these assertions follow the same platform-specific path semantics as module_path().
Useful? React with 👍 / 👎.
…tract The B-class m[0]/m[1] taxonomy tests failed for two different reasons. B10 and B12 hit the epoch-settling bug 2a66471 already fixed on the Pi side: seeding a memory promotes it to verified, which queues a verification event whose ONE epoch bump lands during the next turn's injection, after `setProjectEpoch` has pinned. m[0] then HARD-refolds on project_memory_change and the write lands in m[0] instead of the m[1] delta the test looks in, so the surfacing request is never found. Port the same `verifyAndSettle` drain into the OpenCode suite's `seedMemory`, which is why the Pi mirrors of both already pass. B11 asserted an invariant the design makes unreachable. `sessionMemoryBlockStillEligible` compares every rendered id's render-time content digest against the row's current digest, so an in-place rewrite of a memory that m[0] already rendered makes the cached block ineligible and m[0] MUST re-materialize; no epoch pin can hold it byte-frozen. Both renderers share that gate, so both mirrors asserted a stale-but-frozen m[0] that cannot happen. Hold the test to the contract the gate actually provides instead: the rewrite HARD-refolds m[0] onto the revised bytes, the frozen baseline is gone, and the refold advances the mutation cursor so no <memory-updates> delta remains. Pinning the epoch back stays in the setup because it is what proves the refold comes from the digest gate rather than from a project_memory_change bump. The <memory-updates> lane keeps its coverage in the renderer unit suites, which exercise it directly. Also register the seven shm-hardening drills in the source inventory. Each non-deferred drill applied a crafted invalid state, observed it red, and observed its reverted rerun green against a bound verifier, which is exactly the mechanical `verifier_evidence` disposition every other mutation claim carries. The deferred drill records only why no mutation was applied, so it contributes an item with no claims.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b998c561de
ℹ️ 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".
| let descriptor_path = validated.descriptor_root_path().join("bin/ck-mc-host"); | ||
| let expected = std::fs::read(&descriptor_path).expect("descriptor bytes"); |
There was a problem hiding this comment.
On macOS, descriptor_root_path() returns /dev/fd/<dir-fd>, but /dev/fd entries cannot be traversed as directories (as the new descriptor_path documentation itself notes). Consequently, this initial read of /dev/fd/<n>/bin/ck-mc-host fails before the replacement assertion, so a full cargo test -p mc-host fails on the supported macOS targets; restrict this test to platforms with file-like descriptor paths or use a macOS-compatible assertion.
Useful? React with 👍 / 👎.
…r justifies Review of the previous commit found the deferred-record path had turned a fail-closed error into a silent pass. Before that change the loader rejected `shm-hardening-u6.json` outright (no top-level `command`), so the gate held by accident. Skipping deferred records removed the rejection and rested the guarantee on the failure-hardening matrix gate instead, which does not provide it: `validate-shm-hardening-matrix.ts` never reads mutation artifacts, and CI runs it with `--allow-unresolved`, which exits 0. Marking a record `deferred` was therefore enough to carry an untested hardening claim through the merge gate that exists to stop exactly that. Bind the deferral to the machine-checked condition it already cites. Every deferral's stated reason is that the failure_hardening matrix is unresolved, and that is also the state in which tuple-specific execution is blocked, so a deferral is only defensible while `validateCommittedMatrix()` reports `unresolved`. Freezing the matrix unblocks that execution, so a surviving deferral then fails closed and must be replaced by a real mutation record. An `invalid` matrix is not a licence either: failing its own validation says nothing about whether the drill is inapplicable. Verified by flipping the committed manifest's status to FROZEN, observing `loadMutationEvidence` throw, and restoring the manifest byte-identically. The exported predicate carries that policy in a regression test so the gate cannot quietly reopen.
… bytes `dirtyDiffDigest` hashes the bytes of every path in the campaign's IMPLEMENTATION_FILES manifest, and that manifest covers packages/e2e-tests/tests/cache-invariants.test.ts and tests/pi-cache-invariants.test.ts. Retargeting the B-class tests changed those bytes, so the committed digest no longer described the implementation the campaign had been observed against. Regenerated through the test's own UPDATE_CLAIMS_CRASH_EVIDENCE path rather than by hand-editing the digest: the campaign re-ran the full matrix and recovered every cut, so the artifact records a real observation of the current bytes instead of asserting that an older run covered them. The matrix, summary, limits, and implementationFiles are byte-identical, which is the evidence that only the covered bytes moved and no recorded outcome changed. `limits` scopes this to a single host, so the re-observation also replaces the recorded environment and runtimes with the ones that actually produced it: linux/x64 on bun 1.3.14 and node v24.18.0, where the previous record was linux/arm64 on bun 1.4.0 and node v24.15.0. CI checks those fields for shape only. Re-run the campaign on an arm64 host if that environment is the one worth carrying in the record.
1 participant
Summary
Native lifecycle execution now stays bound to verified generation and closure objects through spawn. The layer also closes restart snapshot drift, prevents dispatch after terminal closure, makes self-exec explicitly debug-test-only, and preserves cleanup ownership across failure paths.
Verification
cargo clippy --workspace --all-targets -- -D warningscargo test -p mc-hostcargo test -p mc-moduleStack