[AI-1680] Quarantine branch-authored MCP config so a reviewer can still read it - #437
Closed
realtonyyoung wants to merge 17 commits into
Closed
[AI-1680] Quarantine branch-authored MCP config so a reviewer can still read it#437realtonyyoung wants to merge 17 commits into
realtonyyoung wants to merge 17 commits into
Conversation
Borrowed snapshots exclude vendor MCP config so no vendor executes it. The cost was that a reviewer could not SEE it either — and the change under review may BE that file. A pull request adding a hostile .kiro/settings/mcp.json was therefore invisible to the reviewer, which could then return clean on exactly the class of change the exclusion exists to defend against. Contained and reviewed are different properties and only the first held. Both now hold. An excluded MCP config is carried into the snapshot under a .kcap-quarantined suffix: readable content at a path no vendor looks for. The manifest gained an optional destination, so the copy, the outside-manifest sweep and the destination verification all agree on where the file lands. Reserved kcap state is untouched by this — .capacitor and .attached must not appear in a snapshot at all, and still throw rather than being quietly turned into a quarantined copy by the same path. Quarantined files are added to .git/info/exclude so they do not surface as untracked additions. A reviewer seeing phantom files would reasonably flag them, and they are kcap's doing rather than the branch's. The destination verification catching my incomplete first pass is worth noting: it compares hashes at the manifest key, so renaming the destination without telling it produced a hard failure rather than a silently wrong snapshot. That check earned its keep. Tests assert both halves — present and byte-identical at the quarantined path, ABSENT at the real one — because a change achieving only one would look like success from the other side. Mutation-proven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoQuarantine vendor MCP configs in borrowed snapshots for safe review
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Two reviewers, five findings between them; four are fixed here and one is a design question I have put to the operator rather than answering unilaterally. The disclosure one is mine and is the worst of them. The manifest source is `git ls-files -co`, which includes UNTRACKED files, so quarantining turned "drop a developer's local-only MCP config" into "copy it into a snapshot the reviewer and its model can read". Quarantine is for BRANCH-authored content — the thing a reviewer is there to judge — so it is now restricted to tracked paths and untracked config is dropped as before. Caught by Qodo; the other reviewer missed it. Destination collisions are now checked independently of source keys. A repo containing both `.mcp.json` and `.mcp.json.kcap-quarantined` maps two sources onto one destination: one overwrites the other, and with identical contents it would even pass verification while silently materialising a single file. A branch can add the colliding name deliberately, so this refuses rather than guesses. The tests were not hermetic: a default DaemonConfig points WorktreeRoot at ~/.capacitor/worktrees, so they were writing borrowed snapshots into real developer state and leaving them. Rooted in a temp directory now. The git capture helper also ignored exit codes, which would report a FAILURE as empty output — and an assertion about absence passes for the wrong reason when the command never ran. Both new guards mutation-proven. Still open, deliberately: a branch `.gitignore` carrying `!*.kcap-quarantined` overrides .git/info/exclude, so the copy appears in git status and `git add -A` stages it into the reviewer's commit. Nothing placed inside the worktree can escape branch-controlled ignore rules, so the fix is to move quarantined content to a sidecar beside the snapshot — which costs the reviewability this PR exists for until the flows layer tells the reviewer where to look. That trade is the operator's call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Decided in-tree over a sidecar. Nothing inside the worktree escapes branch-controlled ignore rules, so a branch can force the quarantined copy into git status and into a reviewer's `git add -A`. A sidecar would close that, but it is invisible to the reviewer until the flows layer points at it — which removes the only thing this change exists to deliver. Weighed on actual harm rather than the label: the residual is diff noise, in a commit reviewers rarely make, of content already present in the branch under review. Execution — the thing every round of this work has been about — is still prevented by the suffix. Recorded where the suffix is defined and filed, instead of being quietly patched around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tity Built with OrdinalIgnoreCase over normalized paths, the tracked set case-folds. On a case-sensitive filesystem — every Linux daemon — an index-tracked .Cursor/mcp.json that is absent on disk admits an untracked, developer-local .cursor/mcp.json as 'tracked', quarantining private config into a snapshot the reviewer and its model can read. Form C folding has the same shape. Compare git's own -z bytes with Ordinal. Case-insensitive comparison stays where it is correct: destination collisions, where the question is whether two entries can occupy one path on this filesystem. The test only reproduces where both spellings can coexist, so it skips on macOS/Windows and runs on Linux CI. Verified by mounting a case-sensitive APFS volume: it fails against the OrdinalIgnoreCase comparer and passes against this one, with an in-run positive control so a run that never quarantined anything cannot pass the absence assertion for the wrong reason.
Collaborator
Author
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
Destinations were reserved after the tracked-deletion skip, so a path tracked in HEAD but deleted in the working tree never claimed its name. With HEAD tracking both .mcp.json and .mcp.json.kcap-quarantined and the latter deleted, the skip freed the slot and the quarantine copy landed on it — the snapshot then shows a reviewer a MODIFIED file where the working tree has a deletion, and verification passes because the deleted path is absent from the manifest. Reserve before the skip: a tracked-but-absent path still owns its name, so this refuses exactly as it already did when both files are present. Both spellings are branch-authored, so refusing is the same answer either way. Test verified by mutation — moving the reservation back after the skip fails it — with a positive control asserting the same shape minus the collision still builds and quarantines.
The tracked check proves the INDEX SPELLING; it says nothing about where the bytes come from. `File.Exists`, `FileInfo` and the read that follows all resolve through a symlinked PARENT, so a tracked `.cursor/mcp.json` whose `.cursor` has since been replaced by a link to somewhere outside the repo still reports as a cached child from `ls-files -co` — looking tracked and branch-authored — while the content read comes from the link's target. Quarantine then publishes it to the reviewer, reopening exactly the local-config disclosure the tracked gate was added to close. Walk components one at a time without following, mirroring FirstRemovableComponent in the workspace-MCP partial. Verified by mutation: removing the check fails the new test. Also (P3): the class now removes the temp roots it creates. Each test needs a fresh repo, so a full run was leaving a cloned repository behind per test.
Fifth closure of one surface, and the previous four were all the same shape: the gate stayed correct about the NAME while something else decided the BYTES. The tracked check authorised git's exact `raw`, but the read used `rel`, and the normalizer folded `\` to `/`. Backslash is a legal Unix filename character, so a branch could track a decoy literally named `.cursor\mcp.json`, satisfy the tracked check on that spelling, and have it rewritten into `.cursor/mcp.json` — opening, hashing and quarantining the developer's UNTRACKED local config. Both manifest passes applied the same substitution, so verification agreed. Structural fix rather than a sixth spelling patch: the validator validates and returns git's path unchanged, so the identity that passes the check is the identity that is read. A path we would have to rewrite to use is one we do not understand, and is refused. A call-site sweep then found the rest of the family: - On WINDOWS the filesystem performs the substitution managed code no longer does — ContainedPath maps `/` to the platform separator, so a Linux-authored index entry with a literal `\` is resolved there as a directory boundary and redirects the read anyway. Refused: git cannot check such a path out on Windows, so there is nothing faithful to build. - The classifiers folded `\` to `/` too, so a top-level file named `.cursor\mcp.json` was treated as vendor config — quarantine would rename an ordinary tracked file and misrepresent the branch. Compared as-is now. Unix case mutation-verified: restoring the rewriting normalizer leaks the secret. The Windows case runs on the CI runner and skips elsewhere.
`git hash-object -w --stdin` inherited the test host's stdin, which fails on a
CI runner ('Unable to add x to database'). The test skips on macOS before the
fixture runs, so the mistake could only surface on the Windows leg — the fixture
mechanics are now validated against a real file instead.
Chased down from what a review round was probing when its own content filter killed it mid-run: git output encoding. A git path is bytes. On Linux — where the daemons run — a filename need not be valid UTF-8, and the index carries whatever bytes it was given, so `ls-files -z` can emit a path that decodes to U+FFFD. Re-encoded for the syscall that becomes EF BF BD, File.Exists says no, and the entry was skipped as a tracked deletion. Both manifest passes agreed, so nothing looked wrong. Silent is the problem. A borrowed snapshot exists so a reviewer can SEE the change; a branch must not be able to hide a tracked file from them by naming it un-decodably. Same rule already applied to a filter driver name that cannot round-trip: what we cannot represent, we refuse. The test delivers the path through `update-index --index-info` on STDIN. A process argument cannot carry those bytes — .NET encodes the string it is given, so `ÿ` in an argument reaches git as valid UTF-8 (C3 BF) and the path decodes cleanly. The first version of this test did exactly that and passed against code with no guard at all; it is mutation-verified now.
…ency Raised by review on the sibling PR (#432) against the filter-driver guard, but ValidateRelativePath rests on exactly the same thing: it refuses a path that cannot round-trip by detecting the U+FFFD a UTF-8 decoder emits for an invalid sequence. NewGitPsi never set StandardOutputEncoding, so decoding was whatever the ambient console encoding happened to be — and a Windows codepage maps 0xff to an ordinary character, leaving no U+FFFD to detect. The path would be accepted, then silently skipped. Same fix, same reason: pinned to UTF-8 with replacement fallback. Whichever of the two PRs lands first, the other's rebase is trivial.
realtonyyoung
commented
Aug 2, 2026
| // for an invalid sequence. Left implicit, redirected output decodes with the ambient console | ||
| // encoding — a Windows codepage maps 0xff to an ordinary character, no U+FFFD appears, and the | ||
| // path is silently accepted and then silently skipped. Replacement fallback makes it sound. | ||
| StandardOutputEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, |
Collaborator
Author
There was a problem hiding this comment.
[P1] Read path-oriented git output as bytes; StandardOutputEncoding still loses a leading BOM
Process.StandardOutput is a StreamReader created with BOM detection enabled, even when StandardOutputEncoding is set. If the first ls-files -z pathname starts with UTF-8 EF BB BF, the reader consumes those bytes instead of returning U+FEFF. Both the manifest and tracked-set calls therefore authorize the same stripped name, ValidateRelativePath sees no U+FFFD, and the filesystem operation acts on a different path. With no stripped path this is silently treated as a tracked deletion; with an ignored local .cursor/mcp.json at the stripped spelling it can be opened and quarantined instead. Capture these path-bearing commands from StandardOutput.BaseStream and strict-decode each NUL-delimited record (or compare the decoded path against the original bytes); pinning the text encoding alone cannot make this round-trip check sound.
realtonyyoung
commented
Aug 2, 2026
| // Destination names, not source keys — a quarantined entry lands under a different path and | ||
| // would otherwise be swept straight back out as "outside the manifest". | ||
| RemoveFilesOutsideManifest(destination, | ||
| manifest.Select(static e => e.Value.DestinationRelative ?? e.Key), ct); |
Collaborator
Author
There was a problem hiding this comment.
[P1] Do not case-fold the effective-destination sweep
These effective destinations are passed to RemoveFilesOutsideManifest, which builds keep with StringComparer.OrdinalIgnoreCase. A staged case-only rename on a case-sensitive source (Foo -> foo) leaves Foo from the cloned HEAD in place because it is considered accepted, then copies foo beside it. The tracked Foo remains unchanged and the changed foo is merely untracked, so a normal git diff can show no change at all. On a case-insensitive volume, FileMode.Create can instead overwrite the old-cased directory entry and verification resolves the same alias, again accepting an approximation. Preserve exact directory-entry identity where the filesystem supports it, and otherwise perform the rename faithfully or refuse the snapshot.
realtonyyoung
commented
Aug 2, 2026
| // separates, and a backslash is just a character in a name. | ||
| return WorkspaceMcpConfigPaths.Any(path => | ||
| rel.Equals(path, StringComparison.OrdinalIgnoreCase) || | ||
| rel.EndsWith("/" + path, StringComparison.OrdinalIgnoreCase)); |
Collaborator
Author
There was a problem hiding this comment.
[P2] Use the same path identity when suppressing the quarantined source
This classifier case-folds on every platform, so on Linux a tracked, present .Cursor/mcp.json is quarantined and the original is swept. ApplyReservedIndexPolicyAsync, however, only runs update-index --skip-worktree for the canonical .cursor/mcp.json; that exact pathspec is absent on a case-sensitive repo, the failure is swallowed, and the snapshot reports a kcap-created deletion of .Cursor/mcp.json. That is the same authorize/use split: one spelling decides to quarantine, another spelling performs the index action. Carry the exact quarantined source paths from the manifest into the index policy (and classify using the actual filesystem’s case semantics).
realtonyyoung
added a commit
that referenced
this pull request
Aug 2, 2026
…tection .NET builds the redirected reader with detectEncodingFromByteOrderMarks enabled regardless of StandardOutputEncoding. Measured: feeding "<BOM>A\0<BOM>B" through the reader yields "A\0<BOM>B" — the FIRST BOM is swallowed, later ones survive. U+FEFF is legal in a git path and in a config subsection, so that silently rewrites the first record of a -z listing, and the authorised name stops being the used name. The capture path now drains BaseStream and decodes it itself. On reachability, stated no more broadly than measured: `config --list` emits system and global config first, which a branch does not control, so the filter inventory cannot be reached this way today. `ls-files` has no such prefix and IS reachable — the regression test belongs in #437, where it can actually fire. I wrote a test here first; it passed against the unfixed code, so it was asserting nothing and has been removed rather than shipped. The fix stays because the helper is shared and the property should not depend on which caller is safe. Also closes the round-13 documentation finding: the XML doc and the test doc still carried the overstatement the README had already dropped. Swept for all copies this time. (README:1143 is about MCP config, a different mechanism — that file is physically neutralised in the worktree, so it is accurate.)
…thority
StandardOutputEncoding does not disable .NET's BOM detection — the redirected
reader is built with detectEncodingFromByteOrderMarks enabled. Measured:
"<BOM>A\0<BOM>B" reads back as "A\0<BOM>B", first one consumed.
The full chain, measured end to end:
- the branch tracks ONE file, `.cursor/mcp.json`
- `git ls-files -z` (the tracked authority) therefore LEADS with the BOM
- the reader strips it, so the tracked set holds `.cursor/mcp.json`
- the manifest from `ls-files -co -z` lists the developer's UNTRACKED
`.cursor/mcp.json` first (ASCII sorts before 0xEF)
- `tracked.Contains(...)` matches a name git does not have
- the untracked secret is read and quarantined into the reviewer's snapshot
The capture path now drains BaseStream and decodes it itself.
Ordering is the vulnerability, so the fixture has to reproduce it: the shared
NewRepo() commits a `.gitkeep`, which sorts before a BOM and would keep the
decoy off the front of the listing. The precondition is asserted in BYTES —
checking it through a StreamReader would hide the exact behaviour under test.
Mutation-verified: restoring the reader leaks the secret.
…isions Both from review; both are fidelity bugs that show a reviewer the wrong tree. 1. The destination sweep folded case, so an UNCOMMITTED case-only rename kept its stale spelling: HEAD holds `notes.md` and the clone checks it out, the manifest holds `Notes.md`, both exist on a case-sensitive filesystem, and the sweep decided the old one was wanted. The rename then does not appear in `git diff` at all. Plain Ordinal is not the answer either — where case does not distinguish files, writing `Notes.md` lands on the same inode and enumeration reports the original spelling, so an exact compare would delete the file just written. So PROBE the destination. A case-sensitive volume on macOS and a case-insensitive mount on Linux both exist, and inferring from the OS gets both wrong. 2. skip-worktree iterated the canonical lowercase list, so it marked nothing when the index held `.Cursor/mcp.json` — and kcap's own exclusion then showed up as a DELETION in the reviewer's tree. Driven from the index's own spellings now. Both mutation-verified, (1) on a mounted case-sensitive APFS volume since macOS cannot otherwise hold the two spellings. The first version of (1) committed the rename, which removes the conflict entirely — it passed against the unfixed sweep and was proving nothing.
…pped Both defects were introduced by my previous commit. 1. The case probe used a FIXED `.kcap-case-probe` and checked `.KCAP-CASE-PROBE`. kcap-cli is public, so a branch shipping a tracked `.KCAP-CASE-PROBE` makes the check find its file: a case-sensitive filesystem reports as insensitive, the sweep falls back to folding, and the stale-spelling bug I had just fixed comes straight back. Random per-probe name now, with both spellings confirmed absent first, and insensitive as the conservative fallback since the sweep then keeps rather than deletes. I have this recorded as a lesson from AI-899 and this is the second time in this branch of work I have written a fixed sentinel anyway. 2. skip-worktree classified EVERY indexed path, and IsWorkspaceMcpConfigPath matches on a `/`-suffix — so `fixtures/.mcp.json` qualified. That file is neither excluded nor quarantined by the root-prefix manifest rules, and marking it before its working-tree bytes are copied over hid a REAL modification from `git status`. It now uses the manifest entries actually remapped to a quarantine destination, which is the exact set intended. Both mutation-verified on a mounted case-sensitive APFS volume. The spoof test's first fixture shipped BOTH probe spellings, which trips the destination-collision refusal — it failed even against fixed code, for an unrelated reason. Note, pre-existing and unchanged: the manifest and destination sets both fold case, so a repo legitimately holding two paths differing only by case cannot be snapshotted on a case-sensitive filesystem. That predates this PR (the manifest dictionary has always folded); flagging rather than widening scope here.
1. The case-folding limitation DOES belong to this PR — I was wrong to call it pre-existing. Reserving destinations before the tracked-deletion skip (the fix for a real hole) means a repo tracking both `Foo` and `foo` with one spelling deleted now aborts, where before the absent entry never reached the manifest dictionary at all. My change widened the limitation rather than inheriting it. The destination set matches the destination filesystem now: two entries contend for one path only where the filesystem says they do. 2. A filename may legitimately CONTAIN U+FFFD — EF BF BD is valid UTF-8 that decodes to exactly the replacement character — so testing the decoded string could not tell it from an invalid byte, and an ordinary `notes<U+FFFD>.md` was refused. Decided on the BYTES now: each NUL-separated record is decoded STRICTLY, and only a genuine decode failure refuses. That deletes the heuristic instead of narrowing it. 3. The quarantine exclude used `*.kcap-quarantined`, which suppresses every untracked file with that suffix — a developer's own `fixtures/result.kcap-quarantined` was copied into the snapshot and then vanished from `git status`, hiding genuine dirty context. Exact destinations now, rooted and escaped, since a branch chooses these names and gitignore has its own metacharacters. All three mutation-verified, (1) on a mounted case-sensitive APFS volume. The suffix test needed `status --porcelain -uall`: plain porcelain collapses untracked files to `?? fixtures/`, which would have passed while proving nothing.
The bytes helper I added last commit redirected stderr and never read it: enough stderr output fills the pipe and git never exits. It also skipped the kill-and-grace the established capture has, so a timeout could leave `git ls-files` running after snapshot creation unwound. Both bugs exist only because the logic was duplicated, so there is one implementation now — a raw-bytes core with the timeout, kill and grace handling, with the string and bytes shapes as thin wrappers over it. Byte callers take stdout as-is; string callers decode from the same bytes. stderr is drained concurrently in both cases and kept for the failure message.
realtonyyoung
added a commit
that referenced
this pull request
Aug 3, 2026
…ree creation (#432) * Disable branch-resolvable clean/smudge filters during worktree creation Follow-up to the worktree containment work. That change stopped branch-authored MCP config from executing and stopped git HOOKS from running during creation, but hooks are only one of the mechanisms by which `git worktree add` can execute branch content. `.gitattributes` is branch content and SELECTS which filter driver applies to a path. The driver's command comes from the operator's config, but a relative command resolves against the worktree — so the branch supplies the executable. core.hooksPath does not affect filters at all, so the existing guard does nothing here. Measured: `filter.x.smudge=./tools/f` plus a branch-committed `tools/f` runs during `worktree add`, before anything has neutralised the tree. Only BRANCH-RESOLVABLE drivers are disabled. Turning off filters wholesale breaks git-lfs, which then yields pointer files instead of content, silently. The distinguishing property is whether the command resolves to branch content, so a relative command is neutralised and a PATH- or absolutely-resolved one is not. Every token is examined, not just the executable: `sh -c 'cat ./tools/x'` has a PATH-resolved executable and a branch-controlled payload. Enumerating the operator's config is sound rather than best-effort: a filter can only run if it is DEFINED there, and the branch can only select from what is defined. `required` is cleared alongside the command. Measured: with `filter.x.required=true` an empty smudge is FATAL and worktree creation fails outright, so suppressing the command alone would turn this guard into a launch failure for any repo using a required filter. The other half of the tracked issue — git's config-based hooks (hook.<name>.event/.command) — is NOT implemented, because it is not a shipped git feature. Measured on git 2.49.0: the config is undocumented in `git help config` and a configured post-checkout hook does not run. It was an unmerged proposal. Recorded on the issue so nobody builds a guard for something that cannot fire. Tests cover both directions of the classifier, the git-lfs regression, a mixed config, a dotted driver name, and an end-to-end case whose control must execute the filter first. That control earned its keep: the first version named the filtered file `data.txt`, which git checks out BEFORE `tools/f` exists, so the exec merely failed and the control did not fire. The attack is ordering-dependent, and the test now says so. Both classifier rules mutation-proven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 1: allowlist driver names instead of classifying commands Review defeated the command classifier four ways, and it is right that a tokeniser cannot do this job: `sh tools` and `python filter.py` execute a branch-supplied file with no path separator at all; `/bin/true;./tools/f` is a single rooted token whose shell runs the relative half; and `%f` is substituted by git AFTER any inspection we could perform. A filter command is a shell program. Deciding what a shell program will execute is not something string analysis can do, so the classifier is gone rather than patched — the fourth time in this area that replacing a string-comparison mechanism was the fix. Containment is now an allowlist of driver NAMES. A branch can only select from drivers the operator already defined, and whether one of those is trusted is the operator's answer to give, not ours to infer. `lfs` is allowlisted because git-lfs is ubiquitous and disabling it fails SILENTLY, yielding pointer files instead of content. Any other custom driver is disabled inside agent worktrees; the README says so plainly. This also removes the false-positive class review flagged separately — `~/.local/bin/filter`, `$HOME/bin/f` and `--endpoint=https://host/path` were all condemned by the separator rule — because no command is parsed. Three further findings, all accepted: Enumeration ran with sourceReadOnly, which sets GIT_CONFIG_NOSYSTEM and would hide a system-scoped driver that is live during materialization. It now runs with the same config visibility as the command it guards. The snapshot checkout executes in `destination` but enumerated from `source`, so a conditional includeIf.gitdir matching the snapshot would introduce a driver the query never saw. It now enumerates its own context. Enumeration is `--name-only -z`, so values never enter the parse. A line-splitting inventory reads a value of `cat\n./tools/f` as a safe record plus an ignored line while git executes the whole thing; names cannot carry that. And only exit 1 — git's "no key matched" — is treated as "no drivers". Any other failure now throws rather than continuing with an empty override set, which would have run the materialization with every driver live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 2: authenticate the lfs binding; refuse unencodable names Three findings, all real, all fixed. The allowlist trusted a NAME, and a name is a convention rather than an identity. Nothing stops a config defining filter.lfs.smudge=./tools/f, and a branch selecting filter=lfs would then ride the allowlist straight to its own file — the same mistake as trusting a command string, reached from the other side. The binding is now authenticated: an allowlisted driver must actually invoke `git-lfs`, decided on the first token's filename. That is NOT a return to the general command classification that was removed; it is the far narrower question "is this the one binary we decided to trust", and anything unreadable or unexpected fails closed and is disabled like any other driver. `-c key=value` splits at the FIRST '='. A driver legally named `evil=x` was therefore written as key `filter.evil`, leaving `filter.evil=x.smudge` live while the override looked applied. Git permits arbitrary subsection characters, so such a name is now refused rather than mis-encoded — a guard that silently does nothing is worse than a launch that fails. The standalone path had no rollback. Every step after the directory exists can throw — the MCP strip and the filter inventory are both fail-closed — and that branch returns no WorktreeInfo, so nothing downstream could remove the partial tree. The linked paths gained rollback earlier; this one never did. Both new guards mutation-proven: dropping the binding check fails all three wrong-binary cases and nothing else, dropping the name check fails only the unencodable-name case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 3: rebind the allowlisted driver instead of vetting it The binding check was unsound, as review said and as I suspected when I asked. Git runs the whole filter value through a shell, so `git-lfs smudge -- %f; ./tools/f` passes any first-token test and then executes branch content; a branch-owned `/repo/tools/git-lfs` passes by basename; and a bare `git-lfs` can be shadowed when the inherited PATH has a relative component. Each is a way for a string to look like the binary without being it — the same class of defect as the command classifier it replaced, one step smaller. So nothing the operator wrote is executed any more. An allowlisted driver is REBOUND to kcap's own command, built from a git-lfs path the daemon resolves itself and verified absolute. Their value is simply overwritten for the guarded commands, which removes the question rather than trying to answer it. If git-lfs cannot be resolved there is nothing trustworthy to substitute, so the driver joins the disable set — pointer files, never an unvetted execution. Accepted cost, now in the README: an operator who wraps git-lfs behind their own script loses that wrapper inside agent worktrees. That wrapper is precisely the branch-reachable indirection this removes. The lfs tests assert BOTH branches rather than whichever the host happens to have, since this machine has no git-lfs and a suite that silently exercised neither would prove nothing. Keeping the refusal of `=`-containing driver names. Review rates the GIT_CONFIG_KEY_n/VALUE_n transport the better answer and it is right, but that is a change to how every git invocation here carries config, and folding a transport rewrite into a round-3 security fix is how the earlier mechanisms in this area went wrong. The refusal is fail-closed and affects only a pathological name; the transport is filed as the follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 4: remove the LFS exemption — four designs, four holes Round 4 found three more, two P1, and all three were in the exemption again: the daemon's PATH is ambient (a shell started inside a repo with a relative or repo-local entry resolves a branch-owned git-lfs to an absolute path and it is rebound as trusted); the resolved path was interpolated unquoted into what is, again, a shell program; and resolving twice left a fail-open window where lfs was neither disabled nor rebound. That is the fourth mechanism in four rounds and every one failed at the same place: classify the command -> shell syntax defeats tokenisation allowlist the name -> a name is a convention, not an identity authenticate the binding -> git runs the value through a shell rebind to a resolved path -> the resolver's PATH is itself ambient The exemption was the defect, not the implementations of it. So there is none. Every defined clean/smudge/process driver is disabled for the guarded commands. Nothing is parsed, nothing is resolved, nothing is authenticated, and there is no name left to impersonate — the property four narrower designs failed to achieve. The cost is real: LFS-tracked files check out as pointer text inside agent worktrees, and a custom filter does not run there. It is in the README and logged per worktree, because a silent content change is the objection I raised against this option originally and logging is what answers it. Deletes more than it adds — no allowlist, no binary constant, no resolver, no authentication, no substitution, and the quoting and double-resolve questions cease to exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make the enumeration test independent of the host's git config CI failed on both platforms with `A_repo_with_no_filters_yields_no_overrides`: the runners have git-lfs installed, so a global filter.lfs.* is in scope and a repo with no filters OF ITS OWN still has filters. Enumeration reads EFFECTIVE config, which is the whole point of it. The test encoded my laptop. It passed here because this machine has no git-lfs, and there is no amount of local running that would have caught it. Replaced with one that computes the expected driver set from git itself and asserts the overrides cover exactly that — no more, no fewer. That is a stronger property than "empty" and it holds on any host. Verified by reproducing the CI environment rather than assuming: re-ran with a temporary HOME carrying a global filter.lfs.* definition, 13/13. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 5: enumerate in the target context; restore deleted README Four findings, and one of them was damage I did. The README edit deleted the "Detach" and "Permissions" bullets. My slicing overshot and took two unrelated pieces of user guidance with it, and they appear nowhere else. Restored; the diff against main now deletes nothing. The doc comment on the hook guard still said clean/smudge filters were deliberately NOT closed and that blunt disabling would break LFS — which is exactly what this PR now does. A comment contradicting the code beside it is worse than none, and it was my own text from three rounds ago. The real one: inventorying the SOURCE while checking out in the TARGET is a context mismatch. Git runs the checkout in the new worktree, where an `includeIf "onbranch:capacitor/**"` or a gitdir-matching conditional include can expose a driver the source never reported — the branch selects it and it runs. Fixed by splitting the add from the checkout: `worktree add --no-checkout` materialises nothing, the inventory is then taken IN the new worktree, and the guarded `reset --hard HEAD` is what populates it. This is the third time in this area that aligning enumeration with the context of execution was the fix. The populate step lives inside the existing rollback, so a filter-inventory failure cleans up like any other fail-closed step. `reset --hard` rather than `checkout -- .`, because --no-checkout leaves the index unpopulated and a pathspec matches nothing — found by running it, not by reasoning. The remaining P2 — a driver defined BETWEEN the inventory and the materialisation — is accepted and not closed. The actor who would have to race it is the operator editing their own config mid-launch; the branch cannot influence it, and the window is now two adjacent commands in the same worktree rather than spanning a network fetch. Mutation-proven: dropping the target-context inventory fails the end-to-end filter test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 6: drop the now-redundant source-context inventory One finding, P2, no P1 — and it is dead code I left behind when the target-context fix landed. `worktree add --no-checkout` materialises nothing, and the guarded reset re-inventories inside the TARGET, which is the context that actually decides which drivers load. The source-context inventory therefore has no containment role at all. Worse, it can REJECT a safe launch: a source-only `includeIf onbranch:main` defining a driver with an `=` in its name throws before the target is even created, though the capacitor/** target would never load it. The comment beside it also still claimed source definitions were "the only ones a branch's .gitattributes can select", which the target-context fix had already disproved — my own text, one round stale again. Removed from both linked-worktree paths. The standalone path keeps its inventory: it materialises through `add -A` rather than a guarded reset, so that is the context where its filters resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 7: restore disabled-filter logging on the standalone path One finding, P2, and it is a regression from round 6. Removing the source-level LogDisabledFilters left the standalone path inventorying and applying overrides inline with no logging — so a standalone worktree would silently disable LFS and custom filters, contradicting the README, which promises the effect is visible rather than mysterious. Silence is the specific failure mode this logging exists to prevent: the whole no-exemption design is defensible only because the operator can see it happen. Computed into a local, logged, then passed to `add -A`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 8: one logged entry point for filter overrides Same class as round 7, in the third path. I fixed standalone and missed the borrowed snapshot, so a reviewer on an LFS host would still have got pointer files silently. Two rounds of the same defect is the signal to stop fixing instances. Computing the overrides and logging them are now one operation, and all three materialising paths route through it. A path cannot apply overrides without logging them because there is no longer a way to ask for one without the other — which is the property I should have reached for when round 7 found the first occurrence rather than patching the site in front of me. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review round 9: stop claiming an outcome that is false for one of the three paths The shared log message said LFS-tracked files appear as pointer text. True for an owned worktree and a standalone snapshot; false for a borrowed snapshot, which overwrites checkout files from the source manifest and rejects source-side pointers outright. Say what was disabled and why, not what the bytes will look like. This logging is the whole reason the no-exemption trade is defensible, so it has to be accurate; the README now distinguishes the paths. * Review round 10: the log makes no claim about resulting bytes at all Round 9 replaced one false outcome sentence with a narrower one that is still false: three callers materialise content three different ways — owned checks out through git, standalone copies source bytes and re-commits with the clean filter off, borrowed overwrites from the source manifest and refuses source pointers — so any single sentence about the resulting bytes is wrong for at least one. Two rounds were spent narrowing that sentence before concluding it should not be in a shared log line at all. The README documents the three paths separately, where there is room to be exact. Also pins git's config canonicalization: a review raised that the enumeration regex is lowercase while git config section and variable names are case-insensitive. Measured — git canonicalizes both to lowercase on output, so [Filter "evil"] Smudge is reported as filter.evil.smudge and the override neutralises it (verified end-to-end: the driver executes without the override and does not with it). Subsection case is preserved and covered by the pattern. The test fails if a future git ever reports keys verbatim. * Fix a live bypass of the guard's own inventory: enumerate without a regex A driver name is bytes, not text. git config accepts a subsection containing a raw 0xff, and `--get-regexp` runs the platform regex in the ambient locale where `.` will not span a byte invalid in that encoding. Measured: with [filter "ev\xffil"] defined, `^filter\..*\.(clean|smudge|process)$` returned NOTHING while `^filter\.` found the key. Empty inventory, no overrides emitted, and a branch selecting that driver in .gitattributes executed it — the exact bypass this file exists to prevent, arriving through the enumeration instead of the command. Enumerate every key with `--list --name-only -z` and match the shape in C#: no pattern, no locale, nothing to slip past. `--list` succeeds on an empty config, so the old exit-1 tolerance goes with it. Such a name also cannot round-trip through a UTF-8 string, so an override built from it would name a DIFFERENT driver while looking applied. That refuses now, alongside the existing `=`/newline/NUL guard, for the same reason: a guard that silently does nothing is worse than a launch that fails. Found while probing a reviewer's mixed-case question. The test carries a control proving plain git really does run the 0xff-named driver, and fails against the previous enumeration. * Pin git output decoding, and scope the README to what the code guarantees Both from review. 1. The containment fails closed on a driver name that cannot round-trip, and it detects that by the U+FFFD a UTF-8 decoder emits for an invalid sequence. But NewGitPsi never set StandardOutputEncoding, so decoding was whatever the ambient console encoding happened to be. A Windows codepage maps 0xff to an ordinary character: no U+FFFD, no refusal, and an override emitted that names a DIFFERENT driver while looking applied. Pinned to UTF-8 with replacement fallback, which is what makes the check sound. The existing end-to-end test needs a POSIX shebang and skips on Windows — precisely the platform at risk — so the refusal now has a cross-platform test with no execution in it. Its first precondition used EffectiveDriverNames, which asks git's own --get-regexp and is blind to this key; that failed, which is a neat independent reproduction of the enumeration bug. 2. The README said no custom filter driver runs "inside an agent worktree". The overrides are per-command, applied to the git commands kcap uses to create and populate the worktree; git the agent runs there afterwards uses the repo's own config. That window — branch content first materialised, before an agent is running — is the real boundary, and the README now says so instead of claiming a property the code does not provide. * Read git output as bytes: StandardOutputEncoding does not stop BOM detection .NET builds the redirected reader with detectEncodingFromByteOrderMarks enabled regardless of StandardOutputEncoding. Measured: feeding "<BOM>A\0<BOM>B" through the reader yields "A\0<BOM>B" — the FIRST BOM is swallowed, later ones survive. U+FEFF is legal in a git path and in a config subsection, so that silently rewrites the first record of a -z listing, and the authorised name stops being the used name. The capture path now drains BaseStream and decodes it itself. On reachability, stated no more broadly than measured: `config --list` emits system and global config first, which a branch does not control, so the filter inventory cannot be reached this way today. `ls-files` has no such prefix and IS reachable — the regression test belongs in #437, where it can actually fire. I wrote a test here first; it passed against the unfixed code, so it was asserting nothing and has been removed rather than shipped. The fix stays because the helper is shared and the property should not depend on which caller is safe. Also closes the round-13 documentation finding: the XML doc and the test doc still carried the overstatement the README had already dropped. Swept for all copies this time. (README:1143 is about MCP config, a different mechanism — that file is physically neutralised in the worktree, so it is accurate.) * Correct the last copy of the LFS overstatement Round 14: no functional findings, one Low — a third copy of the claim that LFS files become pointer text in 'agent worktrees'. True only for owned worktrees; standalone and borrowed snapshots carry the source's own bytes. I said I had swept for all copies last round and had not: I grepped the PHRASING I remembered writing rather than the concept, so a differently-worded copy survived. Swept on 'lfs' + pointer/worktree this time. (WorktreeManager.cs:390 says 'an operator WHOSE LFS-tracked file checks out as pointer text' — conditional, so it is accurate and stays.) * Drop the stale count in the neutralisation list Round 15: no functional findings; the list said 'Two things' and the filter bullet made it three. Removed the count rather than bumping it — a hand- maintained count next to a list drifts every time the list changes, which is exactly what happened here. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…refusals 1. P1, and the worst shape found on this PR — a DESTRUCTIVE write outside the snapshot rather than a disclosure. The clone materialises HEAD, so the destination can already hold a symlink where a quarantine copy is about to land, and FileMode.Create follows it and truncates the target. With `.mcp.json.kcap-quarantined` committed as a link to an external file and the source staging its deletion, the collision set never sees it (gone from the source tree) while the clone still creates it. Checking Directory.Exists caught only the directory case. Any link at the destination is removed, and a linked PARENT refuses, before the write. Mutation-verified: without it the external file really is truncated. 2. `.git/info/exclude` is line-oriented with no escape for a newline, so a tracked path containing one injects extra patterns and hides unrelated untracked context from the reviewer. Legal on Unix, unrepresentable here — refused, like the other identities we cannot reproduce. 3. Classification answers "will a VENDOR open this?", so the filesystem's case rule governs it. On Linux `.Cursor/mcp.json` is a different file no vendor resolves, yet folding quarantined it, marked it skip-worktree, and removed it from `git status` and normal diffs. Both the exclusion gate and the classifier take the probed sensitivity now, and the two classifier copies are one. The newline test needed `update-index -z --index-info`: the default record format is newline-TERMINATED, so the path cannot be expressed at all. The case test first passed against a mutated classifier because the exclusion gate short-circuits ahead of it — the honest mutant is the pre-fix state, both gates folding, and it fails then.
#432 shipped the same BaseStream/BOM fix independently, so all three conflicts were that fix expressed twice plus my later refactor of it. Resolved hunk by hunk rather than taking a side, since the auto-merged regions carry #432's other changes: - the capture keeps the raw-bytes core with concurrent stderr draining (the refactor that removed a duplicated helper), and carries over #432's measured note that `config --list` cannot reach the BOM condition while `ls-files` can; - the strict NUL decoder and the byte/string wrappers are kept — they replaced the inline helper #432 still had; - the NewGitPsi comment takes main's wording, now the canonical text. Verified rather than assumed: exactly one definition of each of RunGitCaptureRaw/Result/Bytes, ReadAllDecodedAsync, ReadAllBytesAsync, DecodeNulSeparatedStrictly and GitOutputEncoding — and #432's own suite, BranchFilterContainmentTests, passes 18/18 against the merged file alongside this PR's 24. A clean build proves neither side's behaviour survived; those tests do.
Collaborator
Author
|
Closing this. The in-tree quarantine approach has a wrong premise, and after twelve review rounds it is not converging. Successor issue: AI-1706. The gap this PR addressed is real and stays open — a reviewer still cannot see vendor MCP config, and the change under review may be that file. Only the mechanism is being abandoned. Why1. A 2. It spread. The suffix mechanism reached into manifest identity, destination mapping, index flags, ignore-file syntax, filesystem case probing, output encoding, collision handling, and symlink safety on both source and destination sides. Nine distinct disclosure paths were closed, plus one destructive bug (a write escaping the snapshot to truncate an external file) — and rounds 8 through 12 were dominated by defects the previous round's fix had introduced. A parent-component symlink write is still unfixed as of this close. DirectionSeparate execution containment from review discoverability instead of overloading the worktree with both: keep excluding the config from the snapshot (today's behaviour on Worth salvagingSeveral fixes here are independently valuable and are listed on AI-1706 for re-landing: per-component destination symlink refusal before The branch is left in place for reference. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Borrowed snapshots exclude vendor MCP config so no vendor executes it (AI-1632). The cost was that a reviewer could not see it either — and the change under review may be that file.
So a pull request adding a hostile
.kiro/settings/mcp.jsonwas invisible to the reviewer, which could then returncleanon exactly the class of change the exclusion exists to defend against. Contained and reviewed are different properties, and only the first held.Both now hold
An excluded MCP config is carried into the snapshot under a
.kcap-quarantinedsuffix: readable content, at a path no vendor looks for. The manifest gained an optional destination so the copy, the outside-manifest sweep and the destination verification all agree on where the file lands..capacitorand.attachedmust not appear in a snapshot at all, and still throw rather than being quietly turned into a quarantined copy by the same code path..git/info/exclude, so they do not surface as untracked additions. A reviewer seeing phantom files would reasonably flag them, and they are kcap's doing rather than the branch's.A note on the existing integrity check
VerifyDestinationManifestAsynccompares hashes at the manifest key, so renaming the destination without telling it produced a hard failure rather than a silently wrong snapshot. My first pass was incomplete and that check is what caught it — worth recording, because a snapshot that quietly disagreed with its manifest is exactly the failure a reviewer would never notice.Tests
Four cases, asserting both halves — present and byte-identical at the quarantined path, absent at the real one — because a change achieving only one would look like success from the other side. Plus reserved state not quarantined, ordinary content unaffected, and no phantom entry in
git status. Mutation-proven: removing the quarantine fails the reviewability cases and nothing else.🤖 Generated with Claude Code