feat(mc-host): materialize immutable harness closures - #53
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}
| (Harness::Pi, "openai-codex") => "openai", | ||
| _ => provider, | ||
| }; | ||
| let actual = presented |
There was a problem hiding this comment.
Critical: every real Send will fail credential_snapshot_mismatch.
CredentialVerifier::verify requires presented.get(canonical_provider) to succeed, but:
ck_mc_host/serve.rs:494unconditionally usesBrocaComponent::new_with_credentials(wasBrocaComponent::newbefore this PR), socredential_verifieris alwaysSomein production.control.rs:373-374defaults a missingcredential_fingerprintsfield to an emptyBTreeMap.BindIdentityinpackages/plugin/src/shared/mc-host-client/types.tshas nocredential_fingerprintsfield, and nothing in this diff adds one to the client.
So presented is always empty for real client requests, and verify() always returns credential_snapshot_mismatch before any Send can succeed.
If this is intentionally deferred to a later PR in the stack (client-side fingerprint population), please note that dependency explicitly — right now this PR alone appears to make Broca non-functional end-to-end.
| let init = storage_init(&root)?; | ||
|
|
||
| let env = EnvSnapshot::capture().map_err(|_| "environment snapshot exceeds bounds")?; | ||
| let env = EnvSnapshot::capture_from( |
There was a problem hiding this comment.
Compounds the credential issue above. This changes the daemon's EnvSnapshot source from EnvSnapshot::capture() (full OS process environment) to EnvSnapshot::capture_from(envelope.credentials...). I couldn't find anywhere on the TS/launcher side (bootstrap.ts, the launcher envelope construction) that populates envelope.credentials — it appears to always be empty in the current wiring.
If that's correct, EnvSnapshot::provider_row will return CredentialMissing for every provider even when ANTHROPIC_API_KEY/OPENAI_API_KEY/GEMINI_API_KEY are genuinely set in the daemon process's real environment — independent of the fingerprint-mismatch issue in broca/mod.rs. Worth confirming this is intentional (and where credentials gets populated) before this ships.
| cancel: CancellationToken, | ||
| model_ref: String, | ||
| ) -> BackendTerminal { | ||
| let mut child_env = match env.provider_row("pi", &request.provider) { |
There was a problem hiding this comment.
New precheck breaks the alias-then-canonical fallback this function exists for.
run_pi now calls env.provider_row("pi", &request.provider) up front, using request.provider (always the canonical form, e.g. "openai") — identically on both the "aliased" first attempt and the "canonical" retry in run_pi_with_provider_fallback. Since credential_failure returns ErrorClass::Permanent (not AuthRequired), a subscription-authenticated user (no OPENAI_API_KEY/GEMINI_API_KEY env var) now fails immediately on the first attempt, and the fallback retry condition (error.class == ErrorClass::AuthRequired, in run_pi_with_provider_fallback) never triggers anyway — even if it did, the retry would hit the exact same precheck and fail the same way.
This contradicts the doc comment directly above run_pi_with_provider_fallback, which says it specifically supports "a user authenticated through the direct openai/google API-key providers [who] has no credentials under the subscription-extension aliases" — that fallback path looks dead for the case it was built for.
| .validate(&digest) | ||
| .map_err(|e| generation_failure(&e))?; | ||
| Ok(digest) | ||
| let launcher = validated.open_verified_file(PRODUCTION_LAUNCHER).ok(); |
There was a problem hiding this comment.
Launcher-verification failure is silently swallowed, defeating the provenance check this PR introduces.
validated.open_verified_file(PRODUCTION_LAUNCHER).ok() discards any Err from hash/size/mode verification (same pattern at line 669). That None is indistinguishable from "no launcher present (dev fixture)" by the time it reaches spawn::spawn_detached (spawn.rs), which then falls back to fexecve'ing std::env::current_exe() — the currently-running CLI — instead of the verified generation launcher.
So a corrupted, deleted, or tampered payload/bin/ck-mc-host in a staged generation doesn't abort start/restart; it silently substitutes a different executable instead of failing closed with something like native_payload_invalid. Consider propagating the verification error instead of .ok()-swallowing it.
| // short-lived CLI can have open; raise if the CLI ever holds | ||
| // more. | ||
| // bounded loop above the CLI's descriptor ceiling. | ||
| if libc::syscall(libc::SYS_close_range, 4u32, u32::MAX, 0u32) < 0 { |
There was a problem hiding this comment.
libc::SYS_close_range has no macOS definition in the libc crate, but this function now has a #[cfg(target_os = "macos")] libc::execve(...) branch a few lines below (line 164), implying macOS is a real target for this binary (consistent with host_target() returning darwin-arm64/darwin-x64 elsewhere in this PR). This line has no cfg guard, so building for macOS will fail with "cannot find value SYS_close_range in module libc". Since there's no macOS CI job for mc-module, this wouldn't be caught before release.
|
|
||
| /// Resolves a listed node to a closure-owned path after revalidating it | ||
| /// through the retained `files/` descriptor. | ||
| pub fn resolve_node(&self, node_path: &str) -> Result<PathBuf, HarnessClosureError> { |
There was a problem hiding this comment.
Verify-then-use-by-path TOCTOU gap. resolve_node opens and hash-verifies the file via open_relative_file + verify_node_file on an fd, then discards that fd and returns a PathBuf string. Callers (broca/pi.rs, broca/opencode.rs) pass that path to std::process::Command::new(path), which re-opens by path at spawn time. If the file at that path is replaced between this verification and the later path-based execve in Command::spawn(), the substituted binary runs without ever having been checked — the fd-based verification here provides no binding to what actually gets executed.
Related: open_relative_file (used here and by copy_node) only checks intermediate path components are directories, never calling verify_owned_directory on them the way the full-tree validate()/validate_tree() path does (owner-only 0700, correct uid). So a loosened intermediate directory permission is caught by store.validate() but not by resolve_node(), and this asymmetry isn't covered by the new test suite.
| STORE_OPEN_WAITING => self | ||
| .store_open | ||
| .waiting_report(now) | ||
| .expect("waiting phase has a waiting report"), |
There was a problem hiding this comment.
This changed from .unwrap_or_else(|| DISPATCH_HEALTH.report(now)) to .expect("waiting phase has a waiting report"). store_open.phase is loaded once at line 12586 to select this match arm, then loaded again inside waiting_report() (line 288) to decide whether to return Some. If a concurrent transition from STORE_OPEN_WAITING to STORE_OPENED (e.g. coordinator.phase.store(STORE_OPENED, Ordering::Release)) happens between those two loads, waiting_report returns None and this now panics instead of falling back gracefully — turning a routine health-check race into a task panic.
| } => { | ||
| spawn::ignore_sigpipe(); | ||
| emit(cmd_start(payload_dir.as_deref())) | ||
| match serve::read_launcher_envelope() { |
There was a problem hiding this comment.
start/restart now unconditionally call serve::read_launcher_envelope(), which does stdin.lock().read_to_end(...) before doing anything else. This is a new stdin dependency: running ck-mc-host start --payload-dir <dir> directly from an interactive terminal (stdin attached to a TTY, not redirected/piped) will now hang indefinitely waiting for EOF, whereas previously start/restart had no stdin read in the parent process. Only wrapper-invoked usage that explicitly closes/redirects stdin is unaffected — worth confirming that's the only supported invocation path.
| ) -> Result<String, CredentialRowError> { | ||
| const DOMAIN: &str = "subc-broca-credential-v1"; | ||
| const CANONICALIZATION: &str = "harness-provider-name-length-value/1"; | ||
| let canonical_provider = match (harness, provider) { |
There was a problem hiding this comment.
Minor maintainability note: this same ("pi", "google-antigravity") => "google" / ("pi", "openai-codex") => "openai" alias table is now duplicated 3-4 times (here, provider_row just above, CredentialVerifier::verify in broca/mod.rs, and pi_model_ref in pi.rs). They currently agree, but adding/renaming a provider alias in only one copy would silently desync credential lookup from fingerprint verification. Worth factoring into one shared function.
|
Automated review summary Reviewed via a multi-agent pass (parallel angle-scanning plus independent verification against the actual source) over the ~6.2k line diff. Inline comments posted on the specific lines below; full findings summarized here for convenience. Likely blocking - please confirm before merge:
If #1-#3 are intentionally deferred to a later PR in this stack (client-side wiring lands separately), it would help to call that dependency out explicitly in the PR description - as written this PR alone appears to make Broca non-functional end-to-end. Security / correctness:
Behavioral regressions / minor:
Smaller notes (not inline-commented, lower confidence/severity):
Nice test coverage on the new harness_closure.rs module (tamper, mode, path-traversal, symlink cases all look solid) - the issues above are mostly at the seams between this PR and the rest of the stack, plus a couple of pre-existing patterns (TOCTOU-by-path) that got new callers. |
1 participant
Summary
Stack
PR 6 of 10. Base:
stack/mc-host-05-payload-packages.Validation
Post-Deploy Monitoring & Validation
Watch
harness_unavailablesubreasons, Broca child cleanup failures, and closure validation errors for one release cycle. Roll back if credential values enter diagnostics or closure descendants survive shutdown. Owner: mc-host maintainers.