fix(ci): restore portable lifecycle source checks - #62
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| run: | | ||
| cargo build -p mc-shm-transport -p mc-host -p mc-shm-native | ||
| cargo build -p mc-module --bin ck-mc-host | ||
| cargo check -p mc-shm-transport --features iceoryx |
There was a problem hiding this comment.
Removing the mc-module build and lifecycle_cli test here doesn't just drop two flaky steps — after this change, no CI job builds, checks, or tests mc-module at all (I grepped every workflow under .github/workflows/; there are zero remaining references to mc-module). That crate contains the scheduler, dispatch, historian, cache-stability transform, and the ck-mc-host lifecycle binary itself, so this is a meaningful coverage gap, not just noise reduction.
Digging into why the build was failing: crates/mc-module/src/lib.rs has unconditional (non-cfg-gated) top-level imports —
use cortexkit_lease::LeaseError;
use cortexkit_store::StoreError;
use cortexkit_store_types::{sqlite_store_path, Isolation, StorageBackend, StorageDescriptor};— against sibling crates that scripts/provision-rust-ci-stubs.sh provisions as empty, zero-export stub crates (#![allow(dead_code)] and nothing else) whenever the real commons//subconscious/ checkouts aren't present, which is always true on a fresh GitHub Actions runner. That means cargo build -p mc-module (and thus --bin ck-mc-host, and the lifecycle_cli test) can never succeed in this workflow, regardless of what mc-module's own code does — it's not an incidental "false failure" introduced by unrelated work, it's structurally guaranteed to fail here.
Given that, deleting the steps "fixes" CI green but silently removes the only signal this repo had on mc-module/ck-mc-host compiling at all. Worth considering instead:
- Making
provision-rust-ci-stubs.shemit type/API-shaped stubs (matching the realLeaseError,StoreError,Isolation, etc. signatures) instead of empty crates, somc-modulecan actually compile in this workflow, or - If real coverage genuinely lives in another (internal/private) CI system, saying so explicitly in the PR description, since "removes false source-build failures without weakening runtime tests" understates that a source-build check and a CLI contract test are being removed outright, not just de-flaked.
There was a problem hiding this comment.
You were right that deleting the steps left mc-module with no CI signal, and that gap is closed on the current head (02c3f17) — though by a later commit in this branch rather than by this diff.
mc-module is built and tested again, in the dedicated mc-host-lifecycle job:
- name: Native module adapter and CLI lifecycle
run: |
cargo test -p mc-module --bin ck-mc-host
cargo test -p mc-module \
--test host_adapter \
--test lifecycle_cliThat job checks out the real ahrav/commons rather than the metadata-only stubs, which is what makes compiling mc-module possible here at all — your root-cause analysis of why the old placement could never compile (unconditional cortexkit_lease/cortexkit_store imports against zero-export stubs) was exactly right, and it is the reason the checks moved to a job with real path-deps instead of being restored in shm-source-build.
ReviewThis is a small, two-file change: a CI workflow trim and an example-file fix. crates/mc-host/examples/synapse_perf.rs — clean. .github/workflows/ci.yml — left an inline comment with the main concern: this removes the only CI coverage of No security issues spotted; this PR only touches CI config and a perf-example fixture, no runtime/production code paths. |
| - name: Native lifecycle binary contract | ||
| run: cargo test -p mc-module --test lifecycle_cli |
There was a problem hiding this comment.
Removing this step (and cargo build -p mc-module --bin ck-mc-host above) drops the only CI coverage of the production ck-mc-host binary and its lifecycle_cli.rs subprocess contract-test suite — I couldn't find mc-module/ck-mc-host built or tested anywhere else in .github/workflows/*.yml.
Given mc-module genuinely depends on real cortexkit-store/cortexkit-lease APIs (not just path resolution) that scripts/provision-rust-ci-stubs.sh only stubs out as empty crates, this was presumably a real "always fails to compile here" situation rather than a flake, so removing it is understandable. But it'd be worth a short comment here (this file already uses a few, e.g. commentlint: allow(JUDGE)) noting where ck-mc-host continues to be compiled/tested (an internal monorepo pipeline with the real crates, presumably) so this isn't mistaken for still being covered by this workflow.
There was a problem hiding this comment.
Addressed on the current head (02c3f17). Rather than documenting where the coverage went, the coverage came back: cargo test -p mc-module --bin ck-mc-host plus --test host_adapter --test lifecycle_cli now run in the mc-host-lifecycle job, which checks out the real ahrav/commons so the cortexkit-* path-deps resolve for real instead of as empty stubs.
The check-plugin job also carries the comment you suggested, explaining why the metadata-only stubs are provisioned there (bun run test ends in test:release, whose dependency-boundary check shells out to cargo tree -p mc-host, and cargo will not load the workspace until every path-dep manifest exists).
| tempfile::tempdir().expect("temp data root") | ||
| } | ||
|
|
||
| fn plant_fifo(path: &Path) { |
There was a problem hiding this comment.
Minor: shelling out to mkfifo (default/umask mode) and then chmod-ing to 0o600 afterward reintroduces a brief window where the FIFO exists with broader permissions, versus the previous rustix::fs::mkfifoat(..., Mode::from_raw_mode(0o600)) which set the mode at creation. Low risk here since call sites use tempfile::tempdir() (0700, so other users can't traverse in), but worth a one-line comment noting that tradeoff was accepted for macOS portability (since mkfifoat apparently isn't usable there), so it isn't "fixed" again later by someone unaware of why.
There was a problem hiding this comment.
Fixed rather than documented, in 02c3f17 — the mode is now applied at creation, so there is no window to explain away:
let status = std::process::Command::new("mkfifo")
.arg("-m")
.arg("600")
.arg(path)
.status()POSIX has mkfifo -m set the permission bits as part of creating the node instead of deriving them from the umask, which I verified rather than assumed:
$ ( umask 000; mkfifo -m 600 a; mkfifo b ); stat -c '%a %n' a b
600 a
666 bThe set_permissions call is gone, since it is now redundant. The rationale you would have wanted in a comment is in the doc comment on plant_fifo: why the shell-out exists at all (rustix gates mkfifoat away from Apple targets and the crate is deny(unsafe_code)), and why -m is load-bearing rather than cosmetic.
Review summarySmall, well-scoped CI-portability fix. Verified the reasoning behind each change:
No security concerns — the |
| - uses: actions/checkout@v5 | ||
| with: | ||
| repository: ahrav/commons | ||
| ref: d2208eda95d845193d84199dbfd0781a1f479291 | ||
| path: commons |
There was a problem hiding this comment.
Cross-repo checkout of a private repo without a token — job will fail on every run.
This actions/checkout step checks out ahrav/commons with no token: input, so it falls back to the workflow's default GITHUB_TOKEN, which is scoped only to ahrav/magic-context. This repo's own comments elsewhere in this file confirm commons is private ("Rust is intentionally absent from public CI because its private ../commons and ../subconscious path-deps are not provisioned here"). No workflow in .github/workflows/*.yml uses a secrets.* PAT for cross-repo checkout (only CLAUDE_CODE_OAUTH_TOKEN appears, for an unrelated action), so there's nothing granting this step access to a different private repository.
Failure scenario: on the next PR/push, the mc-host-lifecycle job's first checkout step fails with a 404/permission error before any test runs, breaking this job unconditionally.
Suggested fix: pass an explicit PAT/deploy-key input, e.g. token: ${{ secrets.COMMONS_CHECKOUT_TOKEN }}, and add the corresponding repo secret.
There was a problem hiding this comment.
Investigated this one and it does not reproduce — ahrav/commons is a public repository, so the default GITHUB_TOKEN can check it out cross-repo without a PAT:
$ gh api repos/ahrav/commons --jq '{full_name, private, visibility}'
{"name":"ahrav/commons","private":false,"visibility":"public"}The signal that pointed you at "private" was a stale comment in this very file describing the ../commons path-deps as private; I have reworded it, since it is the thing that makes this look like a permissions bug when it is not.
The mc-host lifecycle integration failure you saw had a different cause: the PR was CONFLICTING against its base, so GitHub could not build a merge ref and no pull_request workflow ran on that head. That is fixed — the branch is rebased and now reports MERGEABLE.
Both checkouts in the job do now set persist-credentials: false, so no token is left in either .git/config.
| - name: Native module adapter and CLI lifecycle | ||
| run: | | ||
| cargo test -p mc-module --bin ck-mc-host | ||
| cargo test -p mc-module \ | ||
| --test host_adapter \ | ||
| --test lifecycle_cli |
There was a problem hiding this comment.
macOS build/test coverage for mc-module/ck-mc-host/lifecycle_cli is silently dropped.
Before this diff, cargo build -p mc-module --bin ck-mc-host ran unconditionally (both ubuntu-latest and macos-latest) inside shm-source-build's "Source-build transport, host, addon, and iceoryx" step, and cargo test -p mc-module --test lifecycle_cli ran right after it — so both OSes in the matrix built the binary. This PR moves both checks into the new mc-host-lifecycle job, which is pinned to runs-on: ubuntu-latest only, with no macOS leg and no comment explaining the parity loss (unlike the macOS omission elsewhere in this file, which is explicitly justified: "No retained macOS provider: Linux-gated crash/soak harnesses are absent by cfg, so this proves side-effect-free omission").
Failure scenario: a change that breaks mc-module/ck-mc-host only on macOS (e.g. a cfg(unix)-gated path difference, a macOS-specific rustix/libc quirk) now ships without any CI signal, whereas previously the matrixed source-build job would have caught it.
There was a problem hiding this comment.
Fixed on the current head (02c3f17) — the job is matrixed again, so the macOS leg is back:
mc-host-lifecycle:
name: mc-host lifecycle integration (${{ matrix.os }})
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]Your reasoning is what makes this the right fix rather than a comment: the moved tests are unix-gated, not Linux-only, so a cfg(unix) path difference or a macOS rustix/libc quirk genuinely needed a macOS leg to be caught. This PR's own subject matter — mkfifoat not being usable on Apple targets — is an instance of exactly that class of break.
| fn plant_fifo(path: &Path) { | ||
| let status = std::process::Command::new("mkfifo") | ||
| .arg(path) | ||
| .status() | ||
| .expect("run mkfifo"); | ||
| assert!(status.success(), "mkfifo failed for {}", path.display()); | ||
| std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) | ||
| .expect("set fifo mode"); | ||
| } |
There was a problem hiding this comment.
plant_fifo creates the FIFO with loose permissions before tightening them — TOCTOU regression vs. the removed atomic call.
The deleted code created the FIFO atomically at mode 0o600 via rustix::fs::mkfifoat(..., Mode::from_raw_mode(0o600)). This replacement shells out to the mkfifo command (which creates the node using the process umask — typically 0644/0666) and only tightens it to 0o600 in a separate set_permissions call afterward.
Failure scenario: between the mkfifo call returning and set_permissions completing, the FIFO exists on disk with broader-than-intended permissions. These are security/coordination-lock tests (lifecycle.rs), so the very invariant being exercised — that the coordination FIFO is never accessible outside its intended mode — is briefly violated by the test's own setup. On a shared/multi-tenant machine (or under a permissive umask) this is a real, if narrow, window; at minimum it undermines the precision the original atomic call was providing.
Consider setting the umask to 0o077 before calling mkfifo, or restoring atomic creation (e.g. keep using rustix::fs::mkfifoat for the mode-sensitive path and only use the mkfifo shell-out where portability truly requires it).
There was a problem hiding this comment.
Confirmed and fixed in 02c3f17. Your framing is the one I acted on: these are the tests that assert a coordination node is never reachable outside its intended mode, so the fixture must not violate that invariant while setting itself up.
Rather than setting the umask around the call (process-global, and these tests run in parallel) or restoring mkfifoat (rustix gates it away from Apple targets, and the crate is deny(unsafe_code)), the mode is now set at creation:
std::process::Command::new("mkfifo").arg("-m").arg("600").arg(path)Verified that -m really does bypass the umask rather than being masked by it:
$ ( umask 000; mkfifo -m 600 a; mkfifo b ); stat -c '%a %n' a b
600 a
666 bThe separate set_permissions call is removed, so there is no longer a create-then-tighten sequence at all.
| - uses: actions/checkout@v5 | ||
| with: | ||
| repository: ahrav/commons | ||
| ref: d2208eda95d845193d84199dbfd0781a1f479291 | ||
| path: commons |
There was a problem hiding this comment.
This checkout of ahrav/commons uses the default GITHUB_TOKEN, which is scoped to this repo only — no token: override is passed. If ahrav/commons is private, this step will fail to check out the repo for any workflow run that doesn't already have cross-repo access configured (e.g. via a PAT/deploy-key secret), and every job that depends on it will fail.
This isn't hypothetical here: the PR's own statusCheckRollup currently shows mc-host lifecycle integration failing on this exact head commit. Worth confirming whether that failure is this checkout (private-repo auth) vs. something else (e.g. the new test targets themselves) before merging — right now this job can't be relied on as a coverage gate.
There was a problem hiding this comment.
Checked before merging, as you suggested, and the checkout is not the cause — ahrav/commons is public, so the default GITHUB_TOKEN is sufficient:
$ gh api repos/ahrav/commons --jq '{full_name, private, visibility}'
{"name":"ahrav/commons","private":false,"visibility":"public"}The real cause of mc-host lifecycle integration not being a usable gate was that this PR was in a CONFLICTING state against its base branch. GitHub cannot create a merge ref for a conflicted PR, so pull_request-triggered workflows never ran on the head commit — the check was not failing on its merits, it could not start. The branch is now rebased onto the updated base, reports MERGEABLE, and CI is running.
I also reworded the comment in this file that described ../commons as private, since that is what made a permissions explanation look likely.
|
|
||
| mc-host-lifecycle: | ||
| name: mc-host lifecycle integration | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
This new job pins runs-on: ubuntu-latest with no matrix, whereas the cargo build -p mc-module --bin ck-mc-host and cargo test -p mc-module --test lifecycle_cli steps it replaces previously ran unconditionally inside shm-source-build's [ubuntu-latest, macos-latest] matrix (see the removed lines above at ~L131-136). All the tests moved/added here (broca_protocol, broca_subprocess, harness_closure, protocol_vectors, host_adapter, lifecycle_cli) are unix-gated, not Linux-only, so this is a real loss of macOS CI coverage for mc-host/mc-module lifecycle code, not just a reorganization.
Separately: Shared memory source build (macos-latest) is also currently failing on this PR's head commit — may be worth checking whether that's related before this merges, since it touches the same lifecycle/mkfifo code this PR is trying to make more portable.
There was a problem hiding this comment.
Fixed on the current head (02c3f17): mc-host-lifecycle runs os: [ubuntu-latest, macos-latest], so broca_protocol, broca_subprocess, harness_closure, protocol_vectors, host_adapter, and lifecycle_cli all get a macOS leg again. Agreed these are unix-gated rather than Linux-only, so the parity loss was real and not a reorganization.
On the second point — the failing checks on the old head — the root cause was not the lifecycle code. This PR was in a CONFLICTING state against its base, so GitHub could not construct a merge ref and pull_request-triggered workflows could not run on the head commit at all. The branch is now rebased onto the updated base and reports MERGEABLE, and CI is running on 02c3f171.
ReviewScope: CI workflow restructuring for
CI coverage regression: the previous Rust changes (
No security concerns beyond the CI token point above — this diff doesn't touch runtime/production code paths. |
1e20cf0 to
c2c6704
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2c67046c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const PAYLOAD_INDEX_PATH = "release/mc-host-payload-index.json"; | ||
| const STOP_PROVENANCE_PATH = "release/mc-host-n-minus-one-stop.json"; | ||
| const SHA256_RE = /^[0-9a-f]{64}$/; | ||
| const QUALIFICATION_WORKFLOW_PATH = ".github/workflows/mc-host-release-qualification.yml"; |
There was a problem hiding this comment.
The qualified release gate now requires every proof to name this exact workflow and later checks that the GitHub run's path and head_sha match it, but a repo-wide search shows that .github/workflows/mc-host-release-qualification.yml does not exist in this commit. Consequently, no workflow run at this commit can satisfy --check, making production-qualified release evidence impossible to validate until this workflow is added or the constant is changed to an existing qualification workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: .github/workflows/mc-host-release-qualification.yml does not exist, at this commit or on the base branch, so --check in qualified mode cannot be satisfied today.
Leaving it as-is deliberately, with reasoning:
- The constant fails closed. An absent workflow means no proof can claim a qualified source, which is the safe direction for a release gate — it cannot be satisfied by accident.
- It is not introduced or changed by this PR. The constant and the
workflow !== QUALIFICATION_WORKFLOW_PATHcheck both live on the base branch already, so this is a property of the release-evidence stack, not of this CI-portability change. - Nothing in CI invokes the qualified path yet;
verify-mc-host-release-evidenceis not wired intoci.yml.
So the accurate statement is that qualified release evidence is not yet producible, by construction, and the workflow has to land before the first GA attempt. Tracking it as a release-blocking follow-up rather than widening the constant to an existing workflow, which would weaken the gate to make it satisfiable.
| else if (normalized.includes("probe_required")) mapped = "probe_required"; | ||
| else if (normalized.includes("idempotency_conflict")) mapped = "idempotency_conflict"; | ||
| else if (normalized.includes("schema")) mapped = "schema_violation"; | ||
| else if (normalized.includes("daemon_generation_changed")) mapped = "module_restarted"; |
There was a problem hiding this comment.
When the daemon rotates after initialization, the expected-ID fence raises daemon_generation_changed, which this line maps into the existing module_restarted retry path. That path immediately resubmits with the same stale compatibleDaemonId; the ID is cleared only later by logCallFailure, after the retry has failed too. As a result, embedItems always wastes its one restart retry, and the detailed ledger path can consume its durable restart budget without sending work to the new daemon. Re-run managed compatibility and refresh the expected daemon ID before spending the restart retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed on this branch — and the fix is the one you described. daemon_generation_changed no longer routes into the module_restarted retry path:
// A daemon_generation_changed rejection is client-side and pre-publication:
// no request reached the daemon, so it is transient transport failure, not
// `module_restarted` evidence, which spends a page's single durable restart
// budget on an attempt the daemon never saw.
else if (normalized.includes("daemon_generation_changed")) mapped = "transport";And the stale binding is cleared at the fence site before rethrow, not later in logCallFailure:
if (readErrorCode(error) === "daemon_generation_changed") {
this.initialized = false;
this.compatibleDaemonId = null;
throw classified;
}So the restart budget is no longer spent on a rejection the daemon never saw, and the next initialize() revalidates against the live daemon instead of resubmitting the stale compatibleDaemonId. Worth noting the base branch still maps this to module_restarted; this branch is where it is corrected.
| status: null, | ||
| }; | ||
| } | ||
| const catalog = await client.catalogList(); |
There was a problem hiding this comment.
The probe deadline is created before McHostClient.connect, but catalogList() starts a fresh request deadline using the client's original full requestTimeoutMs. If the handshake consumes part of the lifecycle budget and the catalog response stalls, status/doctor can exceed outerAggregateMs by nearly another full budget before reaching the remaining-time check at line 165. Pass the remaining deadline into the catalog request or externally race it so the advertised aggregate lifecycle bound still applies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed — catalog.list is now charged against the remaining lifecycle budget instead of starting a fresh requestTimeoutMs:
const catalogMs = deadline - Date.now();
if (catalogMs <= 0) throw new Error("compatibility probe deadline expired");
const catalog = await client.catalogList({ timeoutMs: catalogMs });catalogList/catalogSnapshot now take an optional timeoutMs, mirroring hostStatus, which was already deadline-bounded — that asymmetry inside the same function was the clearest evidence your reading was right. Since connect is constructed with requestTimeoutMs = budgetMs, the unbounded call could previously let a handshake plus a stalled catalog response run to roughly twice the advertised aggregate before the remaining-time check was reached.
Credit where due: the reviewed version of this landed on the base branch, so what you are seeing here is that fix arriving via the rebase rather than a separate patch.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| : "linux", | ||
| self_fd_verified: target.self_fd_verified, | ||
| target: target.target, | ||
| test_report_path: |
There was a problem hiding this comment.
CRITICAL: Target test report verification can be bypassed when observations contain null
In validateInstalledReleaseEvidenceAgainstArtifacts, the expected observation shape for target proofs sets both test_report_path and test_report_sha256 using ternary fallbacks that default to null. If a target proof's observations has test_report_path: null and test_report_sha256: null (or omits them while declaring null values), expectedObservations matches observations without verifying any test report bytes or checking that a test report exists.
| test_report_path: | |
| test_report_path: typeof observations.test_report_path === "string" && isSafeRelativePath(observations.test_report_path) ? observations.test_report_path : fail(`target proof ${proof.subject} missing test_report_path`), |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed as a real bypass, and closed. The two report fields were the only members of the target expected-observation shape derived from observations itself — every other field comes from evidence — so the comparison was self-referential and a proof declaring test_report_path: null matched a pair of nulls while verifying no report bytes.
I verified the bypass empirically before fixing it: a proof mutated to cite null/null passed validation with nothing thrown.
The fix in the tree is the base branch's verifyTargetTestReport, which is stronger than what I first wrote — it also requires the path to sit under the expected report directory, rejects one report satisfying two targets, and validates the report's own schema/target/passed rather than only its digest:
if (typeof reportPath !== "string" || !isSafeRelativePath(reportPath) ||
!reportPath.startsWith(TEST_REPORT_DIR)) {
fail(`${identity} must cite a test report under ${TEST_REPORT_DIR}`);
}Regression coverage exists for exactly your scenario: a target proof cannot opt out of its test report with a null citation, plus cases for an unrelated file, a report attesting the wrong target, and one report reused across two targets.
| const installedEvidencePath = join(rootDir, EVIDENCE_PATH); | ||
| const installedEvidenceBytes = readFileSync(installedEvidencePath); | ||
| if ( | ||
| canonicalJson(JSON.parse(installedEvidenceBytes.toString("utf8"))) !== |
There was a problem hiding this comment.
WARNING: Unhandled SyntaxError on malformed installed release evidence JSON
When verifying installed evidence attestation in qualified mode, JSON.parse(installedEvidenceBytes.toString("utf8")) is called directly without error handling. If tmp/mc-host-installed-release-evidence.json on disk is truncated or contains invalid JSON, an unhandled SyntaxError crash occurs rather than a structured evidence verification error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 02c3f17. Good catch — the inconsistency was the tell: the proof-artifact parse a few hundred lines above already reported structurally, while this one did not.
// Truncated or hand-mangled bytes on disk are a failed attestation, not a
// crash: an unguarded parse would abort with a bare SyntaxError instead of
// the structured failure every other malformed-input path here reports.
let installedEvidenceValue: unknown;
try {
installedEvidenceValue = JSON.parse(installedEvidenceBytes.toString("utf8"));
} catch {
fail(`installed release evidence at ${EVIDENCE_PATH} is malformed JSON`);
}
if (canonicalJson(installedEvidenceValue) !== canonicalJson(evidence)) {A truncated tmp/mc-host-installed-release-evidence.json now fails as an evidence-verification error naming the file, instead of aborting with a bare SyntaxError.
| epochs: observedEpochsFromMagicContextMetrics(magicContextMetrics), | ||
| evaluatedThrough: "epochs" as const, | ||
| }; | ||
| evaluateCompatibility({ |
There was a problem hiding this comment.
SUGGESTION: Unused evaluateCompatibility invocation in readCompatibilityProbe
evaluateCompatibility is a pure function that returns a CompatibilityVerdict object; its return value is discarded here. The actual compatibility verdict evaluation and check synthesis happen downstream in McHostLifecyclePolicy.applyCompatibility().
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Already resolved — the discarded evaluateCompatibility call is gone from the compatibility probe, and you are right about where the verdict belongs: McHostLifecyclePolicy.applyCompatibility() owns it.
readCompatibilityProbe now calls only evaluateDaemonCompatibility and evaluateModuleCompatibility, and it consumes both return values (daemon.ok, modules.ok) to decide whether to short-circuit before host.status. There is no evaluateCompatibility reference left in managed-policy.ts.
| } | ||
|
|
||
| /** Collect one bounded JSON stream through StreamEnd, preserving item order. */ | ||
| async requestStream( |
There was a problem hiding this comment.
SUGGESTION: Add generic item type parameter to requestStream
requestStream is currently typed to return Promise<unknown[]>. Adding a generic item type parameter (e.g. requestStream<Item = unknown>(handle: RouteHandle, body: unknown, options: RequestOptions = {}): Promise<Item[]>) provides type safety for callers and matches call<Response = unknown>.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Done in 02c3f17 — requestStream takes an item type parameter, matching call<Response = unknown>:
async requestStream<Item = unknown>(
handle: RouteHandle,
body: unknown,
options: RequestOptions & { maxStreamItems?: number } = {},
): Promise<Item[]> {The per-item decode returns json.value as Item, which is the same narrowing idiom parseResponseJson<Response> already uses for the unary path, so both surfaces now behave consistently and callers stop casting unknown[] at each use.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summaries (2 snapshots, latest commit 0efa248)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0efa248)Status: No Issues Found | Recommendation: Merge Files Reviewed (29 files)
Previous review (commit c2c6704)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (27 files)
Reviewed by gemini-3.7-flash · Input: 164.2K · Output: 17.9K · Cached: 1.4M |
There was a problem hiding this comment.
💡 Codex Review
magic-context/packages/plugin/src/shared/mc-host-lifecycle/policy.ts
Lines 319 to 320 in 0efa248
Bind storage readiness to the certified daemon
If mc-host rotates after compatibility succeeds but before this storage probe connects, the default storage probe reads host.status from the new daemon while the returned authenticatedDaemonId still names the old one. The outcome can thus report storage ready for an incarnation that subsequent fenced traffic is guaranteed to reject, wasting the transport's recovery attempt and failing an otherwise healthy operation. Pass the certified daemon ID into the storage probe and reject any handshake or response observed from a different incarnation.
ℹ️ 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".
| // through raceDetached below. | ||
| let flight = this.inflightCompatibility; | ||
| if (!flight) { | ||
| flight = this.compatibilityProbe(Math.max(1, compatibilityBudget), undefined); |
There was a problem hiding this comment.
When concurrent demand-start callers have different remaining deadlines, the first caller creates the shared compatibility flight using its own compatibilityBudget. A nearly expired caller can therefore start a 1–10 ms probe that a later caller with ample time joins; the underlying handshake or RPC then times out on the short budget, so every joined caller fails even though their individual raceDetached deadlines remain live. Run the shared probe with a policy-owned budget and use raceDetached only to enforce each caller's deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one does not reproduce on the current head — it describes code the rebase replaced.
Your premise is that the first caller creates the shared flight with its own compatibilityBudget. That was true of the previous revision, which computed Math.min(COMPATIBILITY_HARD_BUDGET_MS, remainingMs) from the calling waiter. The rebase onto the updated base replaced it with a policy-owned budget:
snapshot = await this.raceDetached(
this.sharedCompatibility(
rootResolution.ok ? rootResolution.root : "\u0000no-root",
this.outerAggregateMs,
),
request.signal,
remainingMs,
);The shared probe is charged this.outerAggregateMs, which is policy-owned and independent of any waiter, and raceDetached(..., remainingMs) enforces each caller's own deadline separately. That is your recommendation verbatim, so a nearly-expired waiter can no longer cap a probe that a later waiter joins.
Worth noting the flight is also keyed per data root now (Map<string, Promise<…>>), so waiters against different roots do not share a flight at all.
| body: encodeBody(body), | ||
| deadline, | ||
| options, | ||
| mode: "stream", |
There was a problem hiding this comment.
For a stream that emits many tiny JSON frames, this mode retains every StreamData body until StreamEnd, while the connection's memory accounting charges only each body's wire byte length and not the per-item object/array overhead. A peer can consequently send millions of one-byte JSON values within the byte cap and exhaust the client heap before the request deadline. Add a retained-item ceiling for stream requests and terminate or cancel the correlation when it is exceeded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already bounded — the ceiling exists and is enforced below the facade, so the unbounded-retention path you describe is not reachable.
requestStream accepts it, client.ts forwards it, and connection.ts applies a default when the caller omits it:
maxStreamItems: params.maxStreamItems ?? DEFAULT_MAX_STREAM_ITEMS,So a stream is bounded by the retained-item ceiling in addition to the pending byte budget, which is the two-part bound the doc comment on requestStream describes. There is coverage for the enforcement path in client.test.ts driving requestStream(handle, …, { maxStreamItems: 2 }).
That means a peer sending millions of one-byte JSON values is terminated on the item ceiling rather than being allowed to exhaust the heap under the byte cap.
| } | ||
| }); | ||
| } | ||
| const snapshot = await this.raceDetached(flight, request.signal, remainingMs); |
There was a problem hiding this comment.
When the daemon disappears during the handshake/catalog/status sequence, the shared compatibility promise rejects here with a raw client or generic probe error. Unlike caller aborts and deadlines, this is an operational lifecycle failure, but demandStart() now propagates it instead of returning a closed DaemonResultV1; callers therefore cannot inspect a stable reason/remediation and may not classify errors such as authenticated peer changed during compatibility probe as recoverable connection failures. Catch non-detachment probe failures and return native_probe_unavailable with null storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already implemented on the current head, and the code is your recommendation almost line for line:
} catch (error) {
// Detachment is the caller's own deadline or signal and stays a
// thrown control outcome. Any other probe failure is an unproven
// compatibility claim, so it becomes a typed closed result rather
// than an unclassified rejection callers cannot act on.
if (error instanceof WaiterDetachedError) throw error;
return {
result: {
...result,
ok: false,
reason: "native_probe_unavailable",
remediation: remediationForReason("native_probe_unavailable"),
},
storage: null,
};
}Non-detachment probe failures — including authenticated peer changed during compatibility probe — return a closed DaemonResultV1 with reason: "native_probe_unavailable", its remediation, and null storage. Only WaiterDetachedError propagates, which is the caller's own signal or deadline rather than a lifecycle failure.
This is another case where the finding was accurate about the revision under review and resolved by the rebase onto the updated base.
| if (readErrorCode(error) === "daemon_generation_changed") { | ||
| this.initialized = false; | ||
| this.compatibleDaemonId = null; | ||
| throw classified; |
There was a problem hiding this comment.
When one managed Synapse call detects a daemon rotation, clearing the provider-wide identity here allows a concurrent call that already passed initialize() to reach callWithRetry() with compatibleDaemonId === null; that call then omits expectedDaemonId and can publish to the newly connected daemon without any lifecycle compatibility proof. The fresh evidence beyond the earlier stale-retry report is this shared-state race introduced by the immediate reset: retain the last certified identity until successful revalidation and make managed publication fail closed whenever no certified identity is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Traced this one carefully because a fail-open publication fence would be serious, and it does not reproduce: managed publication already fails closed on a null identity, twenty lines above the spread you cite.
// The managed lane publishes only against the incarnation the
// lifecycle owner certified. An absent identity means the fence
// cannot be proved, so refuse rather than publish unfenced.
if (this.connectionOrigin === "managed-default" && this.compatibleDaemonId === null)
throw new SynapseEmbeddingError(
"module_restarted",
"managed Synapse lane has no certified daemon identity",
);So the concurrent call in your scenario cannot reach client.call at all. It is refused with module_restarted before publication. The ...(this.compatibleDaemonId === null ? {} : { expectedDaemonId }) spread is only reachable when either the identity is present — in which case the fence is applied — or connectionOrigin !== "managed-default", which is a caller-supplied connection with no managed certification to prove in the first place.
That guard is precisely the second half of your recommendation ("make managed publication fail closed whenever no certified identity is available"), and it is what makes clearing the identity the conservative choice rather than a fail-open one: a racing call gets a typed refusal, not an unfenced publish.
I did apply the first half where it was load-bearing: rebindAfterModuleRestart no longer clears compatibleDaemonId, because initialize is its only writer on the success path and clearing it there could erase an incarnation a sibling had already re-certified.
Review findings from PR 62: - classify daemon_generation_changed as transient transport failure and reset the daemon binding before rethrow, so a pre-publication fence rejection never spends a page's single durable restart budget or deterministically retries a stale identity into page_terminal - delete the discarded evaluateCompatibility call in the compatibility probe; the policy owns the verdict - stop fabricating storage/synapse readiness failures for components the probe never observed after a compatibility short-circuit - run the mc-host lifecycle CI job on ubuntu and macos, and set persist-credentials: false on both checkouts - anchor the attestation runInvocationURI binding to the source repository, matching the anchored run_url validation - compare proof head_sha to the release commit only under requireQualified, where the SHA guard proves it, with its own failure message - guard test-report hashing with existsSync so a missing citation fails structurally instead of crashing with ENOENT - share one daemonIdEquals (constant-time, absent-never-matches) across the client, transport, and managed-policy identity fences - prune route-cache entries stranded under prior daemon-id suffixes - cap the demand-start compatibility probe at a hard budget and coalesce concurrent waiters onto one in-flight probe
Review findings from PR 62 that survive the rebase onto the updated base. The base branch already landed the reviewed versions of the target test-report gate and the catalog.list deadline bound, so this keeps only what it does not have. - report malformed installed-evidence JSON as a verification failure rather than letting an unguarded JSON.parse abort with a bare SyntaxError. Every other malformed-input path in this verifier already reports structurally, including the proof-artifact parse a few hundred lines above. - create the coordination FIFO test fixture with mkfifo -m 600 so the mode is applied at creation. Creating it under the umask and tightening it with a separate chmod left a window where the node was reachable at a wider mode, which is the invariant these tests exercise. - give requestStream an item type parameter, matching call(), so callers are not handed unknown[] to cast at every use. - drop the duplicate daemonIdEquals helper. The base branch fences daemon identity through sameDaemonId, so this branch's copy had no callers left after the rebase. - keep a single execution_provider field in the synapse_perf lane fixture, which both branches had added independently.
0efa248 to
02c3f17
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02c3f17105
ℹ️ 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".
| - uses: actions/checkout@v5 | ||
| with: | ||
| repository: ahrav/commons | ||
| ref: d2208eda95d845193d84199dbfd0781a1f479291 | ||
| path: commons | ||
| persist-credentials: false |
There was a problem hiding this comment.
This new matrix job checks out the repository described elsewhere in this workflow as the private ../commons dependency, but it does not pass a PAT or GitHub App token. The actions/checkout documentation for multiple private repositories states that the default ${{ github.token }} is scoped to the current repository, so both matrix legs fail at this step before running the lifecycle tests. Supply a secret token with access to ahrav/commons.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise is a stale comment rather than the repository's actual visibility — ahrav/commons is public, so the default GITHUB_TOKEN checks it out cross-repo without a secret:
$ gh api repos/ahrav/commons --jq '{full_name, private, visibility}'
{"name":"ahrav/commons","private":false,"visibility":"public"}You quoted this workflow describing ../commons as private, which was fair — the comments did say that, and they were wrong. That is now the second review to reach the same incorrect conclusion from them, so I have rewritten all three in 3353fbd7 to describe the property that actually matters (whether a job checks the sibling sources out or stubs them) instead of repo visibility.
The checkout documentation you cite is correct that github.token is scoped to the current repository; that scoping applies to private cross-repo reads, which is not this case.
For evidence rather than argument: the mc-host-lifecycle job is running on the current head, past the checkout step.
| // A daemon_generation_changed rejection is client-side and pre-publication: | ||
| // no request reached the daemon, so it is transient transport failure, not | ||
| // `module_restarted` evidence, which spends a page's single durable restart | ||
| // budget on an attempt the daemon never saw. | ||
| else if (normalized.includes("daemon_generation_changed")) mapped = "transport"; |
There was a problem hiding this comment.
When a managed daemon rotates during embedItems, callWithRetry now clears the stale identity and throws this newly classified transport error, but the page loop only invokes rebindAfterModuleRestart for module_restarted; its outer catch then swallows the transport failure and advances to the next page, returning no vectors for the affected page even though the rejection occurred before publication. The fresh evidence after the earlier stale-retry report is this new transport mapping, which bypasses the newly added rebind path entirely; handle daemon_generation_changed as an immediate within-deadline rebind without charging the durable restart budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this is a real gap that this branch introduced. Good find — it is the second-order consequence of the reclassification, which the earlier round asked for and I made.
Verified at the page loop:
} catch (error) {
const classified = classifyError(error);
if (classified.code !== "module_restarted" || restarted) throw classified;
restarted = true;
await this.rebindAfterModuleRestart(deadlineAt, signal);
}Once daemon_generation_changed classifies as transport rather than module_restarted, it no longer satisfies that guard, so the rotation is rethrown instead of triggering the rebind — exactly as you describe. The previous mapping charged the durable restart budget for a rejection the daemon never saw; the new one avoids that but skips the rebind, so the page loses its rebind path.
Your proposed shape is the right one: treat daemon_generation_changed as an immediate within-deadline rebind that does not set restarted, so it neither spends the durable budget nor forfeits the page. Holding off on landing it in this PR because it changes durable-ledger retry accounting and belongs with a test that pins "rebinds without consuming the restart budget" — tracking it as a follow-up rather than hand-patching the ledger path late in a CI-portability change.
| expectedStateVersion: row.stateVersion, | ||
| jobId, | ||
| }); | ||
| await this.rebindAfterModuleRestart(deadlineAt, signal); |
There was a problem hiding this comment.
For a managed detailed page that receives module_restarted while polling, this new call already performs a deadline-bounded compatibility rebind, but execution then reaches the pre-existing recertifyForRestart at line 1316 and performs a second demand using the full query timeout. This can extend the operation past the ledger page deadline, and a failure of the redundant second demand records a retryable page failure even though the first rebind succeeded and the restart budget has already been persisted as spent. Keep only the new deadline-aware rebind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — and this is one I had already flagged internally while resolving the rebase, so it is good to have it pinned to specific lines.
Both paths are present: rebindAfterModuleRestart(deadlineAt, signal) at the polling site, and the pre-existing recertifyForRestart(signal) further down. They now overlap because this branch converted three of the base's four recertifyForRestart call sites to the deadline-aware rebind and left the ledger-conflict site on the original.
They are not quite interchangeable, which is why I did not simply delete one during the rebase: rebindAfterModuleRestart throws a typed error, while recertifyForRestart returns a boolean and its call site rethrows the original classified error, preserving error provenance for the ledger row. Collapsing them changes which error a failed page records.
Your two consequences are the ones that matter though — a second full-timeout demand can push past the ledger page deadline, and a failure of the redundant demand records a retryable page failure after the restart budget is already persisted as spent. Consolidating onto the deadline-aware rebind, keeping the original error on the failure path, is the fix; grouping it with the rebind change from your other comment so both land with the ledger tests rather than separately.
Review response and rebase onto the updated baseAll 15 inline review comments now have individual replies. Summarising here because the single most important finding is not in any one thread: this branch had gone stale as a stack entry, and that explains most of the review findings. The merge conflict, and why it matteredThe PR was The cause: PRs #60 ( Resolved by rebasing onto the updated base and, wherever the two sides had solved the same problem, keeping the base's reviewed version. The branch now reports The result is that this PR is finally the size it should be — +169/−60 across 7 files, scoped to CI portability and lifecycle verification, instead of re-litigating two merged stack entries. What that means for the review findingsSeveral findings were real bugs in code this branch was carrying a stale copy of, and the base already fixed them — better than I first did:
Two findings were fixed only on this branch, and survive the rebase:
Fixed in this push (
|
Three comments called ../commons a private repository. It is public, so the default GITHUB_TOKEN checks it out cross-repo without a secret, and two reviewers independently read those comments as evidence that the mc-host-lifecycle checkout must be failing on permissions. The property that actually matters is whether a job checks the sibling sources out or stubs them, so say that instead. Also point at the mc-host-lifecycle job as the place the ck-mc-host build and lifecycle_cli contract now run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3353fbd77a
ℹ️ 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".
| try { | ||
| installedEvidenceValue = JSON.parse(installedEvidenceBytes.toString("utf8")); | ||
| } catch { | ||
| fail(`installed release evidence at ${EVIDENCE_PATH} is malformed JSON`); |
There was a problem hiding this comment.
When --check or --check-schema is run with a truncated or malformed tmp/mc-host-installed-release-evidence.json, main() parses that file through readJson at line 1169 before invoking this validator, so execution still aborts with the bare SyntaxError that this new catch is meant to replace. Guard the initial parse in main/readJson (or have the validator perform the only parse) so the actual CLI path reports the structured verification failure.
Useful? React with 👍 / 👎.
The restored macOS leg immediately found a real macOS bugReporting this because it is the direct consequence of restoring the macOS coverage two reviewers asked for, and it now blocks this PR's own CI.
Three tests, all at Cause
pub(crate) const HARDENED_DIR_FLAGS: OFlags = OFlags::DIRECTORY
.union(OFlags::NOFOLLOW)
.union(OFlags::RDONLY)
.union(OFlags::CLOEXEC);
Refusing symlinked intermediates is deliberate and correct — the comment in This is pre-existing in Fix directionCanonicalise the data root once, up front, then run the hardened no-follow walk over the canonical path. That keeps the anti-swap property for the walk — which is what the hardening is actually for — while tolerating an OS-provided symlink above the trust boundary. Fixing it in the test harness alone would hide the production case. Not landing that here: it changes security-sensitive path handling and deserves its own PR and its own test, rather than being appended to a CI-portability change. Options for this PR are to land that fix first, or to gate these three tests on the canonicalisation landing. Also still red, and not yet attributed
For reference, the two jobs that were failing before the rebase — |
1 participant
Summary
CI now runs the native lifecycle integration suites against a pinned public
ahrav/commonscheckout, including Broca protocol/subprocess, closure, wire-vector, adapter, and CLI lifecycle proofs. FIFO hostile-shape fixtures compile portably on macOS, and the Rust/TypeScript authentication tests use one literal cross-language vector.Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningsStack
Layer 5 of 7 above #46. Parent: #61. Next: #64.