docs(mc-host): document managed lifecycle architecture - #67
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 |
Review summaryThis PR is docs + generated task-tracking data only ( Doc-quality notes below (left as inline comments too):
Everything else (payload_manifest_digest now required + validated, the two-lock lifetime-fence model, the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -518,7 +531,23 @@ Both languages MUST produce identical bytes: UTF-8 pass-through for non-ASCII, t | |||
|
|
|||
| ### 7.6 `host.shutdown` | |||
There was a problem hiding this comment.
This section heading is still ### 7.6 \host.shutdown`but now documentshost.statusfirst (lines 534-543) before getting tohost.shutdown. Consider retitling to ### 7.6 `host.status` and `host.shutdown`so the heading matches the content, and so a reader scanning the TOC/section list can findhost.status`'s home.
There was a problem hiding this comment.
Fixed in ac75372. The heading is now ### 7.6 host.status and host.shutdown (with both operation names in code spans), so it names both operations documented under it.
| ### 7.4 Operation classification | ||
|
|
||
| `route.open`, `catalog.list`, `host.shutdown` (Section 7.6), and `transport.negotiate` (Section 7.7) are the only required channel-0 operations. Every other operation receives terminal `unsupported_operation`; host stays connected if framing remains valid. `transport.activate` and `transport.commit` are candidate-channel-only (Section 7.7): on a bootstrap connection they are unrecognized operations and receive terminal `unsupported_operation` like any other. A client health operation MUST NOT proxy handler health, which remains host-internal. | ||
| `route.open`, `catalog.list`, `host.shutdown` (Section 7.6), `host.status`, and `transport.negotiate` (Section 7.7) are the only required channel-0 operations. Every other operation receives terminal `unsupported_operation`; host stays connected if framing remains valid. `transport.activate` and `transport.commit` are candidate-channel-only (Section 7.7): on a bootstrap connection they are unrecognized operations and receive terminal `unsupported_operation` like any other. `host.status` reads the last completed host-owned health snapshot; it never invokes a handler callback on the requesting connection and exposes only closed component states and sanitized metrics, never handler detail text. |
There was a problem hiding this comment.
route.open, catalog.list, and transport.negotiate each cite their defining section in this sentence, and so does host.shutdown (Section 7.6), but the newly-added host.status has no section pointer even though it's documented in that same Section 7.6. Suggest host.status (Section 7.6)`` for consistency.
There was a problem hiding this comment.
Fixed in ac75372. The §7.4 sentence now reads host.status (Section 7.6) alongside host.shutdown (Section 7.6), so every operation in that list carries its defining section.
| | `reason` | Meaning | | ||
| | --- | --- | | ||
| | `unavailable` | an installed, statically eligible non-TCP offer is dynamically unavailable: provider readiness (`Recovering`/`Quarantined`) or admission pressure. Permanent absence of a provider and statically ineligible offer parameters are NOT `unavailable`; they select TCP with no `reason` | | ||
| | `unavailable` | no installed provider serves an offered non-TCP transport | |
There was a problem hiding this comment.
This flattens the previous distinction between dynamic unavailability (provider Recovering/Quarantined, admission pressure) and permanent absence (previously: selects TCP with no reason at all), and the paragraph below no longer mentions the client's "automatic re-upgrade probe" that used to be authorized specifically by this unavailable reason. Since this is a docs-only PR, can you confirm the re-upgrade-probe behavior was actually removed from the client/host implementation (in an earlier layer of this stack) rather than just undocumented here? If the probe still exists in code, this doc change would silently misdocument real behavior.
There was a problem hiding this comment.
Good catch, and the answer is the one that made this a doc bug rather than a doc-only change: the re-upgrade probe was not removed from the implementation. The doc had dropped a real capability.
Verified in crates/mc-host/src/connection.rs. The host still distinguishes the two cases explicitly:
PreflightEligibility::DynamicallyUnavailablesetsdynamically_unavailable, which becomesSome(FallbackReason::Unavailable)— an installed, statically eligible provider whose readiness or admission pressure is transient.- permanent absence falls through the
None => {}arm and yields a reasonless TCP selection.
The comment on the reason-selection block states the reason unavailable outranks capability_version_mismatch precisely because it is the only reason that authorizes a client re-upgrade probe.
Restored in ac75372. §7.7.3 again separates dynamic unavailability from permanent absence, and the re-upgrade-probe paragraph is back, scoped to exact unavailable.
|
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: dbacfeefec
ℹ️ 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.
Qualified evidence is now required to claim this exact workflow path and to match a successful GitHub run at the release HEAD, but a repo-wide search of .github/workflows finds no mc-host-release-qualification.yml. Consequently, no run satisfying workflow === QUALIFICATION_WORKFLOW_PATH can be produced from this revision, so --check cannot ever accept GA evidence until the referenced workflow is added or the verifier is pointed at an existing qualification workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a fact, and it still holds at the base branch tip (7df4d4b): QUALIFICATION_WORKFLOW_PATH is .github/workflows/mc-host-release-qualification.yml, and .github/workflows/ contains only ci.yml, claude-code-review.yml, retrieval-benchmark.yml, and shm-hardening-optin.yml. No run can satisfy workflow === QUALIFICATION_WORKFLOW_PATH.
Not actionable in this pull request. scripts/verify-mc-host-release-evidence.ts is not part of this change — this branch touches six files (.github/workflows/ci.yml, .gitignore, ARCHITECTURE.md, STRUCTURE.md, docs/mc-host-wire-protocol.md, docs/synapse-model-bundle.md). The verifier arrived on the base branch in 8d7c12d, so the finding belongs to the release-qualification work, and authoring a GA qualification workflow here would be a large change well outside a documentation branch.
One mitigation worth recording: the verifier fails closed rather than silently. verify-mc-host-release-evidence.ts:836-838 checks existsSync on the path and reports qualification workflow ... does not exist in this checkout, so --check refuses GA evidence with a named reason instead of accepting unqualified evidence.
| mc-host-lifecycle: | ||
| name: mc-host lifecycle integration | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The lifecycle build/tests were removed from the existing Linux/macOS matrix and moved into this Ubuntu-only job. That leaves mc-module and ck-mc-host entirely uncompiled on macOS even though this change adds and modifies macOS-specific lifecycle paths such as /dev/fd execution and the close_range fallback; a Darwin-only compile or linkage regression can therefore pass CI and break the supported darwin-arm64 and darwin-x64 payloads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproduced — this reads a revision older than the branch point.
.github/workflows/ci.yml has a mc-host-lifecycle job whose matrix is os: [ubuntu-latest, macos-latest], and it carries a macOS-specific step that compiles exactly the binary you are asking about:
- name: Native lifecycle binary and CLI contract (macOS)
if: runner.os == 'macOS'
run: |
cargo build -p mc-module --bin ck-mc-host
cargo test -p mc-module --test lifecycle_cliThe step's own comment gives the reason you gave: the release ships darwin payloads and the GA evidence gate requires darwin target proofs, so the native lifecycle binary and its CLI contract need macOS proof alongside the Linux integration set. mc-module and ck-mc-host are therefore compiled on macOS, and a Darwin-only compile or linkage regression fails CI.
This branch's only ci.yml change is additive — six lines pinning dtolnay/rust-toolchain@stable in the plugin check job. It removes nothing from any matrix.
| if require_previous_credentials && changed { | ||
| return Err("restart cannot change the active harness selection"); | ||
| } |
There was a problem hiding this comment.
When one product has already started the shared daemon with its OpenCode closure and the Pi owner later supplies its qualified closure, the selection necessarily changes, so this new check rejects the only restart path that could retain both harnesses. The production demand factory builds an envelope only for its own parent package, while stop clears the prior selection; unlike the synthetic smoke's explicit mergedEnvelope, no installed owner can therefore stop and restart with both descriptors. The second harness remains unavailable, or replacing it makes the first unavailable, instead of converging on one shared daemon.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on the base branch, and out of scope here.
crates/mc-module/src/bin/ck_mc_host/serve.rs is not part of this change; this branch touches six documentation, CI, and ignore files. That file was last modified by d2e9a11, titled fix(mc-host): clear stale harness selections instead of wedging on them, which is the branch point for this pull request.
At that revision and at the base tip, merge_selection does merge rather than reject: it starts from previous.clone() and layers each supplied descriptor over it, so an owner that supplies only its own harness retains the other. The rejection you quoted is now gated on require_previous_credentials, which is the restart path, and the second rejection fires only when a new owner cannot preserve the active credential source.
If a merge path is still missing for a specific installed-owner sequence, that belongs on the lifecycle branch that owns serve.rs rather than this one.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous Review Summaries (4 snapshots, latest commit fbc5204)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit fbc5204)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit ac75372)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit 7678eae)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit dbacfee)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Reviewed by gemini-3.7-flash · Input: 109.6K · Output: 5.9K · Cached: 412.3K |
The check-plugin job invokes cargo twice: the dependency-graph warm-up step, and the same `cargo tree -p mc-host` that test:release shells out to. Neither had a toolchain installed, so both resolved to whatever rustc the ubuntu-latest image happens to ship. A runner-image change would then surface as a cargo failure inside an otherwise TypeScript job, with nothing pointing at a missing toolchain. Install stable before the first cargo call, matching shm-crash-recovery, shm-source-build, and mc-host-lifecycle.
7678eae to
b71bf85
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b71bf85ff8
ℹ️ 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".
| {"_type":"issue","id":"magic-context-ymc.7","title":"T6: Wakeup protocol: spin-then-park, eventfd/futex, missed-wakeup proof","description":"BUILD-path only. Latency path: bounded spin (N iterations with spin_loop hint) then park on futex (Linux) — waker only issues syscall when peer state says parked. The hard part is the missed-wakeup race: sleeper must (1) publish 'parking' flag, (2) re-check ring non-empty predicate, (3) then futex_wait with expected value — classic check-flag-check ordering. Document the interleaving argument.\n\nSkills: /performance:linux-event-loop-runtime-mechanics (wake predicates, missed-wakeup prevention), /low-level-systems:scalable-concurrency (spin-then-park construction, spin budget), /low-level-systems:rust-memory-ordering (flag/predicate ordering).\n\nTests: (1) loom model of the park/wake protocol — must include the torn interleaving where wake races park; (2) stress test: producer bursts with random gaps, assert no consumer hang (timeout = fail); (3) SIGSTOP/SIGCONT the consumer mid-run, assert recovery; (4) latency bench: spin-hit path vs park path measured separately.\nAcceptance: loom green; 24h-equivalent stress (nextest slow-timeout bounded) with zero hangs; spin budget tunable via config.","status":"closed","priority":1,"issue_type":"task","owner":"ahravdutta02@gmail.com","created_at":"2026-08-22T23:14:21Z","created_by":"AhravDutta","updated_at":"2026-08-22T23:19:36Z","closed_at":"2026-08-22T23:19:36Z","close_reason":"Superseded by ADOPT iceoryx2 (ymc.2): Event/Listener/WaitSet owns wakeup; no custom futex protocol.","dependencies":[{"issue_id":"magic-context-ymc.7","depends_on_id":"magic-context-ymc","type":"parent-child","created_at":"2026-08-22T23:14:21Z","created_by":"AhravDutta","metadata":"{}"},{"issue_id":"magic-context-ymc.7","depends_on_id":"magic-context-ymc.2","type":"blocks","created_at":"2026-08-22T23:14:32Z","created_by":"AhravDutta","metadata":"{}"},{"issue_id":"magic-context-ymc.7","depends_on_id":"magic-context-ymc.5","type":"blocks","created_at":"2026-08-22T23:14:34Z","created_by":"AhravDutta","metadata":"{}"}],"dependency_count":2,"dependent_count":1,"comment_count":0} | ||
| {"_type":"issue","id":"magic-context-ymc.2","title":"T1 DECISION (RESOLVED): ADOPT iceoryx2 — do not hand-roll shm transport","description":"DECISION: ADOPT iceoryx2. Research 2026-08-22, evidence:\n\n- iceoryx2 v0.9.3 (crates.io, actively maintained, Eclipse project), MIT OR Apache-2.0, MSRV-friendly.\n- Request/response messaging pattern supported natively (plus pub/sub, events).\n- macOS: tier 2, 'done' — removes our memfd-is-Linux-only fallback problem; same transport both platforms, TCP fallback stays for remote/degraded only.\n- Dynamic payload sizes supported (loan_slice / dynamic data) — kills the large-payload task.\n- Lock-free zero-copy queues + segment lifecycle + decentralized crash cleanup built in — exactly the code where hand-rolled bugs live (T3-T7 deleted).\n- Published latency: sub-µs, payload-size-independent; MUST be re-verified on our hardware (T0 budget, T9 verdict).\n\nKnown gap: no async API (roadmap issue #47). Integration = dedicated listener thread bridging iceoryx2 events to the tokio runtime. Accepted cost, scoped in the integration task.\n\nPonytail-review verdict on the build path: T3/T4/T6/T8/T10 native (iceoryx2 owns it), T5 shrinks to service config, T7 shrinks to our-layer hardening. Net: -5 tasks, ~-2500 LoC of unsafe/concurrency code never written.\n\nESCAPE HATCH: reopen build path only if (a) T9 bench on our hardware misses the T0 budget materially, or (b) the sync-bridge integration proves untenable in practice. Closed build tasks retain full specs for that case.","status":"closed","priority":1,"issue_type":"decision","owner":"ahravdutta02@gmail.com","created_at":"2026-08-22T23:13:28Z","created_by":"AhravDutta","updated_at":"2026-08-22T23:19:35Z","closed_at":"2026-08-22T23:19:35Z","close_reason":"Decision resolved: ADOPT iceoryx2. Evidence + escape hatch recorded in description. T0 baseline still runs — it feeds the T9 verdict, no longer gates this decision.","dependencies":[{"issue_id":"magic-context-ymc.2","depends_on_id":"magic-context-ymc","type":"parent-child","created_at":"2026-08-22T23:13:27Z","created_by":"AhravDutta","metadata":"{}"},{"issue_id":"magic-context-ymc.2","depends_on_id":"magic-context-ymc.1","type":"blocks","created_at":"2026-08-22T23:14:32Z","created_by":"AhravDutta","metadata":"{}"}],"dependency_count":1,"dependent_count":7,"comment_count":0} | ||
| {"_type":"issue","id":"magic-context-ymc.3","title":"T2: Transport seam: swappable channel trait + TCP fallback + runtime negotiation","description":"Abstract the client↔daemon channel behind one small trait (send/recv frames) so TCP and shm are interchangeable. Runtime negotiation: client requests shm, daemon grants or falls back to TCP (macOS, remote, version mismatch). No behavior change in this task — TCP impl only, seam + negotiation protocol.\n\nNeeded regardless of T1 verdict. Keep the trait minimal: no async-trait gymnastics beyond what subc-transport already forces (/design-review:async-api-contract-design only if the seam gets contentious).\n\nTests (/testing:test-strategy): CONTRACT test suite written against the trait, run against the TCP impl now, every future impl later — same suite proves swap safety. Negotiation matrix test: grant, fallback, version-mismatch → TCP.\nAcceptance: existing mc-host integration tests green through the seam; contract suite in place.","notes":"Implementation plan: docs/plans/2026-08-23-2129-feat-transport-channel-negotiation-plan.md. Non-interactive document review applied 3 mechanical fixes; 8 decisions and 11 proposed fixes remain available for walkthrough.","spec_id":"docs/plans/2026-08-23-2129-feat-transport-channel-negotiation-plan.md","status":"closed","priority":1,"issue_type":"task","assignee":"AhravDutta","owner":"ahravdutta02@gmail.com","created_at":"2026-08-22T23:13:28Z","created_by":"AhravDutta","updated_at":"2026-08-24T13:41:22Z","started_at":"2026-08-24T05:07:48Z","closed_at":"2026-08-24T13:41:22Z","close_reason":"Closed","dependencies":[{"issue_id":"magic-context-ymc.3","depends_on_id":"magic-context-ymc","type":"parent-child","created_at":"2026-08-22T23:13:27Z","created_by":"AhravDutta","metadata":"{}"}],"dependency_count":0,"dependent_count":3,"comment_count":0} | ||
| {"_type":"issue","id":"magic-context-ymc.3","title":"T2: Transport seam: swappable channel trait + TCP fallback + runtime negotiation","description":"Abstract the client↔daemon channel behind one small trait (send/recv frames) so TCP and shm are interchangeable. Runtime negotiation: client requests shm, daemon grants or falls back to TCP (macOS, remote, version mismatch). No behavior change in this task — TCP impl only, seam + negotiation protocol.\n\nNeeded regardless of T1 verdict. Keep the trait minimal: no async-trait gymnastics beyond what subc-transport already forces (/design-review:async-api-contract-design only if the seam gets contentious).\n\nTests (/testing:test-strategy): CONTRACT test suite written against the trait, run against the TCP impl now, every future impl later — same suite proves swap safety. Negotiation matrix test: grant, fallback, version-mismatch → TCP.\nAcceptance: existing mc-host integration tests green through the seam; contract suite in place.","notes":"Implementation plan: docs/plans/2026-08-23-2129-feat-transport-channel-negotiation-plan.md. Non-interactive document review applied 3 mechanical fixes; 8 decisions and 11 proposed fixes remain available for walkthrough.","spec_id":"docs/plans/2026-08-23-2129-feat-transport-channel-negotiation-plan.md","status":"open","priority":1,"issue_type":"task","owner":"ahravdutta02@gmail.com","created_at":"2026-08-22T23:13:28Z","created_by":"AhravDutta","updated_at":"2026-08-23T21:49:40Z","dependencies":[{"issue_id":"magic-context-ymc.3","depends_on_id":"magic-context-ymc","type":"parent-child","created_at":"2026-08-22T23:13:27Z","created_by":"AhravDutta","metadata":"{}"}],"dependency_count":0,"dependent_count":3,"comment_count":0} |
There was a problem hiding this comment.
This replacement rewinds magic-context-ymc.3 from closed at 2026-08-24 to open with an older timestamp and drops its closed_at, close_reason, started_at, and assignee fields. The same export also rolls back progress or dependency metadata for ymc.8, ymc.10, ymc.12, and a52, so the committed tracking snapshot falsely presents completed or claimed work as available again; regenerate the export from the current Beads state while adding the new c50 issues.
AGENTS.md reference: AGENTS.md:L3-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and your list was complete — I found one more of the same kind while checking it.
Verified against the live Beads database rather than either committed export. bd show magic-context-ymc.3 reports CLOSED, updated 2026-08-24, close reason Closed, assignee AhravDutta, which matches the branch point and contradicts the export committed here. Comparing every record against the branch point found exactly five rewound timestamps — the five you named — plus:
magic-context-c50.4:dependent_count3 → 2 at an unchangedupdated_at, where live Beads reports 3.
Field losses were also real: ymc.3 dropped closed_at, close_reason, started_at, and assignee; ymc.8, ymc.10, and ymc.12 dropped spec_id, assignee, started_at, and in two cases notes and acceptance_criteria.
Resolved in f8968ae by taking the base branch's export, which the merge conflict forced a decision on anyway. That file is the current one: 223 records against this branch's 196, no issue id lost, and no updated_at moved backwards. This branch no longer carries a .beads/issues.jsonl diff at all — its net change is now six documentation, CI, and ignore files — so the rewind is gone at the source rather than patched.
One note on spec_id, since a straight regeneration does not preserve it: that field is absent from all 241 records in the live database, so it is stale metadata in the committed exports rather than live state. Reintroducing it would commit data the source of truth does not hold.
| | `reason` | Meaning | | ||
| | --- | --- | | ||
| | `unavailable` | an installed, statically eligible non-TCP offer is dynamically unavailable: provider readiness (`Recovering`/`Quarantined`) or admission pressure. Permanent absence of a provider and statically ineligible offer parameters are NOT `unavailable`; they select TCP with no `reason` | | ||
| | `unavailable` | no installed provider serves an offered non-TCP transport | |
There was a problem hiding this comment.
For an offered non-TCP transport with no installed provider, the implementation deliberately returns reasonless TCP (connection.rs:943-944, asserted by transport_negotiation.rs:850-867); unavailable is emitted only for an installed, statically eligible provider that is temporarily unavailable (connection.rs:931-954) and is what authorizes a later re-upgrade probe. This new definition states the opposite and removes that recovery rule, leaving the normative protocol incompatible with the shipped host and potentially causing independently implemented clients or hosts to retry permanent absence while suppressing recovery from transient outages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and your code references are accurate — thank you for citing them, they made this quick to confirm.
Verified in crates/mc-host/src/connection.rs: PreflightEligibility::DynamicallyUnavailable is what produces Some(FallbackReason::Unavailable), permanent absence falls through the None => {} arm to a reasonless TCP selection, and the comment on the reason-selection block states that unavailable outranks capability_version_mismatch because it is the only reason authorizing a client re-upgrade probe.
Fixed in ac75372, which landed at 21:50 UTC, about eleven minutes after this comment. §7.7.3 now reads:
unavailable— an installed, statically eligible non-TCP offer is dynamically unavailable: provider readiness (Recovering/Quarantined) or admission pressure. Permanent absence of a provider and statically ineligible offer parameters are NOTunavailable; they select TCP with noreason
and the re-upgrade-probe rule is restored, scoped to exact unavailable, with reasonless selections and every other fallback reason explicitly excluded from starting or extending the probe window.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac75372b8e
ℹ️ 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".
| only from that revalidated generation. macOS uses the host-only lane and | ||
| reports exact readiness `unsupported` / `synapse_unsupported`. |
There was a problem hiding this comment.
On every macOS target, ck-mc-host serve still constructs BrocaComponent (serve.rs:881-889), the composite requires every child initializer to succeed (composite.rs:217-227), and Broca explicitly returns Err on non-Linux platforms (broca/mod.rs:369-383). Consequently the daemon exits before publication and can never reach the newly documented unsupported / synapse_unsupported readiness state. Either supply a non-Linux unavailable Broca component or stop advertising this macOS host-only lane.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and your three code references are exact.
Verified:
serve.rs:946-954builds the composite withBrocaComponent::new_with_credentials(...)as an unconditional child. Note the contrast withsynapse_componentimmediately above it, which is#[cfg(target_os = "macos")]-gated and returnsSynapseComponent::unsupported("synapse_unsupported"). Broca has no such gate.broca/mod.rsSecondaryComponent::initializereturnsErr(InitError("broca requires Linux: crash-ownership records and sweeps depend on /proc process identity"))undercfg!(not(target_os = "linux")).composite.rsinitializeusestokio::try_join!across primary, secondary, and tertiary, so any child error fails the whole initialization.
So a macOS daemon exits before publication, and the readiness value the doc advertised is unobservable over the wire even though the component computes it.
Fixed in fbc5204, taking your second option — stop advertising a startable macOS lane. The bundle doc now states that the Synapse component carries unsupported / synapse_unsupported on macOS, that no client observes it because the daemon never reaches publication, and names the reason (Broca refuses off Linux, the composite requires every child initializer to succeed). It also records the precondition for serving the lane there: a Broca component that initializes off Linux as unavailable rather than failing.
| A separate owner-only directory, `${dataDir}/cortexkit/lifecycle`, carries the cross-process lifecycle transaction lock on its directory inode: mutating lifecycle transactions (launcher start/stop, owned by downstream packaging work) take it exclusively, and probes take it shared when it exists. It is distinct from the runtime-directory instance lock, which only the daemon holds for its whole incarnation. | ||
| Cross-process lifecycle serialization lives in a fixed, version-neutral, owner-only directory directly under the data root: `${dataDir}/.mc-host-coordination/`, containing the never-renamed regular files `transaction.lock` and `lifetime.lock`. Supported code never renames, replaces, or unlinks the directory or either file; same-UID replacement of that root or an ancestor is trusted-policy replacement outside this boundary. | ||
|
|
||
| - `transaction.lock` carries the exclusive flock serializing mutating lifecycle transactions (launcher start/stop, owned by downstream packaging work); probes take it shared, without creating anything, when it exists. |
There was a problem hiding this comment.
When a normal start, stop, or restart holds transaction.lock longer than the four 25 ms acquisition attempts, LifecycleTransactionLock::acquire_shared returns None and probe_lifecycle deliberately proceeds on evidence alone (lifecycle.rs:486-507, 834-837). Therefore probes do not always take the shared lock when it exists as stated here, and a status/doctor invocation can observe an intermediate lifecycle state while a mutation remains in progress. Specify the bounded evidence-only fallback, or require the probe to retain serialization for the full observation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The wording did read as unconditional serialization, and the retry bound you cite is exact.
Verified: instance.rs defines LOCK_RETRY_ATTEMPTS = 4 and LOCK_RETRY_DELAY = 25ms, and flock_bounded returns Ok(false) when a holder outlasts every attempt — its own doc comment says the caller decides, "a mutator reports AlreadyRunning, a probe degrades to evidence-only". LifecycleTransactionLock::acquire_shared maps that to Ok(None), and probe_lifecycle discards it with let _root = ...? before continuing into the bounded reread loop.
Fixed in fbc5204, specifying the bounded fallback rather than requiring the probe to hold the lock for the full observation. The bullet now states that shared acquisition is bounded and not awaited — four attempts 25 ms apart — that a probe which still finds a mutator holding the lock samples on evidence alone, that it therefore does not always exclude an in-flight mutation and may observe an intermediate lifecycle state, and that the bounded reread loop rather than the lock is what makes such a sample coherent. Lock absence is named as degrading the same way, since a probe never creates the coordination root.
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.
Review round resolvedAll ten inline comments answered individually. Summary, with the verification behind each verdict: Fixed
Task export rewind — resolved at the source Verified against the live Beads database, not either committed export, and the finding was correct. The base branch had independently rewritten the same file, which made this a merge conflict as well. Resolved in f8968ae by taking the base branch's export — 223 records against this branch's 196, no id lost, no Not reproduced The macOS lifecycle-coverage finding reads a revision older than the branch point. Out of scope, still open upstream
Local gates: |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbc5204458
ℹ️ 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".
| configuration pins. A nominally matching library with a different hash is | ||
| refused by design. | ||
|
|
||
| The qualified Linux lane uses the pinned |
There was a problem hiding this comment.
At this revision, release/mc-host-production-inputs.lock.json explicitly has production_qualified: false because the offline oracle has not been recorded against the locked model/runtime bytes, and the Linux entry in release/mc-host-payload-index.json is likewise unqualified and unpublished. Calling this the “qualified Linux lane” therefore makes the normative bundle documentation assert the opposite of the release gate; describe it as the pinned candidate until the qualification evidence exists and the gate passes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and the wording was mine — this branch introduced both that sentence and the oracle claim below it.
Verified against the gate artifacts you cite:
release/mc-host-production-inputs.lock.json:production_qualified: false, withunqualifiedcarryingoracle: offline semantic-oracle evidence not yet recorded against real locked ORT/model bytes.release/mc-host-payload-index.json: thelinux-x64-gnuentry isqualified: false, published: false(as are both darwin targets).
So the doc asserted the opposite of the gate on two counts, not one: "the qualified Linux lane", and "The offline oracle passes with networking disabled on glibc 2.28" — that second sentence names precisely the evidence the lock file says is unrecorded.
Fixed in d31444d, taking your framing. The lane is now "the pinned Linux lane", the oracle "runs" rather than passes, qualified manifest in the §5 bullet became pinned manifest, and a new paragraph states that the pins define the candidate the gate evaluates and points at both artifacts as the authority for the verdict.
One deliberate choice: the new paragraph names the gate artifacts and fields rather than restating their present values. Quoting production_qualified: false here would make the doc wrong again the moment the oracle evidence lands, whereas pointing at the gate stays correct in both directions.
| | V52 | First post-auth request is application traffic or another control operation | Host retires setup generation; zero application dispatch and no TCP continuation | | ||
| | V53 | Negotiation receives `unsupported_operation`, `connection_in_use`, malformed response, version mismatch, or unoffered selection | Client retires generation; no application request continues on TCP | | ||
| | V54 | Optional candidate unavailable or capability version mismatched | Valid negotiation explicitly selects offered TCP with the matching fallback reason; generation may continue | | ||
| | V55 | Authenticated `host.status` during post-publication activation | One route-free response reports separate closed component states; no routed application body is sent, handler detail is omitted, and starting storage refreshes until ready or unavailable | |
There was a problem hiding this comment.
For a client that authenticates immediately after publication, HostShared.health_snapshot is initialized with an empty components object and the listener is already published before the asynchronously spawned health task completes its first callback. Such a host.status request can therefore omit all component states and epochs, contrary to this activation vector; managed-policy.ts performs the compatibility status read once and treats those missing epochs as incompatible_epochs, so a fresh compatible daemon can transiently fail its managed lifecycle gate. Seed the initial closed component snapshot before accepting connections, or make the compatibility path wait for the first completed snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real defect, with one disagreement about where the fix belongs.
Verified, and your reading of the ordering is exact:
runtime.rs:841-845initializeshealth_snapshotasHealthReport { status: Degraded, detail: None, metrics: Some(json!({"components": {}})) }— an empty component map.runtime.rs:798-812binds the listener and calls.publish(port, &config.daemon_ver)insidesetup.runtime.rs:884-886then runsspawn_activation_task,spawn_health_task,accept_loop— so publication strictly precedes the health task.runtime.rs:1072is the only writer ofhealth_snapshot, and it runs after the first probe completes.
And the consequence is as you describe: compatibility.ts evaluateEpochCompatibility returns incompatible_epochs with detail epoch <name> is missing or nonnumeric when a value is absent, so a compatible fresh daemon can transiently fail its managed gate with a remediation of align_versions — misleading advice for a daemon that is merely young.
Where I differ: I am not changing V55, because it is not a description. §14 opens with "Every scenario has one required outcome. These are review vectors" — the V-table is normative. V55 requiring separate closed component states is the correct contract, and it is exactly the gate that catches this ordering bug. Weakening it to "may report an empty component map" would delete the requirement that makes the defect visible, and leave managed-policy.ts consuming a contract that permits the failure it trips over.
So the doc stands and the code is non-conforming. Your first option is the right one: seed the closed component snapshot before accepting connections, which satisfies V55 without making the compatibility path wait. That change belongs in crates/mc-host/src/runtime.rs on the lifecycle branch — it is not in this branch, which touches only documentation, CI, and .gitignore.
| - `transaction.lock` carries the exclusive flock serializing mutating lifecycle transactions (launcher start/stop, owned by downstream packaging work); probes take it shared, without creating anything, when it exists. Shared acquisition is bounded, not awaited: four attempts 25 ms apart, after which a probe that still finds a mutator holding the lock samples on evidence alone. A probe therefore does not always exclude an in-flight mutation, and may observe an intermediate lifecycle state; the bounded reread loop, not the lock, is what makes such a sample coherent. Lock absence degrades the same way, since a probe never creates the coordination root. | ||
| - `lifetime.lock` carries the daemon's whole-incarnation exclusive flock, acquired before the runtime-directory instance lock and held through publication cleanup, component shutdown, and callback reaping. | ||
|
|
||
| Neither the runtime-directory inode nor a lock on the managed `${dataDir}/cortexkit/lifecycle` directory prevents replacement-induced overlap on its own: both live inside the replaceable managed subtree, so renaming `run`, `lifecycle`, or the whole `cortexkit` tree would let a successor anchor fresh inodes under the same names. Because the coordination files sit outside that subtree and are never renamed, replacing the managed subtree cannot split either lock: a mutator still contends on the same `transaction.lock` inode, and a successor daemon still blocks on the same `lifetime.lock` inode until the displaced incarnation fully tears down. The runtime-directory instance lock remains as the descriptor-relative publication/cleanup fence. A mutator holding `transaction.lock` must additionally anchor named-namespace mutations to retained `cortexkit`/child descriptors and abort its named-namespace result when parent or child identity drifts. |
There was a problem hiding this comment.
When the managed cortexkit subtree is renamed or replaced while daemon stop is in flight, cmd_stop is the one lifecycle mutator that never captures or verifies a NamespaceAnchor; after shutdown it calls clear_active_selection(), which resolves the canonical path afresh and can therefore delete active.json from the replacement namespace and still report a committed stop. This contradicts the new requirement here and can erase a successor owner's harness selection, so stop must retain and verify the namespace identity around its named cleanup just as start and restart do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and the same reasoning as the V55 thread applies: the requirement is right and cmd_stop is the non-conforming party.
Verified:
ck-mc-host.rs:1183cmd_stopacquiresLifecycleTransactionLock::acquire_exclusiveand then never captures aNamespaceAnchor. The only two captures in the file are at:1007(cmd_start) and:1285(restart).- It calls
serve::clear_active_selection()at:1212and:1241. clear_active_selection(serve.rs:684) resolves the path afresh —runtime_dir_path(None), up two parents, thenjoin("cortexkit").join("mc-host-harness-closures")— with no identity check, so it deletesactive.jsonfrom whatever namespace currently answers to those names.- For contrast,
start_phasetakesanchor: &NamespaceAnchorand callsanchor.verify()before the spawn commit, failing closed on drift.
So a rename or replacement of the managed subtree while stop is in flight can erase a successor owner's selection and still report a committed stop, which is what the sentence at line 160 forbids. I am keeping that sentence: it states the correct rule, and softening it to match cmd_stop would retract the invariant that start and restart already honor.
I am not landing the code fix in this branch, and not only for scope reasons. crates/mc-module/src/bin/ck-mc-host.rs is not part of this change, and the fix needs a design decision this branch should not make unilaterally: what a drifted stop reports. start_phase answers drift with wedged, but cmd_stop carries an explicit comment rejecting wedged for a genuinely stopped host, because it "would route remediation at a daemon process that is not there". Picking the reason means picking from the closed vocabulary in the release contract that managed-policy.ts consumes, and this environment has no bun, so I cannot exercise the TypeScript side of that contract. That belongs with the lifecycle branch that owns both halves.
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.
1 participant
Summary
Documents the shipped
ck-mc-hostlifecycle owner, fixed three-component composition, immutable Broca closures, managed demand behavior, CLI commands, and Synapse package contract. Commits the Beads task export used for this work.Generated machine-specific mc-host qualification evidence is now ignored:
docs/evidence/mc-host-installed-release-evidence.jsondocs/evidence/mc-host-release-qualification.jsondocs/evidence/mc-host-synapse-smoke.jsonVerification
bun run test:release: 136 passedcargo test -p mc-host --test protocol_vectors: 18 passedgit diff --checkStack
Layer 9 of 9 above #46. Parent: #66.