fix(mc-host): harden cross-harness lifecycle integration - #46
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 103 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (72)
Comment |
6f7136d to
1d171f8
Compare
ahrav
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| match observed.state { | ||
| // No lock-held incarnation: nothing to signal, unlink, or clean. | ||
| LifecycleState::Stopped => DaemonResult::new(command, true, "stopped", "already_stopped"), | ||
| // No lock-held incarnation exists. Selector cleanup is best-effort | ||
| // stale-state removal under the transaction ownership boundary. | ||
| LifecycleState::Stopped => match serve::clear_active_selection() { | ||
| Ok(()) => DaemonResult::new(command, true, "stopped", "already_stopped"), | ||
| Err("unsupported active harness selection schema") => { | ||
| DaemonResult::new(command, false, "wedged", "unsupported_state_schema") | ||
| } | ||
| Err(_) => DaemonResult::new(command, false, "wedged", "internal_error"), | ||
| }, |
There was a problem hiding this comment.
The comment says selector cleanup here is "best-effort," but the implementation does the opposite: any clear_active_selection() error — including a plain, transient I/O failure on active-selection.json (permission hiccup, disk error, etc.), not just the deliberate unsupported_state_schema quarantine case — turns a stop on an already-Stopped daemon into a hard failure (wedged/internal_error). The same pattern repeats for the Running branch below (lines 1199-1205): a stop that already succeeded at stop_phase can still be reported as wedged solely because of a bookkeeping-file read/write error, even though the daemon itself is confirmed stopped.
Previously Stopped => already_stopped was a trivially-successful, idempotent no-op. Now callers that rely on stop being safe to call defensively (cleanup scripts, uninstallers, retry loops) can get a wedged result — which per the release contract implies "inspect the daemon process" — even though there's no daemon at all.
If the intent is genuinely best-effort (as the comment claims), consider only hard-failing on the deliberate unsupported_state_schema quarantine case and swallowing/logging other I/O errors instead of surfacing them as wedged. If the strict fail-closed behavior for any selection-file error is intentional, the comment should be updated to say so rather than "best-effort," since the two read very differently to a future maintainer.
There was a problem hiding this comment.
Fixed in f4cc62c. You were right that the implementation contradicted its own "best-effort" comment.
Ordinary cleanup faults now return the stopped result the command earned, on both the already-Stopped and Running branches. The unsupported_state_schema quarantine still fails: it is matched by its own error literal ahead of the catch-all, which is the same discriminator the launcher-envelope sites already use, so arm order carries the distinction rather than a new flag.
The lifecycle test had pinned the wrong behaviour (exit 1 / stopped / internal_error), so it now asserts the success contract and covers the running branch too. Both sub-blocks assert the selection file still exists afterwards, which is what proves the injected fault actually fired instead of clear_active_selection short-circuiting on an absent file.
Review summaryFocused review of the lifecycle/credential-selection hardening in Overall: the core state machine is careful and well-tested. I traced through the
One finding posted inline: the No other correctness, security, or performance issues found in the |
The auth transcript now length-prefixes and folds daemon_ver into both proofs; update the normative formula, regenerate the canonical ServerProof/ClientAuth literals, and document the big-endian length prefix so the committed Rust and TypeScript test vectors trace back to the spec.
fix(mc-host): authenticate daemon version transcripts
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`.
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.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Three review findings in the wire protocol doc, all cases where the
prose contradicted the code it specifies.
Section 7.7.3 had redefined `unavailable` as "no installed provider
serves an offered non-TCP transport", which is the case the host
excludes: permanent absence falls to `None` in negotiate_transport and
yields a reasonless TCP selection, and `FallbackReason::Unavailable` is
set only from `PreflightEligibility::DynamicallyUnavailable`. The
`unprovided_non_tcp_offer_selects_reasonless_tcp` test pins that
behavior. The same hunk had dropped the paragraph authorizing the client
re-upgrade probe, including its bounded-episode deadline and MUST NOT
list, while that behavior ships in McHostClient (`startRecovery` gates
on exactly `unavailable`) and connection.rs cites the section by number.
Both are restored verbatim, so no code-cited requirement is left without
a normative basis.
Section 7.6 specified the sanitized `host.status` metrics as only the
three component state fields, omitting the `metrics.epochs` object the
host emits for `magic-context`. That field is mandatory downstream:
evaluateEpochCompatibility requires all five epochs present, numeric,
and equal, and an absent object collapses to `{}` and fails the gate
with `incompatible_epochs`. Document the five names, the u32 bound, the
all-or-nothing sanitization, and that a sixth epoch is a breaking change
on both sides.
Also retitle 7.6, which specified `host.status` under a heading naming
only `host.shutdown`, and give `host.status` the section pointer 7.4
already gives every neighbor.
The base branch advanced past this branch's point and rewrote .beads/issues.jsonl, which conflicted with the export committed here. Resolve the conflict in favor of the base branch's export. That file is the current one: it carries 223 records against this branch's 196, loses no issue id, and moves no updated_at backwards. The export committed on this branch had rewound six records relative to the branch point -- ymc.3 from closed back to open with its closed_at, close_reason, started_at, and assignee dropped; ymc.8, ymc.10, and ymc.12 from in_progress back to open; a52 to a stale revision that predates its supersede decision; and c50.4 to a stale dependent_count -- so the committed snapshot presented finished and claimed work as available again. Taking the base branch's export removes this branch's task-tracking diff entirely, so the change now touches only documentation, CI, and .gitignore. The base export omits spec_id on ymc.8, ymc.10, and ymc.12. That field is absent from all 241 records in the live Beads database, so it is stale export metadata rather than live state, and reintroducing it would commit data the source of truth does not hold.
Two statements described behavior the implementation does not have. The bundle doc claimed macOS serves the host-only lane and reports exact readiness `unsupported` / `synapse_unsupported`. The Synapse component does carry that readiness on macOS, but no client can observe it, because `ck-mc-host serve` composes Synapse alongside Broca, Broca refuses to initialize off Linux (its crash-ownership records and sweeps read `/proc` process identity), and the composite requires every child initializer to succeed. A macOS daemon therefore exits before publication. Describe that, and name the precondition for serving the lane there: a Broca component that initializes off Linux as unavailable rather than failing. The wire protocol claimed probes take `transaction.lock` shared whenever it exists, which reads as unconditional serialization against a mutator. Shared acquisition is bounded to four attempts 25 ms apart and then samples on evidence alone, so a probe may observe an intermediate lifecycle state while a mutation is in flight. State the bound and name the bounded reread loop, not the lock, as what makes such a sample coherent.
The bundle doc called this the qualified Linux lane and stated that the offline oracle passes. The release gate says otherwise: release/mc-host-production-inputs.lock.json carries `production_qualified: false` with the blocking reason "offline semantic-oracle evidence not yet recorded against real locked ORT/model bytes", and the `linux-x64-gnu` entry in release/mc-host-payload-index.json is `qualified: false, published: false`. Describe the pins as the candidate the gate evaluates, say that the oracle runs rather than that it passes, and point at the two gate artifacts as the authority for the qualification verdict. Naming the artifacts instead of restating their current values keeps the doc correct as the gate advances.
docs(mc-host): document managed lifecycle architecture
…ifecycle # Conflicts: # .beads/issues.jsonl # crates/mc-host/examples/synapse_perf.rs # crates/mc-host/src/generation.rs # crates/mc-host/src/harness_closure.rs # crates/mc-host/src/lifecycle.rs # crates/mc-module/src/bin/ck-mc-host.rs # crates/mc-module/src/bin/ck_mc_host/serve.rs # crates/mc-module/src/bin/ck_mc_host/spawn.rs # crates/mc-module/tests/lifecycle_cli.rs # docs/evidence/claims-backfill/v84-process-crash.json # docs/mc-host-wire-protocol.md # packages/e2e-tests/tests/cache-invariants.test.ts # packages/e2e-tests/tests/pi-cache-invariants.test.ts # packages/plugin/src/features/magic-context/memory/embedding-synapse.ts # packages/plugin/src/hooks/magic-context/module-transport.test.ts # packages/plugin/src/hooks/magic-context/module-transport.ts # packages/plugin/src/shared/mc-host-client/client.ts # packages/plugin/src/shared/mc-host-lifecycle/compatibility.test.ts # packages/plugin/src/shared/mc-host-lifecycle/compatibility.ts # packages/plugin/src/shared/mc-host-lifecycle/contract.ts # packages/plugin/src/shared/mc-host-lifecycle/managed-policy.ts # packages/plugin/src/shared/mc-host-lifecycle/policy.test.ts # packages/plugin/src/shared/mc-host-lifecycle/policy.ts # scripts/build-mc-host-payload.test.ts # scripts/qualify-mc-host-production-inputs.ts # scripts/verify-mc-host-release-evidence.test.ts # scripts/verify-mc-host-release-evidence.ts
…nto feat/mc-host-daemon-lifecycle
…ifecycle # Conflicts: # .beads/issues.jsonl
![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: 5aed815142
ℹ️ 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 (this.connectionOrigin === "managed-default" && this.compatibleDaemonId === null) | ||
| throw new SynapseEmbeddingError( | ||
| "module_restarted", | ||
| "managed Synapse lane has no certified daemon identity", |
There was a problem hiding this comment.
When this provider uses the managed-default connection path without a configured demandStart, demandManagedLane intentionally returns early to support dialing an already-running daemon, but no code sets compatibleDaemonId. This new guard therefore rejects the initial models.list call with module_restarted, making initialization fail before any request is published. Only require the identity fence when a lifecycle owner actually supplied a certification, or derive a certification from the passive authenticated connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 13be47d. This was a live initialization break, not a theoretical one: with no clientFactory and no connection file the origin resolves to managed-default, demandManagedLane returns early by design so an already-running daemon can be dialled, and the only writer of compatibleDaemonId sits past that early return. A probe confirmed initialization failing with zero wire calls.
The guard now keys on the same field the certifier gates on, so the certifier and the fence cannot disagree about which lanes are owned. Concretely: no owner means dial-only and the call carries no expectedDaemonId, where the daemon's own handshake is the only fence available; an owner whose identity was cleared by a rotation is still refused. Reverting to the unconditional guard fails the new passive-dial test, so the fence assertions stayed live.
Worth noting your two comments compose: an owner that returns ok:true with no identity (3888032652) still fails closed, and now also arms backoff.
| if ( | ||
| certified !== null && | ||
| (actual === null || actual === undefined || sameDaemonId(actual, certified)) | ||
| ) { | ||
| return { client: cached, expectedDaemonId: certified }; | ||
| } | ||
| this.invalidateConnection(cached); |
There was a problem hiding this comment.
When no managed lifecycle owner is configured, demandManagedReadiness legitimately returns no daemon identity and compatibleDaemonId remains null. Every later ensureConnected call misses this condition and runs invalidateConnection(cached), so sequential requests redial and reopen all routes; worse, a second session arriving while the first request is pending closes the shared client and can turn that first request into outcome_unknown despite a healthy unchanged daemon. Treat the no-owner passive mode as reusable without an expected identity rather than invalidating it on every call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Reproduced it: with no lifecycle owner, demandManagedReadiness returns undefined, a null certification is stored, and the next call fails the certified !== null check and invalidates, so a probe shows {"connects":2,"closes":1,"sameClient":false} for two sequential requests.
Your second-order point is the more serious half and it also holds: invalidateConnection closes the shared client, and an owner close retires already-invoked work as outcome_unknown, so a second session arriving mid-request can convert the first request's outcome to unknown against a healthy, unchanged daemon. The existing passive test only makes one call, which is why it never caught this.
Tracked as magic-context-xlv with the reproduction and a fix direction (treat no-owner passive mode as reusable without an expected identity). Not patched in this pass because it belongs with the same certification-plumbing change as 3888032657 rather than as an isolated condition tweak.
| if (this.connectionOrigin !== "managed-default") return; | ||
| this.initialized = false; | ||
| this.managedDemand = null; | ||
| const remainingMs = deadlineAt - Date.now(); |
There was a problem hiding this comment.
When SynapseEmbeddingProviderOptions.now supplies a clock other than wall-clock Date.now—the supported clock used to create and evaluate the page's deadlineAt—this subtraction mixes two time bases. A virtual or monotonic clock makes every managed restart appear immediately expired because Date.now() is much larger, while an offset in the other direction can grant the lifecycle demand time beyond the page deadline. Compute the remaining budget with this.now() like the rest of the page lifecycle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in 4f8630a, which landed after this comment was written: the subtraction now uses this.now() rather than Date.now(), and the page deadline is produced from the same injected clock. There is a regression test that supplies a virtual clock and asserts the remaining budget is computed in that base.
The comment was attached to 6f7701f rather than the current head, which is why it still reads as open. The underlying concern was real, though; mixing the injected clock with wall-clock time would have made every managed restart look immediately expired under a virtual or monotonic clock.
|
|
||
| const EVIDENCE_PATH = "docs/evidence/mc-host-installed-release-evidence.json"; | ||
| const QUALIFICATION_PATH = "docs/evidence/mc-host-release-qualification.json"; | ||
| const EVIDENCE_PATH = "tmp/mc-host-installed-release-evidence.json"; |
There was a problem hiding this comment.
After moving the installed evidence to ignored tmp/, a clean release workspace has no file at this path, yet --check-schema still unconditionally calls readJson(EVIDENCE_PATH) before validation. The documented schema-only sub-gate therefore exits with ENOENT unless installed-release evidence has already been produced, contradicting its role as the earlier gate that validates the schema before evidence is read. Validate the schema from the static contract/template, or retain a committed fail-closed schema fixture for this mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. --check-schema does call readJson(EVIDENCE_PATH) unconditionally before validating, and since that path moved under ignored tmp/, a clean workspace exits ENOENT instead of validating — which inverts the documented ordering, where the schema gate runs before evidence is read.
For the record this is adjacent to, not the same as, what 6f7701f fixed: that commit corrected a test that asserted a reason string only present when the ambient tmp/ evidence existed. Same root cause (evidence moved to an ignored path, callers still assume it is there), different code path, and this one is still open.
Tracked as magic-context-r3u, with the fix direction being to branch on the schema-only mode before the read and validate a code-owned static fixture.
`libc::SIGRTMAX()` is defined only for the linux_like, hurd, and solarish targets, so the launcher's pre-exec signal reset failed to compile on macOS and took the whole `mc-host lifecycle integration (macos-latest)` job down with `error[E0425]: cannot find function ... SIGRTMAX in crate libc`. Darwin defines no realtime signals and libc exposes no `NSIG` for it, so the ceiling on that side is the highest signal the platform names, `SIGUSR2`. Resetting past it would only collect `EINVAL`, which the loop already tolerates for `SIGKILL` and `SIGSTOP`.
The consumption-gate test asserted the exact refusal reason "not production-qualified", which the gate only reports once qualification evidence exists to be judged. That evidence lives at `tmp/mc-host-release-qualification.json`, an ignored path, so the assertion held only on a machine that had already generated the file and failed on any clean checkout. CI hit the other branch: qualification evidence rejected: absent (tmp/mc-host-release-qualification.json) The gate is correct in both cases; it refuses either way. The test's claim is that the gate is reachable and fails closed, so assert the shared refusal and accept either reason. Keeping both reasons enumerated, rather than loosening to a bare substring, still rejects an unrelated error reaching this path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f8630a0b9
ℹ️ 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 (outcome.authenticatedDaemonId === undefined) { | ||
| throw new SynapseEmbeddingError( | ||
| "transport", | ||
| "managed lifecycle compatibility returned no daemon identity", | ||
| ); |
There was a problem hiding this comment.
When a configured lifecycle owner returns {ok: true} without authenticatedDaemonId—a shape explicitly allowed by SynapseDemandStart—this branch throws without updating demandFailedUntilMs. Consequently every subsequent initialization immediately invokes the lifecycle demand again, potentially driving repeated native start/probe work at request rate. Treat the missing certification as the same backoff-arming transport failure as a rejected or negative demand.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 13be47d. The return type does permit {ok:true} with no authenticatedDaemonId, and backoff was armed only for an outright rejection and for ok:false; the missing-identity branch threw straight past both. A probe showed two demands with demandFailedUntilMs still 0.
It now arms the same retry window as the other demand failures, so a lifecycle owner that breaks its own contract fails closed and cheaply, instead of driving another native start and storage probe on every subsequent call. The test asserts a second immediate initialization does not re-invoke the demand, and deleting the assignment makes it fail.
| let expectedDaemonId: Uint8Array | undefined; | ||
| try { | ||
| await this.demandManagedReadiness(deadline, signal); | ||
| expectedDaemonId = await this.demandManagedReadiness(deadline, signal); |
There was a problem hiding this comment.
When another session arrives after the first caller has completed lifecycle readiness but while connectClient is still pending, this.connectionPromise is already populated, yet this code performs another managed lifecycle demand before joining it. A burst during a slow dial can therefore launch repeated native start transactions and compatibility/storage probes even though all callers ultimately use the same connection, consuming their deadlines and adding lifecycle-lock contention. Reuse the certification associated with the in-flight connection flight and let callers join that flight without re-demanding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Reproduced with the dial held open: {"demandsBeforeSecond":1,"demandsWhileDialPending":2,"connects":1,"sharedClient":true} — so a second caller arriving during a slow dial does run a second full lifecycle demand even though both callers end up on the same connection. Backoff is skipped once connectionPromise exists, but the demand still runs before the flight is joined.
The current coalescing test uses an explicit connection and never counts demands, which is why this was invisible.
Tracked as magic-context-834. The fix wants the connection flight to carry both the client and the certified daemon id so it can be joined before the demand, which is the same plumbing 3887943085 needs; doing them together rather than twice.
| if: runner.os == 'macOS' | ||
| run: | | ||
| cargo build -p mc-module --bin ck-mc-host | ||
| cargo test -p mc-module --test lifecycle_cli |
There was a problem hiding this comment.
In the new mc-host-lifecycle macOS matrix leg, this command runs the entire lifecycle_cli integration target; cargo test --help confirms that --test <NAME> runs the named test target rather than merely compiling it. Those unguarded Unix tests repeatedly require ck-mc-host start and restart to succeed, but BrocaComponent::initialize unconditionally returns an error on every non-Linux target (crates/mc-host/src/broca/mod.rs:384-389), so the spawned daemon cannot publish and the macOS job necessarily fails. Either make Broca initialize as an unavailable component on macOS or select a macOS-safe lifecycle test set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this is exactly why mc-host lifecycle integration (macos-latest) is red. BrocaComponent::initialize returns Err unconditionally on non-Linux (broca/mod.rs:384-390) because crash-ownership records and sweeps depend on /proc process identity, and ci.yml:266 runs the entire lifecycle_cli target, whose tests require start/restart to succeed. You are right that --test <NAME> runs the target rather than merely compiling it.
Context worth having: that job was previously failing earlier, at compile time, because libc::SIGRTMAX() does not exist on macOS (fixed in 6457d6b). Making it compile is what surfaced the test failures you are describing, so both were real and sequential.
Tracked as magic-context-mqu. The likely resolution is to gate the daemon-requiring tests to Linux so the macOS leg proves the portable CLI contract it was added for, rather than making Broca portable; but that is a deliberate call about what the macOS proof is supposed to assert, so it is not something to quietly narrow.
A `module_restarted` failure cleared `compatibleDaemonId` unconditionally. Overlapping embedding operations can straddle one daemon rotation: a sibling finishes re-certifying and installs the new identity while this attempt's request against the old generation fails a moment later. The late failure then erased the newer certificate, and because the re-certified operation had already spent its single durable restart budget, its next call refused for want of an identity. The attempt now records the identity it published against, and recovery clears only when that identity is still the installed one. `demandManagedLane` is the sole writer and installs a fresh array per certification, so reference equality answers exactly "has a newer certificate landed since this attempt".
`maxStreamItems` was applied with `??` alone, so `Infinity` and `NaN` reached the pending entry untouched and made the `streamItems.length >= maxStreamItems` comparison permanently false. An empty `StreamData` frame charges zero pending bytes while still retaining one decoded wrapper, so the item ceiling is the only bound on that growth; with it disabled a peer could retain wrappers without limit under an untouched byte budget. The ceiling is now validated as a non-negative safe integer before a pending entry exists and before any byte reaches the peer, so a rejected request is never published. Zero stays legal and means retain nothing: the first item satisfies `0 >= 0` and is refused.
The stopped-path cleanup called itself best-effort but propagated every `clear_active_selection` error, so a transient I/O fault on `active-selection.json` reported a `stop` that had already terminated the daemon as a failure. No later lifecycle command could recover the run, and the committed stop was invisible to the caller. Ordinary cleanup faults now yield the stopped result they earned. The unsupported-schema quarantine keeps failing: it is matched by its own error before the catch-all, the same discriminator the launcher-envelope sites already use, so arm order carries the distinction. The lifecycle test previously pinned the failure, so it now asserts the success contract and covers the running branch as well. Both sub-blocks assert the selection file still exists afterwards, which proves the injected fault fired rather than cleanup short-circuiting on an absent file.
The Pi argv contract asserted a `/dev/fd/` prefix unconditionally, which macOS cannot satisfy: `module_path()` hands out the closure pathname there because descriptor paths are not file-like on that target. `scripts/test-rust.sh` runs this test for the whole workspace, so the supported macOS suite failed even though the fallback behaved as implemented. The expectations now branch on `DESCRIPTOR_PATHS_ARE_FILE_LIKE`, the same constant the implementation branches on, so they cannot drift from the platform contract. The non-descriptor arm pins per-node identity and order rather than just a prefix.
The managed-lane fence refused to publish whenever `compatibleDaemonId` was null on a managed-default lane. With no lifecycle owner configured that state is permanent by design: `demandManagedLane` returns early so an already-running daemon can be dialled, and the owner is the only writer of a certified identity. Passive dialling therefore failed initialization on its first `models.list` with zero wire calls. The fence now keys on the same field the certifier gates on, so certifier and fence cannot disagree about which lanes are owned. An owner that certified and then had its identity cleared by a rotation is still refused; a lane with no owner dials with no `expectedDaemonId`, where the daemon's own handshake is the only fence available. A demand that reports success without an identity certifies nothing, so it now arms the same retry window as the other failure outcomes. Otherwise every later call re-entered the demand and drove another native start and storage probe at request rate.
…ifecycle # Conflicts: # crates/mc-host/tests/harness_closure.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00a8029632
ℹ️ 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".
| return catalog; | ||
| }; | ||
|
|
||
| await expect(readCompatibilitySnapshot(expired, Date.now() + 1)).rejects.toThrow( |
There was a problem hiding this comment.
When the event loop spends more than 1 ms between calculating this deadline and readCompatibilityProbe checking it, the probe correctly expires before calling catalogList, so calls remains empty and the assertion below fails. This occurred when the test ran with the other changed lifecycle tests, while isolated runs passed, making CI timing-dependent; use a controlled clock or synchronize expiration after catalog entry instead of relying on a 1 ms wall-clock window.
Useful? React with 👍 / 👎.
| let store = store_at(root.path()); | ||
| let digest = stage_default(&store, src.path()); | ||
| let validated = store.validate(&digest).expect("validate"); | ||
| let descriptor_path = validated.descriptor_root_path().join("bin/ck-mc-host"); |
There was a problem hiding this comment.
When the workspace Rust suite runs on macOS, this constructs /dev/fd/<directory-fd>/bin/ck-mc-host, but descriptor_root_path explicitly documents that directory traversal through its returned path is Linux-only. Darwin's /dev/fd/<fd> duplicates the descriptor rather than providing a traversable directory symlink, so the immediately following read fails with ENOTDIR; branch this test by descriptor-path capability or run it only on Linux.
Useful? React with 👍 / 👎.
`BrocaComponent::initialize` refuses to run anywhere but Linux, because its crash-ownership records and sweeps read process identity out of `/proc`. Every daemon invocation builds Broca into the composite, so no spawned daemon can publish on another platform and the five proofs that require one cannot pass there. Those five now carry a target gate, along with the helpers and imports that go dead without them. The nine CLI-contract tests stay portable: each returns its verdict during argument parsing, record decode, or generation resolution, all of which precede the spawn, so they never reach Broca. Linux still runs all fourteen. The reduced selection compiles warning-clean under `-D warnings`, which is what proves the gate covers exactly the items that go dead and none that stay live.
Two defects shared one cause: the in-flight dial recorded no certification, so callers could neither tell a passive connection from an uncertified one nor inherit what the demand behind the flight had already proved. A transport with no lifecycle owner never names a daemon, so its stored certification was null and every later request failed the identity check and invalidated. That reopened the connection per request, and because closing a shared client retires already-invoked work as `outcome_unknown`, a second session arriving mid-request could turn the first request's outcome unknown against a healthy, unchanged daemon. A certification now distinguishes "no owner exists to name an identity" from "no demand has settled", so a passive connection is reusable while an owner-certified one still invalidates when the identity it proved is replaced. The flight carries that certification, and callers join it before demanding again, so a burst during a slow dial no longer spends each caller's deadline on a duplicate start transaction and compatibility probe.
The schema gate runs ahead of the stage that writes the installed evidence, and that path is not tracked between releases, so reading it made the gate report a missing input instead of a schema defect. It had two such dependencies, not one: the evidence document itself and an unconditional digest of the qualification artifact. It now validates a contract-derived reference document built by this file's own builder and checked by its own validator, so the two disagreeing is still a failure with nothing on disk. Every cited digest is a self-evident placeholder that satisfies the required shape without resembling a real citation, and the document stays unqualified so the placeholder ban on GA evidence still holds. The digest inputs move into a shared fail-closed builder rather than being duplicated; `--write-template` output is byte-identical. The GA path is unchanged and still requires the evidence.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 637a47d40b
ℹ️ 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 (signal?.aborted) { | ||
| throw signal.reason ?? new Error("module transport call aborted"); | ||
| } | ||
| return await joinable; |
There was a problem hiding this comment.
When a second request arrives while another caller's dial is pending, this direct await checks the signal only once and never races the shared flight against the joining caller's deadline or a later abort. A slow or stalled dial created with the first caller's longer budget can therefore keep the second request pending well beyond its own timeout or cancellation; join the shared work without cancelling it, but detach this waiter using its deadline and signal.
Useful? React with 👍 / 👎.
The lifecycle path walk opens every component of the data root with `O_NOFOLLOW`, so a symlinked ancestor is refused rather than traversed. The darwin temporary directory sits under `/var`, which is a symlink to `private/var`, so the walk returns `ELOOP` before it ever resolves the runtime directory. Both walks map that refusal onto `wedged`: the read-only probe through `Insecure`, and the creating coordination-lock walk through `Io`. Every proof that asserts an observed state therefore reads `wedged` there regardless of what the root contains. Those proofs now carry a target gate, joining the ones that need a published daemon, along with the helpers and imports that go dead without them. The two that assert only argument parsing and version metadata never resolve a root, so they stay portable and remain what the darwin leg asserts. Linux still runs all fourteen. The reduced selection compiles warning-clean under `-D warnings`, and un-gating a single helper there fails on dead code, which is what proves the gate covers exactly the items that go dead.
Records the findings the PR #46 review pass verified but did not fix, plus the four it did. Thirteen stay open, four are closed. The open set is dominated by lifecycle and platform gaps that need design work rather than a local patch: a data root reached through a symlinked ancestor is refused and reported as wedged, which is why the darwin lifecycle proofs are gated; the GA release check requires a qualification workflow that is not in the tree; macOS Pi modules run from a replaceable pathname rather than a verified inode; and the retired memories table is still queried by the incident pool and the retrieval benchmark, which fails those two jobs on every pull request while main skips them. This export is additive against the current file: it adds seventeen records and drops none. It travels through git because the Dolt remote is not reachable from this machine, so the tracked export is the only carrier between machines; that limitation is itself recorded here.
1 participant
Summary
startnon-preemptive while explicit restart merges qualified harness selectiontmp/pathsStack
PR 10 of 17. Base:
stack/mc-host-09-release-verification(#52).Focused review follow-ups:
Validation
cargo test -p mc-hostcargo test -p mc-modulecargo clippy --workspace --all-targets -- -D warningsbun run typecheck,bun run build,bun run lintKnown Residuals
MC_HOST_CANARY_*inputs.