fix(containment): apply project-root ownership before bootstrap creates runtime state (#2679) - #2801
zaxbysauce wants to merge 5 commits into
Conversation
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
…es runtime state (#2679) Plugin bootstrap and mcp serve --dir now resolve the project root once, synchronously, before any init-path or first-write consumer touches a directory, using a decision form (resolveProjectRootDecision) that shares assertProjectRoot's ancestor walk. An ordinary child of a project root that owns .swarm/ state resolves to the owning root (bounded always- visible hint + durable advisory record; manifest stays fail-open); declared nested roots, worktrees, and standalone roots stay independent; indeterminable ownership fails closed for state writes. All project- surface consumers (state, config, telemetry, bundled skills, knowledge, teardown) thread the resolved root; concurrent boots cannot interleave a child write.
…trap root (#2679 review) The approved-reviewer scope lifecycle reads the plan ledger (loadPlanJsonOnly) to resolve its task id, which is project-surface state; on a redirected boot it must read the owning project root, not the opened ordinary child, or the auto-review feature silently no-ops. Reclassified in the static source-scan guardrail accordingly.
5900ae4 to
3ce5e2f
Compare
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) 🔍 PR Intent
📦 Implementation SummaryThe PR adds ✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 | SUPPORTED |
src/index.ts:1136 — resolveProjectRootDecision(ctx.directory) before parallel init I/O |
| O-002 | SUPPORTED |
src/cli/mcp.ts:113 — resolveProjectRootDecision(resolved) in resolveMcpRoot |
| O-003 | SUPPORTED |
src/index.ts:1144 redirect branch, src/cli/mcp.ts:122–128 |
| O-004 | SUPPORTED |
walkProjectBoundary → marker-root short-circuit via hasExplicitProjectBoundary |
| O-005 | SUPPORTED |
projectIndicatorState consulted only for .swarm-bearing ancestors |
| O-006 | SUPPORTED |
walkProjectBoundary returns fail-closed for depth/ancestor-swarm/ancestor-indicators |
| O-007 | PARTIALLY_SUPPORTED |
Hint is unconditional (correct); advisory record is unconditional by design, but writeBootstrapRootRedirectRecord has unbounded sync I/O on the init path |
| O-008 | SUPPORTED |
tests/unit/index-bootstrap-root-sources-2679.test.ts statically enforces the binding split |
| O-009 | SUPPORTED |
tests/unit/index-bootstrap-late-writer-2679.test.ts C5/C6 cover both races |
🚨 Confirmed Findings
[HIGH] resolveProjectRootDecision can throw through resolveMcpRoot and initializeOpenCodeSwarm
- Locations:
src/cli/mcp.ts:113,src/index.ts:1136 - Why it matters: If
realpathSyncthrows a non-ENOENT exception (ENOMEM, EIO, EBUSY, symlink cycle with loop detection failure), the exception propagates uncaught. InresolveMcpRootthis aborts the CLI with a raw stack trace instead of a user-friendly{ error }return. IninitializeOpenCodeSwarmthis crashes bootstrap entirely. - Evidence:
resolveProjectRootDecisionatsrc/utils/project-boundary.ts:298catches only ENOENT/ENOTDIR:try { resolved = dependencies.realpathSync(directory); } catch { return { kind: 'fail-closed', ... }; } // all other codes propagate
resolveMcpRoot(src/cli/mcp.ts:113) calls it without a try-catch wrapper.initializeOpenCodeSwarm(src/index.ts:1136) likewise calls it bare. - Fix direction: Wrap the
resolveProjectRootDecision(resolved)call inresolveMcpRootwithtry { decision = ... } catch { return { error: ... } }and the call ininitializeOpenCodeSwarmwithtry { rootDecision = ... } catch { rootDecision = { kind: 'fail-closed', directory: ctx.directory, reason: 'internal error' } }. Alternatively, makeresolveProjectRootDecisioncatch allrealpathSyncerrors internally and returnfail-closed.
[LOW] biome-ignore suppression at src/cli/mcp.ts:124 has no effect
- Location:
src/cli/mcp.ts:124 - Static analysis ground truth: "Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule."
- Evidence: Authoritative biome result; the
biome-ignore lint/suspicious/noConsolecomment on theconsole.logcall has no biome rule to suppress at that line. - Fix direction: Remove the suppression comment.
🔬 Unverified but Plausible Risks
- Risk:
writeBootstrapRootRedirectRecorduses blockingmkdirSync+writeFileSyncon the init path. On a cold disk or highly contended filesystem this is unbounded blocking I/O before parallelization begins.- Why suspicious:
src/index.ts:1268calls it afterconfigLoadP(sequential chain point). Path exists within.swarm/advisories/which is typically pre-warmed. - What would verify it: Profile a cold-boot on a spinner/remote NFS home. The record is best-effort advisory; if the write blocks, the whole init blocks.
- Why suspicious:
- Risk: The
redirectedFromfield inresolveMcpRoot's success shape may be silently dropped by callers using explicit destructuring (const { root } = result) rather than'root' in resultguards.- Why suspicious: Not observed in the diff (no caller changes), but is a real API surface change documented in the PR.
- What would verify it: Audit every caller of
resolveMcpRoot.
🧪 Test / Coverage Gaps
- Gap: No test exercises
resolveMcpRootwhenresolveProjectRootDecisionthrows (ENOMEM/EIO mid-walk). The existing MCP tests use only valid filesystem fixtures.- Evidence:
tests/unit/mcp/offline-wiring-2499.test.ts— allresolveMcpRoottests usecanonicalMkdtemp(valid dirs) or nonexistent paths that hit the pre-decision guards.
- Evidence:
📋 Shipped-vs-Claimed Gaps
None found — the PR delivers all stated obligations.
🔁 Validation Provenance
| Finding | Outcome | Reason |
|---|---|---|
[biome suppressions/unused] at src/cli/mcp.ts:124 |
KEEP (LOW) | Authoritative biome ground truth; suppression has no effect |
resolveProjectRootDecision() not wrapped in try-catch in resolveMcpRoot |
KEEP (HIGH) | Non-ENOENT realpathSync errors propagate uncaught; realpathSync CAN throw EACCES, EIO, ENOMEM,ELOOP |
writeBootstrapRootRedirectRecord uses sync I/O on init path |
DROP | Advisory-only, best-effort by design, path within .swarm/advisories/ which is pre-warmed after redirect; blocking window is bounded |
resolveProjectRootDecision not wrapped in try-catch in initializeOpenCodeSwarm |
KEEP (HIGH) | Same structural gap as resolveMcpRoot; a filesystem exception mid-walk aborts bootstrap |
Short-circuit condition bootstrapStateWritesEnabled && hasSwarmState(bootstrapRoot) order risk |
DROP | Intentional left-associative ordering; comment in diff documents the design |
writeBootstrapRootRedirectRecord called before config loaded — can't be quiet-gated |
DROP | Intentionally unconditional per PR design; the release fragment explicitly documents this |
resolveMcpRoot return type change may silently drop redirectedFrom |
DROP | Pre-existing API design concern; TypeScript users get a type warning; not a PR-introduced defect |
Console.warn/log includes ctx.directory (user-supplied) |
DROP | Plugin context directory is host-supplied trust root; terminal ANSI injection requires active attacker with filesystem write to supply the path; not a PR change |
console.log reassignment without try/finally in MCP tests |
DROP | Pre-existing test infrastructure issue; not introduced by PR |
Hardcoded Bun.sleep(4000) / (2000) / SETTLE_MS=3500 |
DROP | Pre-existing pattern used throughout the suite; bounded drains, not indefinite waits |
afterEach best-effort cleanup silently swallowing errors |
DROP | Intentional "best-effort" per test design; no evidence of actual pollution |
writeBootstrapRootRedirectRecord unconditional advisory write |
DROP | By-design; release fragment discloses and documents the operator implications |
Blind-spot finding added: resolveProjectRootDecision's realpathSync catch is not exhaustive — ENOMEM, EIO, ELOOP (on some platforms), and EACCES from realpathSync are not caught, propagating as unhandled exceptions. This is the same root cause as the two HIGH findings above.
Merge Recommendation
BLOCK — one HIGH finding (unhandled exception propagation through resolveMcpRoot + initializeOpenCodeSwarm) requires a fix. The biome suppression is trivial to remove in the same commit.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | resolveProjectRootDecision can throw; callers do not catch) |
| Input validation present | resolveMcpRoot does not catch non-ENOENT from realpathSync) |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Closes #2679
Summary
Plugin bootstrap and
mcp serve --dirnow apply the repository's project-boundary policy ONCE, synchronously, before any init-path or first-write consumer touches a directory.resolveProjectRootDecision(src/utils/project-boundary.ts) is a decision form ofassertProjectRoot's ancestor walk sharing the same implementation, so the two cannot drift:.swarm/state (.swarm/dir + project indicator in an ancestor): the boot resolves to the owning root. All project-surface consumers —.swarmstate, snapshot rehydration, project config under.opencode/, telemetry, observability lineage, bundled-skill sync, agent overrides, knowledge/curation hooks, teardown — use the resolved root. One unconditional bounded stderr hint names the owning root (plus a/swarm diagnoseadvisory and a durable.swarm/advisories/bootstrap-root-redirect.jsonrecord under the owning root). The manifest stays fail-open (agents/tools still register)..gitfile/dir, linked worktrees, submodules,.opencode/dirs) and standalone roots stay independent — behavior unchanged. An indicator-only ancestor without.swarm/does not capture the boot.mcp serve --dirapplies the same decision: redirect to the owning root with a stdout line naming the served root, or fail-closed rejection.Before this change, opening an ordinary subdirectory while its parent project owned
.swarm/state silently created a complete second runtime-state tree under the child (advisories, automation status, bundled skills, telemetry, DB surfaces) — reproduced live on the pre-fix tree and pinned by frozen acceptance checks.Invariant audit
.swarm-bearing ancestors, no subprocess. Measuredscripts/repro-704.mjson the fixed tree: T1 71.4ms (deadline 400ms), T2 112.4ms, T3 33.0ms — all PASS; direct A/B boot probe base 200ms vs fixed 201ms.bun run buildOK;node --input-type=module -e "await import('./dist/index.js')"OK; bundle-portability + bundle-plugin-shape tests 12/12 pass.git -C <child>never spawns for redirected boots).docs/engineering-invariants.md("Bootstrap project-root ownership", invariant 4) with the ordinary-child/nested/worktree/.opencode/standalone rules,ctx.directory/--direxamples, fail-closed conditions, and the explicit distinction from [Workstream D] PR 14 of 17: Make live state and hydration project-owned and generation-fenced #2667's process-global hydration eviction.assertProjectRootsemantics unchanged, message text byte-identical).canonicalMkdtemp, nomock.module, no raw clock); check:mock-cleanup / check:test-file-cap / check:test-tmpdir / check:test-clock all clean of new violations.canonicalProjectKey/hydrationProjectKeyof the owning root; at most one extradirectoryKeyMemoentry per redirected boot, withinMAX_DIRECTORY_KEY_MEMO = 64).guardrails.enabled === falsesecurity warning continues to fire against the APPLIED configuration (the parent's for a redirected boot; the always-visible redirect hint names the owning root so the attribution is traceable — pinned by test, disclosed in the release fragment).console.warn), the same surface as the unconditional startup version line (Bug Report: gitExec fails with ENOENT (posix_spawn 'git') on macOS despite /usr/bin/git existing #2236 precedent), deliberately NOT quiet-gated becausequietdefaults true and would suppress a containment signal;/swarm diagnoseadvisory entry included.tests/unit/index-commands.test.tsgreen.docs/releases/pending/2679-project-root-ownership-bootstrap.md) with an operator-action section (pre-existing child trees are NOT migrated/deleted; redirected boots inherit the parent's project-level config flags); release-owned version files untouched.Acceptance Criteria -> Evidence
.swarmstate; actionable parent-root hint; bounded manifest.opencode, worktree, standalone-root controls remain independent.opencodedir), standalone root, and the indicator-only-parent edge intests/unit/index-bootstrap-root-ownership-2679.test.ts(9/9).tests/unit/index-bootstrap-root-ownership-2679.test.ts(realserver()boots) andtests/unit/index-bootstrap-late-writer-2679.test.ts(concurrent double-boot race + registeredtool.execute.aftersnapshot writer — both assert no child tree and parent-attributed state). Frozen C5 (race) and C6 (late writer) RED at base → GREEN at head, independently re-run by reviewer and critic.Test plan
repro-check.sh run(base 9ba5b41 → head): 4 DISCRIMINATING RED→GREEN, 4 PRESERVING GREEN→GREEN; checkpoint manifest verified byte-identical (verify-checkpointOK).project-boundary-resolver-2679(9),index-bootstrap-root-ownership-2679(9, real boots),index-bootstrap-late-writer-2679(2),index-bootstrap-root-sources-2679(4, static source-scan guardrail — fails 3/4 on the pre-fix tree), extendedmcp/offline-wiring-2499(+3) — 37/37 pass.check:events(catalog producer citations repointed after the src/index.ts line shifts — commit d52cf69),check:registry-citations,atomic-write-ratchetall green.bun run typecheckclean;biome ci src tests0 new findings;check:mock-cleanup,check:invariants,check:test-file-cap,check:test-tmpdir,check:test-clockclean of new violations;scan-deferredclean; build + Node-ESM import +repro-704as above.Known environment note (not a regression): under harnesses that redirect
HOME/USERPROFILE,os.homedir()no longer matches the on-disk home, so the weak-container exemption cannot recognize it and a real home owning.swarm/+.opencode/may claim temp workspaces — identical to the pre-existingassertProjectRootsemantics for tool writes; cannot occur on clean CI runners; all repro-704 deadlines still pass.Risk and merge status
History note: after the original publication (head 52588e1) the branch was rebased onto advanced main (PR #2797) because GitHub dropped the push events for two consecutive heads; the final-critic verified the rebase is content-faithful to the approved change (zero semantic deltas on any #2679 surface) and re-approved at this head.
Risk is medium: init-path change with containment semantics for redirected boots only — normal boots (markers/standalone) are byte-identical (
bootstrapRoot === ctx.directory), pinned GREEN-to-GREEN. Cross-model gates: plan critic (3 rounds, APPROVE), independent implementation reviewer (APPROVE; its one Important finding fixed in-commit and delta re-approved), final critic (APPROVE; independently re-ran the frozen checks, guardrail, and suites).PR head: 9ec3b60
Merging stays human-gated; no merge is performed by this workflow.
Waivers (or none)
None.