From 1ec3bf0b6cd43bc803e7f341b28731251b2c026e Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 09:50:10 -0400 Subject: [PATCH 01/12] docs: spec for vendor MCP config discovery-scope exclusion Design for making the borrowed-snapshot exclusion follow vendor discovery (the execution cwd's ancestor chain) rather than the repository root, and adding .github/mcp.json to the canonical path list. Co-Authored-By: Claude Opus 5 --- ...03-vendor-config-discovery-scope-design.md | 193 ++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-04-ai1703-vendor-config-discovery-scope-design.md 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..1d3a374f4 --- /dev/null +++ b/docs/superpowers/specs/2026-08-04-ai1703-vendor-config-discovery-scope-design.md @@ -0,0 +1,193 @@ +# 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). + +## The defect + +`WorktreeManager.WorkspaceMcpConfigPaths` is a list of **root-relative** paths. Every consumer +matches it against a path that is relative to the repository root: + +- `IsUnderExcluded(rel, exclusions, caseSensitive)` — the borrowed-snapshot manifest filter + (`WorktreeManager.cs`), a plain `rel == prefix || rel.StartsWith(prefix + "/")`; +- `NeutralizeWorkspaceMcpConfig(worktreePath)` — the owned-worktree strip, which walks the + *components of each relative path* from the worktree root; +- `ApplyReservedIndexPolicyAsync` — the `skip-worktree` marking; +- `ExtractReviewContextEntriesAsync` — the AI-1706 reserved-path classifier. + +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. + +Both vendor claims in the issue were re-verified against vendor documentation rather than accepted: + +- **Codex** loads project config from `.codex/config.toml` "ordered from the project root down to + your current working directory (closest wins; trusted projects only)". A worktree under the repo's + own `.capacitor/` inherits the repo's trust by design (that is why worktrees are placed there), so + the trust gate does not save us. +- **Copilot CLI** walks "from your current working directory up to the repository root" loading + both `.mcp.json` and `.github/mcp.json`, with `.mcp.json` winning in the same directory. + `.github/mcp.json` is **not on our list at all** — the list has `.github/copilot/mcp.json`, a + different path. GitHub's docs also state Copilot CLI does *not* read `.vscode/mcp.json`; that entry + stays, because VS Code and other CLIs do, and the list is deliberately wider than the set of + vendors known to read each file. + +Note that the two vendors walk the chain in opposite directions. That is the same set of +directories either way: **the ancestor chain of the execution cwd, from the snapshot root down to +the cwd, inclusive.** + +## 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**, and the fix must not be designed around it. +`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 a reason unrelated to the walk: every owned-worktree +launch runs at the worktree root. `CreateAsync` and `BuildStandaloneSnapshotAsync` return a +`WorktreeInfo` whose `Path` is the worktree root, and no caller narrows it. With cwd == root, the +ancestor chain is `[root]` and root-scoped matching is complete. The owned path is therefore left +alone here, and this document records why, so the next reader does not re-derive it. + +The direct-borrow path (`WorktreeInfo.Borrowed(cwd)`, non-snapshot) is out of scope by construction: +it is the user's own checkout, guarded by a certified read-only runtime boundary, and nothing is +stripped there today. + +## Design + +### One derivation, several consumers + +The existing code has a load-bearing comment: "Two lists of the same thing is how that happened" — +the `.kiro/settings/mcp.json` escape came from `SnapshotExcludedPaths` and `WorkspaceMcpConfigPaths` +being maintained separately. Widening the model must not reintroduce a second list. + +Introduce a single value computed once per snapshot build: + +```csharp +internal sealed record SnapshotExclusionPlan( + ImmutableArray VendorConfigPaths, // expanded along the cwd ancestor chain + string[] SnapshotExclusions); // .capacitor, .attached, vendor paths, caller extras + +internal static SnapshotExclusionPlan PlanSnapshotExclusions( + string relativeCwd, IEnumerable? additional = null); +``` + +`VendorConfigPaths` is the cross product of `WorkspaceMcpConfigPaths` with the ancestor chain of +`relativeCwd`: for `relativeCwd = "src/cli"` and the canonical entry `.mcp.json`, it yields +`.mcp.json`, `src/.mcp.json`, `src/cli/.mcp.json`. For `relativeCwd = "."` it is exactly today's +list, so the overwhelmingly common root-cwd launch is byte-for-byte unchanged. + +`SnapshotExclusions` is `[".capacitor", ".attached", ..VendorConfigPaths, ..additional]`, replacing +the `SnapshotExcludedPaths` static property. The static property is removed rather than left beside +the new one — leaving it is precisely the two-lists shape that caused the original escape. + +`BuildIndependentSnapshotAsync` / `BuildIndependentSnapshotOnceAsync` take the plan instead of a +`string[] exclusions`, so the manifest filter, the reserved index policy and the review-context +extractor are all driven from one object that a single call site built. + +### Consumers + +1. **Manifest filter** (`ReadSourceManifestAsync` → `IsUnderExcluded`): pass + `plan.SnapshotExclusions`. No change to the matching logic; only the input widens. The + `.attached` / `.capacitor` reserved-path throw stays keyed on those two names, unchanged. + +2. **Reserved index policy** (`ApplyReservedIndexPolicyAsync`): mark `plan.VendorConfigPaths` + `skip-worktree`, for the same reason the current code marks the canonical list — a tracked + `src/.mcp.json` absent from the snapshot would otherwise show as a deletion in the reviewer's + `git status` and diff, and could produce a review finding about a deletion kcap performed. + Today this issues one `git update-index` per path inside a `try`/`catch` for "absent from the + index". The expanded list is `paths × depth`, so instead intersect with the index listing already + read at the top of `BuildIndependentSnapshotOnceAsync` (`ls-files --stage -z`) and issue a single + batched `update-index --skip-worktree --` call for the paths actually present. This removes the + catch-swallow as well: with membership established from the listing, a failure is a real failure + and should propagate. + +3. **Review context** (`ExtractReviewContextEntriesAsync`, AI-1706): build `reserved` from + `plan.VendorConfigPaths`, not from `WorkspaceMcpConfigPaths`. Without this the fix would *widen* + the AI-1680 blind spot it is adjacent to: `src/.mcp.json` would become excluded from the snapshot + (good) while remaining invisible to the reviewer (bad) — a hostile config one directory down, + contained but unreviewable. The two must move together. + + `ValidateReviewContextManifest`'s cap `manifest.Entries.Length > WorkspaceMcpConfigPaths.Length` + becomes `> plan.VendorConfigPaths.Length`, threaded to the validator. The invariant it encodes — + at most one entry per reserved path — is preserved, over the expanded set. + `matchedCanonicalPaths` already keys on the matched canonical path, which is now the concrete + expanded path, so the collision check keeps its meaning without change. + + `MaxReviewContextBytes` (256 KiB total) is unchanged and is the real bound on this surface. A + deep cwd multiplies the number of *candidate* paths, not the bytes admitted. + +### Deriving `relativeCwd` + +`CreateBorrowedSnapshotAsync` already computes `relativeCwd` and already rejects an escape +(`borrowed_snapshot_cwd_outside_source`). `SyncFromSourceCoreAsync` receives `executionPath` and +already validates containment in `target`; it derives its own relative form the same way. + +Both must produce the same normalized shape (`/` separators, `.` for the root, no trailing slash, +no `.` or `..` components) or the two build paths would exclude different sets and a per-round +refresh would reintroduce a file the initial build excluded. One private normalizer, used by both, +with a test asserting the initial build and the refresh produce identical plans for the same cwd. + +Depth is bounded by the existing containment checks; no separate cap is introduced, and the batched +`update-index` means depth no longer costs a process spawn per path. + +### The `.github/mcp.json` addition + +Added to `WorkspaceMcpConfigPaths`. Independent of the scope change and live even at cwd == root: +`.github/mcp.json` is unprotected today at the repository root of every borrowed snapshot. + +`.copilot/mcp-config.json` is added alongside the existing `.copilot/mcp.json`. GitHub documents +`~/.copilot/mcp-config.json` as user-scope, so the workspace-relative form is not a documented +discovery path; it is added under the same standing rationale as the rest of the list — the entry +costs nothing and the list exists so the next vendor is safe before anyone thinks about it. The +existing `.copilot/mcp.json` entry is kept for the same reason. + +## Testing + +Every test states which discovery shape it defends and carries a **positive control** proving the +file would otherwise be present — the DoD requires it, and this codebase has been bitten by +containment tests that passed because the fixture never produced the file at all. + +1. **Codex sub-cwd** — source repo with `src/.codex/config.toml`; borrowed snapshot with + `requestedCwd = /src`; assert absent from the snapshot. Control: the same build with + `requestedCwd = ` leaves `src/.codex/config.toml` present, proving the fixture writes a + real file and that the assertion is about the cwd chain rather than about the file never existing. +2. **Copilot `.github/mcp.json` at root** — present in source, absent from the snapshot. Control: a + sibling `.github/unrelated.json` survives, proving the exclusion is path-scoped and the fixture + populates `.github/`. +3. **Intermediate directory** — `relativeCwd = "a/b"`, config at `a/.mcp.json`; excluded. Asserts the + whole chain is covered, not just the two endpoints. +4. **Sibling not excluded** — `relativeCwd = "a"`, config at `b/.mcp.json`; **present**. This is the + test that keeps the rule from silently becoming "every directory in the tree", which would strip + this repository's own committed `kcap/.mcp.json` from every snapshot. +5. **Root cwd unchanged** — for `relativeCwd = "."`, `plan.VendorConfigPaths` equals + `WorkspaceMcpConfigPaths` exactly. Pins the no-regression claim for the common launch. +6. **Review-context parity** — a tracked hostile `src/.kiro/settings/mcp.json` with + `requestedCwd = /src`: absent from the snapshot *and* present in the review-context + manifest with its exact path. This is the test that keeps containment and reviewability moving + together; the AI-1706 tests only cover the root case. +7. **Refresh parity** — initial build and `SyncBorrowedSnapshotFromSourceAsync` for the same cwd + produce identical exclusion plans. +8. **Reserved index policy** — a tracked `src/.mcp.json` under `relativeCwd = "src"` does not appear + as a deletion in the snapshot's `git status`. +9. **List membership** — the existing `WorkspaceMcpNeutralizationTests` membership assertion is + extended to require `.github/mcp.json` and `.copilot/mcp-config.json`. + +The AI-1632 live certification (`KCAP_WORKSPACE_MCP_CERT=1`) is re-run unchanged, control included: +this widens the excluded set and must not disturb the measured spawn behaviour at the root. + +## Out of scope + +- The owned-worktree strip, for the reason recorded above (cwd is always the worktree root). If a + sub-cwd owned launch is ever added, `NeutralizeWorkspaceMcpConfig` gains the same chain and this + document is the reason it must. +- Re-checking vendor discovery beyond Codex and Copilot. Kiro, Gemini and Cursor were measured + root-scoped during AI-1632 and the ancestor-chain rule strictly widens their coverage; nothing + here narrows any vendor. +- AI-1675 (`CopyDirectory` recursion / symlink dereference). Untouched, and repairing it here would + arm the exfiltration that issue describes. From 0a1e9047184988ea1aab6fb7996f304ca92cbffc Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:02:34 -0400 Subject: [PATCH 02/12] docs: spec revision 2 after round 1 of spec review Derives the cwd prefix from git's own path bytes (rev-parse --show-prefix) rather than the filesystem, classifies through one byte-level matcher shared with the review-context extractor, corrects the reserved-index intersection to the destination index, adds a vendor discovery matrix and explicit caps. Co-Authored-By: Claude Opus 5 --- ...03-vendor-config-discovery-scope-design.md | 383 ++++++++++++------ 1 file changed, 252 insertions(+), 131 deletions(-) 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 index 1d3a374f4..7068df2b7 100644 --- 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 @@ -3,17 +3,21 @@ 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 2 — rewritten after round 1 of spec review. The load-bearing change from revision 1 is +that the cwd prefix is now derived from **git's own path bytes**, not from the filesystem, and that +one byte-level classifier serves both the exclusion filter and the review-context extractor. + ## The defect `WorktreeManager.WorkspaceMcpConfigPaths` is a list of **root-relative** paths. Every consumer -matches it against a path that is relative to the repository root: +matches it against a path relative to the repository root: -- `IsUnderExcluded(rel, exclusions, caseSensitive)` — the borrowed-snapshot manifest filter - (`WorktreeManager.cs`), a plain `rel == prefix || rel.StartsWith(prefix + "/")`; -- `NeutralizeWorkspaceMcpConfig(worktreePath)` — the owned-worktree strip, which walks the - *components of each relative path* from the worktree root; +- `IsUnderExcluded(rel, exclusions, caseSensitive)` — the borrowed-snapshot manifest filter, a plain + `rel == prefix || rel.StartsWith(prefix + "/")` over a decoded string; +- `NeutralizeWorkspaceMcpConfig(worktreePath)` — the owned-worktree strip; - `ApplyReservedIndexPolicyAsync` — the `skip-worktree` marking; -- `ExtractReviewContextEntriesAsync` — the AI-1706 reserved-path classifier. +- `ExtractReviewContextEntriesAsync` → `ClassifyReservedPath` — the AI-1706 reserved-path classifier, + over raw bytes. A borrowed snapshot can execute in a directory **below** the repository root. `CreateBorrowedSnapshotAsync(sourceRepoRoot, requestedCwd, …)` takes the two independently, and @@ -23,22 +27,64 @@ So a review flow started from `/src` produces a snapshot whose cwd is ` VendorConfigPaths, // expanded along the cwd ancestor chain - string[] SnapshotExclusions); // .capacitor, .attached, vendor paths, caller extras - -internal static SnapshotExclusionPlan PlanSnapshotExclusions( - string relativeCwd, IEnumerable? additional = null); -``` - -`VendorConfigPaths` is the cross product of `WorkspaceMcpConfigPaths` with the ancestor chain of -`relativeCwd`: for `relativeCwd = "src/cli"` and the canonical entry `.mcp.json`, it yields -`.mcp.json`, `src/.mcp.json`, `src/cli/.mcp.json`. For `relativeCwd = "."` it is exactly today's -list, so the overwhelmingly common root-cwd launch is byte-for-byte unchanged. +### The pathname-namespace rule -`SnapshotExclusions` is `[".capacitor", ".attached", ..VendorConfigPaths, ..additional]`, replacing -the `SnapshotExcludedPaths` static property. The static property is removed rather than left beside -the new one — leaving it is precisely the two-lists shape that caused the original escape. +Round 1's first finding is the one that reshapes this design: **a prefix derived from the filesystem +is not in the same namespace as the paths git reports**, and concatenating one onto the other +produces a comparison that can silently fail to match. Three concrete divergences were named, all +real: -`BuildIndependentSnapshotAsync` / `BuildIndependentSnapshotOnceAsync` take the plan instead of a -`string[] exclusions`, so the manifest filter, the reserved index policy and the review-context -extractor are all driven from one object that a single call site built. +1. **Unicode normalization.** On macOS a directory created as NFC is reported by the filesystem as + NFD. `NormalizeRelativePath` already rejects any git path that is not NFC — so the *git* side is + guaranteed NFC or the build fails closed — but nothing normalizes the *prefix*. An NFD prefix + against an NFC git path under-excludes. +2. **Case sensitivity read from the wrong volume.** `caseSensitive` is probed with + `ProbeCaseSensitiveFileSystem(destination)` and then applied to paths resolved against the + *source*. A case-insensitive source plus a case-sensitive snapshot volume yields `SRC` from the + requested cwd and `src/.mcp.json` from git, and no match. +3. **Rooted relative results.** `Path.GetRelativePath` returns a *rooted* path when the two paths are + on different Windows volumes. The existing guard tests only for `".."` and a `"../"` prefix, so + revision 1's claim that escape "is already rejected" was false. -### Consumers +The fix is to stop deriving the prefix from the filesystem at all. -1. **Manifest filter** (`ReadSourceManifestAsync` → `IsUnderExcluded`): pass - `plan.SnapshotExclusions`. No change to the matching logic; only the input widens. The - `.attached` / `.capacitor` reserved-path throw stays keyed on those two names, unchanged. +**Derive the prefix from git.** Run, in the source repository, with the process cwd set to the +requested cwd: -2. **Reserved index policy** (`ApplyReservedIndexPolicyAsync`): mark `plan.VendorConfigPaths` - `skip-worktree`, for the same reason the current code marks the canonical list — a tracked - `src/.mcp.json` absent from the snapshot would otherwise show as a deletion in the reviewer's - `git status` and diff, and could produce a review finding about a deletion kcap performed. - Today this issues one `git update-index` per path inside a `try`/`catch` for "absent from the - index". The expanded list is `paths × depth`, so instead intersect with the index listing already - read at the top of `BuildIndependentSnapshotOnceAsync` (`ls-files --stage -z`) and issue a single - batched `update-index --skip-worktree --` call for the paths actually present. This removes the - catch-swallow as well: with membership established from the listing, a failure is a real failure - and should propagate. - -3. **Review context** (`ExtractReviewContextEntriesAsync`, AI-1706): build `reserved` from - `plan.VendorConfigPaths`, not from `WorkspaceMcpConfigPaths`. Without this the fix would *widen* - the AI-1680 blind spot it is adjacent to: `src/.mcp.json` would become excluded from the snapshot - (good) while remaining invisible to the reviewer (bad) — a hostile config one directory down, - contained but unreviewable. The two must move together. - - `ValidateReviewContextManifest`'s cap `manifest.Entries.Length > WorkspaceMcpConfigPaths.Length` - becomes `> plan.VendorConfigPaths.Length`, threaded to the validator. The invariant it encodes — - at most one entry per reserved path — is preserved, over the expanded set. - `matchedCanonicalPaths` already keys on the matched canonical path, which is now the concrete - expanded path, so the collision check keeps its meaning without change. - - `MaxReviewContextBytes` (256 KiB total) is unchanged and is the real bound on this surface. A - deep cwd multiplies the number of *candidate* paths, not the bytes admitted. - -### Deriving `relativeCwd` - -`CreateBorrowedSnapshotAsync` already computes `relativeCwd` and already rejects an escape -(`borrowed_snapshot_cwd_outside_source`). `SyncFromSourceCoreAsync` receives `executionPath` and -already validates containment in `target`; it derives its own relative form the same way. - -Both must produce the same normalized shape (`/` separators, `.` for the root, no trailing slash, -no `.` or `..` components) or the two build paths would exclude different sets and a per-round -refresh would reintroduce a file the initial build excluded. One private normalizer, used by both, -with a test asserting the initial build and the refresh produce identical plans for the same cwd. +``` +git -c core.quotePath=false rev-parse --show-prefix +``` -Depth is bounded by the existing containment checks; no separate cap is introduced, and the batched -`update-index` means depth no longer costs a process spawn per path. +and read the raw bytes. This returns the cwd's path relative to the work-tree top **in git's own +spelling and byte representation** — the same namespace as `ls-files` output — or empty for the +root. It removes the separator, rooted-path and `..` classes outright, because the value never +passes through `Path.GetRelativePath`. `core.quotePath=false` is required or non-ASCII components +come back C-quoted. + +The result is then put through the existing `NormalizeRelativePath` (after strict UTF-8 decoding), +so a prefix carrying `\`, CR, LF, a `.git` component, or non-NFC bytes fails the build closed +exactly as a manifest path would. The existing filesystem-derived `relativeCwd` is kept **only** for +`ContainedPath(final, relativeCwd)` and `Directory.Exists(executionPath)`, which are filesystem +operations and belong in filesystem terms — and its rooted-path gap is fixed there independently +(`Path.IsPathRooted` rejected alongside the `..` checks), because that check guards a path escape +regardless of this feature. + +**Compare prefixes permissively, and say which way the error goes.** Even with a git-derived prefix, +an exact byte comparison is not obviously right: git's index spelling and git's cwd resolution can +still disagree on a case-insensitive volume. So the directory-prefix portion of a match is compared +with ASCII case folding applied *unconditionally*, independent of the probed `caseSensitive`, and +with both sides already NFC by construction. + +This is deliberately over-broad, and the error direction is the argument: an over-broad prefix +excludes a vendor config file at a *differently-cased sibling directory* — content that is excluded +anyway under any spelling, and that the review-context server still surfaces. An under-broad prefix +leaves a hostile config live in the tree. Over-exclusion is a non-event; under-exclusion is the +vulnerability this issue exists to close. + +### One classifier, not two + +Round 1's fifth finding: passing one array to two matchers does not make them agree. +`IsUnderExcluded` compares decoded strings with `StringComparison.OrdinalIgnoreCase` (full Unicode +case folding); `ClassifyReservedPath` compares raw bytes with `AsciiPathEquals` (ASCII folding only). +With a purely ASCII canonical list the difference was unobservable. A cwd prefix can contain +non-ASCII, so it becomes observable — a path could be excluded by one and `Unrelated` to the other, +which is precisely the "contained but not reviewable" state this change must not create. + +So the expansion is classified **once**, over raw bytes, by `ClassifyReservedPath`, and both +consumers read that one result: -### The `.github/mcp.json` addition +```csharp +internal sealed record SnapshotExclusionPlan( + ImmutableArray VendorConfigPaths, // canonical list × the git-derived ancestor chain + ImmutableArray VendorConfigPathBytes, + string[] SnapshotExclusions); // .capacitor, .attached, vendor paths, caller extras -Added to `WorkspaceMcpConfigPaths`. Independent of the scope change and live even at cwd == root: -`.github/mcp.json` is unprotected today at the repository root of every borrowed snapshot. +internal static SnapshotExclusionPlan PlanSnapshotExclusions( + string gitRelativeCwd, IEnumerable? additional = null); +``` -`.copilot/mcp-config.json` is added alongside the existing `.copilot/mcp.json`. GitHub documents -`~/.copilot/mcp-config.json` as user-scope, so the workspace-relative form is not a documented -discovery path; it is added under the same standing rationale as the rest of the list — the entry -costs nothing and the list exists so the next vendor is safe before anyone thinks about it. The -existing `.copilot/mcp.json` entry is kept for the same reason. +`ReadSourceManifestAsync` already iterates raw records and calls `NormalizeRelativePath` on the +decoded form; it gains a `ClassifyReservedPath` call on the raw bytes *before* decoding, and treats +`Exact` and `Descendant` as excluded. `IsUnderExcluded` is retained only for `.capacitor`, +`.attached` and caller-supplied `excludePaths`, which are ASCII constants and daemon-supplied — the +namespace question does not arise for them. The vendor list no longer flows through it. + +A test asserts the invariant directly: for a corpus of paths spanning case and normalization +variants, `excluded(path) == (ClassifyReservedPath(path) != Unrelated)` for every vendor path in the +plan. That is the lockstep property, stated as an equivalence rather than as "we passed the same +array to both". + +The `SnapshotExcludedPaths` static property is **removed**, not left beside the new plan. Leaving it +is precisely the two-lists shape whose comment in the code today reads "Two lists of the same thing +is how that happened". + +### What "reviewable" actually means — narrowed, not claimed + +Round 1 is right that containment and reviewability range over different data. Containment operates +on the working tree (`ls-files -co --exclude-standard`); review context contains **index stage-0 +blobs only**, and the manifest says so in a field (`UnstagedAndUntrackedOmitted: true`). So an +*untracked* reserved config, or unstaged working-tree bytes of a tracked one, is contained but not +reviewable. + +That is a pre-existing property of AI-1706, not something this change introduces, and it is not +silently inherited: the reviewer is told, by that manifest field, which bytes it is looking at. This +design states the narrowing explicitly and does not widen it — carrying untracked working-tree bytes +into review context would reintroduce the AI-1680 failure that killed the previous attempt, where +a developer's `skip-worktree` local override would have been published to the reviewer's model. + +The lockstep property this design does claim is therefore precise: **for tracked stage-0 content, +every path excluded from the snapshot is classified as reserved by the extractor.** + +### Ordering + +`CreateReviewContextGenerationAsync` runs before `ReadSourceManifestAsync` in +`BuildIndependentSnapshotOnceAsync`. That is safe here because the plan is computed once, before +either, and is immutable; both read the same `VendorConfigPathBytes`. Both also read the same +`initialIndex` / source, and the existing end-of-build re-check (`sourceHead`, `initialIndex`, +`ManifestsEqual`) still fails the whole build with `SourceChangedException` if the source moved +underneath. Failed generations are deleted on every throw path already. + +### Reserved index policy + +Round 1's third finding is a real bug in revision 1: `initialIndex` is read from **source**, while +`update-index` runs in **destination**. The destination is a fresh clone checked out at `HEAD`, so a +path that is staged-but-not-committed in the source is in the source index and *not* in the +destination index. Batching it in would make `update-index --skip-worktree` fail on a legitimate +snapshot — and revision 1 also proposed promoting that failure from a swallowed catch to a hard +error, which together would refuse the launch. + +Corrected: read the **destination** index after checkout (`git -C destination ls-files -z`), +intersect the plan's vendor paths with that, and mark only paths proven present. With membership +established, a failure is a real failure and propagates. The source-side consistency re-check at the +end of the build is unchanged and still catches a source that moved. + +Round 1's fourth finding bounds the mechanism: + +- The batch is fed on **stdin**: `git update-index --skip-worktree -z --stdin`. This removes the + `ARG_MAX` ceiling for a deep cwd and removes pathspec interpretation of a leading `:` or `-` in an + ancestor directory name — `--stdin` paths are literal. +- Aggregate pathname bytes are `O(depth²)`, not `O(depth)`. An explicit cap on the plan's candidate + count and on its aggregate path bytes is added, rejected at plan construction with a coded error + (`borrowed_snapshot_cwd_too_deep`), so the bound is stated rather than inherited from `ARG_MAX`. +- `MaxReviewContextBytes` charges only blob content. The serialized manifest is bounded separately — + path strings, base64 expansion and JSON overhead are not free — with its own cap and coded error. + Revision 1's claim that the content cap was "the real bound" was wrong. + +### Deriving the prefix on both build paths + +`CreateBorrowedSnapshotAsync` and `SyncFromSourceCoreAsync` both compute the git-relative cwd the +same way, through one private helper, from the same `git rev-parse --show-prefix` primitive. If they +diverged, a per-round refresh would reintroduce a file the initial build excluded. + +That is asserted at the security boundary, not at the helper — see test 7. ## Testing -Every test states which discovery shape it defends and carries a **positive control** proving the -file would otherwise be present — the DoD requires it, and this codebase has been bitten by -containment tests that passed because the fixture never produced the file at all. +Every test names the discovery shape it defends and carries a **positive control** proving the file +would otherwise be present. Round 1 found four of revision 1's tests could pass vacuously; those are +rewritten here. -1. **Codex sub-cwd** — source repo with `src/.codex/config.toml`; borrowed snapshot with +1. **Codex sub-cwd** — source repo with tracked `src/.codex/config.toml`; borrowed snapshot with `requestedCwd = /src`; assert absent from the snapshot. Control: the same build with - `requestedCwd = ` leaves `src/.codex/config.toml` present, proving the fixture writes a - real file and that the assertion is about the cwd chain rather than about the file never existing. -2. **Copilot `.github/mcp.json` at root** — present in source, absent from the snapshot. Control: a - sibling `.github/unrelated.json` survives, proving the exclusion is path-scoped and the fixture - populates `.github/`. -3. **Intermediate directory** — `relativeCwd = "a/b"`, config at `a/.mcp.json`; excluded. Asserts the - whole chain is covered, not just the two endpoints. -4. **Sibling not excluded** — `relativeCwd = "a"`, config at `b/.mcp.json`; **present**. This is the - test that keeps the rule from silently becoming "every directory in the tree", which would strip - this repository's own committed `kcap/.mcp.json` from every snapshot. -5. **Root cwd unchanged** — for `relativeCwd = "."`, `plan.VendorConfigPaths` equals + `requestedCwd = ` leaves it present. +2. **Copilot `.github/mcp.json` at root** — the file is **tracked**, and the test asserts it appears + in `ls-files -co` before asserting it is absent from the snapshot. (Revision 1 used a surviving + sibling, which proved only that something under `.github/` was copied.) +3. **Intermediate directory** — `relativeCwd = "a/b"`, tracked config at `a/.mcp.json`; excluded. + Control: a root-cwd build of the same fixture leaves `a/.mcp.json` present. +4. **Sibling not excluded** — `relativeCwd = "a"`, config at `b/.mcp.json`; **present**. Keeps the + rule from silently becoming the whole-tree variant rejected above. +5. **Root cwd unchanged** — for an empty git prefix, `plan.VendorConfigPaths` equals `WorkspaceMcpConfigPaths` exactly. Pins the no-regression claim for the common launch. -6. **Review-context parity** — a tracked hostile `src/.kiro/settings/mcp.json` with - `requestedCwd = /src`: absent from the snapshot *and* present in the review-context - manifest with its exact path. This is the test that keeps containment and reviewability moving - together; the AI-1706 tests only cover the root case. -7. **Refresh parity** — initial build and `SyncBorrowedSnapshotFromSourceAsync` for the same cwd - produce identical exclusion plans. -8. **Reserved index policy** — a tracked `src/.mcp.json` under `relativeCwd = "src"` does not appear - as a deletion in the snapshot's `git status`. -9. **List membership** — the existing `WorkspaceMcpNeutralizationTests` membership assertion is - extended to require `.github/mcp.json` and `.copilot/mcp-config.json`. - -The AI-1632 live certification (`KCAP_WORKSPACE_MCP_CERT=1`) is re-run unchanged, control included: -this widens the excluded set and must not disturb the measured spawn behaviour at the root. +6. **Classifier equivalence** — over a corpus including NFC/NFD pairs, mixed case, and non-ASCII + ancestor names: for every path, snapshot exclusion and `ClassifyReservedPath != Unrelated` agree. + This is the lockstep test. +7. **Refresh parity at the boundary** — an initial sub-cwd snapshot; then add and modify configs at + the root, an intermediate directory and the cwd; run `SyncBorrowedSnapshotFromSourceAsync`; assert + every ancestor config is absent, each tracked one appears in the newly published review context, a + sibling survives, and no kcap-created deletion appears in `git status`. Includes a refresh whose + source spelling differs in case from the destination spelling. (Revision 1 compared two plan + objects, which proves nothing about the consumers.) +8. **Reserved index policy** — a **HEAD-tracked** `src/.mcp.json` under `relativeCwd = "src"`: assert + the destination index contains it, that its skip-worktree bit is set, and that `git status` in the + snapshot is clean. Negative control: with the policy disabled the same fixture reports a deletion, + proving the assertion is not vacuous. +9. **Staged-only addition** — `src/.mcp.json` added to the source index but not committed: the build + succeeds (it is not in the destination index, so it is not batched), the file is excluded from the + snapshot, and it appears in review context. This is the case revision 1's design would have + crashed on. +10. **Rooted / escaping cwd** — a cwd that yields a rooted `Path.GetRelativePath` result is rejected; + `..` and `../` remain rejected. +11. **Depth cap** — a cwd deep enough to exceed the candidate/aggregate-byte cap is rejected with the + coded error rather than producing an oversized batch. +12. **List membership** — the existing `WorkspaceMcpNeutralizationTests` membership assertion is + extended to require `.github/mcp.json` and `.copilot/mcp-config.json`. + +The AI-1632 live certification (`KCAP_WORKSPACE_MCP_CERT=1`) is re-run unchanged, with its existing +control, which asserts the declared command *does* spawn when the guard is removed. This change +widens the excluded set and must not disturb the measured root-level behaviour. ## Out of scope - The owned-worktree strip, for the reason recorded above (cwd is always the worktree root). If a sub-cwd owned launch is ever added, `NeutralizeWorkspaceMcpConfig` gains the same chain and this document is the reason it must. -- Re-checking vendor discovery beyond Codex and Copilot. Kiro, Gemini and Cursor were measured - root-scoped during AI-1632 and the ancestor-chain rule strictly widens their coverage; nothing - here narrows any vendor. +- Widening review context to untracked or working-tree bytes. Stated above as a deliberate + narrowing. - AI-1675 (`CopyDirectory` recursion / symlink dereference). Untouched, and repairing it here would arm the exfiltration that issue describes. From 951b69955f1a59e4a65886454f6dd607d6eeb8c5 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:12:47 -0400 Subject: [PATCH 03/12] docs: spec revision 3 after round 2 of spec review Withdraws the unconditional case fold (a launch-refusal primitive), pins the rev-parse --show-prefix byte protocol, persists the git prefix across a refresh instead of re-deriving it, extends EnsureSeparateRoots to resolved paths, and corrects the whole-tree rejection rationale. Co-Authored-By: Claude Opus 5 --- ...03-vendor-config-discovery-scope-design.md | 505 ++++++++++-------- 1 file changed, 274 insertions(+), 231 deletions(-) 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 index 7068df2b7..0182e2344 100644 --- 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 @@ -3,21 +3,18 @@ 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 2 — rewritten after round 1 of spec review. The load-bearing change from revision 1 is -that the cwd prefix is now derived from **git's own path bytes**, not from the filesystem, and that -one byte-level classifier serves both the exclusion filter and the review-context extractor. +Revision 3, after two rounds of spec review. Revision 2 moved the cwd prefix from the filesystem to +git's own bytes. Revision 3 replaces revision 2's unconditional case fold — which was a launch-refusal +primitive — pins the `--show-prefix` byte protocol, persists the prefix across a refresh instead of +re-deriving it, and corrects a factually wrong justification. ## The defect -`WorktreeManager.WorkspaceMcpConfigPaths` is a list of **root-relative** paths. Every consumer -matches it against a path relative to the repository root: - -- `IsUnderExcluded(rel, exclusions, caseSensitive)` — the borrowed-snapshot manifest filter, a plain - `rel == prefix || rel.StartsWith(prefix + "/")` over a decoded string; -- `NeutralizeWorkspaceMcpConfig(worktreePath)` — the owned-worktree strip; -- `ApplyReservedIndexPolicyAsync` — the `skip-worktree` marking; -- `ExtractReviewContextEntriesAsync` → `ClassifyReservedPath` — the AI-1706 reserved-path classifier, - over raw bytes. +`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 @@ -27,288 +24,334 @@ So a review flow started from `/src` produces a snapshot whose cwd is ` VendorConfigPaths, // canonical list × the git-derived ancestor chain - ImmutableArray VendorConfigPathBytes, - string[] SnapshotExclusions); // .capacitor, .attached, vendor paths, caller extras - -internal static SnapshotExclusionPlan PlanSnapshotExclusions( - string gitRelativeCwd, IEnumerable? additional = null); + 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` already iterates raw records and calls `NormalizeRelativePath` on the -decoded form; it gains a `ClassifyReservedPath` call on the raw bytes *before* decoding, and treats -`Exact` and `Descendant` as excluded. `IsUnderExcluded` is retained only for `.capacitor`, -`.attached` and caller-supplied `excludePaths`, which are ASCII constants and daemon-supplied — the -namespace question does not arise for them. The vendor list no longer flows through it. - -A test asserts the invariant directly: for a corpus of paths spanning case and normalization -variants, `excluded(path) == (ClassifyReservedPath(path) != Unrelated)` for every vendor path in the -plan. That is the lockstep property, stated as an equivalence rather than as "we passed the same -array to both". +`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. -The `SnapshotExcludedPaths` static property is **removed**, not left beside the new plan. Leaving it -is precisely the two-lists shape whose comment in the code today reads "Two lists of the same thing -is how that happened". +`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". -### What "reviewable" actually means — narrowed, not claimed +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. -Round 1 is right that containment and reviewability range over different data. Containment operates -on the working tree (`ls-files -co --exclude-standard`); review context contains **index stage-0 -blobs only**, and the manifest says so in a field (`UnstagedAndUntrackedOmitted: true`). So an -*untracked* reserved config, or unstaged working-tree bytes of a tracked one, is contained but not -reviewable. - -That is a pre-existing property of AI-1706, not something this change introduces, and it is not -silently inherited: the reviewer is told, by that manifest field, which bytes it is looking at. This -design states the narrowing explicitly and does not widen it — carrying untracked working-tree bytes -into review context would reintroduce the AI-1680 failure that killed the previous attempt, where -a developer's `skip-worktree` local override would have been published to the reviewer's model. +### Refresh: persist the prefix, never re-derive it -The lockstep property this design does claim is therefore precise: **for tracked stage-0 content, -every path excluded from the snapshot is classified as reserved by the extractor.** +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. -### Ordering - -`CreateReviewContextGenerationAsync` runs before `ReadSourceManifestAsync` in -`BuildIndependentSnapshotOnceAsync`. That is safe here because the plan is computed once, before -either, and is immutable; both read the same `VendorConfigPathBytes`. Both also read the same -`initialIndex` / source, and the existing end-of-build re-check (`sourceHead`, `initialIndex`, -`ManifestsEqual`) still fails the whole build with `SourceChangedException` if the source moved -underneath. Failed generations are deleted on every throw path already. +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. ### Reserved index policy -Round 1's third finding is a real bug in revision 1: `initialIndex` is read from **source**, while -`update-index` runs in **destination**. The destination is a fresh clone checked out at `HEAD`, so a -path that is staged-but-not-committed in the source is in the source index and *not* in the -destination index. Batching it in would make `update-index --skip-worktree` fail on a legitimate -snapshot — and revision 1 also proposed promoting that failure from a swallowed catch to a hard -error, which together would refuse the launch. +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. The concrete reserved +path list is written into the generation alongside the manifest and threaded into every validation +call, so the validator checks that every entry path is a member of that set — strictly stronger than +the old length cap, which only bounded the count. + +### 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. -Corrected: read the **destination** index after checkout (`git -C destination ls-files -z`), -intersect the plan's vendor paths with that, and mark only paths proven present. With membership -established, a failure is a real failure and propagates. The source-side consistency re-check at the -end of the build is unchanged and still catches a source that moved. +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.** -Round 1's fourth finding bounds the mechanism: +### The non-borrowed sync path -- The batch is fed on **stdin**: `git update-index --skip-worktree -z --stdin`. This removes the - `ARG_MAX` ceiling for a deep cwd and removes pathspec interpretation of a leading `:` or `-` in an - ancestor directory name — `--stdin` paths are literal. -- Aggregate pathname bytes are `O(depth²)`, not `O(depth)`. An explicit cap on the plan's candidate - count and on its aggregate path bytes is added, rejected at plan construction with a coded error - (`borrowed_snapshot_cwd_too_deep`), so the bound is stated rather than inherited from `ARG_MAX`. -- `MaxReviewContextBytes` charges only blob content. The serialized manifest is bounded separately — - path strings, base64 expansion and JSON overhead are not free — with its own cap and coded error. - Revision 1's claim that the content cap was "the real bound" was wrong. +`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. -### Deriving the prefix on both build paths +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. It therefore takes a plan derived for its own execution path, including the widened ancestor +rule, and gets consumers 1 and 2. Only consumer 3 (review context) is absent, because it passes no +review-context root. -`CreateBorrowedSnapshotAsync` and `SyncFromSourceCoreAsync` both compute the git-relative cwd the -same way, through one private helper, from the same `git rev-parse --show-prefix` primitive. If they -diverged, a per-round refresh would reintroduce a file the initial build excluded. +### The `.github/mcp.json` addition -That is asserted at the security boundary, not at the helper — see test 7. +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** proving the file -would otherwise be present. Round 1 found four of revision 1's tests could pass vacuously; those are -rewritten here. - -1. **Codex sub-cwd** — source repo with tracked `src/.codex/config.toml`; borrowed snapshot with - `requestedCwd = /src`; assert absent from the snapshot. Control: the same build with - `requestedCwd = ` leaves it present. -2. **Copilot `.github/mcp.json` at root** — the file is **tracked**, and the test asserts it appears - in `ls-files -co` before asserting it is absent from the snapshot. (Revision 1 used a surviving - sibling, which proved only that something under `.github/` was copied.) -3. **Intermediate directory** — `relativeCwd = "a/b"`, tracked config at `a/.mcp.json`; excluded. - Control: a root-cwd build of the same fixture leaves `a/.mcp.json` present. -4. **Sibling not excluded** — `relativeCwd = "a"`, config at `b/.mcp.json`; **present**. Keeps the - rule from silently becoming the whole-tree variant rejected above. -5. **Root cwd unchanged** — for an empty git prefix, `plan.VendorConfigPaths` equals - `WorkspaceMcpConfigPaths` exactly. Pins the no-regression claim for the common launch. -6. **Classifier equivalence** — over a corpus including NFC/NFD pairs, mixed case, and non-ASCII - ancestor names: for every path, snapshot exclusion and `ClassifyReservedPath != Unrelated` agree. - This is the lockstep test. -7. **Refresh parity at the boundary** — an initial sub-cwd snapshot; then add and modify configs at - the root, an intermediate directory and the cwd; run `SyncBorrowedSnapshotFromSourceAsync`; assert - every ancestor config is absent, each tracked one appears in the newly published review context, a - sibling survives, and no kcap-created deletion appears in `git status`. Includes a refresh whose - source spelling differs in case from the destination spelling. (Revision 1 compared two plan - objects, which proves nothing about the consumers.) -8. **Reserved index policy** — a **HEAD-tracked** `src/.mcp.json` under `relativeCwd = "src"`: assert - the destination index contains it, that its skip-worktree bit is set, and that `git status` in the - snapshot is clean. Negative control: with the policy disabled the same fixture reports a deletion, - proving the assertion is not vacuous. -9. **Staged-only addition** — `src/.mcp.json` added to the source index but not committed: the build - succeeds (it is not in the destination index, so it is not batched), the file is excluded from the - snapshot, and it appears in review context. This is the case revision 1's design would have - crashed on. -10. **Rooted / escaping cwd** — a cwd that yields a rooted `Path.GetRelativePath` result is rejected; +Every test names the discovery shape it defends and carries a positive control. Rounds 1 and 2 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** — on a case-sensitive volume, cwd `a`, tracked `A/.mcp.json`: **present**, + and the build succeeds. This is the test that pins revision 2's withdrawn fold: with it, the file + was excluded and, with `a/.mcp.json` also tracked, the build failed with a path collision. +6. **`--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. Run on macOS (case-insensitive by default) with an alternate-case entry and with + composed/decomposed names, since that is the volume where the two spellings can diverge. This is + the independent oracle; the plan builder is not permitted to be its own oracle. +7. **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. +8. **Non-ASCII prefix refused** — a cwd whose git prefix is non-ASCII fails with + `borrowed_snapshot_cwd_prefix_non_ascii` rather than building. +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 the set computed + from the real git prefix and the real source listing. Replaces revision 2's test 6, which asserted + exclusion against the same classifier exclusion now calls — true by construction. +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; 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. -11. **Depth cap** — a cwd deep enough to exceed the candidate/aggregate-byte cap is rejected with the - coded error rather than producing an oversized batch. -12. **List membership** — the existing `WorkspaceMcpNeutralizationTests` membership assertion is - extended to require `.github/mcp.json` and `.copilot/mcp-config.json`. +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. +17. **Non-borrowed sync** — `SyncFromSourceAsync` with a sub-cwd execution path excludes ancestor + configs, proving the static-property removal did not drop its exclusions. +18. **Oversized ancestor config** — one tracked ancestor `.mcp.json` over 256 KiB trips + `borrowed_snapshot_review_context_capacity_exceeded`, documenting the pre-existing DoS that the + separately-filed issue covers, so a later reader does not mistake it for a regression here. +19. **List membership** — `.github/mcp.json` and `.copilot/mcp-config.json` required. The AI-1632 live certification (`KCAP_WORKSPACE_MCP_CERT=1`) is re-run unchanged, with its existing -control, which asserts the declared command *does* spawn when the guard is removed. This change -widens the excluded set and must not disturb the measured root-level behaviour. +control asserting the declared command *does* spawn when the guard is removed. ## Out of scope -- The owned-worktree strip, for the reason recorded above (cwd is always the worktree root). If a - sub-cwd owned launch is ever added, `NeutralizeWorkspaceMcpConfig` gains the same chain and this - document is the reason it must. -- Widening review context to untracked or working-tree bytes. Stated above as a deliberate - narrowing. -- AI-1675 (`CopyDirectory` recursion / symlink dereference). Untouched, and repairing it here would - arm the exfiltration that issue describes. +- 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). From d116df1db134fc522081cbfb5a2d88e0c946ab2a Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:21:58 -0400 Subject: [PATCH 04/12] docs: spec revision 4 after round 3 of spec review Collapses the launch path and the classifier onto one git-derived prefix (the round-3 critical: two independent derivations could disagree and the appeal to a fail-closed side effect was unsound), narrows the non-ASCII prefix refusal to case-insensitive destinations, specifies resolving a not-yet-created snapshot root, and documents bind-mount/volume-alias aliasing as a trusted-configuration residual rather than claiming closure. Co-Authored-By: Claude Opus 5 --- ...03-vendor-config-discovery-scope-design.md | 191 +++++++++++++----- 1 file changed, 138 insertions(+), 53 deletions(-) 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 index 0182e2344..a92facc22 100644 --- 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 @@ -3,10 +3,11 @@ 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 3, after two rounds of spec review. Revision 2 moved the cwd prefix from the filesystem to -git's own bytes. Revision 3 replaces revision 2's unconditional case fold — which was a launch-refusal -primitive — pins the `--show-prefix` byte protocol, persists the prefix across a refresh instead of -re-deriving it, and corrects a factually wrong justification. +Revision 4, after three 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 @@ -54,13 +55,28 @@ The property that keeps it that way is `EnsureSeparateRoots(source, root)`, whic 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 and this design closes: 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 -therefore extended to compare fully resolved paths as well as lexical ones, with the same coded -error, and gains the test round 2 asked for. +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 @@ -137,15 +153,49 @@ has. The parse is therefore pinned: 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. The remainder must be **ASCII** (every byte `< 0x80`). If not, the launch is refused with - `borrowed_snapshot_cwd_prefix_non_ascii` — see below. -5. Strict-UTF-8 decode, then `NormalizeRelativePath`, which now sees a well-formed relative path and +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. - -The filesystem-derived `relativeCwd` is retained **only** for `ContainedPath(final, relativeCwd)` and -`Directory.Exists(executionPath)`, which are filesystem operations. Its rooted-path gap — round 1's -finding, real and independent of this feature — is fixed there by rejecting `Path.IsPathRooted` -alongside the existing `..` checks. +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 @@ -166,20 +216,28 @@ input is that it is probed on the destination — the volume the vendor actually - **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 combination round 1 raised (case-insensitive source, case-sensitive destination, -prefix `SRC` versus index `src`) does not under-exclude: the file is materialised at the index -spelling `src/…`, while the execution path is `ContainedPath(final, "SRC")`, which does not exist on a -case-sensitive destination — so the launch fails closed with `borrowed_snapshot_cwd_missing` rather -than running in a directory whose config was not excluded. - -**The ASCII-only prefix restriction (step 4) is what makes this complete.** `AsciiPathEquals` folds -ASCII only; a case-insensitive volume also equates non-ASCII pairs such as `Å`/`å`, which that -matcher would miss — a genuine under-exclusion. Rather than build a second, Unicode-aware folding -path and have two matchers again, a non-ASCII cwd prefix is refused with a coded error. The cost is -bounded and visible: a review flow launched from a subdirectory whose name is not ASCII fails loudly -with a specific error instead of silently under-excluding. The prefix is the operator's own launch -cwd, not branch content, so this is a configuration limitation rather than an attacker-facing one. -The canonical suffixes are all ASCII already, so nothing else is affected. +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. ### One classifier, not two @@ -224,6 +282,20 @@ So the git prefix is computed **once**, at `CreateBorrowedSnapshotAsync`, and pe 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 @@ -291,7 +363,7 @@ standing rationale. ## Testing -Every test names the discovery shape it defends and carries a positive control. Rounds 1 and 2 each +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 @@ -301,30 +373,41 @@ found tests that could pass vacuously; those are rewritten rather than patched. 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** — on a case-sensitive volume, cwd `a`, tracked `A/.mcp.json`: **present**, - and the build succeeds. This is the test that pins revision 2's withdrawn fold: with it, the file - was excluded and, with `a/.mcp.json` also tracked, the build failed with a path collision. -6. **`--show-prefix` protocol, against real git** — invoke the actual command from a real +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. Run on macOS (case-insensitive by default) with an alternate-case entry and with - composed/decomposed names, since that is the volume where the two spellings can diverge. This is - the independent oracle; the plan builder is not permitted to be its own oracle. -7. **Root prefix** — the empty-prefix path is exercised through the real command output (`"\n"`), not + listing, on macOS, with an alternate-case entry. The composed/decomposed pair is asserted here as + the **coded prefix rejection** on a case-insensitive destination, not as a byte comparison — + revision 3's test description was self-contradictory, since a non-ASCII prefix is refused there + before any comparison happens. A separate case on a case-sensitive volume asserts a non-ASCII + prefix is admitted and compared byte-exactly. +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. -8. **Non-ASCII prefix refused** — a cwd whose git prefix is non-ASCII fails with - `borrowed_snapshot_cwd_prefix_non_ascii` rather than building. 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 the set computed - from the real git prefix and the real source listing. Replaces revision 2's test 6, which asserted - exclusion against the same classifier exclusion now calls — true by construction. + 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; 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. + 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. @@ -338,12 +421,14 @@ found tests that could pass vacuously; those are rewritten rather than patched. `..` 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. + 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** — `SyncFromSourceAsync` with a sub-cwd execution path excludes ancestor configs, proving the static-property removal did not drop its exclusions. 18. **Oversized ancestor config** — one tracked ancestor `.mcp.json` over 256 KiB trips - `borrowed_snapshot_review_context_capacity_exceeded`, documenting the pre-existing DoS that the - separately-filed issue covers, so a later reader does not mistake it for a regression here. + `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. The AI-1632 live certification (`KCAP_WORKSPACE_MCP_CERT=1`) is re-run unchanged, with its existing From 490f3fadb9049f0d232cefe45cef7ef9cc7d75e9 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:26:41 -0400 Subject: [PATCH 05/12] docs: spec revision 5 after round 4 of spec review Replaces the non-borrowed sync overloads with one that requires a source-side cwd (the old shape left no way to derive a prefix except the banned filesystem inference), pins where the prefix is captured versus where the plan is built relative to the destination case probe, persists the actual matched paths rather than canonical spellings so validation needs no second matcher, and corrects the non-ASCII test expectations to the pinned parse order. Co-Authored-By: Claude Opus 5 --- ...03-vendor-config-discovery-scope-design.md | 95 ++++++++++++++++--- 1 file changed, 81 insertions(+), 14 deletions(-) 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 index a92facc22..f613f0ff8 100644 --- 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 @@ -3,7 +3,7 @@ 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 4, after three rounds of spec review. Revision 2 moved the cwd prefix from the filesystem to +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 @@ -239,6 +239,29 @@ This is a user-visible compatibility limitation on one platform class, stated as 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 @@ -323,10 +346,23 @@ Bounds, all of which revision 2 got wrong or left open: ### 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. The concrete reserved -path list is written into the generation alongside the manifest and threaded into every validation -call, so the validator checks that every entry path is a member of that set — strictly stronger than -the old length cap, which only bounded the count. +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 @@ -351,8 +387,29 @@ 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. It therefore takes a plan derived for its own execution path, including the widened ancestor -rule, and gets consumers 1 and 2. Only consumer 3 (review context) is absent, because it passes no +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 @@ -385,11 +442,14 @@ found tests that could pass vacuously; those are rewritten rather than patched. 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 composed/decomposed pair is asserted here as - the **coded prefix rejection** on a case-insensitive destination, not as a byte comparison — - revision 3's test description was self-contradictory, since a non-ASCII prefix is refused there - before any comparison happens. A separate case on a case-sensitive volume asserts a non-ASCII - prefix is admitted and compared byte-exactly. + 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 @@ -423,13 +483,20 @@ found tests that could pass vacuously; those are rewritten rather than patched. 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** — `SyncFromSourceAsync` with a sub-cwd execution path excludes ancestor - configs, proving the static-property removal did not drop its exclusions. +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. From 5c52b7e23b9028064f1a9b152339a745609e7270 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:49:24 -0400 Subject: [PATCH 06/12] Scope vendor MCP exclusion to the execution cwd's ancestor chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The canonical vendor-config list is root-relative and every consumer matched it that way, but a borrowed snapshot can execute BELOW the repository root: CreateBorrowedSnapshotAsync takes the git root and the user's cwd independently, and returns the execution path as WorktreeInfo.Path. A review flow started from /src therefore left src/.codex/config.toml live in the tree the reviewer runs in. Codex layers .codex/config.toml from the root down to the cwd; Copilot and Claude Code walk from the cwd upward. Either way the reachable set is the ancestor chain, so that is what is now excluded. Also adds .github/mcp.json, which the list never had at all (it carried .github/copilot/mcp.json, a different path) and which was unprotected at the root of every borrowed snapshot regardless of scope, plus .copilot/mcp-config.json under the list's standing "wider than known readers" rationale. One prefix, from git, for both the classifier and the launch. It comes from `rev-parse --show-prefix`, not the filesystem: a .NET-derived prefix is not in the same pathname namespace as the paths ls-files reports (NFD/NFC on macOS, rooted results across Windows volumes), and deriving the launch cwd separately from the classified prefix lets the two disagree — at which point an unexcluded path materialises the alternate-spelling directory itself and the vendor launches somewhere nothing was excluded. One classifier, not two. Vendor paths no longer appear in the snapshot exclusion list at all; they are matched only through ClassifyReservedPath, which the review-context extractor also uses. The two had different case semantics (OrdinalIgnoreCase vs ASCII-only) — unobservable while the list was ASCII constants, and exactly how a path becomes contained but unreviewable. Other consequences: skip-worktree now intersects the DESTINATION index (the source index contains staged-but-uncommitted paths the destination clone does not, which would have failed update-index on a legitimate snapshot) and is batched on stdin; EnsureSeparateRoots compares resolved paths as well as lexical ones; the refresh path carries the prefix rather than re-deriving it; and the execution directory is created rather than required, since a cwd whose only content was vendor config now yields no directory at all. Notes: bind-mount and SUBST aliasing of WorktreeRoot remain a documented trusted-configuration residual. A non-ASCII cwd prefix is refused on case-insensitive volumes only, as a stated compatibility limitation. The pre-existing review-context capacity DoS is unchanged and tracked separately. Spec: docs/superpowers/specs/2026-08-04-ai1703-vendor-config-discovery-scope-design.md Co-Authored-By: Claude Opus 5 --- .../Services/AgentOrchestrator.cs | 8 +- .../Services/WorktreeManager.ExclusionPlan.cs | 252 ++++++++++ .../Services/WorktreeManager.ReviewContext.cs | Bin 16658 -> 20506 bytes .../Services/WorktreeManager.WorkspaceMcp.cs | 16 +- .../Services/WorktreeManager.cs | 225 ++++++--- .../BorrowedSnapshotExclusionScopeTests.cs | 475 ++++++++++++++++++ .../AcpHostedAgentRuntimeFactoryLiveTests.cs | 2 +- .../WorkspaceMcpNeutralizationTests.cs | 29 +- .../WorktreeManagerTests.cs | 4 +- 9 files changed, 943 insertions(+), 68 deletions(-) create mode 100644 src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs create mode 100644 test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs 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..e2b0a5b2b --- /dev/null +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs @@ -0,0 +1,252 @@ +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 sourceCwd, CancellationToken ct) { + var raw = await RunGitCaptureBoundedAsync( + sourceCwd, GitTimeout, MaxCwdPrefixCaptureBytes, ct, + "-c", "core.quotePath=false", "rev-parse", "--show-prefix"); + + return ParseGitRelativeCwd(raw); + } + + /// 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); + 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) { + try { process.Kill(entireProcessTree: true); } catch { /* already gone */ } + throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); + } + stdout.Write(buffer, 0, read); + } + await process.WaitForExitAsync(timeoutCts.Token); + } catch (OperationCanceledException) { + try { process.Kill(entireProcessTree: true); } catch { /* already gone */ } + throw new InvalidOperationException( + $"git {string.Join(' ', args)} timed out after {timeout.TotalSeconds:F0}s"); + } + var stderr = await stderrTask; + if (process.ExitCode != 0) + throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {stderr}"); + return stdout.ToArray(); + } + + /// 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); + try { + 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; + } catch (OperationCanceledException) { + try { process.Kill(entireProcessTree: true); } catch { /* already gone */ } + throw new InvalidOperationException( + $"git {string.Join(' ', args)} timed out after {timeout.TotalSeconds:F0}s"); + } + var stderr = await 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 02d52eebda7ba4644442709748a46016c2f44ad4..5dbb362b7365026cda658d5a418a34cadf97211f 100644 GIT binary patch delta 3472 zcmb7H-EJdE6(+MYFp6e}9c`A??$t>o$j^G(GdtNJ*u>eylM#VrJdA_Tg3wI4y4qbk z-Ca#r+m3;i5m#`75WNx-S0k|xfRqveIldW_CxhmNCg#6n$l3G#OJbF+GwdD^*qo z^hA^~*|LC7+n}DXYHM@V(q{7ksnc9!RwELTh4kHS=P?|Ll%o@cqRemrN}W5i1~E?% zj5khFW>P0_o+s5(Skb zEfszK%TExSR5NEDle;RDCd92ZK(p?mNvr^lLrYmi;fl#= zGb)8iTrq^?1f{X1C96-*eIX=EYq0nwHn$$o4^3f9L}}s5G9^%G0|x>qmhAf7_qOPJ zdY}IK2NxPm-l45^`usP)ru)IS=veQwA?mqXGe0@1j&jGs#x>-1WdquyNV7`;jMALn z={}pGk+3k6m=U7DL#|VyK2pdeM0Qhl(gZQi{LC{U7C4u1MaLwhR5@Q{v;{>%CwFCn zLb1(gU`0ZPF-ufmmpTO<$Yvt9u>oZ_T{tp9%^V^#ZzIO4?ZFyp@H&RG!k|ac2A~acu=?k9s*Z2pe0$&3V}~}6XoK$Gdv~*bdw;RIcH_~*3!VFv zxjKhL!>8ifB^!g{aG47rJJ+zs;=|& zFPzPYDKyQ2MrM$l;~R^if-@bu1I#ldIm_)BOFQ%ki25%9XWpPLL`Ky=xXmA->x|QYci0=aE&V>pYBw z0{k%r6t5g0(lPQY(ZgQ(xO?~7CqLLbBIul}98{SHEC0f*Shbh&MaP&-ODLqxRl>W1 z4JOn<#!mR|>I~z~LN%cPAz;)Q^aE%M;!s||^YiZ0b~89`KM8nBJ)aZVWPHV2BCORB z^U9Uc&7x@l&ut-_K`Qe}z_>nl#T((+#D-cOa?zOxYAnwZ048P!5&UcESMTR4T$NiSd?z#cJ%8cnW9H;K5qc z1Gg_Sl{Ks83GZhtx12nx0PRc=)SAa)r{zI;E#PD8!HM|}CVE$u)w4S*)jw_@-DzB~ z1J$a2_h9L)`PD}YtN&Y^{6<%|O|`MRJmt4%B(vMI6wApAn+vIqW!^Sfr28)1Z@XOw zO`jsndQli^H4;7Sw&fUW2e^rn6(Z{^*ZQ;ipplsRR-`(OknYir<<{=wkwJrdG z0T)6Rr510rReGDaMXM*-%^E<4u%TSZ-vF7tC1e z30$18;PEbloZz4bN)0l9M&Q;QZ*qjDvFEySUvh;d#xJ_D;bmk{L_iKAJd-;&nsDs5GW#9JmyLF-HXTz_{1&ErH z9ao>AjSak=9e#}>ndk` W*SKD(K5M*Q-P`#2+22-=-ue$j<9@*a delta 280 zcmbQWfN@e2;|6Bd$HCMSul;c?5% zN!1HVO-ymj$=NI=Zo@d4UD{)FtaL0RP?Ji1QEG8&QCVt=f~`V$eo=ODL1J>MZ*qZi zeqLH;dO%`HMlqKH63`1y%}Grz(I^1QDA?L5XgDY4<>zH4C+1iwfW&kZoGMFFi>(w~ z^OEyZGV{{)LPOjv^xRWRz~W$inwpw$GxxJYy78NB{YG`hrAQvFAncp~>Q4G6l jCfB)IO@5##IN4uaYw`he&dK}DxF@rk`fWaE<|7FJ?RZ+@ 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..7a938b20a 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(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); @@ -529,48 +553,48 @@ 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 the source, excluding vendor config + /// 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[] excludePaths, CancellationToken ct) { - await SyncFromSourceAsync( - sourceRepoRoot, targetWorktreePath, targetWorktreePath, excludePaths, ct); - } - - public async Task SyncFromSourceAsync( - string sourceRepoRoot, string targetWorktreePath, string executionPath, + string sourceRepoRoot, string sourceCwd, string targetWorktreePath, string[] excludePaths, CancellationToken ct) { + var gitRelativeCwd = await ReadGitRelativeCwdAsync(Path.GetFullPath(sourceCwd), 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 +603,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 +619,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 +643,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 +669,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 +694,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 +710,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 +729,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 +744,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 +885,58 @@ 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"); } + static bool IsAtOrUnder(string candidate, string root) { + var prefix = root.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; + return candidate.Equals(root, FileSystemPathComparison) || + candidate.StartsWith(prefix, FileSystemPathComparison); + } + + /// Resolves links up to the deepest component that exists, then appends the rest 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. + static string ResolveDeepestExisting(string path) { + var full = Path.GetFullPath(path); + var tail = new List(); + var current = full; + while (true) { + if (Path.Exists(current)) { + var resolved = new DirectoryInfo(current).LinkTarget is null && new FileInfo(current).LinkTarget is null + ? current + : Path.GetFullPath( + new DirectoryInfo(current).ResolveLinkTarget(returnFinalTarget: true)?.FullName + ?? new FileInfo(current).ResolveLinkTarget(returnFinalTarget: true)?.FullName + ?? current); + tail.Reverse(); + return tail.Count == 0 ? resolved : Path.Combine([resolved, .. tail]); + } + var parent = Path.GetDirectoryName(current); + if (string.IsNullOrEmpty(parent) || parent == current) return full; + tail.Add(Path.GetFileName(current)); + current = parent; + } + } + internal static string NormalizeRelativePath(string raw) { if (raw.Length == 0 || raw.StartsWith('/') || raw.Contains('\\') || raw.Contains('\r') || raw.Contains('\n') || @@ -949,16 +1033,45 @@ 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(); + if (ClassifyReservedPath(record.Span, plan.Reserved, caseSensitive).Kind + != ReservedPathMatchKind.Exact) + 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..76a60e7df --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs @@ -0,0 +1,475 @@ +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(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, 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 { } + } + } + + // ---------- 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(); From 64380a528b002688b9f098c252eee56eab626a69 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:51:52 -0400 Subject: [PATCH 07/12] Fix a stray NUL byte in a doc comment and a duplicated summary tag The NUL made git classify WorktreeManager.ReviewContext.cs as binary, so the file diffed as "Binary files differ" and was unreviewable. Co-Authored-By: Claude Opus 5 --- .../Services/WorktreeManager.ReviewContext.cs | Bin 20506 -> 20521 bytes .../Services/WorktreeManager.cs | 9 ++++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs index 5dbb362b7365026cda658d5a418a34cadf97211f..e3cab49bc9a69610051a5b04bce779039951a9b7 100644 GIT binary patch delta 31 lcmbQWfN|vl#tmBR0!fL<*~K}D#TmM#x&}b7S)2X62LQ803eErk delta 16 YcmZ3vfN|CW#tmBRj0~Ht*x!2q05YZplK=n! diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index 7a938b20a..94bcf6e7e 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -550,11 +550,10 @@ 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 the source, excluding vendor config - /// along the ancestor chain of . + /// 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 From 4283361b3622b67c3fe27d0785e9372a328ba7b8 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:04:12 -0400 Subject: [PATCH 08/12] Address round 1 of code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all real: - The index policy marked only Exact matches skip-worktree while the manifest filter excludes Exact AND Descendant. The rationale was backwards: a repo can track `.mcp.json/child` (the config pathname as a directory) and each such child IS an index entry. Left unmarked it reads as a deletion, and an ordinary git operation could restore it into a live vendor-config tree. Now marks every non-Unrelated match. - ResolveDeepestExisting only tested LinkTarget on the deepest existing component, so with /alias -> /real and an ordinary /alias/existing, the containment check returned the lexical path and still missed a snapshot root reaching inside the source through the ancestor link. Now resolves every component of the existing prefix, with a bounded hop count for chains. - The git-derived prefix was not checked to belong to the named source repo. rev-parse reports whatever repository it DISCOVERS at the cwd, so a nested or foreign repository yielded a prefix in another namespace that was then matched against this source's ls-files output — the exact invariant the derivation exists to hold. --show-toplevel is now captured and required to resolve to the source root. - Both git helpers killed inline and threw, leaving the child unreaped and the pump tasks unobserved; the stdin helper also leaked a running child on a non-cancellation write fault. Cleanup moved into finally via a shared TerminateAndDrainAsync. Three regression tests added. Note on the first: on the BORROWED path a reserved path tracked as a directory is refused earlier by the pre-existing review-context guard, so the deletion case is unreachable there — that fail-closed behaviour is pinned, and the index fix is exercised through the sync path, which has no review context. Co-Authored-By: Claude Opus 5 --- .../Services/WorktreeManager.ExclusionPlan.cs | 61 ++++++++++-- .../Services/WorktreeManager.cs | 61 ++++++++---- .../BorrowedSnapshotExclusionScopeTests.cs | 95 ++++++++++++++++++- 3 files changed, 193 insertions(+), 24 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs index e2b0a5b2b..36a278a07 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs @@ -69,7 +69,22 @@ internal sealed record SnapshotExclusionPlan( /// core.quotePath=false is required, or non-ASCII components come back C-quoted. /// internal static async Task ReadGitRelativeCwdAsync( - string sourceCwd, CancellationToken ct) { + 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"); @@ -77,6 +92,21 @@ internal static async Task ReadGitRelativeCwdAsync( 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) { @@ -198,17 +228,19 @@ static async Task RunGitCaptureBoundedAsync( while (true) { var read = await process.StandardOutput.BaseStream.ReadAsync(buffer, timeoutCts.Token); if (read == 0) break; - if (stdout.Length + read > maxBytes) { - try { process.Kill(entireProcessTree: true); } catch { /* already gone */ } + if (stdout.Length + read > maxBytes) throw new InvalidOperationException("borrowed_snapshot_cwd_prefix_malformed"); - } stdout.Write(buffer, 0, read); } await process.WaitForExitAsync(timeoutCts.Token); } catch (OperationCanceledException) { - try { process.Kill(entireProcessTree: true); } catch { /* already gone */ } throw new InvalidOperationException( $"git {string.Join(' ', args)} timed out after {timeout.TotalSeconds:F0}s"); + } finally { + // Every abnormal exit ran 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); } var stderr = await stderrTask; if (process.ExitCode != 0) @@ -216,6 +248,18 @@ static async Task RunGitCaptureBoundedAsync( return stdout.ToArray(); } + /// 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 failure here is swallowed deliberately: this runs in a finally, and the + /// original exception is more useful to an operator than whatever went wrong tidying up after it. + /// + static async Task TerminateAndDrainAsync(Process process, params Task[] pumps) { + try { if (!process.HasExited) process.Kill(entireProcessTree: true); } catch { /* already gone */ } + try { await process.WaitForExitAsync(CancellationToken.None); } catch { /* already reaped */ } + foreach (var pump in pumps) + try { await pump; } catch { /* observed, not handled */ } + } + /// 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 @@ -232,6 +276,8 @@ static async Task RunGitWithNulStdinAsync( var stderrTask = ReadAllDecodedAsync(process.StandardError.BaseStream, timeoutCts.Token); var stdoutTask = ReadAllDecodedAsync(process.StandardOutput.BaseStream, timeoutCts.Token); 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); @@ -241,9 +287,12 @@ static async Task RunGitWithNulStdinAsync( await process.WaitForExitAsync(timeoutCts.Token); await stdoutTask; } catch (OperationCanceledException) { - try { process.Kill(entireProcessTree: true); } catch { /* already gone */ } 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); } var stderr = await stderrTask; if (process.ExitCode != 0) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index 94bcf6e7e..89f762462 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -509,7 +509,7 @@ public async Task CreateBorrowedSnapshotAsync( throw new InvalidOperationException("borrowed_snapshot_cwd_outside_source"); if (!Directory.Exists(cwd)) throw new InvalidOperationException("borrowed_snapshot_cwd_missing"); - var gitRelativeCwd = await ReadGitRelativeCwdAsync(cwd, ct); + var gitRelativeCwd = await ReadGitRelativeCwdAsync(source, cwd, ct); var root = Path.GetFullPath(Path.Combine(config.WorktreeRoot, "borrowed-snapshots")); EnsureSeparateRoots(source, root); CreateOwnerOnlyDirectory(root); @@ -562,7 +562,8 @@ public async Task CreateBorrowedSnapshotAsync( public async Task SyncFromSourceAsync( string sourceRepoRoot, string sourceCwd, string targetWorktreePath, string[] excludePaths, CancellationToken ct) { - var gitRelativeCwd = await ReadGitRelativeCwdAsync(Path.GetFullPath(sourceCwd), ct); + var gitRelativeCwd = await ReadGitRelativeCwdAsync( + Path.GetFullPath(sourceRepoRoot), Path.GetFullPath(sourceCwd), ct); _ = await SyncFromSourceCoreAsync( sourceRepoRoot, targetWorktreePath, gitRelativeCwd, excludePaths, reviewContextRoot: null, ct); @@ -910,30 +911,51 @@ static bool IsAtOrUnder(string candidate, string root) { candidate.StartsWith(prefix, FileSystemPathComparison); } - /// Resolves links up to the deepest component that exists, then appends the rest literally. + /// 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. + /// 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; - while (true) { - if (Path.Exists(current)) { - var resolved = new DirectoryInfo(current).LinkTarget is null && new FileInfo(current).LinkTarget is null - ? current - : Path.GetFullPath( - new DirectoryInfo(current).ResolveLinkTarget(returnFinalTarget: true)?.FullName - ?? new FileInfo(current).ResolveLinkTarget(returnFinalTarget: true)?.FullName - ?? current); - tail.Reverse(); - return tail.Count == 0 ? resolved : Path.Combine([resolved, .. tail]); - } + + // 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); + for (var hop = 0; 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); + } + } + + return tail.Count == 0 ? resolved : Path.Combine([resolved, .. tail]); } internal static string NormalizeRelativePath(string raw) { @@ -1059,8 +1081,15 @@ static async Task ApplyReservedIndexPolicyAsync( 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.Exact) + == 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. diff --git a/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs b/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs index 76a60e7df..78437b8a8 100644 --- a/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/BorrowedSnapshotExclusionScopeTests.cs @@ -224,7 +224,8 @@ 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(cwd, CancellationToken.None); + 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. @@ -237,7 +238,7 @@ public async Task Show_prefix_at_the_repository_root_is_empty() { using var fixture = NewFixture(); var prefix = await WorktreeManager.ReadGitRelativeCwdAsync( - fixture.Source, CancellationToken.None); + fixture.Source, fixture.Source, CancellationToken.None); await Assert.That(prefix).IsEqualTo(""); } @@ -424,6 +425,96 @@ await Assert.That(async () => await manager.CreateBorrowedSnapshotAsync( } } + // ---------- 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] From c8d617678f21c489527aee46f189a6296e9828fc Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:10:55 -0400 Subject: [PATCH 09/12] Address round 2 of code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - IsAtOrUnder now normalises both operands to NFC. Case folding alone is not enough on a normalisation-insensitive volume: macOS treats composed and decomposed `café` as one directory while no StringComparison makes those strings equal, so a source spelled one way and a snapshot root spelled the other failed both the lexical and resolved checks and still landed inside the source. True filesystem identity stays out of reach (.NET exposes no portable device/inode pair) and remains in the documented trusted-configuration residual. - TerminateAndDrainAsync bounds every wait. It runs from a finally, so an unbounded WaitForExitAsync after a failed kill — or an unbounded pump await when a surviving descendant inherited the redirected pipe — would swallow the original timeout or overflow exception by never returning. Past a 5s budget the streams are abandoned, with a terminal continuation on each pump so an abandoned one cannot go unobserved. - The link-resolution hop loop now fails closed on exhaustion instead of continuing with a half-resolved path. Co-Authored-By: Claude Opus 5 --- .../Services/WorktreeManager.ExclusionPlan.cs | 33 +++++++++++++++---- .../Services/WorktreeManager.cs | 20 ++++++++++- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs index 36a278a07..33885759b 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs @@ -248,16 +248,37 @@ static async Task RunGitCaptureBoundedAsync( 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 failure here is swallowed deliberately: this runs in a finally, and the - /// original exception is more useful to an operator than whatever went wrong tidying up after it. + /// + /// 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) { - try { if (!process.HasExited) process.Kill(entireProcessTree: true); } catch { /* already gone */ } - try { await process.WaitForExitAsync(CancellationToken.None); } catch { /* already reaped */ } - foreach (var pump in pumps) - try { await pump; } catch { /* observed, not handled */ } + using var budget = new CancellationTokenSource(CleanupBudget); + try { if (!process.HasExited) process.Kill(entireProcessTree: true); } + catch { /* already gone, or genuinely unkillable — the bounded waits below cover both */ } + 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. diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index 89f762462..9889bce81 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -905,7 +905,18 @@ static void EnsureSeparateRoots(string source, string snapshotRoot) { throw new InvalidOperationException("borrowed_snapshot_root_inside_source"); } + /// Ancestry over path STRINGS, with both operands normalised to NFC first. + /// Case folding alone is not enough on a normalisation-insensitive volume: a typical macOS + /// filesystem treats café 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 then fail both the lexical and the resolved check and still + /// land the snapshot inside the source. + /// This does not reach true filesystem identity — .NET exposes no portable device/inode pair — + /// so exotic equivalences remain in the same trusted-configuration residual as bind mounts and SUBST + /// aliases, 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); @@ -944,7 +955,12 @@ static string ResolveDeepestExisting(string path) { .Split(Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries); foreach (var component in components) { resolved = Path.Combine(resolved, component); - for (var hop = 0; hop < maxLinkHops; hop++) { + // 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 @@ -953,6 +969,8 @@ static string ResolveDeepestExisting(string path) { 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]); From a7ffeb2852769b2efa930205fa93291b0fb718a7 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:16:37 -0400 Subject: [PATCH 10/12] Address round 3 of code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The NFC folding added last round was unconditional, which is correct on a normalisation-insensitive volume and WRONG on Linux, where composed and decomposed names are distinct directories — a valid layout would have been refused. Now probed on the source (the volume a nested snapshot root would have to be on), same shape as the existing case probe, falling back to folding when the probe cannot run. - TerminateAndDrainAsync falls back to a plain Kill() after a failed tree kill. Tree enumeration can fail where killing the process itself succeeds, and returning with a live owned child is a leak: disposing Process does not terminate it. The bounded waits stay, since neither kill form is guaranteed. - Both git helpers capture stderr INSIDE the protected block. Awaiting it after the finally re-awaited a pump the cleanup may have abandoned, waited out the remainder of the git timeout, and surfaced a raw task exception instead of the contextual timeout message. Co-Authored-By: Claude Opus 5 --- .../Services/WorktreeManager.ExclusionPlan.cs | 17 +++++-- .../Services/WorktreeManager.cs | 48 ++++++++++++++++--- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs index 33885759b..f0d585f4a 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs @@ -223,6 +223,7 @@ static async Task RunGitCaptureBoundedAsync( 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) { @@ -233,16 +234,19 @@ static async Task RunGitCaptureBoundedAsync( 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 ran through here: the overflow throw and the cancellation branch both + // 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); } - var stderr = await stderrTask; if (process.ExitCode != 0) throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {stderr}"); return stdout.ToArray(); @@ -267,7 +271,11 @@ static async Task RunGitCaptureBoundedAsync( 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 genuinely unkillable — the bounded waits below cover both */ } + 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 @@ -296,6 +304,7 @@ static async Task RunGitWithNulStdinAsync( 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. @@ -307,6 +316,7 @@ static async Task RunGitWithNulStdinAsync( } 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"); @@ -315,7 +325,6 @@ static async Task RunGitWithNulStdinAsync( // running child and two unobserved pumps behind. await TerminateAndDrainAsync(process, stdoutTask, stderrTask); } - var stderr = await stderrTask; if (process.ExitCode != 0) throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {stderr}"); } diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index 9889bce81..55b530d24 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -900,23 +900,57 @@ static void DeleteTreeNoFollow(string path) { /// worktree-metadata gate already documents as defeating a different path-identity check. /// static void EnsureSeparateRoots(string source, string snapshotRoot) { - if (IsAtOrUnder(snapshotRoot, source) || - IsAtOrUnder(ResolveDeepestExisting(snapshotRoot), ResolveDeepestExisting(source))) + // Probed on the SOURCE, which is the volume that matters: for the snapshot root to land inside the + // source it has to be on the source's filesystem. Never inferred from the OS — a normalisation- + // sensitive volume can be mounted on macOS and an insensitive one on Linux. + var foldNormalization = ProbeNormalizationInsensitive(source); + if (IsAtOrUnder(snapshotRoot, source, foldNormalization) || + IsAtOrUnder(ResolveDeepestExisting(snapshotRoot), ResolveDeepestExisting(source), + foldNormalization)) throw new InvalidOperationException("borrowed_snapshot_root_inside_source"); } - /// Ancestry over path STRINGS, with both operands normalised to NFC first. + /// Whether 's filesystem treats composed and decomposed spellings + /// of the same name as one entry. + /// Probed rather than assumed, and the direction matters both ways. On a typical macOS volume + /// they ARE one directory, so comparing without folding lets a snapshot root spelled one way sit + /// inside a source spelled the other. On Linux they are genuinely DISTINCT directories, so folding + /// unconditionally would refuse a perfectly valid configuration — a fail-closed availability + /// regression rather than an escape, but a regression. + /// Falls back to folding when the probe cannot run: over-refusing a legitimate layout is the + /// better failure of the two. + static bool ProbeNormalizationInsensitive(string directory) { + // U+00E9 versus "e" + U+0301 — the same grapheme, two encodings. + var stem = "norm-probe-" + Guid.NewGuid().ToString("N"); + var composed = Path.Combine(directory, stem + "é"); + var decomposed = Path.Combine(directory, stem + "é"); + try { + using (new FileStream(composed, FileMode.CreateNew, FileAccess.Write, FileShare.None)) { } + return File.Exists(decomposed); + } catch { + return true; + } finally { + try { File.Delete(composed); } catch { } + try { File.Delete(decomposed); } catch { } + } + } + + /// Ancestry over path STRINGS, folding Unicode normalisation only when the filesystem does. /// Case folding alone is not enough on a normalisation-insensitive volume: a typical macOS /// filesystem treats café 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 then fail both the lexical and the resolved check and still - /// land the snapshot inside the source. + /// land the snapshot inside the source. Folding UNCONDITIONALLY is equally wrong in the other + /// direction — on Linux those are distinct directories and a valid layout would be refused — which is + /// why is probed rather than assumed. /// This does not reach true filesystem identity — .NET exposes no portable device/inode pair — /// so exotic equivalences remain in the same trusted-configuration residual as bind mounts and SUBST /// aliases, documented on . - static bool IsAtOrUnder(string candidate, string root) { - candidate = candidate.Normalize(NormalizationForm.FormC); - root = root.Normalize(NormalizationForm.FormC); + static bool IsAtOrUnder(string candidate, string root, bool foldNormalization) { + if (foldNormalization) { + 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); From 5c6eb0fcbb11bc670ed7ed6c52da69baae011a9f Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:21:13 -0400 Subject: [PATCH 11/12] Address round 4 of code review by removing the normalization probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 flagged that unconditional NFC folding over-refuses on a normalization-sensitive volume. The probe I added to make it conditional was worse than the problem: File.Exists reports false for access and I/O errors as well as absence, so a failed probe read as "normalization-sensitive" and silently reopened the containment bypass it exists to close — fail-open. It also wrote into the user's own checkout, which the source manifest reads as untracked content, and its cleanup deleted a second pathname it had never created. So the probe is gone and the fold is unconditional again, with the cost stated where the decision lives: the refusal needs an operator to have spelled the source and worktree root with different normalizations of the same name on such a volume, and it fails closed with a specific coded error. A clear error in a vanishingly rare configuration beats a bypass in a common one. Co-Authored-By: Claude Opus 5 --- .../Services/WorktreeManager.cs | 77 +++++++------------ 1 file changed, 29 insertions(+), 48 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index 55b530d24..ece43aa9c 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -900,57 +900,38 @@ static void DeleteTreeNoFollow(string path) { /// worktree-metadata gate already documents as defeating a different path-identity check. /// static void EnsureSeparateRoots(string source, string snapshotRoot) { - // Probed on the SOURCE, which is the volume that matters: for the snapshot root to land inside the - // source it has to be on the source's filesystem. Never inferred from the OS — a normalisation- - // sensitive volume can be mounted on macOS and an insensitive one on Linux. - var foldNormalization = ProbeNormalizationInsensitive(source); - if (IsAtOrUnder(snapshotRoot, source, foldNormalization) || - IsAtOrUnder(ResolveDeepestExisting(snapshotRoot), ResolveDeepestExisting(source), - foldNormalization)) + if (IsAtOrUnder(snapshotRoot, source) || + IsAtOrUnder(ResolveDeepestExisting(snapshotRoot), ResolveDeepestExisting(source))) throw new InvalidOperationException("borrowed_snapshot_root_inside_source"); } - /// Whether 's filesystem treats composed and decomposed spellings - /// of the same name as one entry. - /// Probed rather than assumed, and the direction matters both ways. On a typical macOS volume - /// they ARE one directory, so comparing without folding lets a snapshot root spelled one way sit - /// inside a source spelled the other. On Linux they are genuinely DISTINCT directories, so folding - /// unconditionally would refuse a perfectly valid configuration — a fail-closed availability - /// regression rather than an escape, but a regression. - /// Falls back to folding when the probe cannot run: over-refusing a legitimate layout is the - /// better failure of the two. - static bool ProbeNormalizationInsensitive(string directory) { - // U+00E9 versus "e" + U+0301 — the same grapheme, two encodings. - var stem = "norm-probe-" + Guid.NewGuid().ToString("N"); - var composed = Path.Combine(directory, stem + "é"); - var decomposed = Path.Combine(directory, stem + "é"); - try { - using (new FileStream(composed, FileMode.CreateNew, FileAccess.Write, FileShare.None)) { } - return File.Exists(decomposed); - } catch { - return true; - } finally { - try { File.Delete(composed); } catch { } - try { File.Delete(decomposed); } catch { } - } - } - - /// Ancestry over path STRINGS, folding Unicode normalisation only when the filesystem does. - /// Case folding alone is not enough on a normalisation-insensitive volume: a typical macOS - /// filesystem treats café 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 then fail both the lexical and the resolved check and still - /// land the snapshot inside the source. Folding UNCONDITIONALLY is equally wrong in the other - /// direction — on Linux those are distinct directories and a valid layout would be refused — which is - /// why is probed rather than assumed. - /// This does not reach true filesystem identity — .NET exposes no portable device/inode pair — - /// so exotic equivalences remain in the same trusted-configuration residual as bind mounts and SUBST - /// aliases, documented on . - static bool IsAtOrUnder(string candidate, string root, bool foldNormalization) { - if (foldNormalization) { - candidate = candidate.Normalize(NormalizationForm.FormC); - root = root.Normalize(NormalizationForm.FormC); - } + /// 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); From 2bfd1ff81d81d6cbbfad518b5d84df799a03bcef Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:24:20 -0400 Subject: [PATCH 12/12] Apply the same cwd admission checks on the public sync overload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo finding: SyncFromSourceAsync ran rev-parse in a caller-supplied sourceCwd with no containment check of its own. The work-tree-top check inside ReadGitRelativeCwdAsync already refuses a foreign repository, so the security hole qodo describes is closed — but this overload still had no containment or existence check, so a bad cwd surfaced as a raw git failure instead of a coded error. Now mirrors CreateBorrowedSnapshotAsync. Co-Authored-By: Claude Opus 5 --- .../Services/WorktreeManager.cs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs index ece43aa9c..77e8f88c8 100644 --- a/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs +++ b/src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs @@ -562,8 +562,20 @@ public async Task CreateBorrowedSnapshotAsync( public async Task SyncFromSourceAsync( string sourceRepoRoot, string sourceCwd, string targetWorktreePath, string[] excludePaths, CancellationToken ct) { - var gitRelativeCwd = await ReadGitRelativeCwdAsync( - Path.GetFullPath(sourceRepoRoot), Path.GetFullPath(sourceCwd), 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"); + + var gitRelativeCwd = await ReadGitRelativeCwdAsync(source, cwd, ct); _ = await SyncFromSourceCoreAsync( sourceRepoRoot, targetWorktreePath, gitRelativeCwd, excludePaths, reviewContextRoot: null, ct);