diff --git a/docs/superpowers/specs/2026-08-04-ai1703-vendor-config-discovery-scope-design.md b/docs/superpowers/specs/2026-08-04-ai1703-vendor-config-discovery-scope-design.md new file mode 100644 index 000000000..f613f0ff8 --- /dev/null +++ b/docs/superpowers/specs/2026-08-04-ai1703-vendor-config-discovery-scope-design.md @@ -0,0 +1,509 @@ +# Vendor MCP config exclusion must follow vendor discovery, not the repository root + +Design for AI-1703. Builds on the merged AI-1632 containment (kcap-cli#427) and the merged +AI-1706 review-context server (kcap-cli#443). + +Revision 5, after four rounds of spec review. Revision 2 moved the cwd prefix from the filesystem to +git's own bytes. Revision 3 withdrew an unconditional case fold that was a launch-refusal primitive, +pinned the `--show-prefix` byte protocol, and persisted the prefix across a refresh. Revision 4 +closes the last structural hole: **the launch path and the classifier are now derived from the same +prefix**, so they cannot diverge. + +## The defect + +`WorktreeManager.WorkspaceMcpConfigPaths` is a list of **root-relative** paths, and every consumer +matches it against a path relative to the repository root: the borrowed-snapshot manifest filter +(`IsUnderExcluded`), the owned-worktree strip (`NeutralizeWorkspaceMcpConfig`), the `skip-worktree` +marking (`ApplyReservedIndexPolicyAsync`), and the AI-1706 reserved-path classifier +(`ClassifyReservedPath`). + +A borrowed snapshot can execute in a directory **below** the repository root. +`CreateBorrowedSnapshotAsync(sourceRepoRoot, requestedCwd, …)` takes the two independently, and +`AgentOrchestrator` passes `snapshotGitRoot` and `borrowedSnapshotSource` — the git root and the +user's actual cwd. The returned `WorktreeInfo.Path` is the *execution path*, not the snapshot root. +So a review flow started from `/src` produces a snapshot whose cwd is `/src`, and +`src/.codex/config.toml` is matched against a list containing only `.codex/config.toml`. It is not +excluded, and it lands in the tree the reviewer executes in. + +Separately and independently of scope, `.github/mcp.json` is **not on the list at all** — the list has +`.github/copilot/mcp.json`, a different path. That gap is live today at the repository root of every +borrowed snapshot. + +## Vendor discovery matrix + +| Path | Vendor | Documented discovery | Within ancestor chain? | +|---|---|---|---| +| `.mcp.json` | Claude Code | Searches **upward** through parents, merging; continues above the git root | yes, plus see "above the snapshot root" | +| `.mcp.json`, `.github/mcp.json` | Copilot CLI | Walks cwd → repository root; `.mcp.json` wins in the same directory | yes | +| `.codex/config.toml` | Codex | Layers repository root → cwd, closest wins, trusted projects only | yes | +| `.cursor/mcp.json` | Cursor | Project root only; no documented nested discovery | yes (subset) | +| `.gemini/settings.json` | Gemini | Workspace root; measured root-scoped during AI-1632 | yes (subset) | +| `.kiro/settings/mcp.json` | Kiro | Workspace root; **measured** to spawn at session setup | yes (subset) | +| `.vscode/mcp.json` | editor-generic | Workspace-folder root; GitHub documents Copilot CLI does **not** read it; kept for VS Code and others | yes (subset) | +| `.copilot/mcp.json`, `.copilot/mcp-config.json` | Copilot | GitHub documents `~/.copilot/mcp-config.json` as **user**-scope; no documented workspace form | speculative, kept under the list's standing "wider than known readers" rationale | + +No supported vendor is documented to search **downward** into descendants, or into a sibling of the +cwd. + +### Above the snapshot root + +Claude Code's upward walk does not stop at the git root, so the physical ancestors of the snapshot +matter. Those are `…/borrowed-snapshots/`, then `config.WorktreeRoot`, then whatever contains it — +daemon- and user-owned, holding no branch content. + +The property that keeps it that way is `EnsureSeparateRoots(source, root)`, which already refuses a +snapshot root at or under the source checkout with `borrowed_snapshot_root_inside_source`. Round 2 +raised this as a Critical bypass; it does not reproduce, because that guard exists. + +It has one real residual, which round 2 named: the guard is a **lexical** prefix comparison over +`Path.GetFullPath` output, which does not resolve symlinks. A `WorktreeRoot` configured as a symlink +whose target is inside the source checkout passes the string comparison and lands the snapshot under +the source anyway — at which point the source's own root `.mcp.json` is a physical ancestor of the +reviewer's cwd and is loaded by an upward-walking vendor. + +The guard is extended to compare **resolved** paths as well as lexical ones, same coded error. Two +details round 3 was right to demand: + +- **Resolving a path that does not exist yet.** `borrowed-snapshots` is created by this very call, so + "fully resolve the final path" is undefined. The guard walks from the configured root toward the + leaf, resolves the **deepest existing ancestor**, and appends the remaining components literally + without re-resolving — so a component substituted later cannot be followed, and a nonexistent leaf + does not defeat the check. +- **What it does not close.** Resolution handles symlinks and Windows junctions. It does **not** + close a Unix bind mount of a source subdirectory at an apparently external path, nor Windows SUBST + or 8.3 short-name aliases unless the chosen final-path API canonicalises them. Those remain a + **trusted-configuration residual**: `WorktreeRoot` is daemon operator configuration, so reaching + them requires an already-compromised host config rather than branch content, and this file's own + class documentation elsewhere already records the same alias classes defeating a different + path-identity check. This is stated as a residual, not claimed as closure, and test 16 is scoped to + the symlink class only — it must not be described as proving the broader invariant. + +### Threat-model boundary, stated rather than assumed + +The property delivered is: **no branch-authored vendor config is loaded by the vendor process the +daemon launches, at the cwd it launches it in, without model involvement.** That is the AI-1632 +property — Kiro was measured spawning a declared command at session setup, no prompt, no tool call. + +It does not cover a model that deliberately changes directory and launches another supported CLI in a +descendant or sibling. That is not a config-discovery hole: a model that can spawn a CLI is already +executing arbitrary commands, and the boundary there is the reviewer's OS sandbox (the AI-1584 +profile), not this path list. + +### Alternative considered and rejected: exclude at every directory + +Whole-tree exclusion would also cover the nested-launch case, and — because the AI-1706 +review-context server surfaces the content either way — at no review-coverage cost. + +**Revision 2 rejected it for a reason that was false**, and round 2 was right to say so. The claim +was that whole-tree exclusion would newly expose the fail-closed `MaxReviewContextBytes` cap. It +would not: one tracked ancestor `.mcp.json` over 256 KiB already trips +`borrowed_snapshot_review_context_capacity_exceeded` today, and the ancestor chain itself admits +`paths × depth` entries rather than "at most 8". Whole-tree exclusion increases the number of trigger +locations for an existing DoS; it does not introduce the class. + +The rejection stands on the two reasons that survive scrutiny: + +1. **Scope.** Given the explicitly scoped property above — the launched vendor, at its launch cwd, + without model involvement — the ancestor chain is exactly the closure. Whole-tree buys coverage + only for the nested-launch case, which is deliberately the sandbox's problem. +2. **Functional regression.** It deletes sibling configs that no vendor in the launch can discover, + including this repository's own committed `kcap/.mcp.json`. + +The pre-existing review-context capacity DoS is real, is not introduced or widened here, and is +**filed separately** rather than folded into this change. Containment must not be gated on +review-context capacity: the right shape is to contain everything and emit a bounded manifest that +declares what it omitted, which is a change to AI-1706's contract, not to this exclusion rule. + +## Correction to the issue's premise + +AI-1703 states that "AI-1632's owned-worktree neutralization walks real ancestor directories and does +cover this". **That is wrong.** `NeutralizeWorkspaceMcpConfig` walks the components of each *relative +path* (`.kiro`, then `settings`, then `mcp.json`) so it can unlink the first component that is a +symlink. It does not walk directories of the tree. It is exactly as root-scoped as the borrowed path. + +It is nonetheless not a live defect, for an unrelated reason: every owned-worktree launch runs at the +worktree root. `CreateAsync` and `BuildStandaloneSnapshotAsync` both return a `WorktreeInfo` whose +`Path` is the worktree root, and `AgentOrchestrator` uses `worktree.Path` directly as the launch cwd +without narrowing. With cwd == root the chain is `[root]` and root-scoped matching is complete. + +The direct-borrow path (`WorktreeInfo.Borrowed(cwd)`, non-snapshot) is out of scope by construction: +it is the user's own checkout behind a certified read-only runtime boundary, and nothing is stripped +there today. + +## Design + +### Deriving the prefix: byte-exact protocol + +The prefix comes from git, not the filesystem, so it lands in the same namespace as `ls-files` and +the separator, rooted-path and `..` classes disappear by construction: + +``` +git -c core.quotePath=false rev-parse --show-prefix +``` + +run in the source repository with the process cwd set to the requested cwd, captured as raw bytes. +`core.quotePath=false` is required or non-ASCII components come back C-quoted. + +Round 2 is right that "then pass it through `NormalizeRelativePath`" is not a specification — that +function rejects LF, an empty string, and a trailing empty component, all three of which this output +has. The parse is therefore pinned: + +1. The capture must end with exactly one `0x0A`. Zero, more than one, or any `0x0D` anywhere is + rejected (`borrowed_snapshot_cwd_prefix_malformed`). Strip that one byte. +2. If what remains is empty, the cwd **is** the repository root: the chain is `[""]` and no further + parsing happens. This is the common case and it must not go through the path validator at all. +3. Otherwise the remainder must end with exactly one `/`. Strip it. +4. Strict-UTF-8 decode, then `NormalizeRelativePath`, which now sees a well-formed relative path and + applies the existing `\`, CR, LF, `.`/`..`, `.git` and NFC rules. +5. If the destination is case-**insensitive** and the prefix is not ASCII, the launch is refused with + `borrowed_snapshot_cwd_prefix_non_ascii` — see "Case", below. A non-ASCII prefix on a + case-sensitive destination is admitted and compared byte-exactly. + +The capture itself is bounded at **4 KiB** before decoding, rejected with the same +`borrowed_snapshot_cwd_prefix_malformed`. The later depth and aggregate-byte caps would eventually +reject an absurd prefix, but only after the capture helper has already allocated it; bounding at the +read is the cheaper and more honest place. + +### One prefix for the classifier and the launch + +Round 3's critical finding: revision 3 still located the execution directory with the +*filesystem*-derived `relativeCwd` while classifying with the *git*-derived prefix, and two +independently derived spellings can disagree. The concrete bypass: + +- the source working directory is physically `src`, so `rev-parse --show-prefix` returns `src/`; +- the caller-derived relative cwd is `SRC`; +- the source index contains `SRC/.mcp.json` — reachable when a branch authored on a case-sensitive + system is checked out on a case-insensitive one. + +The plan then excludes `src/.mcp.json` and not `SRC/.mcp.json`; copying the latter *creates* +`final/SRC`, so `Directory.Exists(executionPath)` succeeds, and the vendor launches in a directory +whose config was never excluded. Revision 3's appeal to `borrowed_snapshot_cwd_missing` firing was +therefore unsound — a benign tracked `SRC/keep` produces the same directory-creating side effect +without any config at all. + +The fix is to remove the second derivation rather than to reconcile the two. **The execution path is +`ContainedPath(final, GitRelativeCwd)`**, derived from the same prefix the classifier uses. This is +also simply more correct: the snapshot materialises every file at its *git* path, so the directories +that exist in the snapshot carry git's spelling, and the git prefix is the only spelling guaranteed +to name one of them. + +The filesystem-derived `relativeCwd` survives only as an authorization-time containment check on the +*requested* cwd — that it is not `..`, not `../…`, and (round 1's finding, real and independent of +this feature) not rooted, which `Path.GetRelativePath` can return across Windows volumes. It never +locates a directory again. + +A source index that genuinely carries both `src/…` and `SRC/…` still fails closed on a +case-insensitive destination: `ReadSourceManifestAsync` builds its dictionary with +`OrdinalIgnoreCase` there and throws `borrowed_snapshot_path_collision`. That is pre-existing +behaviour, not something this design adds. + +### Case: use the probed volume, not an unconditional fold + +Revision 2 folded ASCII case on the directory prefix unconditionally. Round 2 showed that is a +**launch-refusal primitive**: on a case-sensitive destination, tracked `a/.mcp.json` and +`A/.mcp.json` both fold to one canonical candidate, `matchedCanonicalPaths.Add` fails, and +`borrowed_snapshot_review_context_path_collision` refuses every launch of that repository. It also +excluded a sibling no vendor in the launch can discover, contradicting this design's own sibling rule. +Both objections are correct and the unconditional fold is withdrawn. + +The comparison instead uses the **existing probed `caseSensitive`**, and the reason it is the right +input is that it is probed on the destination — the volume the vendor actually executes on: + +- **Case-insensitive destination.** `SRC` and `src` are the same directory there, so folding is + correct and no case-varying sibling can exist to collide. This is the case that matters, because it + is where a `--show-prefix` spelling taken from the on-disk cwd can differ from the index spelling + and silently fail an exact match. +- **Case-sensitive destination.** `a` and `A` are genuinely distinct, so not folding is correct: the + sibling rule holds and the collision primitive does not exist. + +The cross-volume divergence round 1 raised is no longer a matcher question at all: with one prefix +feeding both the classifier and the launch, there is no second spelling to disagree with. + +**Non-ASCII prefixes, narrowly.** `AsciiPathEquals` folds ASCII only, while a case-insensitive volume +also equates pairs such as `Å`/`å` — a genuine under-exclusion. Revision 3 refused every non-ASCII +prefix outright. Round 3 is right that this is broader than the matcher requires and that the +justification ("the prefix is the operator's cwd, not branch content") was wrong: the *directory +name* is branch-authored, so an internationalised — or deliberately hostile — repository could make +every sub-cwd launch in that part of the tree fail. + +The rule is therefore narrowed to exactly where equivalence cannot be proved: + +- **Case-sensitive destination.** A non-ASCII prefix is admitted and compared byte-exactly. Both + sides are NFC (the index side by `NormalizeRelativePath`, the prefix side by the same call), so + exact comparison is sound and no folding is involved. +- **Case-insensitive destination.** ASCII prefixes fold correctly; a non-ASCII prefix is refused with + `borrowed_snapshot_cwd_prefix_non_ascii`, because proving equivalence would require a second, + Unicode-aware matcher — which is the two-matcher defect this design exists to remove. + +This is a user-visible compatibility limitation on one platform class, stated as such rather than +dressed up as a security property, and the error is coded so the daemon can surface something +actionable. All canonical suffixes are ASCII, so nothing else is affected. + +### Sequencing: where the prefix is captured and where the plan is built + +Step 5 makes prefix acceptance depend on `caseSensitive`, which is probed inside +`BuildIndependentSnapshotOnceAsync` **after** the destination checkout. Revision 3 also described the +caller building the plan and passing it to the builder. Round 4 is right that both cannot hold, and +that this is a security-relevant contract rather than a code detail — so it is pinned: + +1. **At creation**, before any snapshot exists: capture `--show-prefix` and apply parse steps 0–4 + (bound, framing, root special case, decode, `NormalizeRelativePath`). This yields + `GitRelativeCwd`, and it is what gets persisted on `WorktreeInfo`. +2. **Inside each build attempt**, after the destination checkout and its `ProbeCaseSensitiveFileSystem` + call: apply step 5 (the destination-dependent non-ASCII rule) and construct the plan **exactly + once**, before review-context extraction and before manifest filtering, so both consumers read one + plan classified under one probe result. + +The probe must be taken on the **actual destination**, never on its parent or on the configured +worktree root as a stand-in: case behaviour can differ per directory on supported platforms, and a +substituted probe would silently classify under the wrong semantics. + +`BuildIndependentSnapshotAsync` retries once on `SourceChangedException` against a freshly created +destination, so the probe and the plan are per-attempt by construction — a retry must not reuse the +previous attempt's plan. + +### One classifier, not two + +`IsUnderExcluded` compares decoded strings with `StringComparison.OrdinalIgnoreCase` (full Unicode +folding); `ClassifyReservedPath` compares raw bytes with `AsciiPathEquals` (ASCII only). Invisible +with an ASCII-only canonical list; observable the moment a prefix can vary. So the vendor list flows +through `ClassifyReservedPath` **only**: + +```csharp +internal sealed record SnapshotExclusionPlan( + string GitRelativeCwd, // "" for the repository root + ImmutableArray VendorConfigPaths, // canonical list × the ancestor chain + ReadOnlyMemory[] VendorConfigPathBytes, // the same set, as the classifier consumes it + string[] SnapshotExclusions); // .capacitor, .attached, caller extras only +``` + +`ReadSourceManifestAsync` calls `ClassifyReservedPath` on the raw bytes before decoding — preserving +the existing classify-before-decode guarantee — and treats `Exact` and `Descendant` as excluded. +`IsUnderExcluded` is retained only for `.capacitor`, `.attached` and caller-supplied `excludePaths`, +which are ASCII daemon-supplied constants where the namespace question does not arise. The vendor +paths are no longer in `SnapshotExclusions` at all, which is what makes "one classifier" true rather +than asserted. + +`SnapshotExcludedPaths` is **removed**, not left beside the plan — leaving it is the two-lists shape +whose comment in the code today reads "Two lists of the same thing is how that happened". + +Round 2 is right that `ImmutableArray` is not deeply immutable and that a `string[]` is not +immutable at all. The plan is a value the daemon constructs and never publishes; the byte set is +stored as `ReadOnlyMemory` and the plan is documented as *not* being a security boundary in +itself — the guarantee is that it is built once per build and passed, not mutated. Stating that is +better than claiming an immutability the type system does not give. + +### Refresh: persist the prefix, never re-derive it + +Round 2's second finding is a genuine hole in revision 2. `SyncFromSourceCoreAsync` has only +`sourceRepoRoot`, a target root and an `executionPath` **inside the target**. Deriving a source-relative +prefix from the target filesystem would reintroduce exactly the namespace problem the git derivation +exists to remove. + +So the git prefix is computed **once**, at `CreateBorrowedSnapshotAsync`, and persisted on +`WorktreeInfo` (`GitRelativeCwd`, alongside `SnapshotRoot` and `ReviewContextRoot`). The refresh path +takes it as a parameter and never recomputes it. A refresh that is not given one is a programming +error and throws; it does not silently fall back to a filesystem derivation. + +Round 3 asked for the lifecycle acceptance criteria, which is a fair demand on an in-memory field. +The evidence that in-memory is sufficient: the only borrowed-refresh caller is +`AgentOrchestrator.TryRefreshBorrowedSnapshotAsync`, which reads `agent.Worktree` off the in-memory +`AgentInstance` created at launch and passes `agent.Worktree.SnapshotRoot`, `.Path` and +`.ReviewContextRoot` from that same object. `AgentInstance` does not survive a daemon restart — a +restarted daemon reports zero live agents, which is why `OrphanedHostedAgentReaper` exists — so there +is no reconstruction path that could arrive without the field, and none that could be tempted back +into recomputation. That refresh also already re-authorizes and requires +`SameFileSystemPath(auth.CanonicalCwd, source)`, so the source identity cannot drift underneath the +persisted prefix. + +Should a durable agent registry ever be added, `GitRelativeCwd` must be part of what it persists; +the throw is what makes that failure loud rather than silent. + +### Reserved index policy + +Revision 2 intersected against `initialIndex`, read from **source**, while `update-index` runs in +**destination** — a fresh clone checked out at `HEAD`. A staged-but-uncommitted `src/.mcp.json` is in +the source index and not in the destination index, so revision 2 would have batched it and, having +promoted the failure to hard, refused a legitimate launch. + +Corrected: read the **destination** index after checkout (`git -C destination ls-files -z`), intersect +the plan's vendor paths with that, mark only paths proven present, and let a failure propagate. The +end-of-build source consistency re-check is unchanged. + +Bounds, all of which revision 2 got wrong or left open: + +- The batch is fed on stdin — `git update-index --skip-worktree -z --stdin` — removing the `ARG_MAX` + ceiling and pathspec interpretation of a leading `:` or `-` in an ancestor name. +- Aggregate pathname bytes are `O(depth²)`. Concrete caps, enforced at plan construction and rejected + with `borrowed_snapshot_cwd_too_deep`: **depth ≤ 32** ancestor components, and **aggregate vendor + path bytes ≤ 64 KiB**. With a 10-entry canonical list that is at most 330 candidates. +- `MaxReviewContextBytes` charges only blob content; the serialized manifest also carries path + strings, base64 expansion and JSON framing. A separate **1 MiB** cap on the serialized manifest is + enforced on write *and* checked before parsing on read, so an oversized manifest is rejected before + allocation rather than after. Revision 2's claim that the content cap was "the real bound" was + wrong. + +### Review-context validation + +`ValidateReviewContextManifest` capped `Entries.Length` against the static canonical list. With an +expanded per-build set, the validator needs the same set the extractor used. + +Round 4 caught the trap in the obvious implementation. A review-context entry preserves the **actual +git path**, while `VendorConfigPaths` holds **canonical expanded spellings**. On a case-insensitive +destination a tracked `SRC/.MCP.JSON` is legitimately classified against canonical `src/.mcp.json` — +so exact membership against the canonical set would reject a valid entry, and relaxing it to an +`OrdinalIgnoreCase` set would reintroduce a second matcher, which is the defect this design exists to +remove. + +So the generation persists the **exact actual paths `ClassifyReservedPath` matched**, not the +canonical set, and the validator checks exact membership against those. No folding happens at +validation time, and there is no second matcher: the only case decision was made once, by the +classifier, at extraction. This is strictly stronger than the old length cap, which bounded only the +count. + +Test 20 exercises the round trip that none of the other tests reached: a case-varied tracked config on +a case-insensitive destination, surviving generation serialization and read-time validation. + +### Reviewability, narrowed rather than claimed + +Containment ranges over the working tree (`ls-files -co --exclude-standard`); review context contains +**index stage-0 blobs only**, and the manifest declares it (`UnstagedAndUntrackedOmitted: true`). An +untracked reserved config, or unstaged bytes of a tracked one, is therefore contained but not +reviewable. + +That is pre-existing AI-1706 behaviour and is deliberately **not** widened. Carrying untracked +working-tree bytes into review context is what killed the predecessor effort (AI-1680): a developer's +`skip-worktree` local override would be published to the reviewer's model. The lockstep property this +design claims is correspondingly precise: **for tracked stage-0 content, every path excluded from the +snapshot is classified reserved by the extractor.** + +### The non-borrowed sync path + +`SyncFromSourceAsync` (the public overloads, `reviewContextRoot: null`) takes an `executionPath` and +previously received `SnapshotExcludedPaths`. Removing that static property must not silently drop even +its root-level exclusions. + +It has **no production callers today** — the only call sites are in +`test/Capacitor.Cli.Tests.Unit/WorktreeManagerTests.cs` and +`test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryLiveTests.cs`. That is a caller +invariant, not a guarantee: it is a public method that produces a tree an agent could be launched +into. + +Round 4 caught a contradiction in revision 3: this path was kept in scope for consumers 1 and 2, while +the filesystem-relative derivation it would have had to use was banned everywhere else. Left as +written, the only way to implement it was to recreate exactly the derivation the round-3 Critical +removed. + +So the API changes rather than inferring. The overloads that take only a target-side execution path +are **removed** — with no production callers, deleting them is cheaper and safer than preserving an +unsafe inference — and replaced by one that requires a **source-side cwd**: + +```csharp +public async Task SyncFromSourceAsync( + string sourceRepoRoot, string sourceCwd, string targetWorktreePath, + string[] excludePaths, CancellationToken ct); +``` + +`sourceCwd` goes through the same `rev-parse --show-prefix` derivation as +`CreateBorrowedSnapshotAsync`, and the one resulting prefix drives both the plan and the target +execution path — the same single-prefix rule as the borrowed path. A caller that cannot supply a +source cwd cannot use this method; there is no fallback. + +Consumers 1 and 2 apply. Only consumer 3 (review context) is absent, because this overload passes no +review-context root. + +### The `.github/mcp.json` addition + +Added to `WorkspaceMcpConfigPaths`, independent of the scope change and live even at cwd == root. +`.copilot/mcp-config.json` is added alongside the existing `.copilot/mcp.json` under the list's +standing rationale. + +## Testing + +Every test names the discovery shape it defends and carries a positive control. Rounds 1-3 each +found tests that could pass vacuously; those are rewritten rather than patched. + +1. **Codex sub-cwd** — tracked `src/.codex/config.toml`, `requestedCwd = /src`, absent from the + snapshot. Control: root-cwd build of the same fixture leaves it present. +2. **Copilot `.github/mcp.json` at root** — the file is **tracked**, asserted present in `ls-files -co` + before asserting it is absent from the snapshot. +3. **Intermediate directory** — cwd `a/b`, tracked `a/.mcp.json` excluded. Control: root-cwd build of + the same fixture leaves it present. +4. **Sibling not excluded** — cwd `a`, `b/.mcp.json` present. +5. **Case-sensitive sibling and collision** — on a case-sensitive volume, cwd `a`, with **both** + `a/.mcp.json` and `A/.mcp.json` tracked: the former is excluded, the latter is **present**, and the + build **succeeds**. Tracking only `A` would not exercise the collision, which is the failure + revision 2's withdrawn fold produced (`borrowed_snapshot_review_context_path_collision`). +6. **Cross-volume alternate prefix** — the round-3 bypass fixture, and the reason the two derivations + were collapsed into one: a source whose on-disk directory is `src` (so `--show-prefix` yields + `src/`), a requested cwd spelled `SRC`, and a source index carrying `SRC/.mcp.json`. Assert the + launch does not end up in a directory whose config survived — with a variant carrying a benign + tracked `SRC/keep` instead of a config, since that reproduces the directory-creating side effect + revision 3 wrongly relied on failing. +7. **`--show-prefix` protocol, against real git** — invoke the actual command from a real + subdirectory and compare the parsed bytes against the directory prefixes in a real `ls-files` + listing, on macOS, with an alternate-case entry. The non-ASCII cases assert the error the **pinned + parse order** actually produces, which round 4 caught revision 3 getting wrong: normalization + (step 4) runs before the destination-dependent rule (step 5), so an **NFD** non-ASCII prefix fails + with `borrowed_snapshot_invalid_path`, and only an **NFC** non-ASCII prefix on a case-insensitive + destination reaches `borrowed_snapshot_cwd_prefix_non_ascii`. A third case on a case-sensitive + volume asserts an NFC non-ASCII prefix is admitted and compared byte-exactly. The order is kept as + pinned — normalizing before the case-dependent rule is correct, since step 5's reasoning assumes + an NFC operand. +8. **Root prefix** — the empty-prefix path is exercised through the real command output (`"\n"`), not + through a pre-normalized `""`, and yields a plan equal to `WorkspaceMcpConfigPaths` exactly. +9. **NFD is rejected, not matched** — an NFD path in the index fails the build via the existing + normalization rule. Asserted explicitly, because revision 2's phrasing implied NFD and NFC were + treated as equivalent. +10. **End-to-end exclusion oracle** — for a fixture spanning ancestor, sibling, descendant, mixed-case + and non-ASCII-filename paths, the set of paths absent from the snapshot equals a **hard-coded + expected set written out per fixture**. It must not be computed by `ClassifyReservedPath`, + `PlanSnapshotExclusions`, or any helper production shares — round 3 is right that an oracle built + from the code under test is true by construction and would pass against an identically wrong + matcher. Replaces revision 2's test 6, which had exactly that defect. +11. **Refresh parity at the boundary** — initial sub-cwd snapshot; add and modify configs at root, an + intermediate directory and the cwd; `SyncBorrowedSnapshotFromSourceAsync` with the **persisted** + prefix taken off the `WorktreeInfo` the creation returned — driven through + `TryRefreshBorrowedSnapshotAsync`'s own path, not by hand-passing the field, so the test exercises + the real caller. Assert every ancestor config absent, each tracked one present in the newly + published review context, sibling survives, no kcap-created deletion in `git status`. A separate + case asserts a refresh given no persisted prefix throws rather than re-deriving. +12. **Reserved index policy** — HEAD-tracked `src/.mcp.json` under cwd `src`: destination index + contains it, skip-worktree bit set, `git status` clean. Negative control: with the policy disabled + the same fixture reports a deletion. +13. **Staged-only addition** — `src/.mcp.json` in the source index but not committed: the build + **succeeds**, the file is excluded, and it appears in review context. This is the case revision 2 + would have crashed on. +14. **Caps at the boundary** — depth exactly 32 succeeds, 33 fails; aggregate bytes exactly at the + limit succeeds, one over fails; a serialized manifest one byte over 1 MiB is rejected on write and + on read. +15. **Rooted / escaping cwd** — a cwd yielding a rooted `Path.GetRelativePath` result is rejected; + `..` and `../` remain rejected. +16. **Symlinked snapshot root** — `WorktreeRoot` a symlink resolving inside the source checkout is + rejected with `borrowed_snapshot_root_inside_source`, with a control proving the lexical check + alone passes it. Scoped to the symlink class; the bind-mount and volume-alias residuals are + documented above and are **not** claimed to be covered by this test. +17. **Non-borrowed sync** — the **new** `SyncFromSourceAsync(sourceRepoRoot, sourceCwd, …)` overload + with a sub-cwd excludes ancestor configs, proving the static-property removal did not drop its + exclusions. Includes the cross-volume alternate-spelling case from test 6, since round 4 is right + that a same-spelling fixture would pass while missing exactly that problem. +18. **Oversized ancestor config** — one tracked ancestor `.mcp.json` over 256 KiB trips + `borrowed_snapshot_review_context_capacity_exceeded`. This documents the **pre-existing** DoS + covered by the separately-filed issue so a later reader does not mistake it for a regression here. + It is a characterization test, not a passing security property, and is named and commented so. +19. **List membership** — `.github/mcp.json` and `.copilot/mcp-config.json` required. +20. **Reserved-set round trip** — on a case-insensitive destination, a tracked config whose actual git + path varies in case from the canonical spelling (`SRC/.MCP.JSON` against `src/.mcp.json`) is + classified, written into the generation, serialized, and passes read-time validation. This is the + path where an exact-membership check against *canonical* spellings would wrongly reject, and where + a relaxed check would smuggle a second matcher back in. + +The AI-1632 live certification (`KCAP_WORKSPACE_MCP_CERT=1`) is re-run unchanged, with its existing +control asserting the declared command *does* spawn when the guard is removed. + +## Out of scope + +- The owned-worktree strip (cwd is always the worktree root). +- Widening review context to untracked or working-tree bytes. +- The pre-existing review-context capacity DoS — filed separately; test 18 documents it. +- AI-1675 (`CopyDirectory` recursion / symlink dereference). diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs index d520cde39..46765e31c 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs @@ -2359,7 +2359,13 @@ async Task TryRefreshBorrowedSnapshotAsync(AgentInstance agent) { throw new InvalidOperationException($"borrow_auth_failed: {auth.Reason ?? "source_identity_changed"}"); var generation = await _worktreeManager.SyncBorrowedSnapshotFromSourceAsync( agent.Worktree.SourceRepo, agent.Worktree.SnapshotRoot ?? agent.Worktree.Path, - agent.Worktree.Path, [], agent.Worktree.ReviewContextRoot + // The prefix computed at creation, carried — never re-derived. The only path available + // here is the TARGET-side execution path, and deriving from that is what lets the launch + // cwd and the exclusion classifier end up on two different spellings. + agent.Worktree.GitRelativeCwd + ?? throw new InvalidOperationException( + "borrowed_snapshot_git_relative_cwd_missing"), + [], agent.Worktree.ReviewContextRoot ?? throw new InvalidOperationException( "borrowed_snapshot_review_context_missing"), timeout.Token); var reviewerToken = agent.ReviewerBridgeToken diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs new file mode 100644 index 000000000..f0d585f4a --- /dev/null +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs @@ -0,0 +1,331 @@ +using System.Collections.Immutable; +using System.Diagnostics; +using System.Text; + +namespace Capacitor.Cli.Daemon.Services; + +public partial class WorktreeManager { + /// Ceiling on the raw rev-parse --show-prefix capture, applied at the read. + /// The depth and aggregate-byte caps below would eventually reject an absurd prefix, but only + /// after the capture helper had already allocated it. Bounding here is cheaper and does not depend on + /// a later stage noticing. + internal const int MaxCwdPrefixCaptureBytes = 4 * 1024; + + /// Ancestor components admitted between the repository root and the execution cwd. + internal const int MaxCwdDepth = 32; + + /// Ceiling on the summed UTF-8 length of the expanded vendor paths. + /// The expansion is canonical paths × (depth + 1) entries but O(depth²) aggregate + /// bytes, because each deeper level repeats the whole prefix. Capping the count alone would not bound + /// the bytes. + internal const int MaxVendorPathAggregateBytes = 64 * 1024; + + /// + /// The exclusion decision for one snapshot build: which concrete vendor-config paths are reserved, + /// given where the reviewer will actually execute. + /// + /// Why a plan rather than a static list. Vendors resolve workspace MCP config along the + /// ancestor chain of their cwd — Codex layers .codex/config.toml from the repository root down + /// to the cwd, Copilot and Claude Code walk from the cwd upward — so a root-relative list is only + /// complete when the cwd IS the root. A borrowed snapshot may execute below the root, and a root-only + /// list left src/.codex/config.toml live in the tree the reviewer runs in. + /// + /// One list, one classifier. The vendor paths deliberately do NOT appear in + /// : they are matched only through ClassifyReservedPath, which + /// is also what the review-context extractor uses. Two matchers over the same set is how "contained + /// but not reviewable" gets created — they folded case differently (OrdinalIgnoreCase versus + /// ASCII-only), which was unobservable while the list was ASCII constants and stops being so the + /// moment a cwd prefix can vary. + /// + /// Not a security boundary in itself. byte[] and string[] members are not + /// deeply immutable and this type does not pretend otherwise. The guarantee is procedural: exactly one + /// plan is built per build attempt, inside that attempt, and passed to the consumers — see + /// . + /// + internal sealed record SnapshotExclusionPlan( + string GitRelativeCwd, + ImmutableArray VendorConfigPaths, + (string Canonical, byte[] Bytes)[] Reserved, + string[] SnapshotExclusions); + + /// + /// Reads the execution cwd's path as git spells it, relative to the work-tree top. Empty when + /// the cwd is the repository root. + /// + /// Why git and not the filesystem. A prefix taken from .NET is not in the same pathname + /// namespace as the paths ls-files reports, and concatenating one onto the other produces a + /// comparison that can silently fail to match: macOS reports NFD for a directory created as NFC (and + /// rejects non-NFC git paths, so only the prefix side could + /// diverge); Path.GetRelativePath returns a ROOTED path across Windows volumes; and separator + /// conventions differ. Reading it from git deletes all three classes rather than guarding them. + /// + /// The one prefix. The value returned here locates the execution directory + /// (ContainedPath(snapshot, prefix)) as well as driving the plan. An earlier revision used the + /// filesystem spelling for the launch and this one for classification; two independent derivations can + /// disagree, and then an unexcluded path materialises the alternate-spelling directory itself — so the + /// vendor launches in a directory whose config was never excluded. Do not reintroduce a second + /// derivation. + /// + /// core.quotePath=false is required, or non-ASCII components come back C-quoted. + /// + internal static async Task ReadGitRelativeCwdAsync( + string sourceRepoRoot, string sourceCwd, CancellationToken ct) { + // The prefix is only meaningful against the repository whose manifest it will filter, and + // `rev-parse` reports whatever repository git DISCOVERS at the cwd. A cwd inside a nested + // repository — or in an entirely different one — would otherwise yield a prefix in a foreign + // namespace that is then matched against this source's `ls-files` output, which is precisely the + // "one namespace" invariant this derivation exists to hold. So the work-tree top is captured and + // required to be the source root before the prefix is trusted. + var topRaw = await RunGitCaptureBoundedAsync( + sourceCwd, GitTimeout, MaxCwdPrefixCaptureBytes, ct, + "-c", "core.quotePath=false", "rev-parse", "--show-toplevel"); + var top = ParseSingleLine(topRaw); + if (top.Length == 0 || + !ResolveDeepestExisting(top).Equals( + ResolveDeepestExisting(sourceRepoRoot), FileSystemPathComparison)) + throw new InvalidOperationException("borrowed_snapshot_cwd_foreign_repository"); + + var raw = await RunGitCaptureBoundedAsync( + sourceCwd, GitTimeout, MaxCwdPrefixCaptureBytes, ct, + "-c", "core.quotePath=false", "rev-parse", "--show-prefix"); + + return ParseGitRelativeCwd(raw); + } + + /// Strips exactly one trailing LF and refuses CR or any embedded LF — the same framing rules + /// as the prefix parse, for a command whose output is one path rather than a repository-relative + /// path. + static string ParseSingleLine(ReadOnlySpan raw) { + if (raw.Length == 0 || raw[^1] != (byte)'\n') + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + var body = raw[..^1]; + if (body.IndexOf((byte)'\n') >= 0 || body.IndexOf((byte)'\r') >= 0) + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + try { return StrictUtf8.GetString(body); } + catch (DecoderFallbackException ex) { + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed", ex); + } + } + + /// Byte-exact parse of rev-parse --show-prefix output. Split out so it is testable + /// against captured bytes without a live repository, and so the framing rules are stated once. + internal static string ParseGitRelativeCwd(ReadOnlySpan raw) { + // Exactly one trailing LF, and no CR or embedded LF anywhere. Anything else is not this + // command's output shape and is refused rather than repaired. + if (raw.Length == 0 || raw[^1] != (byte)'\n') + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + var body = raw[..^1]; + if (body.IndexOf((byte)'\n') >= 0 || body.IndexOf((byte)'\r') >= 0) + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + + // The repository root. Deliberately returned BEFORE the path validator, which rejects the empty + // string — this is the common case and it is not a path. + if (body.Length == 0) return ""; + + if (body[^1] != (byte)'/') + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + body = body[..^1]; + if (body.Length == 0) + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + + string decoded; + try { decoded = StrictUtf8.GetString(body); } + catch (DecoderFallbackException ex) { + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed", ex); + } + + // Normalisation runs BEFORE the case-sensitivity rule in PlanSnapshotExclusions, so that rule can + // assume an NFC operand. A consequence worth knowing: an NFD prefix fails here as + // borrowed_snapshot_invalid_path, not as the non-ASCII rejection. + return NormalizeRelativePath(decoded); + } + + /// + /// Expands the canonical vendor-config list across the ancestor chain of + /// , inclusive of both the repository root and the cwd itself. + /// + /// Not every directory in the tree. A sibling of the cwd is deliberately left alone: no + /// supported vendor is documented to discover config there, and stripping it would delete content the + /// launch cannot reach — including this repository's own committed kcap/.mcp.json. The property + /// this delivers is scoped to the vendor the daemon launches, at the cwd it launches it in, without + /// model involvement; a model that deliberately changes directory and starts another CLI is the OS + /// sandbox's problem, not this list's. + /// + /// Case. is probed on the DESTINATION, which is the + /// volume the vendor executes on, and that is what makes it the right input. On a case-insensitive + /// destination SRC and src are one directory, so folding is correct and no case-varying + /// sibling can exist; on a case-sensitive one they are distinct, so NOT folding is correct. An earlier + /// revision folded unconditionally and thereby handed a hostile branch a launch-refusal primitive: + /// tracked a/.mcp.json and A/.mcp.json both collapsed onto one canonical candidate and + /// the extractor's collision check refused every launch of that repository. + /// + /// Non-ASCII prefixes. Admitted on a case-sensitive destination, where both sides are NFC + /// and an exact comparison is sound. Refused on a case-insensitive one, because that volume also + /// equates pairs such as Å/å which the ASCII-only matcher would miss — and proving the + /// equivalence would require a second, Unicode-aware matcher, which is the defect this design removes. + /// This is a stated compatibility limitation on one platform class, not a security property. + /// + internal static SnapshotExclusionPlan PlanSnapshotExclusions( + string gitRelativeCwd, bool caseSensitive, IEnumerable? additional = null) { + if (!caseSensitive && !IsAscii(gitRelativeCwd)) + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_non_ascii"); + + var components = gitRelativeCwd.Length == 0 + ? [] + : gitRelativeCwd.Split('/', StringSplitOptions.RemoveEmptyEntries); + if (components.Length > MaxCwdDepth) + throw new InvalidOperationException("borrowed_snapshot_cwd_too_deep"); + + var directoryPrefixes = new List(components.Length + 1) { "" }; + var accumulated = ""; + foreach (var component in components) { + accumulated += component + "/"; + directoryPrefixes.Add(accumulated); + } + + var paths = ImmutableArray.CreateBuilder( + directoryPrefixes.Count * WorkspaceMcpConfigPaths.Length); + var reserved = new List<(string, byte[])>(paths.Capacity); + long aggregate = 0; + foreach (var prefix in directoryPrefixes) + foreach (var canonical in WorkspaceMcpConfigPaths) { + var path = prefix + canonical; + var bytes = Encoding.UTF8.GetBytes(path); + aggregate += bytes.Length; + if (aggregate > MaxVendorPathAggregateBytes) + throw new InvalidOperationException("borrowed_snapshot_cwd_too_deep"); + paths.Add(path); + reserved.Add((path, bytes)); + } + + // .capacitor and .attached only — plus whatever the caller added. The vendor paths are NOT here: + // they are matched exclusively through ClassifyReservedPath. These are ASCII daemon-supplied + // constants, so the namespace question the vendor paths raise does not arise for them. + string[] exclusions = [".capacitor", ".attached", .. additional ?? []]; + + return new SnapshotExclusionPlan( + gitRelativeCwd, paths.ToImmutable(), [.. reserved], exclusions); + } + + static bool IsAscii(string value) { + foreach (var c in value) if (c > 0x7F) return false; + return true; + } + + /// Captures a git command's stdout, refusing rather than truncating past + /// . Reads BaseStream, not the StreamReader, because + /// StandardOutputEncoding alone does not disable the reader's BOM detection. + static async Task RunGitCaptureBoundedAsync( + string cwd, TimeSpan timeout, int maxBytes, CancellationToken ct, params string[] args) { + var psi = NewGitPsi(cwd, args, sourceReadOnly: true); + using var process = Process.Start(psi)!; + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(timeout); + using var stdout = new MemoryStream(); + var stderrTask = ReadAllDecodedAsync(process.StandardError.BaseStream, timeoutCts.Token); + var stderr = ""; + try { + var buffer = new byte[4096]; + while (true) { + var read = await process.StandardOutput.BaseStream.ReadAsync(buffer, timeoutCts.Token); + if (read == 0) break; + if (stdout.Length + read > maxBytes) + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + stdout.Write(buffer, 0, read); + } + await process.WaitForExitAsync(timeoutCts.Token); + // Captured INSIDE the protected block. Awaiting it after the finally would re-await a pump + // the cleanup may have abandoned, wait out the remainder of the git timeout, and surface a + // raw task exception instead of this method's contextual timeout message. + stderr = await stderrTask; + } catch (OperationCanceledException) { + throw new InvalidOperationException( + $"git {string.Join(' ', args)} timed out after {timeout.TotalSeconds:F0}s"); + } finally { + // Every abnormal exit runs through here: the overflow throw and the cancellation branch both + // used to kill inline and leave the stderr pump unobserved and the child unreaped. + // Process.Dispose is not a termination guarantee. + await TerminateAndDrainAsync(process, stderrTask); + } + if (process.ExitCode != 0) + throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {stderr}"); + return stdout.ToArray(); + } + + /// Budget for cleanup after a git helper has already failed. Bounded on purpose — see below. + /// + static readonly TimeSpan CleanupBudget = TimeSpan.FromSeconds(5); + + /// Kills the process if it is still running, waits for it to be reaped, and observes the + /// supplied pump tasks so a faulted read cannot surface as an unobserved exception. + /// + /// Every wait is bounded. This runs from a finally, so anything unbounded here + /// swallows the original timeout or overflow exception by never returning. An unbounded + /// WaitForExitAsync hangs forever if the kill genuinely failed, and an unbounded pump await + /// hangs if a surviving descendant inherited the redirected pipe. Past the budget the streams are + /// abandoned rather than awaited — the original exception is the thing that matters. + /// + /// Failures here are swallowed deliberately: the reason the command failed is more useful to an + /// operator than whatever went wrong tidying up after it. + /// + static async Task TerminateAndDrainAsync(Process process, params Task[] pumps) { + using var budget = new CancellationTokenSource(CleanupBudget); + try { if (!process.HasExited) process.Kill(entireProcessTree: true); } + catch { /* already gone, or tree enumeration failed — the direct kill below is the fallback */ } + // Tree termination can fail while killing the process itself still succeeds, and returning from + // here with a live owned child is a leak: disposing Process does not terminate it. + try { if (!process.HasExited) process.Kill(); } + catch { /* genuinely unkillable — the bounded waits below stop us hanging on it */ } + try { await process.WaitForExitAsync(budget.Token); } catch { /* reaped, or over budget */ } + foreach (var pump in pumps) { + // Abandoning a pump past the budget must not leave its exception unobserved — WaitAsync + // observes only the wait, not the underlying task — so every pump also gets a terminal + // continuation regardless of which way this goes. + _ = pump.ContinueWith(static t => _ = t.Exception, + CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, + TaskScheduler.Default); + try { await pump.WaitAsync(budget.Token); } + catch { /* observed, or abandoned past the budget */ } + } + } + + /// Runs a git command feeding as NUL-separated stdin. + /// Used for the skip-worktree batch. Passing the paths as argv would hit ARG_MAX + /// for a deep cwd — the expansion is O(depth²) aggregate bytes — and would also let an ancestor + /// directory whose name begins with : or - be read as pathspec syntax. --stdin + /// paths are literal. + static async Task RunGitWithNulStdinAsync( + string cwd, TimeSpan timeout, IReadOnlyCollection lines, + CancellationToken ct, params string[] args) { + var psi = NewGitPsi(cwd, args); + psi.RedirectStandardInput = true; + using var process = Process.Start(psi)!; + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(timeout); + var stderrTask = ReadAllDecodedAsync(process.StandardError.BaseStream, timeoutCts.Token); + var stdoutTask = ReadAllDecodedAsync(process.StandardOutput.BaseStream, timeoutCts.Token); + var stderr = ""; + try { + // Disposing the stream is the EOF signal git waits for; it must happen even if a write + // faults part-way, or the child blocks on a read that will never complete. + await using (var input = process.StandardInput.BaseStream) { + foreach (var line in lines) { + await input.WriteAsync(StrictUtf8.GetBytes(line), timeoutCts.Token); + await input.WriteAsync(new byte[] { 0 }, timeoutCts.Token); + } + } + await process.WaitForExitAsync(timeoutCts.Token); + await stdoutTask; + stderr = await stderrTask; // inside the protected block, as above + } catch (OperationCanceledException) { + throw new InvalidOperationException( + $"git {string.Join(' ', args)} timed out after {timeout.TotalSeconds:F0}s"); + } finally { + // Covers the cancellation branch AND an IOException mid-write, which previously left a + // running child and two unobserved pumps behind. + await TerminateAndDrainAsync(process, stdoutTask, stderrTask); + } + if (process.ExitCode != 0) + throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {stderr}"); + } +} diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs index 02d52eebd..e3cab49bc 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs @@ -37,8 +37,36 @@ public partial class WorktreeManager { public const string ReviewContextSuffix = ".review-context"; const string ReviewContextManifestName = "manifest.json"; const long MaxReviewContextBytes = 256L * 1024; + + /// Ceiling on the SERIALIZED manifest, distinct from , + /// which charges only blob content. Path strings, base64's 4/3 expansion and JSON framing are not free, + /// so the content cap is not by itself a bound on the file this writes and later re-reads. + /// + /// Derived from the content cap, not chosen. Each admitted byte can appear twice — once + /// base64-encoded (4/3) and once in Text, where JSON escaping of a control character costs six + /// bytes (backslash-u-0000). Worst case is therefore about 256 KiB × (4/3 + 6) ≈ 1.9 MiB before + /// paths, hashes and framing. A first attempt at 1 MiB was below that and rejected a manifest the + /// content cap had already accepted — a fail-closed refusal of a legitimate snapshot. 4 MiB clears the + /// worst case with headroom while still bounding the read. + const long MaxReviewContextManifestBytes = 4L * 1024 * 1024; + static readonly UTF8Encoding StrictUtf8 = new(false, true); + /// Reads a manifest, refusing anything past + /// BEFORE allocating it — checking after the read would already have paid the cost. + static async Task ReadManifestBytesAsync(string path, CancellationToken ct) { + await using var stream = new FileStream(path, new FileStreamOptions { + Mode = FileMode.Open, Access = FileAccess.Read, Share = FileShare.Read, + Options = FileOptions.Asynchronous | FileOptions.SequentialScan + }); + if (stream.Length > MaxReviewContextManifestBytes) + throw new InvalidOperationException( + "borrowed_snapshot_review_context_manifest_too_large"); + var buffer = new byte[stream.Length]; + await stream.ReadExactlyAsync(buffer, ct); + return buffer; + } + public static string ReviewContextRootFor(string snapshotRoot) => snapshotRoot.TrimEnd(Path.DirectorySeparatorChar) + ReviewContextSuffix; @@ -54,7 +82,7 @@ static BorrowedReviewContextGeneration PublishReviewContextGeneration( async Task CreateReviewContextGenerationAsync( string source, string reviewContextRoot, string sourceHead, - byte[] listing, bool caseSensitive, CancellationToken ct) { + byte[] listing, bool caseSensitive, SnapshotExclusionPlan plan, CancellationToken ct) { CreateOwnerOnlyDirectory(reviewContextRoot); var generationId = Guid.NewGuid().ToString("N"); var preparing = Path.Combine(reviewContextRoot, ".preparing-" + generationId); @@ -62,7 +90,7 @@ async Task CreateReviewContextGenerationAsync( try { CreateOwnerOnlyDirectory(preparing); var entries = await ExtractReviewContextEntriesAsync( - source, listing, caseSensitive, ct); + source, listing, caseSensitive, plan, ct); var manifest = new BorrowedReviewContextManifest( 1, @@ -75,14 +103,28 @@ async Task CreateReviewContextGenerationAsync( [.. entries.OrderBy(entry => entry.Path, StringComparer.Ordinal)]); var json = JsonSerializer.SerializeToUtf8Bytes( manifest, BorrowedReviewContextJsonContext.Default.BorrowedReviewContextManifest); + // MaxReviewContextBytes charges only blob CONTENT. The serialized form also carries path + // strings, base64's 4/3 expansion and JSON framing, so it needs its own ceiling — enforced + // here on write and again before parsing on read, so an oversized manifest is refused before + // it is allocated rather than after. + if (json.LongLength > MaxReviewContextManifestBytes) + throw new InvalidOperationException( + "borrowed_snapshot_review_context_manifest_too_large"); var manifestPath = Path.Combine(preparing, ReviewContextManifestName); await WriteOwnerOnlyFileAsync(manifestPath, json, ct); - var verifiedJson = await File.ReadAllBytesAsync(manifestPath, ct); + var verifiedJson = await ReadManifestBytesAsync(manifestPath, ct); var verifiedManifest = JsonSerializer.Deserialize( verifiedJson, BorrowedReviewContextJsonContext.Default.BorrowedReviewContextManifest) ?? throw new InvalidOperationException("borrowed_snapshot_review_context_invalid_manifest"); - ValidateReviewContextManifest(verifiedManifest, generationId, sourceHead); + // The reserved set the extractor actually matched — the ACTUAL git paths, not the plan's + // canonical spellings. On a case-insensitive destination a tracked `SRC/.MCP.JSON` legitimately + // classifies against canonical `src/.mcp.json`, so validating exact membership against the + // canonical set would reject a valid entry — and relaxing it to OrdinalIgnoreCase would put a + // second matcher back in, which is the defect this design removes. The case decision is made + // once, by the classifier, at extraction. + var matchedPaths = entries.Select(entry => entry.Path).ToHashSet(StringComparer.Ordinal); + ValidateReviewContextManifest(verifiedManifest, generationId, sourceHead, matchedPaths); return new BorrowedReviewContextGeneration(generationId, preparing, verifiedJson); } catch { @@ -92,10 +134,13 @@ async Task CreateReviewContextGenerationAsync( } static async Task> ExtractReviewContextEntriesAsync( - string source, byte[] listing, bool caseSensitive, CancellationToken ct) { - var reserved = WorkspaceMcpConfigPaths - .Select(path => (Canonical: path, Bytes: Encoding.UTF8.GetBytes(path))) - .ToArray(); + string source, byte[] listing, bool caseSensitive, SnapshotExclusionPlan plan, + CancellationToken ct) { + // The plan's set, not WorkspaceMcpConfigPaths: containment and reviewability have to range over + // the same paths, or a config one directory down becomes excluded from the snapshot (good) while + // staying invisible to the reviewer (bad) — contained but unreviewable, which is precisely the + // state this whole surface exists to avoid. + var reserved = plan.Reserved; var matchedCanonicalPaths = new HashSet(StringComparer.Ordinal); var entries = new List(); long totalBytes = 0; @@ -192,18 +237,24 @@ static async Task> ExtractReviewContextEntriesA static void ValidateReviewContextManifest( BorrowedReviewContextManifest manifest, string expectedGenerationId, - string expectedSourceHead) { + string expectedSourceHead, + IReadOnlySet matchedPaths) { if (manifest.SchemaVersion != 1 || manifest.GenerationId != expectedGenerationId || manifest.SourceHead != expectedSourceHead || manifest.Provenance != "git-index-stage-0" || manifest.WorkingTreeBytes || !manifest.UnstagedAndUntrackedOmitted || - manifest.Entries.Length > WorkspaceMcpConfigPaths.Length) + manifest.Entries.Length > matchedPaths.Count) throw new InvalidOperationException( "borrowed_snapshot_review_context_invalid_manifest"); long total = 0; foreach (var entry in manifest.Entries) { + // Exact membership in the set the classifier actually matched. Strictly stronger than the + // count cap this replaces, which bounded how many entries there were but not which. + if (!matchedPaths.Contains(entry.Path)) + throw new InvalidOperationException( + "borrowed_snapshot_review_context_invalid_manifest"); byte[] content; try { content = Convert.FromBase64String(entry.Base64); } catch (FormatException ex) { diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.WorkspaceMcp.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.WorkspaceMcp.cs index 5588840d8..884366124 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.WorkspaceMcp.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.WorkspaceMcp.cs @@ -27,15 +27,27 @@ public partial class WorktreeManager { /// protected by their own argv, a property of each launcher rather than of the worktree. Kiro arrived /// with no gate at all and nobody noticed, so the list covers every hosted vendor's file plus the /// editor-generic ones — the point is that the next vendor is safe before anyone thinks about it. + /// + /// These are the names; the SCOPE is separate. Every entry is relative to a directory, + /// not to the repository root. For a borrowed snapshot the set of directories is the ancestor chain of + /// the execution cwd — see . Reading this list as root-relative is + /// what left src/.codex/config.toml live in a snapshot launched from src. /// internal static readonly ImmutableArray WorkspaceMcpConfigPaths = [ - ".mcp.json", // Claude Code / generic + ".mcp.json", // Claude Code / generic; Copilot CLI also reads it ".cursor/mcp.json", ".gemini/settings.json", ".kiro/settings/mcp.json", - ".vscode/mcp.json", // editor-generic; several CLIs read it + ".vscode/mcp.json", // editor-generic; GitHub documents Copilot CLI does NOT read it, + // but VS Code and other CLIs do + ".github/mcp.json", // Copilot CLI, alongside .mcp.json in the same walk. The list + // long carried .github/copilot/mcp.json, a DIFFERENT path, so this + // one was unprotected at every snapshot root. ".github/copilot/mcp.json", ".copilot/mcp.json", + ".copilot/mcp-config.json", // GitHub documents ~/.copilot/mcp-config.json as USER scope; the + // workspace form is not documented and is carried under the + // "wider than known readers" rationale above ".codex/config.toml" ]; diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index 4351a6315..77e8f88c8 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -16,6 +16,17 @@ public record WorktreeInfo( string? FetchedRef = null, string? SnapshotRoot = null, string? ReviewContextRoot = null) { internal BorrowedReviewContextGeneration? ReviewContextGeneration { get; init; } + /// The execution cwd as git spells it, relative to the work-tree top; empty at the root. + /// Computed once at creation and carried, never recomputed. A per-round refresh has only a + /// TARGET-side execution path, so re-deriving would mean a filesystem-relative derivation — the exact + /// thing that lets the launch path and the exclusion classifier disagree. Null means "not a borrowed + /// snapshot"; a refresh that finds it null on one throws rather than falling back. + /// In-memory is sufficient today because the only refresh caller reads it off the + /// AgentInstance created at launch, and an AgentInstance does not survive a daemon + /// restart. If a durable agent registry is ever added, this must be part of what it persists. + /// + public string? GitRelativeCwd { get; init; } + /// A borrowed cwd (local in-place launch) the daemon does NOT own. Cleanup /// never removes it — the guard enforces that. public static WorktreeInfo Borrowed(string cwd) => new(cwd, "", cwd, IsStandalone: false); @@ -35,13 +46,11 @@ public partial class WorktreeManager(DaemonConfig config, ILogger - /// Lazy, not a field initializer. Static field initializers across PARTIAL FILES have no - /// useful ordering, and WorkspaceMcpConfigPaths lives in the other partial — as a field this read - /// it while still default, and spreading a default ImmutableArray threw inside the type - /// initializer, which would have broken every worktree creation at runtime. - static string[]? _snapshotExcludedPaths; - internal static string[] SnapshotExcludedPaths => - _snapshotExcludedPaths ??= [".capacitor", ".attached", .. WorkspaceMcpConfigPaths]; + /// Superseded by . This was a static + /// [".capacitor", ".attached", ..WorkspaceMcpConfigPaths], which is only complete when the + /// reviewer executes at the repository root. It is gone rather than kept alongside the plan: two lists + /// of the same thing is exactly how .kiro/settings/mcp.json survived into a launched snapshot in + /// the first place. const int MaxSnapshotFiles = 50_000; const long MaxSnapshotBytes = 2L * 1024 * 1024 * 1024; static StringComparison FileSystemPathComparison => @@ -489,9 +498,18 @@ public async Task CreateBorrowedSnapshotAsync( string sourceRepoRoot, string requestedCwd, string? name, CancellationToken ct) { var source = Path.GetFullPath(sourceRepoRoot); var cwd = Path.GetFullPath(requestedCwd); - var relativeCwd = Path.GetRelativePath(source, cwd).Replace(Path.DirectorySeparatorChar, '/'); - if (relativeCwd == ".." || relativeCwd.StartsWith("../", StringComparison.Ordinal)) + // Containment check on the REQUESTED cwd, and nothing else. This value never locates a directory: + // the execution path below comes from the git-derived prefix, so that the launch and the exclusion + // classifier cannot be reading two different spellings of the same place. + // Path.IsPathRooted matters: GetRelativePath returns a ROOTED path across Windows volumes, which + // neither of the ".." tests catches. + var requestedRelative = Path.GetRelativePath(source, cwd).Replace(Path.DirectorySeparatorChar, '/'); + if (Path.IsPathRooted(requestedRelative) || requestedRelative == ".." || + requestedRelative.StartsWith("../", StringComparison.Ordinal)) throw new InvalidOperationException("borrowed_snapshot_cwd_outside_source"); + if (!Directory.Exists(cwd)) + throw new InvalidOperationException("borrowed_snapshot_cwd_missing"); + var gitRelativeCwd = await ReadGitRelativeCwdAsync(source, cwd, ct); var root = Path.GetFullPath(Path.Combine(config.WorktreeRoot, "borrowed-snapshots")); EnsureSeparateRoots(source, root); CreateOwnerOnlyDirectory(root); @@ -503,20 +521,26 @@ public async Task CreateBorrowedSnapshotAsync( var promoted = false; try { var reviewContextGeneration = await BuildIndependentSnapshotAsync( - source, staging, SnapshotExcludedPaths, reviewContextRoot, ct) + source, staging, gitRelativeCwd, [], reviewContextRoot, ct) ?? throw new InvalidOperationException("borrowed_snapshot_review_context_missing"); Directory.Move(staging, final); promoted = true; reviewContextGeneration = PublishReviewContextGeneration( reviewContextGeneration, reviewContextRoot); - var executionPath = relativeCwd == "." + var executionPath = gitRelativeCwd.Length == 0 ? final - : ContainedPath(final, relativeCwd); - if (!Directory.Exists(executionPath)) - throw new InvalidOperationException("borrowed_snapshot_cwd_missing"); + : ContainedPath(final, gitRelativeCwd); + // Created rather than required to exist. Widening the exclusion to the cwd's own directory + // made a new case reachable: a cwd whose only content IS vendor config now yields no + // directory at all in the snapshot, and throwing here would refuse the launch for exactly + // the repositories this change exists to protect. An empty cwd is the truthful result — + // everything that was there was excluded. Safe to create: RemoveFilesOutsideManifest has + // already deleted every reparse point, so no component of this path can be a link. + Directory.CreateDirectory(executionPath); return new WorktreeInfo(final == executionPath ? final : executionPath, "", source, IsStandalone: true, SnapshotRoot: final, ReviewContextRoot: reviewContextRoot) { - ReviewContextGeneration = reviewContextGeneration + ReviewContextGeneration = reviewContextGeneration, + GitRelativeCwd = gitRelativeCwd }; } catch { DeleteTreeNoFollow(staging); @@ -526,51 +550,63 @@ public async Task CreateBorrowedSnapshotAsync( } } - /// Rebuilds a borrowed snapshot from a pristine independent generation, then replaces - /// the live snapshot contents. The source repository is never used as the reviewer's cwd and - /// reviewer-created git metadata cannot survive into the next round. + /// Rebuilds from a pristine independent generation, + /// then replaces the live contents — the source repository is never used as the reviewer's cwd, and + /// reviewer-created git metadata cannot survive into the next round. Vendor config is excluded along + /// the ancestor chain of . + /// Takes a SOURCE cwd, not a target execution path. The overloads this replaces took only + /// a target-side path, which left no way to obtain the git-derived prefix except by re-deriving it from + /// the target filesystem — the derivation that lets the launch and the classifier disagree. They had no + /// production callers, so removing them is cheaper than preserving an unsafe inference. There is no + /// fallback: a caller that cannot supply a source cwd cannot use this. public async Task SyncFromSourceAsync( - string sourceRepoRoot, string targetWorktreePath, + string sourceRepoRoot, string sourceCwd, string targetWorktreePath, string[] excludePaths, CancellationToken ct) { - await SyncFromSourceAsync( - sourceRepoRoot, targetWorktreePath, targetWorktreePath, excludePaths, ct); - } + // The same admission checks CreateBorrowedSnapshotAsync applies to its requested cwd. The + // work-tree-top check inside ReadGitRelativeCwdAsync already refuses a foreign repository, so + // these are defence in depth — but they turn "git failed: ..." into a specific coded error, and + // this overload had no containment check of its own at all. + var source = Path.GetFullPath(sourceRepoRoot); + var cwd = Path.GetFullPath(sourceCwd); + var requestedRelative = Path.GetRelativePath(source, cwd).Replace(Path.DirectorySeparatorChar, '/'); + if (Path.IsPathRooted(requestedRelative) || requestedRelative == ".." || + requestedRelative.StartsWith("../", StringComparison.Ordinal)) + throw new InvalidOperationException("borrowed_snapshot_cwd_outside_source"); + if (!Directory.Exists(cwd)) + throw new InvalidOperationException("borrowed_snapshot_cwd_missing"); - public async Task SyncFromSourceAsync( - string sourceRepoRoot, string targetWorktreePath, string executionPath, - string[] excludePaths, CancellationToken ct) { + var gitRelativeCwd = await ReadGitRelativeCwdAsync(source, cwd, ct); _ = await SyncFromSourceCoreAsync( - sourceRepoRoot, targetWorktreePath, executionPath, + sourceRepoRoot, targetWorktreePath, gitRelativeCwd, excludePaths, reviewContextRoot: null, ct); } internal async Task SyncBorrowedSnapshotFromSourceAsync( - string sourceRepoRoot, string targetWorktreePath, string executionPath, + string sourceRepoRoot, string targetWorktreePath, string gitRelativeCwd, string[] excludePaths, string reviewContextRoot, CancellationToken ct) => await SyncFromSourceCoreAsync( - sourceRepoRoot, targetWorktreePath, executionPath, + sourceRepoRoot, targetWorktreePath, gitRelativeCwd, excludePaths, reviewContextRoot, ct) ?? throw new InvalidOperationException("borrowed_snapshot_review_context_missing"); async Task SyncFromSourceCoreAsync( - string sourceRepoRoot, string targetWorktreePath, string executionPath, + string sourceRepoRoot, string targetWorktreePath, string gitRelativeCwd, string[] excludePaths, string? reviewContextRoot, CancellationToken ct) { if (string.IsNullOrEmpty(sourceRepoRoot)) throw new ArgumentException("Source repo root must not be empty.", nameof(sourceRepoRoot)); if (string.IsNullOrEmpty(targetWorktreePath)) throw new ArgumentException("Target worktree path must not be empty.", nameof(targetWorktreePath)); + ArgumentNullException.ThrowIfNull(gitRelativeCwd); var source = Path.GetFullPath(sourceRepoRoot); var target = Path.GetFullPath(targetWorktreePath); - var execution = Path.GetFullPath(executionPath); + // Derived from the SAME prefix the exclusion plan is built from — never from the target + // filesystem. ContainedPath re-checks that it stays inside the target. + var execution = gitRelativeCwd.Length == 0 ? target : ContainedPath(target, gitRelativeCwd); if (string.Equals(source, target, StringComparison.Ordinal)) throw new InvalidOperationException($"Source and target paths are the same: {source}"); if (!Directory.Exists(source)) throw new InvalidOperationException($"Source repo root does not exist: {source}"); - if (!string.Equals(execution, target, FileSystemPathComparison) && - !execution.StartsWith(target.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar, - FileSystemPathComparison)) - throw new InvalidOperationException("borrowed_snapshot_execution_path_outside_target"); if (!File.Exists(Path.Combine(source, ".git")) && !Directory.Exists(Path.Combine(source, ".git"))) throw new InvalidOperationException($"Source path does not appear to be a git repo (no .git entry): {source}"); @@ -579,9 +615,8 @@ await SyncFromSourceCoreAsync( var staging = Path.Combine(parent, Path.GetFileName(target) + ".refresh-" + Guid.NewGuid().ToString("N")[..8]); BorrowedReviewContextGeneration? generation = null; try { - var exclusions = SnapshotExcludedPaths.Concat(excludePaths).Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); generation = await BuildIndependentSnapshotAsync( - source, staging, exclusions, reviewContextRoot, ct); + source, staging, gitRelativeCwd, excludePaths, reviewContextRoot, ct); ReplaceTreeContentsNoFollow(target, staging, execution); if (generation is not null) generation = PublishReviewContextGeneration( @@ -596,13 +631,16 @@ await SyncFromSourceCoreAsync( } async Task BuildIndependentSnapshotAsync( - string source, string destination, string[] exclusions, + string source, string destination, string gitRelativeCwd, string[] excludePaths, string? reviewContextRoot, CancellationToken ct) { for (var attempt = 0; attempt < 2; attempt++) { BorrowedReviewContextGeneration? generation = null; try { + // The plan is built INSIDE the attempt, because it depends on the destination's probed + // case sensitivity and each retry creates a fresh destination. A retry must not reuse the + // previous attempt's plan. generation = await BuildIndependentSnapshotOnceAsync( - source, destination, exclusions, reviewContextRoot, ct); + source, destination, gitRelativeCwd, excludePaths, reviewContextRoot, ct); return generation; } catch (SourceChangedException) when (attempt == 0) { DeleteTreeNoFollow(destination); @@ -617,7 +655,7 @@ await SyncFromSourceCoreAsync( } async Task BuildIndependentSnapshotOnceAsync( - string source, string destination, string[] exclusions, + string source, string destination, string gitRelativeCwd, string[] excludePaths, string? reviewContextRoot, CancellationToken ct) { var parent = Directory.GetParent(destination)?.FullName ?? throw new InvalidOperationException("Snapshot destination has no parent directory."); @@ -643,18 +681,22 @@ await SyncFromSourceCoreAsync( await RunGitBestEffort(destination, "reflog", "expire", "--expire=now", "--all"); var fetchHead = Path.Combine(destination, ".git", "FETCH_HEAD"); if (File.Exists(fetchHead)) File.Delete(fetchHead); + // Probed on the ACTUAL destination, never on its parent or the configured worktree root: case + // behaviour can differ per directory, and a substituted probe would classify under the wrong + // semantics. Everything downstream reads this one result. var caseSensitive = ProbeCaseSensitiveFileSystem(destination); + var plan = PlanSnapshotExclusions(gitRelativeCwd, caseSensitive, excludePaths); if (reviewContextRoot is not null) generation = await CreateReviewContextGenerationAsync( - source, reviewContextRoot, sourceHead, initialIndex, caseSensitive, ct); + source, reviewContextRoot, sourceHead, initialIndex, caseSensitive, plan, ct); if (SplitNulRecords(initialIndex) .Any(entry => entry.Span.StartsWith("160000 "u8))) throw new InvalidOperationException("borrowed_snapshot_submodules_unsupported"); - var manifest = await ReadSourceManifestAsync(source, exclusions, caseSensitive, ct); - await ApplyReservedIndexPolicyAsync(destination); + var manifest = await ReadSourceManifestAsync(source, plan, caseSensitive, ct); + await ApplyReservedIndexPolicyAsync(destination, plan, caseSensitive, ct); await CopyManifestAsync(source, destination, manifest, ct); RemoveFilesOutsideManifest(destination, manifest.Keys, caseSensitive, ct); VerifyIndependentGit(destination, source); @@ -664,7 +706,7 @@ await SyncFromSourceCoreAsync( var finalIndex = await RunGitCaptureBytes(source, GitTimeout, true, ct, "ls-files", "--stage", "-z"); var finalManifest = await ReadSourceManifestAsync( - source, exclusions, caseSensitive, ct); + source, plan, caseSensitive, ct); if (!string.Equals(sourceHead, finalHead, StringComparison.Ordinal) || !initialIndex.AsSpan().SequenceEqual(finalIndex) || !ManifestsEqual(manifest, finalManifest)) @@ -680,7 +722,7 @@ await SyncFromSourceCoreAsync( } static async Task> ReadSourceManifestAsync( - string source, string[] exclusions, bool caseSensitive, CancellationToken ct) { + string source, SnapshotExclusionPlan plan, bool caseSensitive, CancellationToken ct) { var stdout = await RunGitCaptureBytes(source, GitTimeout, true, ct, "ls-files", "-co", "--exclude-standard", "-z"); // A stage-only addition has no working-tree bytes to mirror. Skip those exact raw paths @@ -699,6 +741,14 @@ static async Task> ReadSourceManifestAsync( foreach (var rawBytes in SplitNulRecords(stdout)) { ct.ThrowIfCancellationRequested(); if (deletedPaths.Contains(Convert.ToBase64String(rawBytes.Span))) continue; + // Vendor config is matched HERE, on the raw bytes, by the same classifier the review-context + // extractor uses — and before decoding, preserving that extractor's classify-before-decode + // guarantee. Routing it through IsUnderExcluded instead would be a second matcher with + // different case-folding semantics (OrdinalIgnoreCase there, ASCII-only here), which is how a + // path becomes excluded by one and invisible to the other. + if (ClassifyReservedPath(rawBytes.Span, plan.Reserved, caseSensitive).Kind + != ReservedPathMatchKind.Unrelated) + continue; string raw; try { raw = StrictUtf8.GetString(rawBytes.Span); } catch (DecoderFallbackException ex) { @@ -706,7 +756,8 @@ static async Task> ReadSourceManifestAsync( "borrowed_snapshot_invalid_path_encoding", ex); } var rel = NormalizeRelativePath(raw); - if (IsUnderExcluded(rel, exclusions, caseSensitive)) { + // Only .capacitor, .attached and caller-supplied excludes reach this — ASCII daemon constants. + if (IsUnderExcluded(rel, plan.SnapshotExclusions, caseSensitive)) { var pathComparison = caseSensitive ? StringComparison.Ordinal : StringComparison.OrdinalIgnoreCase; @@ -846,13 +897,112 @@ static void DeleteTreeNoFollow(string path) { Directory.Delete(path); } + /// Refuses a snapshot root at or under the source checkout. + /// Why it matters beyond tidiness. Claude Code's workspace .mcp.json lookup walks + /// UPWARD and does not stop at the git root, so the snapshot's physical ancestors are reachable. If the + /// snapshot landed under the source, the source's own root config would be an ancestor of the + /// reviewer's cwd and would load — which no amount of excluding inside the snapshot prevents. + /// Lexical AND resolved. The lexical comparison alone is defeated by a + /// WorktreeRoot configured as a symlink whose target is inside the source: the string test + /// passes and the snapshot lands there anyway. + /// Residual, accepted and stated. Resolution handles symlinks and Windows junctions. It + /// does NOT close a Unix bind mount of a source subdirectory at an apparently external path, nor SUBST + /// or 8.3 aliases. WorktreeRoot is daemon OPERATOR configuration, so reaching those needs an + /// already-compromised host config rather than branch content — the same alias classes this file's + /// worktree-metadata gate already documents as defeating a different path-identity check. + /// static void EnsureSeparateRoots(string source, string snapshotRoot) { - var prefix = source.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; - if (snapshotRoot.Equals(source, FileSystemPathComparison) || - snapshotRoot.StartsWith(prefix, FileSystemPathComparison)) + if (IsAtOrUnder(snapshotRoot, source) || + IsAtOrUnder(ResolveDeepestExisting(snapshotRoot), ResolveDeepestExisting(source))) throw new InvalidOperationException("borrowed_snapshot_root_inside_source"); } + /// Ancestry over path STRINGS, with both operands normalised to NFC. + /// + /// Why fold at all. Case folding alone is not enough on a normalisation-insensitive + /// volume: a typical macOS filesystem treats caf\u00e9 composed and decomposed as ONE directory, + /// while no StringComparison makes those two strings equal. A source spelled one way and a + /// configured snapshot root spelled the other would otherwise fail both the lexical and the resolved + /// check and still land the snapshot inside the source. + /// + /// Why unconditionally, and what it costs. On a normalisation-SENSITIVE volume those are + /// genuinely distinct directories, so folding can refuse a layout that is actually fine. The refusal + /// needs an operator to have spelled the source and the worktree root with different normalisations of + /// the same name, on such a volume, and it fails closed with a specific coded error — so the cost is a + /// clear error in a vanishingly rare configuration, against a containment bypass in a common one. + /// + /// A probe was written to make this conditional and then REMOVED. Deciding by probe meant + /// creating a file inside the user's own checkout — which the source manifest reads as untracked + /// content — and deleting a second pathname the probe had not created; and its lookup used + /// File.Exists, which reports false for access and I/O errors as well as for absence, so + /// a failed probe read as "normalisation-sensitive" and silently reopened the bypass. Fail-open plus a + /// destructive cleanup is a worse trade than an over-refusal. + /// + /// True filesystem identity would settle it, but .NET exposes no portable device/inode pair, so + /// exotic aliases stay in the trusted-configuration residual documented on + /// . + static bool IsAtOrUnder(string candidate, string root) { + candidate = candidate.Normalize(NormalizationForm.FormC); + root = root.Normalize(NormalizationForm.FormC); + var prefix = root.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; + return candidate.Equals(root, FileSystemPathComparison) || + candidate.StartsWith(prefix, FileSystemPathComparison); + } + + /// Resolves links along the whole existing prefix of , then appends + /// whatever does not exist yet literally. + /// The snapshot root is created by the very call that checks it, so "resolve the final path" is + /// undefined. Appending the tail without re-resolving also means a component substituted after the + /// check cannot be followed by this function. + /// Every component, not just the deepest. An earlier version tested LinkTarget on + /// the deepest existing component alone, so with /alias -> /real and an ordinary + /// /alias/existing, resolving /alias/existing/new returned the lexical path and the + /// containment check still missed a snapshot root reaching inside the source through the ancestor + /// link. Chains are followed with a bounded iteration count rather than trusted to terminate. + /// + static string ResolveDeepestExisting(string path) { + const int maxLinkHops = 64; + var full = Path.GetFullPath(path); + var tail = new List(); + var current = full; + + // Split off the components that do not exist yet; they are re-appended verbatim below. + while (!Path.Exists(current)) { + var parent = Path.GetDirectoryName(current); + if (string.IsNullOrEmpty(parent) || parent == current) return full; + tail.Add(Path.GetFileName(current)); + current = parent; + } + tail.Reverse(); + + // Walk the existing prefix component by component, resolving each link as it is encountered so an + // ancestor link is followed rather than skipped. + var resolved = Path.GetPathRoot(current) ?? ""; + var components = current[resolved.Length..] + .Split(Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries); + foreach (var component in components) { + resolved = Path.Combine(resolved, component); + // ResolveLinkTarget(returnFinalTarget: true) already follows a chain, so this loop is + // belt-and-braces for a target that is itself a link relative to a different parent. It fails + // CLOSED on exhaustion rather than continuing with a half-resolved path — silently carrying on + // is how a containment check ends up comparing something that is not the real location. + var hop = 0; + for (; hop < maxLinkHops; hop++) { + var target = new DirectoryInfo(resolved).LinkTarget is not null + || new FileInfo(resolved).LinkTarget is not null + ? new DirectoryInfo(resolved).ResolveLinkTarget(returnFinalTarget: true)?.FullName + ?? new FileInfo(resolved).ResolveLinkTarget(returnFinalTarget: true)?.FullName + : null; + if (target is null) break; + resolved = Path.GetFullPath(target); + } + if (hop == maxLinkHops) + throw new InvalidOperationException("borrowed_snapshot_path_link_chain_too_deep"); + } + + return tail.Count == 0 ? resolved : Path.Combine([resolved, .. tail]); + } + internal static string NormalizeRelativePath(string raw) { if (raw.Length == 0 || raw.StartsWith('/') || raw.Contains('\\') || raw.Contains('\r') || raw.Contains('\n') || @@ -949,16 +1099,52 @@ static bool ManifestsEqual( pair.Value.Length == other.Length && pair.Value.Hash.AsSpan().SequenceEqual(other.Hash)); /// Marks the excluded config paths skip-worktree so their absence from the snapshot is - /// not reported as a change. - /// This iterated a hard-coded pair while the snapshot excluded the same pair. Now that the - /// exclusions fold in , a tracked .kiro/settings/mcp.json - /// would show up as a DELETION inside the snapshot — polluting git status and diffs, and capable - /// of producing a review finding about a deletion kcap performed. Driven from the same list, so the two - /// cannot drift apart again. - static async Task ApplyReservedIndexPolicyAsync(string destination) { - foreach (var path in WorkspaceMcpConfigPaths) - try { await RunGit(destination, GitTimeout, "update-index", "--skip-worktree", "--", path); } - catch { /* absent from index */ } + /// not reported as a change — otherwise a tracked src/.mcp.json shows up as a DELETION in the + /// reviewer's git status and diff, and can produce a review finding about a deletion kcap + /// performed. + /// + /// Reads the DESTINATION index, not the source's. The destination is a fresh clone checked + /// out at HEAD, so a path staged-but-uncommitted in the source is in the SOURCE index and absent + /// here. Intersecting against the source listing would batch such a path and fail + /// update-index on a perfectly legitimate snapshot. + /// + /// Membership decides, so failure is real. The previous version swallowed every error to + /// cover "absent from the index". With membership established from the listing, a failure means + /// something else went wrong and it propagates. + /// + /// Batched on stdin. The expanded set is paths × depth entries and O(depth²) + /// aggregate bytes, so argv could exceed ARG_MAX; --stdin also makes the paths literal, + /// where an ancestor directory named :foo or -foo would otherwise be read as pathspec + /// syntax. + /// + /// The targets are the index's OWN spellings, taken from the listing, so they are guaranteed to + /// name entries git will accept — and the case decision is made once, by the shared classifier. + /// + static async Task ApplyReservedIndexPolicyAsync( + string destination, SnapshotExclusionPlan plan, bool caseSensitive, CancellationToken ct) { + var indexListing = await RunGitCaptureBytes(destination, GitTimeout, false, ct, "ls-files", "-z"); + var targets = new List(); + foreach (var record in SplitNulRecords(indexListing)) { + ct.ThrowIfCancellationRequested(); + // Every non-Unrelated match, Exact AND Descendant — the same set ReadSourceManifestAsync + // excludes. An earlier version marked only Exact on the reasoning that a descendant of a + // config path is not an index entry, which is backwards: the reserved parent may not be an + // entry, but a repository CAN track `.codex/config.toml/child` (the config pathname as a + // directory), and each such child is a real index entry. Omitted from the snapshot and left + // unmarked, it reads as a deletion — and an ordinary git operation in the snapshot could + // restore it and rebuild a live vendor-config tree. + if (ClassifyReservedPath(record.Span, plan.Reserved, caseSensitive).Kind + == ReservedPathMatchKind.Unrelated) + continue; + // A non-UTF8 index path cannot have matched an ASCII candidate, so this cannot throw for a + // path that reached here; the guard is for the impossible case rather than a silent skip. + targets.Add(StrictUtf8.GetString(record.Span)); + } + + if (targets.Count > 0) + await RunGitWithNulStdinAsync( + destination, GitTimeout, targets, ct, "update-index", "--skip-worktree", "-z", "--stdin"); + Directory.CreateDirectory(Path.Combine(destination, ".git", "info")); File.AppendAllText(Path.Combine(destination, ".git", "info", "exclude"), "\n.attached/\n"); } diff --git a/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs b/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs new file mode 100644 index 000000000..78437b8a8 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs @@ -0,0 +1,566 @@ +using System.Diagnostics; +using System.Text; +using Capacitor.Cli.Daemon; +using Capacitor.Cli.Daemon.Services; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Capacitor.Cli.Tests.Unit; + +/// +/// Vendor MCP config must be excluded along the ancestor chain of the execution cwd, not only at the +/// repository root. +/// +/// Every test here carries a positive control. A containment test that never produced the +/// file it claims to exclude passes for the wrong reason, and this surface has produced that mistake +/// repeatedly. Where the control is a second build with a different cwd, the assertion is that the SAME +/// fixture yields the file — so "absent" can only mean the exclusion acted. +/// +public class BorrowedSnapshotExclusionScopeTests { + // ---------- fixture ---------- + + sealed record Fixture(string Source, string SnapshotRoot) : IDisposable { + public void Dispose() { + TryDelete(Source); + TryDelete(SnapshotRoot); + } + + static void TryDelete(string path) { + try { if (Directory.Exists(path)) Directory.Delete(path, recursive: true); } catch { } + } + } + + static Fixture NewFixture(params (string Path, string Content)[] tracked) { + var stem = Guid.NewGuid().ToString("N")[..8]; + var source = Path.Combine(Path.GetTempPath(), "kcap-excl-src-" + stem); + var snapshotRoot = Path.Combine(Path.GetTempPath(), "kcap-excl-wt-" + stem); + Directory.CreateDirectory(source); + Directory.CreateDirectory(snapshotRoot); + + Git(source, "init", "-q"); + Git(source, "config", "user.email", "test@example.com"); + Git(source, "config", "user.name", "Test"); + // A file at the root so the repo has content independent of the paths under test. + Write(source, "README.md", "readme"); + foreach (var (path, content) in tracked) Write(source, path, content); + Git(source, "add", "-A"); + Git(source, "commit", "-q", "-m", "fixture"); + + return new Fixture(source, snapshotRoot); + } + + static void Write(string root, string relative, string content) { + var full = Path.Combine(root, relative.Replace('/', Path.DirectorySeparatorChar)); + Directory.CreateDirectory(Path.GetDirectoryName(full)!); + File.WriteAllText(full, content); + } + + static WorktreeManager NewManager(Fixture fixture) => + new(new DaemonConfig { WorktreeRoot = fixture.SnapshotRoot }, + NullLogger.Instance); + + static async Task SnapshotAsync(Fixture fixture, string relativeCwd) => + await NewManager(fixture).CreateBorrowedSnapshotAsync( + fixture.Source, + relativeCwd.Length == 0 + ? fixture.Source + : Path.Combine(fixture.Source, relativeCwd.Replace('/', Path.DirectorySeparatorChar)), + null, CancellationToken.None); + + static bool ExistsInSnapshot(WorktreeInfo snapshot, string relative) => + File.Exists(Path.Combine( + snapshot.SnapshotRoot!, relative.Replace('/', Path.DirectorySeparatorChar))); + + static void Git(string cwd, params string[] args) { + var psi = new ProcessStartInfo("git", args) { + WorkingDirectory = cwd, RedirectStandardOutput = true, RedirectStandardError = true + }; + using var proc = Process.Start(psi)!; + proc.WaitForExit(); + if (proc.ExitCode != 0) + throw new InvalidOperationException( + $"git {string.Join(' ', args)} failed: {proc.StandardError.ReadToEnd()}"); + } + + static string GitCapture(string cwd, params string[] args) { + var psi = new ProcessStartInfo("git", args) { + WorkingDirectory = cwd, RedirectStandardOutput = true, RedirectStandardError = true + }; + using var proc = Process.Start(psi)!; + var stdout = proc.StandardOutput.ReadToEnd(); + proc.WaitForExit(); + return stdout; + } + + /// Whether the volume backing the temp directory distinguishes case. Probed, never inferred + /// from the OS: a case-sensitive APFS volume on macOS and a case-insensitive mount on Linux both + /// exist, and several assertions here are only meaningful on one side of that. + static bool TempIsCaseSensitive() { + var dir = Path.Combine(Path.GetTempPath(), "kcap-case-" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateDirectory(dir); + try { + File.WriteAllText(Path.Combine(dir, "probe"), ""); + return !File.Exists(Path.Combine(dir, "PROBE")); + } finally { + try { Directory.Delete(dir, recursive: true); } catch { } + } + } + + // ---------- 1. Codex sub-cwd, with a root-cwd positive control ---------- + + [Test] + public async Task Codex_config_below_the_root_is_excluded_when_the_cwd_is_that_directory() { + using var fixture = NewFixture(("src/.codex/config.toml", "[mcp_servers.x]\ncommand = \"/bin/sh\"\n")); + + var snapshot = await SnapshotAsync(fixture, "src"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "src/.codex/config.toml")).IsFalse() + .Because("Codex layers .codex/config.toml from the repository root down to the cwd, so a " + + "root-scoped list leaves the cwd's own layer live"); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + [Test] + public async Task Control_the_same_codex_fixture_survives_a_root_cwd_build() { + using var fixture = NewFixture(("src/.codex/config.toml", "[mcp_servers.x]\ncommand = \"/bin/sh\"\n")); + + var snapshot = await SnapshotAsync(fixture, ""); + try { + await Assert.That(ExistsInSnapshot(snapshot, "src/.codex/config.toml")).IsTrue() + .Because("without this the exclusion test above could pass because the fixture never " + + "produced the file, or because sub-directories are dropped for some other reason"); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 2. .github/mcp.json at the root ---------- + + [Test] + public async Task Copilot_github_mcp_json_is_excluded_at_the_root() { + using var fixture = NewFixture( + (".github/mcp.json", "{\"mcpServers\":{}}"), + (".github/workflows/ci.yml", "name: ci")); + + // The control is on the SOURCE side: prove the file is really tracked and would be copied, + // rather than inferring it from a surviving sibling (which only proves .github/ was populated). + await Assert.That(GitCapture(fixture.Source, "ls-files", "-co", "--exclude-standard")) + .Contains(".github/mcp.json"); + + var snapshot = await SnapshotAsync(fixture, ""); + try { + await Assert.That(ExistsInSnapshot(snapshot, ".github/mcp.json")).IsFalse() + .Because("Copilot CLI reads .github/mcp.json; the list carried .github/copilot/mcp.json, " + + "a different path, so this was unprotected at every snapshot root"); + await Assert.That(ExistsInSnapshot(snapshot, ".github/workflows/ci.yml")).IsTrue() + .Because("the exclusion is path-scoped, not a blanket drop of .github/"); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 3. an intermediate directory on the chain ---------- + + [Test] + public async Task Config_in_an_intermediate_ancestor_is_excluded() { + using var fixture = NewFixture(("a/.mcp.json", "{}"), ("a/b/keep.txt", "keep")); + + var snapshot = await SnapshotAsync(fixture, "a/b"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "a/.mcp.json")).IsFalse() + .Because("the whole chain root..cwd is covered, not just its two endpoints"); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + [Test] + public async Task Control_the_same_intermediate_fixture_survives_a_root_cwd_build() { + using var fixture = NewFixture(("a/.mcp.json", "{}"), ("a/b/keep.txt", "keep")); + + var snapshot = await SnapshotAsync(fixture, ""); + try { + await Assert.That(ExistsInSnapshot(snapshot, "a/.mcp.json")).IsTrue(); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 4. a sibling of the cwd is NOT excluded ---------- + + [Test] + public async Task Sibling_of_the_cwd_is_not_excluded() { + using var fixture = NewFixture(("b/.mcp.json", "{}"), ("a/keep.txt", "keep")); + + var snapshot = await SnapshotAsync(fixture, "a"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "b/.mcp.json")).IsTrue() + .Because("no supported vendor discovers config in a sibling of its cwd. Excluding it " + + "anyway would strip content the launch cannot reach — this repository's own " + + "committed kcap/.mcp.json among it"); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 5. case-sensitive sibling AND the collision it used to cause ---------- + + [Test] + public async Task Case_varying_sibling_survives_and_the_build_succeeds_on_a_case_sensitive_volume() { + if (!TempIsCaseSensitive()) { + // Not skipped silently: on a case-insensitive volume `a` and `A` ARE one directory, so the + // property under test does not exist there and asserting it would be meaningless. + await Assert.That(true).IsTrue(); + return; + } + + using var fixture = NewFixture(("a/.mcp.json", "{}"), ("A/.mcp.json", "{}")); + + // Both tracked deliberately. An earlier revision folded ASCII case unconditionally, which + // collapsed these onto ONE canonical candidate and made the review-context collision check + // refuse every launch of the repository — a launch-refusal primitive handed to a hostile branch. + // Tracking only `A` would not reproduce that. + var snapshot = await SnapshotAsync(fixture, "a"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "a/.mcp.json")).IsFalse(); + await Assert.That(ExistsInSnapshot(snapshot, "A/.mcp.json")).IsTrue() + .Because("on a case-sensitive volume A/ is a genuine sibling the vendor cannot discover"); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 7. --show-prefix framing, against real git ---------- + + [Test] + public async Task Show_prefix_bytes_agree_with_the_paths_ls_files_reports() { + using var fixture = NewFixture(("src/cli/keep.txt", "keep")); + var cwd = Path.Combine(fixture.Source, "src", "cli"); + + var prefix = await WorktreeManager.ReadGitRelativeCwdAsync( + fixture.Source, cwd, CancellationToken.None); + + // The oracle is git's OWN listing, not our plan builder — a builder validated against itself + // would pass with an identically wrong derivation. + await Assert.That(GitCapture(fixture.Source, "ls-files", "-co", "--exclude-standard")) + .Contains(prefix + "/keep.txt"); + } + + [Test] + public async Task Show_prefix_at_the_repository_root_is_empty() { + using var fixture = NewFixture(); + + var prefix = await WorktreeManager.ReadGitRelativeCwdAsync( + fixture.Source, fixture.Source, CancellationToken.None); + + await Assert.That(prefix).IsEqualTo(""); + } + + // ---------- 8. root prefix goes through the real command output ---------- + + [Test] + public async Task Root_prefix_expands_to_exactly_the_canonical_list() { + // Through the parser, from the bytes git actually emits at the root ("\n") — not from a + // pre-normalized "", which would bypass the framing rules entirely. + var prefix = WorktreeManager.ParseGitRelativeCwd("\n"u8); + var plan = WorktreeManager.PlanSnapshotExclusions(prefix, caseSensitive: true); + + await Assert.That(plan.VendorConfigPaths.Length) + .IsEqualTo(WorktreeManager.WorkspaceMcpConfigPaths.Length); + foreach (var path in WorktreeManager.WorkspaceMcpConfigPaths) + await Assert.That(plan.VendorConfigPaths).Contains(path); + } + + [Test] + [Arguments("")] // no trailing LF + [Arguments("src/")] // no trailing LF + [Arguments("src\n")] // no trailing slash on a non-root prefix + [Arguments("src/\n\n")] // more than one LF + [Arguments("src/\r\n")] // CR anywhere + [Arguments("/\n")] // slash-only remainder + public async Task Malformed_show_prefix_output_is_refused(string raw) { + await Assert.That(() => WorktreeManager.ParseGitRelativeCwd(Encoding.UTF8.GetBytes(raw))) + .Throws(); + } + + // ---------- 9/10. non-ASCII prefixes, at the pinned parse order ---------- + + [Test] + public async Task Nfc_non_ascii_prefix_is_refused_only_on_a_case_insensitive_destination() { + var prefix = WorktreeManager.ParseGitRelativeCwd("café/\n"u8); + + // Case-sensitive: admitted, compared byte-exactly, no folding involved. + var plan = WorktreeManager.PlanSnapshotExclusions(prefix, caseSensitive: true); + await Assert.That(plan.VendorConfigPaths).Contains("café/.mcp.json"); + + // Case-insensitive: refused, because that volume also equates pairs the ASCII-only matcher + // would miss, and proving the equivalence would mean a second Unicode-aware matcher. + await Assert.That(() => WorktreeManager.PlanSnapshotExclusions(prefix, caseSensitive: false)) + .Throws(); + } + + [Test] + public async Task Nfd_prefix_fails_at_normalization_not_at_the_non_ascii_rule() { + // Normalization (NormalizeRelativePath) runs BEFORE the case-dependent rule, so that rule can + // assume an NFC operand. The consequence is that an NFD prefix never reaches it. + var nfd = "café/\n"; + + await Assert.That(() => WorktreeManager.ParseGitRelativeCwd(Encoding.UTF8.GetBytes(nfd))) + .Throws(); + } + + // ---------- 11. caps at the boundary ---------- + + [Test] + public async Task Depth_at_the_cap_is_admitted_and_one_over_is_refused() { + var atCap = string.Join('/', Enumerable.Range(0, WorktreeManager.MaxCwdDepth).Select(i => "d" + i)); + var overCap = atCap + "/one-too-many"; + + var plan = WorktreeManager.PlanSnapshotExclusions(atCap, caseSensitive: true); + await Assert.That(plan.VendorConfigPaths.Length) + .IsEqualTo((WorktreeManager.MaxCwdDepth + 1) * WorktreeManager.WorkspaceMcpConfigPaths.Length); + + await Assert.That(() => WorktreeManager.PlanSnapshotExclusions(overCap, caseSensitive: true)) + .Throws(); + } + + [Test] + public async Task Aggregate_path_bytes_are_capped_independently_of_depth() { + // Aggregate bytes grow O(depth squared) because each level repeats the whole prefix, so a + // shallow-but-wide cwd can blow the byte budget while passing the depth check. + var wide = string.Join('/', Enumerable.Range(0, 8).Select(_ => new string('x', 250))); + + await Assert.That(() => WorktreeManager.PlanSnapshotExclusions(wide, caseSensitive: true)) + .Throws(); + } + + // ---------- 12/13. reserved index policy ---------- + + [Test] + public async Task Tracked_config_below_the_root_does_not_show_as_a_deletion() { + using var fixture = NewFixture(("src/.mcp.json", "{}"), ("src/keep.txt", "keep")); + + var snapshot = await SnapshotAsync(fixture, "src"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "src/.mcp.json")).IsFalse(); + // The skip-worktree bit is what keeps the reviewer's `git status` clean; without it the + // reviewer sees a deletion kcap performed and can legitimately file a finding about it. + await Assert.That(GitCapture(snapshot.SnapshotRoot!, "status", "--porcelain").Trim()) + .IsEqualTo(""); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + [Test] + public async Task Staged_but_uncommitted_config_does_not_fail_the_build() { + using var fixture = NewFixture(("src/keep.txt", "keep")); + // In the SOURCE index but not in HEAD — so it is absent from the destination's index, which is + // checked out at HEAD. Intersecting the skip-worktree batch against the SOURCE index instead + // would batch this path and fail update-index on a perfectly legitimate snapshot. + Write(fixture.Source, "src/.mcp.json", "{}"); + Git(fixture.Source, "add", "src/.mcp.json"); + + var snapshot = await SnapshotAsync(fixture, "src"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "src/.mcp.json")).IsFalse(); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 14. review context and containment move together ---------- + + [Test] + public async Task Config_excluded_below_the_root_is_still_reachable_to_the_reviewer() { + const string hostile = "{\"mcpServers\":{\"x\":{\"command\":\"/bin/sh\"}}}"; + using var fixture = NewFixture(("src/.kiro/settings/mcp.json", hostile)); + + var snapshot = await SnapshotAsync(fixture, "src"); + try { + await Assert.That(ExistsInSnapshot(snapshot, "src/.kiro/settings/mcp.json")).IsFalse() + .Because("Kiro was measured spawning the declared command at session setup"); + + // Contained is not enough: the change under review may BE this file. If exclusion widened + // without the extractor widening with it, a hostile config one directory down would be + // contained AND invisible — a reviewer could return clean on exactly the change the + // exclusion exists to defend against. + var manifest = Directory.EnumerateFiles( + snapshot.ReviewContextRoot!, "manifest.json", SearchOption.AllDirectories) + .Select(File.ReadAllText) + .Single(); + await Assert.That(manifest).Contains("src/.kiro/settings/mcp.json"); + await Assert.That(manifest).Contains(Convert.ToBase64String(Encoding.UTF8.GetBytes(hostile))); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 15. rooted / escaping cwd ---------- + + [Test] + public async Task Cwd_outside_the_source_is_refused() { + using var fixture = NewFixture(); + var outside = Path.Combine(Path.GetTempPath(), "kcap-outside-" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateDirectory(outside); + try { + await Assert.That(async () => await NewManager(fixture).CreateBorrowedSnapshotAsync( + fixture.Source, outside, null, CancellationToken.None)) + .Throws(); + } finally { + try { Directory.Delete(outside, recursive: true); } catch { } + } + } + + // ---------- 16. symlinked snapshot root resolving inside the source ---------- + + [Test] + public async Task Snapshot_root_symlinked_inside_the_source_is_refused() { + if (OperatingSystem.IsWindows()) { + // Windows needs Developer Mode or elevation to create a symlink. + await Assert.That(true).IsTrue(); + return; + } + + using var fixture = NewFixture(); + var inside = Path.Combine(fixture.Source, "nested-worktrees"); + Directory.CreateDirectory(inside); + var link = Path.Combine(Path.GetTempPath(), "kcap-link-" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateSymbolicLink(link, inside); + try { + // Control: the lexical comparison alone passes this — the link's own path is outside the + // source, which is exactly why resolving is required rather than nice to have. + await Assert.That(link.StartsWith(fixture.Source, StringComparison.Ordinal)).IsFalse(); + + var manager = new WorktreeManager( + new DaemonConfig { WorktreeRoot = link }, NullLogger.Instance); + await Assert.That(async () => await manager.CreateBorrowedSnapshotAsync( + fixture.Source, fixture.Source, null, CancellationToken.None)) + .Throws() + .Because("Claude Code's .mcp.json lookup walks upward past the git root, so a snapshot " + + "under the source would sit beneath the source's own config"); + } finally { + try { Directory.Delete(link); } catch { } + } + } + + // ---------- descendants of a reserved path are index entries too ---------- + + /// A repository CAN track .mcp.json/child — the config pathname as a DIRECTORY — and + /// that child is a real index entry, which is why the index policy marks every non-Unrelated + /// match rather than only Exact. + /// On the BORROWED path this state is unreachable: the review-context extractor refuses a + /// reserved path that is a directory before the index policy is ever reached. That is pre-existing + /// fail-closed behaviour, pinned here so a later change cannot quietly relax it into the very + /// deletion-and-restore case the index fix defends against. + [Test] + public async Task A_reserved_path_tracked_as_a_directory_refuses_a_borrowed_build() { + using var fixture = NewFixture((".mcp.json/child", "{}"), ("keep.txt", "keep")); + + await Assert.That(async () => await SnapshotAsync(fixture, "")) + .Throws(); + } + + /// The same fixture through the path that has NO review context, where the descendant does + /// reach the index policy. Marked skip-worktree, it leaves a clean status; marked only on + /// Exact, it would read as a deletion the reviewer could file a finding about — and an + /// ordinary git operation could restore it, rebuilding a live vendor-config tree. + [Test] + public async Task A_tracked_descendant_of_a_reserved_path_does_not_show_as_a_deletion() { + using var fixture = NewFixture(("keep.txt", "keep")); + var manager = NewManager(fixture); + + // The target is an existing snapshot — SyncFromSourceAsync replaces contents, it does not create + // the tree. Built before the descendant exists, so the sync below is the operation under test. + var snapshot = await SnapshotAsync(fixture, ""); + try { + var target = snapshot.SnapshotRoot!; + + Write(fixture.Source, ".mcp.json/child", "{}"); + Git(fixture.Source, "add", "-A"); + Git(fixture.Source, "commit", "-q", "-m", "config as a directory"); + + await manager.SyncFromSourceAsync( + fixture.Source, fixture.Source, target, [], CancellationToken.None); + + await Assert.That(File.Exists(Path.Combine(target, ".mcp.json", "child"))).IsFalse(); + await Assert.That(GitCapture(target, "status", "--porcelain").Trim()).IsEqualTo(""); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- the prefix must belong to the repository whose manifest it filters ---------- + + [Test] + public async Task A_cwd_in_a_nested_repository_is_refused() { + using var fixture = NewFixture(("keep.txt", "keep")); + // A nested repository inside the source tree. `rev-parse` run there reports the NESTED repo's + // work-tree top, so its prefix is in a different namespace from the source's ls-files output — + // and matching one against the other is exactly the invariant this derivation exists to hold. + var nested = Path.Combine(fixture.Source, "vendored"); + Directory.CreateDirectory(nested); + Git(nested, "init", "-q"); + + await Assert.That(async () => await NewManager(fixture).CreateBorrowedSnapshotAsync( + fixture.Source, nested, null, CancellationToken.None)) + .Throws(); + } + + // ---------- snapshot root reaching the source through an ANCESTOR link ---------- + + [Test] + public async Task Snapshot_root_reaching_the_source_through_an_ancestor_symlink_is_refused() { + if (OperatingSystem.IsWindows()) { + await Assert.That(true).IsTrue(); + return; + } + + using var fixture = NewFixture(); + // The link is an ANCESTOR of the configured root, and the configured root's own deepest existing + // component is an ordinary directory. Resolving only that deepest component returns the lexical + // path and the containment check passes — which is the bug this covers. + var inside = Path.Combine(fixture.Source, "nested", "existing"); + Directory.CreateDirectory(inside); + var link = Path.Combine(Path.GetTempPath(), "kcap-anc-" + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateSymbolicLink(link, Path.Combine(fixture.Source, "nested")); + try { + var manager = new WorktreeManager( + new DaemonConfig { WorktreeRoot = Path.Combine(link, "existing") }, + NullLogger.Instance); + await Assert.That(async () => await manager.CreateBorrowedSnapshotAsync( + fixture.Source, fixture.Source, null, CancellationToken.None)) + .Throws(); + } finally { + try { Directory.Delete(link); } catch { } + } + } + + // ---------- 17. the non-borrowed sync path ---------- + + [Test] + public async Task Non_borrowed_sync_excludes_ancestor_config_for_its_source_cwd() { + using var fixture = NewFixture(("src/.mcp.json", "{}"), ("src/keep.txt", "keep")); + var manager = NewManager(fixture); + + var snapshot = await SnapshotAsync(fixture, "src"); + try { + // Re-sync through the public overload, which now REQUIRES a source-side cwd. The overloads + // it replaced took only a target-side path, leaving no way to obtain the git prefix except + // by re-deriving it from the target filesystem. + await manager.SyncFromSourceAsync( + fixture.Source, Path.Combine(fixture.Source, "src"), + snapshot.SnapshotRoot!, [], CancellationToken.None); + + await Assert.That(ExistsInSnapshot(snapshot, "src/.mcp.json")).IsFalse(); + await Assert.That(ExistsInSnapshot(snapshot, "src/keep.txt")).IsTrue(); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } + + // ---------- 18. the refresh path carries the persisted prefix ---------- + + [Test] + public async Task Refresh_reuses_the_persisted_prefix_and_keeps_ancestor_config_out() { + using var fixture = NewFixture(("src/keep.txt", "keep")); + var manager = NewManager(fixture); + + var snapshot = await SnapshotAsync(fixture, "src"); + try { + await Assert.That(snapshot.GitRelativeCwd).IsEqualTo("src"); + + // A config appearing between rounds is the case that matters: the refresh must exclude it + // using the prefix computed at CREATION, since it has only a target-side path of its own. + Write(fixture.Source, "src/.mcp.json", "{}"); + Write(fixture.Source, ".mcp.json", "{}"); + Git(fixture.Source, "add", "-A"); + Git(fixture.Source, "commit", "-q", "-m", "adds config"); + + await manager.SyncBorrowedSnapshotFromSourceAsync( + fixture.Source, snapshot.SnapshotRoot!, snapshot.GitRelativeCwd!, + [], snapshot.ReviewContextRoot!, CancellationToken.None); + + await Assert.That(ExistsInSnapshot(snapshot, "src/.mcp.json")).IsFalse(); + await Assert.That(ExistsInSnapshot(snapshot, ".mcp.json")).IsFalse(); + await Assert.That(ExistsInSnapshot(snapshot, "src/keep.txt")).IsTrue(); + } finally { await WorktreeManager.RemoveAsync(snapshot); } + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryLiveTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryLiveTests.cs index 4f78d0784..a07b7dc41 100644 --- a/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryLiveTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryLiveTests.cs @@ -266,7 +266,7 @@ await Assert.That(submitted).Contains(UntrackedSentinel) await runtime.WaitForTurnIdleAsync(startCts.Token); File.WriteAllText(protectedPath, "ROUND2\n"); await manager.SyncFromSourceAsync( - sourceDir.FullName, snapshot.Path, [], startCts.Token); + sourceDir.FullName, sourceDir.FullName, snapshot.Path, [], startCts.Token); File.Delete(markerPath); await runtime.SendUserInputAndWaitForWriteAsync( "Read protected.txt and call submit_review_result exactly once with verdict CLEAN and put its exact contents in summary. Do not modify files."); diff --git a/test/Capacitor.Cli.Tests.Unit/WorkspaceMcpNeutralizationTests.cs b/test/Capacitor.Cli.Tests.Unit/WorkspaceMcpNeutralizationTests.cs index e4f861993..bb0cac452 100644 --- a/test/Capacitor.Cli.Tests.Unit/WorkspaceMcpNeutralizationTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/WorkspaceMcpNeutralizationTests.cs @@ -60,7 +60,8 @@ await Assert.That(File.Exists(Path.Combine(wt, relative.Replace('/', Path.Direct public async Task Every_hosted_vendors_workspace_file_is_covered() { foreach (var expected in new[] { ".mcp.json", ".cursor/mcp.json", ".gemini/settings.json", ".kiro/settings/mcp.json", - ".vscode/mcp.json", ".github/copilot/mcp.json", ".copilot/mcp.json", + ".vscode/mcp.json", ".github/mcp.json", ".github/copilot/mcp.json", + ".copilot/mcp.json", ".copilot/mcp-config.json", ".codex/config.toml" }) await Assert.That(WorktreeManager.WorkspaceMcpConfigPaths).Contains(expected); } @@ -362,14 +363,30 @@ await Assert.That(File.Exists(marker)) /// [Test] public async Task Borrowed_snapshots_exclude_every_workspace_mcp_config_path() { - var excluded = WorktreeManager.SnapshotExcludedPaths; + var plan = WorktreeManager.PlanSnapshotExclusions("", caseSensitive: true); foreach (var path in WorktreeManager.WorkspaceMcpConfigPaths) - await Assert.That(excluded).Contains(path); + await Assert.That(plan.VendorConfigPaths).Contains(path); - // The pre-existing entries must survive the fold-in. - await Assert.That(excluded).Contains(".capacitor"); - await Assert.That(excluded).Contains(".attached"); + // At the repository root the expansion must be EXACTLY the canonical list — the overwhelmingly + // common launch shape, and the no-regression claim for it. + await Assert.That(plan.VendorConfigPaths.Length) + .IsEqualTo(WorktreeManager.WorkspaceMcpConfigPaths.Length); + + // The pre-existing entries must survive. They live in SnapshotExclusions, not VendorConfigPaths: + // vendor paths go exclusively through the shared byte classifier, these two do not. + await Assert.That(plan.SnapshotExclusions).Contains(".capacitor"); + await Assert.That(plan.SnapshotExclusions).Contains(".attached"); + } + + /// The two most recently added paths, called out separately from the membership sweep above + /// because each is a distinct defect: .github/mcp.json is a Copilot discovery path the list + /// simply never had (it carried .github/copilot/mcp.json, a different file), and it was + /// unprotected at the root of every borrowed snapshot regardless of cwd scope. + [Test] + public async Task Canonical_list_covers_the_copilot_paths_that_were_missing() { + await Assert.That(WorktreeManager.WorkspaceMcpConfigPaths).Contains(".github/mcp.json"); + await Assert.That(WorktreeManager.WorkspaceMcpConfigPaths).Contains(".copilot/mcp-config.json"); } /// Windows needs Developer Mode or elevation to create a symlink, so these assert POSIX diff --git a/test/Capacitor.Cli.Tests.Unit/WorktreeManagerTests.cs b/test/Capacitor.Cli.Tests.Unit/WorktreeManagerTests.cs index a44c5c58e..69d78b99b 100644 --- a/test/Capacitor.Cli.Tests.Unit/WorktreeManagerTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/WorktreeManagerTests.cs @@ -263,7 +263,7 @@ public async Task BorrowedSnapshot_IsIndependent_CopiesDirtyContext_AndRefreshes File.WriteAllText(Path.Combine(snapshot.Path, "reviewer-created.txt"), "must disappear"); File.WriteAllText(Path.Combine(snapshot.Path, ".git", "reviewer-metadata"), "must disappear"); File.WriteAllText(Path.Combine(clone, "untracked.txt"), "two"); - await manager.SyncFromSourceAsync(clone, snapshot.Path, [], CancellationToken.None); + await manager.SyncFromSourceAsync(clone, clone, snapshot.Path, [], CancellationToken.None); await Assert.That(File.Exists(Path.Combine(snapshot.Path, "reviewer-created.txt"))).IsFalse(); await Assert.That(File.Exists(Path.Combine(snapshot.Path, ".git", "reviewer-metadata"))).IsFalse(); @@ -313,7 +313,7 @@ public async Task BorrowedSnapshot_RefreshPreservesRunningExecutionDirectory() { File.WriteAllText(Path.Combine(sourceCwd, "round.txt"), "two"); File.WriteAllText(Path.Combine(snapshot.Path, "reviewer-created.txt"), "remove"); await manager.SyncFromSourceAsync( - clone, snapshot.SnapshotRoot!, snapshot.Path, [], CancellationToken.None); + clone, sourceCwd, snapshot.SnapshotRoot!, [], CancellationToken.None); await Assert.That(holder!.HasExited).IsFalse(); await Assert.That(Directory.Exists(snapshot.Path)).IsTrue();