fix(ci): restore portable lifecycle source checks - #62
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
![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.
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.
| 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.
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.
| - 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.
| 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).
| - 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.
|
|
||
| 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.
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. |
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.