fix(install): scope the worktree git-add shield per-worktree (#384) - #385
Conversation
The shield appended its patterns to `.git/info/exclude`, which for a linked worktree is the MAIN repo's — shared with the operator's checkout, permanent, and unversioned. Those patterns name paths projects legitimately track (`.claude/skills`, `.claude/settings.json`), so every NEW file under them silently stopped being staged by `git add -A` in the main checkout, long after the run: one report lost 51 files and three whole skills across two upgrade commits. Write a private exclude in the worktree's own gitdir instead, activated with `git config --worktree core.excludesFile` (gated on extensions.worktreeConfig). Scope is that worktree alone; lifetime ends with `git worktree remove`, which deletes the gitdir — so no remover is needed. The shared exclude is never written again: on any refusal or fault the shield is skipped with a degrade reason, since a legacy fallback would reintroduce the bug. The worktree-scoped key shadows the operator's own core.excludesFile, so its lines are copied into the private file when it is created (falling back to git's XDG default). Enabling extensions.worktreeConfig is a permanent repo-format change; it is refused where git requires core.bare/core.worktree be moved first.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe git-add shield now uses serialized, byte-preserving, worktree-scoped exclude files activated through ChangesWorktree shield isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant provision_worktree
participant _worktree_local_exclude
participant git
participant private_exclude
participant WorktreeFlow
provision_worktree->>_worktree_local_exclude: provide shield patterns
_worktree_local_exclude->>git: probe configuration and inherited excludes
_worktree_local_exclude->>private_exclude: preserve and append bytes
_worktree_local_exclude->>git: activate worktree core.excludesFile
_worktree_local_exclude->>WorktreeFlow: return degradation reason
WorktreeFlow->>WorktreeFlow: journal and notify degradation
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0802fb8861
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return subprocess.run( | ||
| ["git", "-C", str(worktree), *args], | ||
| capture_output=True, | ||
| timeout=_SHIELD_GIT_TIMEOUT_S, | ||
| ) |
There was a problem hiding this comment.
This helper introduces a new family of bare subprocess.run(["git", ...]) calls outside verify._run_git, so the shield bypasses the repository's centralized timeout, locale, and Git error-handling behavior. Extend _run_git with the required byte-output/non-raising mode rather than creating a second runner; the repository explicitly requires every Git subprocess to use that chokepoint.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 6d2936f, which closes #389.
verify.git_bytes is now the single runner for all eight of the shield's calls (nine, with
the version probe the sibling finding added). _shield_git and its hardcoded
_SHIELD_GIT_TIMEOUT_S are gone.
The reason this had been deferred rather than skipped: it was never a pure swap. Through the
chokepoint a timeout arrives as GitError, which is not a subprocess.SubprocessError,
and a spawn failure as GitSpawnError, which is not an OSError — so the four except
tuples wrapping these calls would have kept passing every existing test while covering
nothing. All four are re-derived:
| guard | now | why |
|---|---|---|
rev-parse arm |
GitError |
wraps the git call and nothing else, and every fault it can raise is a GitError |
_worktree_local_exclude tail |
(GitError, OSError, RuntimeError, UnicodeError) |
git faults typed; the rest still cover .resolve(), file I/O, and the encode direction |
_shield_inherited_excludes |
(GitError, OSError, UnicodeError) |
same, with the two non-git members left for the file read |
legacy_exclude_pollution |
(GitError, OSError, UnicodeError) |
shares the runner, so it moved too |
Each has an ablated fault-injection test. Worth reporting that the first pass left
_shield_inherited_excludes' GitError untested — its ablation passed, which is the only
thing that would have caught it — so survives_a_git_fault_while_reading_the_users_excludes
exists because of that ablation.
Two behavior changes fall out and are in the CHANGELOG: the shield now honors the configured
[limits] git_timeout_s rather than a hardcoded 120s mirror of its default, and it carries the
LC_ALL=C pin, so a degrade reason quoting git's stderr stays stable English (#236).
One correction to the finding as filed, since it may matter elsewhere: AGENTS.md's chokepoint
invariant has no enforcing test — tests/test_portability_guard.py scans for tmux argv,
os.kill and shell=True, never for ["git", …]. Two bypasses survive this PR
(install._warn_if_policy_tracked and tui/app.py:_commit_subject, the latter a live crash
path: text=True under an except (OSError, SubprocessError) that cannot catch the
UnicodeDecodeError #377 was about). Filed separately rather than widened into this PR.
| ): | ||
| refused = "core.worktree" | ||
| else: | ||
| enabled = _shield_git(worktree, "config", "extensions.worktreeConfig", "true") |
There was a problem hiding this comment.
On Git 2.13–2.19, the preceding --absolute-git-dir probe succeeds, so this writes extensions.worktreeConfig even though those Git versions do not support it. As the added docs/FEATURES.md:79 warning itself notes, Git older than 2.20 refuses a repository carrying this extension, meaning the running process can make the operator's repository inaccessible before the subsequent activation attempt. Check the Git capability/version before making this permanent format change and degrade without writing it when unsupported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 3ff7097.
_shield_enable_worktree_config now opens with a git version probe and refuses below 2.20,
before any other probe and long before the write. An unparseable answer refuses too: an
irreversible repo-format change is not the place to guess, so _git_version_at_least returns
False for anything that is not a git version <major>.<minor> line (the tail is vendor soup —
2.44.0.windows.1, 2.39.5 (Apple Git-154) — so it parses only the two leading components).
Your finding understated it, and that changed where the gate goes. --type= is itself git
2.18, so on 2.13–2.17 the core.bare probe on the line below exits non-zero and is read as
"not bare" — a safety gate degrading open, on the exact repo shape git-worktree(1) says
must be hand-migrated first. Putting the version check above the probes rather than merely
before the write makes that pair unreachable, which is why it sits where it does.
One qualification on the severity, for the record. git-worktree(1)'s "Older Git versions will
refuse to access repositories with this extension" is the documented authority and this PR's
own docstring already cited it — but I could not test a pre-2.20 binary, and at the default
core.repositoryFormatVersion = 0 git's extension handling is permissive (verified on 2.55:
worktreeConfig is honored at v0 with no error, and v0 repos do not validate extensions at
all). So the certain harm is narrower than "the operator's repository becomes inaccessible":
a permanent useless repo-format key, the silently-open core.bare gate below 2.18, and an
unshielded provision. The gate is warranted regardless — the write cannot be undone, and on a
git that cannot use it there is nothing on the other side of the trade.
Tested by refuses_to_enable_extension_over_old_git (only the version call is faked, so the
assertion reads the real repo's shared config; the enable is also made to raise, since "the
key is absent afterwards" would hold if the write merely failed) plus a nine-case parse table.
Ablated: deleting the gate fires the enable and the fake raises.
| probe = _shield_git(worktree, "config", "--type=bool", "--get", "extensions.worktreeConfig") | ||
| if probe.returncode == 0 and os.fsdecode(probe.stdout).strip() == "true": | ||
| return None |
There was a problem hiding this comment.
If extensions.worktreeConfig=true is present only in the user's global or system Git config, this unscoped lookup returns true and skips enabling the extension in the repository's shared config. Git still does not consider the repository worktree-config-enabled, so the later git config --worktree core.excludesFile ... fails and provisioning continues unshielded, allowing the unit's git add -A to stage generated tool files. Read this format extension explicitly from common_dir/config, as the safety probes immediately below already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in c6d5960. The probe now names the shared config file exactly like the two
gates below it, and shared moved above it so all three reads share one binding.
Reproduced before fixing, on git 2.55, with the extension present only in global config:
$ git -C <linked-worktree> config --get extensions.worktreeConfig
true # the unscoped probe believes this
$ git -C <linked-worktree> config --worktree core.excludesFile /tmp/x
fatal: --worktree cannot be used with multiple working trees unless the config
extension worktreeConfig is enabled. # rc 128
So the outcome is the one you described: enable skipped, activation fails, provisioning
continues unshielded. It does return a degrade reason and gets journaled, so it is a loud skip
rather than a silent one — but a repo one write away from being shielded should not be skipped
at all.
The adjacent comment was the part I would most like on record: it claimed the check "cannot be
answered by a value the operator set globally", which was true of the two --file gates below
it and false of the probe directly above them — a comment asserting the property the code
was missing. It is rewritten to cover all three, and to say why this one in particular needs
it: extensions.* is repo-format state git honors only from the repo's own config, so an
unscoped read of it is a question about the wrong file.
Tested by enables_extension_when_only_the_global_config_claims_it — plants the key via
GIT_CONFIG_GLOBAL, pinned inside a monkeypatch.context() around the shield call alone
(0c21f3f is the standing lesson about a leaked GIT_CONFIG_* pin breaking Windows CI through
core.autocrlf). Ablated: with --file dropped, reason comes back as the fatal: above.
| answer = _shield_git(worktree, "config", "--type=path", "--get", "core.excludesFile") | ||
| if answer.returncode == 0 and answer.stdout.strip(): | ||
| source = Path(os.fsdecode(answer.stdout).strip()) |
There was a problem hiding this comment.
When core.excludesFile is relative, git config --type=path --get can return that relative value verbatim; Git invoked with -C <worktree> resolves it from the worktree, while this Path(...) resolves it from the orchestrator process's current directory. The wrong or absent file is therefore copied, after which the worktree-scoped setting shadows the operator's real exclude file and the automated git add -A can stage files they configured Git to ignore. Resolve relative values using Git's worktree/config context rather than Python's current directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in ab0cef9. A non-absolute source is now joined onto the worktree before
is_file() sees it.
Confirmed both halves of the mechanism on git 2.55 rather than taking them from the docs:
git config --type=path --getexpands~and stops there — a relative value comes back
verbatim (rel/ignorein,rel/ignoreout).- git resolves such a value against the worktree top level, not the process cwd. Probed by
running from a subdirectory holding a competingrel/ignore: git used the top-level one and
ignored the closer file. Soworktree /is the right base, and it is also what-Cis
handed.
The consequence is worth restating because it is the quiet one: the usual outcome is not
"reads the wrong file" but "reads no file" — is_file() false, the seed returns "",
silently — and the worktree-scoped core.excludesFile written moments later then shadows
the operator's real one. Their patterns stop applying inside the worktree and git add -A
stages what they told git to ignore, which is precisely the leak the seed exists to prevent,
caused by the seed.
Tested by seeds_a_relative_excludesfile_resolved_like_git. The monkeypatch.chdir in it is
load-bearing, not hygiene, and I checked that rather than assuming: with the fix ablated and
the test run from inside the worktree, it passes — the buggy code resolves the same path by
accident. Run from anywhere else, the ablation fails it.
…ed-exclude # Conflicts: # CHANGELOG.md
The three env vars exist to force `_worktree_local_exclude` down the XDG fallback branch, but they stayed set for the rest of the test — and GIT_CONFIG_NOSYSTEM changes what git thinks of files already on disk. Git for Windows ships `core.autocrlf = true` in its SYSTEM config, so `worktree_add` checked out CRLF; the later `git status` then re-read those same files with autocrlf switched off and every tracked one came back modified. Windows-only, which is why both Linux legs stayed green. Scoped to the probe with monkeypatch.context(). The status assertion needs no pinning: the exclude is activated through a worktree-scoped `core.excludesFile`, i.e. repo-local config. Reproduced on Linux with GIT_CONFIG_SYSTEM pointing at an autocrlf=true config: the old test fails there with the identical assertion and the new one passes.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The docstring named #377 as the blocker for routing through verify's chokepoint. #377 has merged: verify.git_bytes now returns CompletedProcess[bytes] with the rc intact, which is precisely the shape this helper asked for. Left as written, it justifies standing outside an AGENTS.md invariant on grounds that no longer hold. What actually blocks absorption now is the exception guards — a timeout arrives as GitError, not subprocess.SubprocessError, and a spawn fault as GitSpawnError, not OSError, so the tuples at these call sites would stop covering what they were written for. Filed as #389.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
`extensions.worktreeConfig` and `git config --worktree` are both git 2.20. Below that the shield wrote the extension anyway — a permanent repo-format change git-worktree(1) says older git refuses to open, in exchange for an activation that then fails and degrades. The gate runs ahead of every probe rather than just before the write, because `--type=` is itself git 2.18: on an older git the `core.bare` read exits non-zero and reads as "not bare", silently opening the safety gate. Refusing at the top keeps that pair unreachable. An unparseable version refuses too — an irreversible write is not the place to guess.
…384) The probe was the one read in this function that went by no scope at all, so a `worktreeConfig = true` in the operator's ~/.gitconfig answered it. The shield then concluded the repository was already enabled, skipped the write, and `git config --worktree` died with git's own `cannot be used with multiple working trees` — a repo one write away from being shielded, skipped instead. `extensions.*` is repo-format state git honors only from the repository's own config, so the probe now names that file like the two gates below it. The comment claiming immunity from a global value was true of those two and false of this one; it now covers all three.
…#384) `--type=path` expands `~` and stops there, so a relative `core.excludesFile` arrives verbatim. Git resolves such a value against the worktree's top level; `Path(value)` resolved it against the orchestrator's cwd — reading the wrong file or, far more often, none. The miss is silent: `is_file()` comes back false, the seed returns "", and the worktree-scoped `core.excludesFile` then shadows the operator's real one. Their patterns stop applying inside the worktree and the unit's `git add -A` stages what they told git to ignore — the leak the seed exists to prevent, arrived at by the seed itself.
`_shield_git` was a bare `subprocess.run`, which AGENTS.md's chokepoint invariant otherwise forbids. #377 landed `verify.git_bytes` — bytes stdout, returncode returned rather than raised — which is exactly the shape the shield needs, so the helper is gone and its eight call sites go through it. The substance is the guards, not the swap. Through the chokepoint a timeout arrives as `GitError` and a spawn failure as its `GitSpawnError` subclass, so every tuple that named `subprocess.SubprocessError` or `OSError` for those two would have kept passing tests while covering nothing. All three are re-derived: the rev-parse arm narrows to `GitError` (it wraps the git call and nothing else), the tail and the excludes seed each swap `SubprocessError` for it. Behavior: the shield now honors `limits.git_timeout_s` instead of a hardcoded 120s mirror of its default, and carries the `LC_ALL=C` pin, so a degrade reason quoting git's stderr stays stable English (#236).
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf8648d22f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| existing = ( | ||
| exclude.read_text(encoding="utf-8") if existed else _shield_inherited_excludes(worktree) | ||
| ) |
There was a problem hiding this comment.
When the first write fails after exclude.touch()—for example, due to ENOSPC—the empty target remains while atomic_write_text removes only its temporary file. If provisioning is retried for the same mounted worktree, existed is now true, so this branch treats the empty placeholder as authoritative instead of calling _shield_inherited_excludes; the subsequent successful worktree-scoped activation then omits and shadows the operator's global/XDG ignore patterns, allowing git add -A to stage files they configured Git to ignore. Remove the newly created placeholder on failure or otherwise distinguish an interrupted creation from an initialized private exclude.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid mechanism, fixed in cc9e338 — but latent rather than live, and the difference is
worth recording, because the fix is placed for the reason the reachability check turned up
rather than the one the finding gives.
The mechanism holds exactly as described, and I confirmed each link rather than reading it off:
atomic_write_text's own docstring promises a failure "leaves the original untouched and
removes the temp", and the original here is the zero-byte file exclude.touch() just made, so
it survives. The tail's except returns a reason instead of raising, and it sits above the
config --worktree activation — so that first attempt activates nothing, and an unactivated
private exclude does nothing at all. The harm is entirely deferred to a second attempt, which
would read existed = True, skip the seed, and then activate a core.excludesFile shadowing
the operator's own.
The premise that does not hold is "if provisioning is retried for the same mounted
worktree". provision_worktree has one call site, behind open_unit_workspace →
verify.worktree_add, which never passes --force and requires a non-existent target. Checked
on git 2.55 rather than from the docs:
git worktree prunedeletes the entire.git/worktrees/<id>admin dir — the
placeholder's own home.discard_worktreeruns it unconditionally on every resume re-mount,
so the re-add gets a fresh admin dir,existedis False, and the inherited excludes are
seeded.- When that best-effort prune fails, re-adding at the stale path does not adopt it:
fatal: '../wt' is a missing but already registered worktree(rc 128) →GitError→ the unit
goesDEFERRED, so provisioning is never reached at all. - A leftover non-empty dir (
keep_failed) fails the same way —fatal: already exists. - The other resume arms go through
reopen_unit, which only rebuilds a dataclass from persisted
state;recovery_flownever provisions.
So nothing in the engine can currently observe the placeholder. Fixed anyway, because the defect
is in the helper's contract and the barrier protecting it lives two modules away in teardown
code — a future caller would inherit it silently. The test's docstring records that barrier, so
the guard does not later read as dead code and get simplified away.
One deviation from the suggested remedy. You offered either removing the placeholder on
failure or otherwise distinguishing an interrupted creation; I took the second. An
except-and-unlink handler cannot run when the process is killed between touch() and the
write, and this orchestrator kills sessions — so the durable signal is the zero-byte file
itself:
existed = exclude.is_file() and exclude.stat().st_size > 0That covers both shapes, and it costs nothing in the one case where our own write legitimately
produces an empty file (no inherited excludes and no patterns): re-seeding then is a no-op, or
picks up excludes added since. Git treats an empty exclude and an absent one alike. The stat()
sits inside the existing try, so a delete racing between the two calls degrades to a reason
like every other file fault here.
New test: test_shield_reseeds_after_an_interrupted_creation — a real two-attempt sequence
against a linked worktree, injecting the ENOSPC on the first and asserting the placeholder is
present and zero-byte before the second runs. That middle assertion is load-bearing: without
it the test passes just as well when the first attempt leaves no file at all, which is the one
state it is not about. Ablated per AGENTS.md — restoring existed = exclude.is_file() fails it
with the private exclude reduced to ['/probe-384'], the operator's mine.log dropped and
untracked in the worktree.
`exclude.touch()` creates the private exclude before `atomic_write_text` fills it, and a fault in between leaves a zero-byte file behind — the helper's own docstring says a failure "leaves the original untouched", and the original is that placeholder. Read back as an initialized exclude, it skipped the inherited-excludes seed and then activated a `core.excludesFile` shadowing the operator's own, un-ignoring inside the worktree everything they ignore globally. Latent, not live: every engine re-mount of a unit path runs `worktree_prune`, which deletes the whole per-worktree gitdir, and a failed prune makes `git worktree add` refuse the stale path. Fixed at the helper's own contract so a future caller cannot inherit it. Size rather than unlinking the placeholder on the way out: no handler runs when the process is killed in that window.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Review round 2 (Codex, 2026-07-29)One finding this round, valid and fixed in
The mechanism was real: Fixed regardless, because the defect is in the helper's own contract while the barrier protecting New test One caveat for whoever mergesGreptile has produced no review of this PR at all. All five of its "reviews" here are the CodeRabbit and Codex have both genuinely reviewed the current head: CodeRabbit's latest pass
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc9e3389bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lines = os.fsdecode(answered.stdout).splitlines() | ||
| if len(lines) != 2: | ||
| return f"could not read this worktree's git dirs ({worktree}): {lines}" |
There was a problem hiding this comment.
On POSIX, when the repository path contains a newline, both paths emitted by this combined rev-parse command contain that newline, so splitlines() produces more than two entries and this branch returns a degradation before installing the shield. Provisioning has already copied the tool files at that point, allowing the unit's later git add -A to stage and commit them. Read the two raw path answers separately, rather than using newline as a record delimiter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 9fd0f73. The shield now asks one question per call.
Both halves confirmed on git 2.55 rather than taken from the docs, because the finding
turns on whether git quotes such a path and whether the rest of the shield could work at
one anyway:
$ git -C <linked-worktree> rev-parse --absolute-git-dir --git-common-dir
b'…/re\npo/.git/worktrees/wt\n…/re\npo/.git\n' # raw, unquoted
splitlines() -> 4 entries # -> len(lines) != 2 -> degrade
No core.quotePath-style escaping on this path, so the newline really does arrive as
data in a field the code was reading as a delimiter.
The degrade was a false one, which is what makes this worth fixing rather than
documenting. Everything downstream works at such a path — I checked before assuming
the parse was the only obstacle:
$ git -C <wt> config --worktree core.excludesFile '…/re\npo/.git/worktrees/wt/info/exclude'
$ cat …/config.worktree
[core]
excludesFile = /…/re\npo/.git/worktrees/wt/info/exclude # escaped, round-trips
$ git -C <wt> status --porcelain
?? notshielded.txt # the shielded sibling is gone
So the shield was skipped over a repo it would have shielded correctly — and, as the
finding says, skipped after provision_worktree copied the tool files, which is the
one ordering where a degrade is not a safe default.
Read separately rather than made NUL-delimited because there is no NUL option to reach
for: rev-parse has no -z. It echoes one back as a literal argument (measured), which
is worth recording next to the fix so the "just add -z" simplification is not tried
later. A path ending in a newline stays out of reach — inherent to rev-parse's output
format, not to this parse.
The len(lines) != 2 degrade is kept as an emptiness guard on the two answers, so the
two-arm taxonomy is unchanged (git answered but unreadably ⇒ a reason, never a silent
skip). It is defensive: an rc of 0 from rev-parse always prints a path. Same standing
as the os.fsdecode placement two lines above, which the function's comment already
labels that way.
Test
test_shield_survives_a_newline_in_the_repository_path — a real repo at …/re\npo, a
real linked worktree, POSIX-only. The assertion that carries it is git's own answer, not
the private file's content: after the shield, a file matching the pattern is invisible to
git status inside the worktree. Ablated by restoring the combined call, which fails it
on the first assertion with the four-entry split printed in the reason.
One thing the fix turned up
The rc check's comment claimed it also caught "a git too old for --absolute-git-dir
(2.13)". It does not, and could not: rev-parse echoes an unknown option and exits 0
(measured — git rev-parse --absolutely-not-an-option → rc 0, prints the flag). On such
a git the helper would read the flag's own name as the gitdir. Not live, and not a
regression from this change — the same was true of the combined call — because the git
2.20 refusal in _shield_enable_worktree_config returns above every filesystem write.
The comment is corrected to say where that case is really handled, rather than left
crediting a check that never performed it.
uv run pytest -q -n auto: 3732 passed, 24 skipped. uv run pyright 0 errors,
trunk fmt + full trunk check clean.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5982dcaa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # MUST be bytes — with `patterns` left as str every one of them reads as | ||
| # absent and gets re-appended on every re-provision. | ||
| wanted = [os.fsencode(p) for p in patterns] | ||
| new = [p for p in wanted if p not in present] |
There was a problem hiding this comment.
When the inherited core.excludesFile contains a requested shield pattern followed by a negation that re-includes it—for example, /.claude/skills followed by !/.claude/skills—this set-membership dedupe considers the positive pattern present and does not append it at the end. Git honors the later negation, so the provisioned files remain untracked and the unit's subsequent git add -A can stage them; this reproduces on Git 2.43. Deduplication must account for pattern order/negations or ensure the required shield pattern is effective at the end of the generated file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, live, and silent. Fixed in f7f8bce. Reproduced end-to-end through
_worktree_local_exclude itself rather than through the line, with git's own output as
the oracle — an operator core.excludesFile holding your exact example, then a real file
provisioned into the worktree:
inherited: /.claude/skills
!/.claude/skills
reason : None <- nothing failed
private exclude: '/.claude/skills\n!/.claude/skills\n'
git status -uall: '?? .claude/skills/tool.md'
git add -An : "add '.claude/skills/tool.md'"
reason is None is the part that makes this worth fixing rather than reporting: the
shield's whole degrade policy is to skip loudly, and this path skipped nothing — it
succeeded while producing a file that does not shield.
Your reachability is wider than your write-up claims, and narrower in a way worth
recording. Measured, all four variants against the same fixture:
/.claude/skills + !/.claude/skills -> STAGEABLE (your case)
/.claude/skills + !.claude/skills -> STAGEABLE negation need not repeat the spelling
/.claude/skills + !*.md -> shielded git never descends into an excluded dir
/.claude/skills + !/.claude -> shielded re-including a PARENT leaves the child matched
!/.claude/skills + /.claude/skills -> shielded the positive is last, so it decides
So the defeating shape is precisely a negation of the pattern's own directory, below
it. The dedupe key only has to match the positive exactly; the negation does not.
The fix is the invariant rather than the case (this PR has paid three times for
enumerating failure shapes one review round at a time). A pattern may be skipped only
where it is already guaranteed effective — where it sits after the last negation in
the file:
last_negation = max(
(i for i, line in enumerate(lines) if line.startswith(b"!")), default=-1
)
settled = set(lines[last_negation + 1 :])That is airtight without reimplementing git's matcher: if the pattern is below every !
line, then for any path it covers the last matching pattern is either it or a positive
below it, and both ignore. Anything earlier gets appended, which is always safe because
the appended copy is last and therefore decides. It is deliberately conservative across
the line your finding sits on — the !*.md and !/.claude rows above are already safe
and still get a duplicate line, because distinguishing them is exactly git's matching
semantics and guessing at those is how this bug arrived. startswith(b"!") takes no
lstrip, which is git-correct twice: git does not strip leading whitespace from a
pattern, and \! escapes a literal leading !.
Behavior is unchanged where no negation exists (last_negation == -1 makes settled the
whole file, today's set), so the re-provision idempotence path is untouched — the private
file ends with these patterns, which is after any negation copied in.
Test shape, because the obvious one cannot bite here: the function returns None both
with the bug and with the fix, so a reason assertion proves nothing — the same gap that
let an earlier defect on this PR sit under a passing fault-injection test for two rounds.
The new test asserts through git status --porcelain -uall, with a non-vacuity check that
the operator's negation really reached the private file. Ablation run, not assumed:
restoring settled = set(lines) fails it at the status assertion with
?? .claude/skills/tool.md, and the two assertions above it still pass — confirming they
are not what is holding the test up.
Suite 3785, pyright 1.1.411 and trunk clean, and the install-scoped ablation set is now
33/33 failing as required.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_install.py`:
- Around line 1845-1858: Update the hand-rolled repository setup around the git
subprocess calls in this test to use an isolated git configuration, preventing
host global/system settings and HOME-based templates or hooks from affecting
init, commit, push, and worktree operations. Set the relevant environment
variables for every subprocess, or explicitly disable signing where needed while
preserving the existing repository setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a9e6e81-16a0-4b20-ace4-66c63cc1d6d5
📒 Files selected for processing (5)
CHANGELOG.mdsrc/bmad_loop/install.pysrc/bmad_loop/platform_util.pytests/test_install.pytests/test_platform_util.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f8bce368
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| activated = git_bytes( | ||
| worktree, "config", "--worktree", "core.excludesFile", str(exclude) | ||
| ) |
There was a problem hiding this comment.
When the inherited environment supplies core.excludesFile through GIT_CONFIG_COUNT/GIT_CONFIG_KEY_*, Git's command scope outranks this worktree-scoped value. This write still returns success, but subsequent Git commands inherit the same override and never read the private exclude, so the provisioned tool files remain visible to git add -A without any degradation warning. Detect this higher-priority origin or prevent it from overriding the shield before reporting activation as successful.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, live, and silent. Fixed in b5f0834. Reproduced end to end on a real linked
worktree with the shield fully installed, before anything changed — git's own output as the
oracle, not the return value:
baseline status -uall : [] <- shielded
resolves to : <wt>/.git/worktrees/wt/info/exclude
scope : worktree
GIT_CONFIG_COUNT=1 ... status -uall : [?? tool-dir/file.txt] <- SHIELD DEFEATED
add -An : [add 'tool-dir/file.txt'] <- it would be committed
resolves to : /tmp/.../operator-ignores
scope : command
reason was None throughout, so nothing was journaled and nothing notified. This is round
17's shape one level up: that one was present is not effective (a pattern in the file that
a negation below cancelled), this one is written is not effective.
Why the fix does not do what you recommended
You suggested detecting the higher-priority origin. I measured the channels first, and the
enumeration is not writable:
| git 2.20.4 | git 2.55.0 | |
|---|---|---|
GIT_CONFIG_COUNT/KEY_n/VALUE_n |
inert (2.31) | shield defeated |
GIT_CONFIG_PARAMETERS 'k=v' |
shield defeated | shield defeated |
GIT_CONFIG_PARAMETERS 'k'='v' |
fatal: bogus format |
shield defeated |
what git -c itself emits |
'k=v' |
'k'='v' |
Three things follow, and the third is the one that decided the shape:
- The channel you named does not exist at this shield's own 2.20 floor.
GIT_CONFIG_COUNT
is git 2.31 — 2.31.0's notes, "Two new ways to feed configuration variable-value pairs via
environment variables have been introduced, and the way GIT_CONFIG_PARAMETERS encodes
variable/value pairs has been tweaked to make it more robust." Anos.environcheck written
against your comment would be blind exactly where the shield still runs. GIT_CONFIG_PARAMETERSis a second channel in two mutually incompatible encodings. The
pre-2.31 form is the only one honored at both ends; the modern form is a hardfatalat
2.20.4. So the enumeration is not one name, it is three forms with a version condition on each.git -con a session's own command line is a fourth, and it is not in our environment at
all. No origin check can see it.
So the fix asks git what it resolved instead of where an override came from:
resolved = git_bytes(worktree, "config", "-z", "--type=path", "--get", "core.excludesFile")
...
if effective != os.fsencode(str(exclude)):
fault = "the write succeeded but another configuration scope outranks it, ..."One call, no environment-variable names, no new version floor — the read shape is the seed
read's, already measured at 2.20.4 and 2.55.0. It covers all four channels and whatever git
adds next. --show-scope would name the winning scope and make the reason better, and is
deliberately unused: it is git 2.26, above the floor, and a probe flag with its own version
floor turns every rc-branch below it into a silent default — the --type= 2.18 trap from round 1.
This is the same lesson as rounds 5→7, one level out. Those funnelled the ways a call can
fail; a complete funnel over that still said nothing about whether the call achieved what it
was for. It is placed inside the existing try, so its own two failure shapes reach the existing
rollback rather than growing a second one.
Three things the finding did not name
- The exposure is exactly one call site, and that is provable. All four safety probes
(core.bare,core.worktree,extensions.worktreeConfig,core.sharedRepository) read
through--file <shared>/config, which is immune to command scope — measured at both
versions: withcore.bare=trueinjected, the--fileread still answersfalse. So none of
the gates can be spoofed this way, and only the activation needed the post-condition. - The rollback stays extension-only, and that needed measuring rather than assuming. This is
the first degrade reachable after a write that succeeded, so unlike every other rollback test
the worktree-scoped key really is sitting inconfig.worktree. Unsetting
extensions.worktreeConfigmakes git stop reading that file at all, so the leftover key reads
back as unset (rc 1) even though it is still on disk — inert, not removed, and pinned that way.
A second--unsetwould be a second write that can fail, i.e. the enumeration again. - Report-only, not neutralize. You offered "or prevent it from overriding the shield" and I
declined it on the argument already recorded atinstall.py:749-781: the coding-CLI sessions
have shell access, andtmux new-window -ecan only overlay an environment variable, never
unset one, so scrubbing could not make the guarantee hold — and silently overriding config an
operator deliberately set is provision_worktree's exclude patterns are repo-wide and permanent: new files under .claude/skills silently stop being staged in the main checkout #384 in mirror image.
Tests and ablations
6 new cases. The regression is parametrized over GIT_CONFIG_COUNT and GIT_CONFIG_PARAMETERS
because an origin-detection fix would pass one and fail the other, with the COUNT case skipped
below 2.31 and non-vacuity asserted inside the same environment (git really resolves the
operator's file). Plus the read-back's own rc and raise arms, and the rollback pair.
The fault-injection fakes key on state, not argv — the seed read and the verification read
use byte-identical arguments and real git tells them apart only by what was written in between,
so an argv-keyed fake would fault the seed and test a different arm entirely. Each asserts the
fake actually saw the activation.
18/18 ablations fail their tests, including the nine original shield gates re-run since the
activation arm was restructured underneath them. Two are worth reporting because they changed the
work:
- My first
last_negationablation passed, which this PR has taught me to read as "untested
gate". It was mine that was wrong:-1 and max(...)evaluates tomax(...), so the ablation
was a no-op. Third instance of the mis-targeted-ablation trap here. - Deleting the read-back's rc guard does not fall through to success — measured by running it
and printing the value, not predicted. An unread stdout isb"", which compares unequal to the
written path, so the mismatch arm one line down still degrades and still returns a reason; it
just returns the wrong one, naming''as what git reads.assert reason is not Nonewould
pass against that bug, so the test asserts the wording. The test's docstring said the wrong
thing about this until the measurement corrected it.
uv run pytest -q -n auto: 3791 passed, 24 skipped (3785 + 6). uv run pyright 0 errors,
trunk fmt + full trunk check clean. Ablations re-run after trunk fmt, since formatting is
how anchors rot silently.
…ote (#384) `git config --worktree core.excludesFile` returning rc 0 proved the value was stored, not that git reads it. `command` scope outranks `worktree` and is fed by the environment the orchestrator was launched with, so an operator carrying an ambient `core.excludesFile` got a shield that reported success and never applied: the provisioned tool files stayed stageable by the unit's `git add -A`, with nothing journaled because nothing had failed. Round 17's shape one level up — PRESENT is not EFFECTIVE became WRITTEN is not EFFECTIVE. The activation now asks git which excludes file it resolves and compares it to the path just written. A mismatch, an unexpected rc, or a raise all feed the `fault` string that already existed, so the existing single rollback point and degrade reason handle it — no new except arm and no second rollback site. Detecting the override's ORIGIN was rejected, and measured rather than reasoned: git 2.20.4 git 2.55.0 GIT_CONFIG_COUNT/KEY_n/VALUE_n inert (2.31) shield defeated GIT_CONFIG_PARAMETERS 'k=v' shield defeated shield defeated GIT_CONFIG_PARAMETERS 'k'='v' fatal: bogus shield defeated what `git -c` itself emits 'k=v' 'k'='v' Three names in two incompatible encodings, and the one the finding named does not exist at this shield's own floor; a `git -c` on a session's own command line is a fourth that never reaches our environment. The post-condition covers all of them. `--show-scope` would name the winning scope but is git 2.26, above the floor, so it is deliberately unused. The rollback stays extension-only: unsetting `extensions.worktreeConfig` makes git stop reading `config.worktree` entirely, so the key the activation did land is inert (measured). A second unset would be a second write that can fail. Report-only by design — the sessions have shell access, and `tmux new-window -e` can only overlay, never unset, so scrubbing cannot make the guarantee hold. 6 new cases; 18/18 ablations fail their tests, including the nine original shield gates re-run since the activation arm was restructured underneath them.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bbefed04f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if value in ("umask", "0") or value.lower() in ("false", "no", "off"): | ||
| return None |
There was a problem hiding this comment.
When a repository uses a private octal value such as core.sharedRepository=0600, this allowlist incorrectly classifies it as shared and skips the shield, leaving provisioned tool files eligible for the unit's git add -A. Checked with Git 2.43: git init -h documents --shared[=<permissions>], and git init --shared=0600 stores 0600 while creating .git as 0700 with no group-access bits, so this is not a repository shared between OS users; recognize owner-only octal modes instead of routing every numeric value except 0 to the refusal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — a scope decision, not a dispute of the mechanism. Your reading of 0600 is correct, and I re-measured it at both ends of this shield's supported range rather than relying on the 2.43 run. Verdict column is git's OWN answer — the mode of a loose object git writes under umask 077 — not a reading of config.c; non-root, id -u checked:
core.sharedRepository = git 2.20.4 git 2.55.0
0600 -r-------- -r-------- owner-only
0700 -r-------- -r-------- owner-only
0640 -r--r----- -r--r----- group
0660 -r--r----- -r--r----- group
0666 -r--r--r-- -r--r--r-- everybody
So 0600 (and 0700) grant no group access at either end, and refusing them is a false refusal rather than a correct classification. One version difference that does not change the conclusion: git init --shared=0600 creates .git as drwx------ at 2.55.0 and drwx--S--- at 2.20.4 — setgid, still with no group access bits.
The gate stays as written anyway. bmad-loop does not support a repository configured with core.sharedRepository, whatever mode the value names. The allowlist accepts exactly the values that say the feature is off (umask, 0, false/no/off); 0600 says it is on with an owner-only mode, which is the configuration being declined rather than a value being misclassified. That follows the earlier decision on this PR to drop cross-user repositories from scope — the lock machinery rounds 13/14/16 built for them was removed in ba9f11c/f5982dc, and this gate is what implements the drop.
The cost is real and bounded, and worth stating rather than arguing away: an operator whose repository sets core.sharedRepository = 0600 is genuinely single-user and loses the shield. The skip is reported, not silent — worktree_flow._exclude_degraded journals worktree-exclude-degraded with the value named in the reason and calls gates.notify (inert unless notify.file/notify.desktop is configured). That is the tradeoff the gate's docstring already states — "a false refusal is a reported skip, a false accept is the bug this gate exists to prevent" — applied in the direction you flagged, with the scope decision deciding which side it falls on.
No issue filed: supporting the key is out of scope, so this is not deferred work.
There was a problem hiding this comment.
Retracting the decline above — this is now fixed in 6be43e0. The maintainer reversed the scope call after reading it, so the gate accepts owner-only octal modes rather than refusing every numeric value except 0. Your finding stands as written.
Two things the measurement changed about the fix, neither of which was in either of our write-ups:
It is a mask test, not a literal set, and I mispredicted both boundary rows before measuring them. git masks the value with 0666, so 0711 IS private — the execute bits are discarded — while 0755 is NOT, because its group/other read bits survive. A fix that special-cased 0600, or that reasoned from "no execute bits", gets one of those two wrong. Full table, measured at 2.20.4 and 2.55.0 by the mode of a loose object git writes under umask 077, byte-identical across all 23 rows:
0 / 00 / 0000 ....... r-------- PRIVATE 0400 / 0200 ... REJECTED (128)
0600 / 0700 / 0711 .. r-------- PRIVATE -0600 ......... REJECTED
+0600 ............... r-------- PRIVATE 0o600 / 0_600 . REJECTED
01 / 1 .............. r--r----- group 0600x ......... REJECTED
0640 / 0660 ......... r--r----- group
02 / 2 / 07777 ...... r--r--r-- everybody
0666 / 0755 ......... r--r--r-- everybody
0604 ................ r-----r-- other
The parse could not be delegated to Python. int(value, 8) accepts 0o600 and 0_600, which git rejects — so using it would have accepted a repository git cannot open. The value is matched against [0-7]+ before conversion. That is stricter than git's strtol in the other direction too: git accepts a leading + and leading whitespace (+0600 really is private to it) and this refuses them, which is the direction this gate is contracted to prefer.
Two rows are deliberately left refused. The legacy 1/2 need no special case despite git special-casing them, because neither satisfies & 0600 and both land on the same refusal — an arm for them would have been indistinguishable from dead code. And the empty value stays refused even though git reads it as private, because --get answers it identically to a valueless key, which is shared; the ambiguity is what is refused, not the value.
Covered by a 39-row parametrized test over the table above plus two sandbox tests, with six ablations confirmed to fail. The harm assertion in the accept test was verified with the two assertions ahead of it disabled, so what bites is git's own status — ?? probe-384 — rather than the return value.
With `core.excludesFile` and `XDG_CONFIG_HOME` unset, git falls back to
`$HOME/.config/git/ignore` and locates it with `getenv("HOME")` on every
platform. `Path.home()` does not: `ntpath.expanduser` reads `USERPROFILE`
first and never consults `HOME`, while Git for Windows derives `HOME`
in-process preferring a `HOMEDRIVE`+`HOMEPATH` home share. The two disagree
whenever a shell sets `HOME` (Git Bash, MSYS2) or the home is a network
drive, and the miss is silent: the wrong path is not a file, the seed comes
back empty, and the activation then shadows the operator's real global
ignores.
Ask git instead — `--type=path` interpolates a leading `~/` through the same
`getenv("HOME")`, which names no environment variable and is correct on every
platform. Measured identical at 2.20.4 and 2.55.0. A home git will not
resolve is UNKNOWN, not absent, so it degrades with a reason rather than
being guessed into an empty seed.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The shared-repository gate accepted only `umask`, `0` and the false booleans, so every octal filemode went to the refusal — including `0600`, which grants no group or other access at all. Such a repository is private to its owner, not shared between OS users, and refusing it skipped the shield and left the provisioned tool files eligible for the unit's `git add -A`: the bug this branch exists to fix, reintroduced by the guard against a different one. Mirror git's own parse instead (`setup.c::git_config_perm`): mask the value with 0666 and accept it when no group or other bit survives. `0711` is private (the mask discards execute bits) while `0755` is not (its read bits survive) — both rows measured, both mispredicted beforehand. The pattern is applied before the conversion and is deliberately stricter than strtol in the refusing direction: Python's int(value, 8) accepts `0o600` and `0_600`, which git rejects, so parsing with Python's rules would accept a repository git cannot open. A leading `+` or whitespace is refused though git accepts it — a false refusal is a reported skip. The empty value stays refused: git reads it as private, but `--get` cannot tell it from a valueless key, which is shared. Verdicts measured at git 2.20.4 and 2.55.0, byte-identical across all 23 rows, by the mode of a loose object git writes under `umask 077`.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6be43e0dd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| which is the shape `git init --shared` writes: measured, `--shared=group` records | ||
| `sharedrepository = 1`, the legacy numeric form the table above lists as group. | ||
| """ | ||
| raw = _shield_shared_config(worktree, str(common_dir / "config"), "core.sharedRepository") |
There was a problem hiding this comment.
This common-config-only lookup misses core.sharedRepository=group stored in the target's config.worktree when extensions.worktreeConfig is already enabled. Git 2.43's git config -h describes --worktree as using the per-worktree config, and I verified that Git resolves this value at worktree scope and creates group-readable objects despite the common config having no such key. Such a multi-user worktree therefore passes this refusal and creates the owner-only 0600 common lock in platform_util.py:310; a peer then cannot acquire it and continues provisioning without the shield. Fresh evidence beyond the earlier multi-user lock threads is this worktree-scope path around the replacement refusal gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Mechanism confirmed, reachability absent — recording this as latent rather than fixing it in-PR. Both halves measured at both ends of this shield's supported range (non-root, id -u checked), not reasoned from config.c; the verdict column is git's OWN answer, the mode of a loose object git writes under umask 077:
git 2.20.4 git 2.55.0
A. fresh `git worktree add` (extension already on)
creates .git/worktrees/<id>/config.worktree? .... NO NO
B. object from the linked worktree, key absent .... -r-------- -r--------
C. object from the linked worktree, key at that
worktree's OWN scope (config.worktree) ......... -r--r----- -r--r-----
D. this gate's read, `config --file <common>/config
--get core.sharedRepository` ................... rc 1 rc 1
Row C is your finding and it is correct: git resolves core.sharedRepository at worktree scope, the common-config read answers absent, and the gate accepts. Row B is the control that makes C non-vacuous.
What is missing is an occupant for that file. The shield only ever runs on a linked worktree this process created moments earlier — workspace.open_unit_workspace → verify.worktree_add → worktree_flow.provision_worktree → _worktree_local_exclude, and the main checkout is refused one gate above this one (git_dir == common_dir). Row A says worktree add leaves that admin dir with no config.worktree at all, at both versions. Nothing else in the tree writes worktree scope: the only git config --worktree write in src/ is the shield's own core.excludesFile, which runs after this gate, and a re-mount goes through discard_worktree → worktree_prune, which deletes .git/worktrees/<id> whole (re-adding at a path whose admin entry is stale is refused outright — fatal: '<p>' is a missing but already registered worktree, rc 128). So an operator has no window in which to place the value, and no prior run leaves one behind.
One adjacent row worth stating because it is the obvious next question: a core.sharedRepository in the main worktree's .git/config.worktree does not reach a linked worktree either — measured, an object written from the linked worktree with the key set there comes back -r--------. Worktree scope really is per-worktree, so the target's own admin dir is the only file that could carry it.
So the verdict is latent, not invalid, and that changes where the fix belongs. The gate is correct only because of a barrier two modules away, which is exactly the shape that gets "simplified" later. The gate's docstring already states its scope narrowing for global config; it now states this one too — that worktree scope is unread, that git honors it, and that the provisioning flow is what makes the omission unreachable. If a re-provision path onto an existing worktree is ever added, or anything upstream of the shield starts writing the unit worktree's config.worktree, this gate has to widen to a second --file <git_dir>/config.worktree read at the same time.
No issue filed: the widening has no reachable trigger to track, and the condition that would create one is recorded at the code that would have to change.
…384) The sharedRepository gate reads only the shared config. git honors the key at worktree scope too (measured 2.20.4 + 2.55.0), so the gate is correct only because provisioning always hands it a worktree git created moments earlier, with no config.worktree in its admin dir. Record that barrier and what would have to change with it.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Fixes #384.
Fixes #389.
The bug
provision_worktreeshields the tool files it writes (skill trees, the per-CLI hookconfig, seeded configs) from the unit's
git add -A._worktree_local_excluderesolvedits target with
git rev-parse --git-common-dir, which from a linked worktree answerswith the main repo's
.git— so the patterns were appended to the repository-wide.git/info/exclude.Two properties combined badly. Scope: that file is shared with the operator's own
checkout and every sibling worktree. Lifetime: nothing ever removed the lines. And the
patterns name paths projects legitimately track (
.claude/skills,.claude/settings.json), so every newly created file under them silently stopped beingstaged in the main checkout, indefinitely. The reporter lost 51 files and three whole
skills across two installer-upgrade commits, with nothing in either diff able to reveal
why —
.git/info/excludeis unversioned by construction. The old docstring's defense("does not affect already-tracked files") is exactly what made it invisible: the tracked
files kept diffing normally while their new siblings vanished.
The fix (option 1 of the issue, maintainer-confirmed)
Write the patterns to a private
<worktree gitdir>/info/exclude(
.git/worktrees/<id>/info/exclude) and activate it withgit config --worktree core.excludesFile <abs path>, gated onextensions.worktreeConfig. Mechanics sandbox-verified on git 2.55, including the issue'sown finding that a private exclude does nothing without the config key.
git worktree remove/prunedeletes the whole per-worktree gitdir,taking the exclude and the
config.worktreepointing at it. No remover to keep instep, which is what the old design lacked.
.git/info/excludeis never written again. Everyrefusal or fault SKIPS the shield and returns a degrade reason (journaled as
worktree-exclude-degraded, unchanged); a legacy fallback would reintroduce provision_worktree's exclude patterns are repo-wide and permanent: new files under .claude/skills silently stop being staged in the main checkout #384.Design points worth review:
core.excludesFileshadows the operator's own (verified: gittakes the most specific scope and does not concatenate). Their file's lines are copied
into the private exclude when it is created, falling back to git's documented
$XDG_CONFIG_HOME/git/ignoredefault — best-effort and silent.extensions.worktreeConfigis a permanent repo-format change, set once and neverremoved (git < 2.20 refuses a repo carrying it — git-worktree(1)). It is refused outright
in the two shapes git's own docs say must be hand-migrated first (
core.bare = trueorcore.worktreein the shared config), and — since the Codex review below — behind agit versiongate, so it is never written on a git that could not use it anyway. Theshield degrades rather than rearranging someone's repo layout.
verify.git_bytes, the _run_git's decode fault escapes the GitError taxonomy on -z output #377 chokepoint accessor. Thesubstance was re-deriving the guards, not the swap: through the chokepoint a timeout
arrives as
GitErrorand a spawn failure as itsGitSpawnErrorsubclass, so the fourexcepttuples namingsubprocess.SubprocessError/OSErrorfor that pair would havekept passing tests while covering nothing. Each is re-derived and each has an ablated
fault-injection test. Closes Route install._shield_git through verify.git_bytes now that #377 has landed #389.
None; everything aftergit answered returns a reason. Behavior change worth naming: the shield now honors
[limits] git_timeout_sinstead of a hardcoded 120s, and carriesLC_ALL=C(safe_rollback matches git's English "did not match" — benign restore no-op raises GitError under a localized git #236).#375machinery (dedupe,touch()beforeatomic_write_textso the umask mode is kept)is retained;
#381's interleaving race is mooted for new writes — every linkedworktree used to resolve to one shared target, and now each has its own.
Legacy lines an older bmad-loop already wrote into
.git/info/excludeare notauto-removed (documented in CHANGELOG + FEATURES). A
validatewarning for them isfollow-up work, not part of this PR.
On the size (~850 LOC, above CONTRIBUTING's 800 ceiling)
This is one logical change and does not split without red CI. The five existing
provision_worktreeshield tests and the whole #359 best-effort block assert the exactshared-file behavior being removed — subject paths, degrade reasons, the fault-injection
subjects (a main checkout no longer reaches the write path at all), and the #374 stub git,
which now has to dispatch on subcommand. Adapting them in place is the behavior change;
landing the source first leaves them red, and landing them first leaves them asserting
nothing. New tests are ~9 cases plus their rationale, in this repo's usual comment density.
The review round below added ~250 more (four defect fixes, five new tests, five ablations).
Flagging rather than splitting: three of the four are one-to-three-line corrections to code
this PR introduces, so landing #385 without them ships known defects, and the fourth closes
a follow-up (#389) whose whole risk was the guards moving underneath a helper this PR wrote.
Happy to split the chokepoint commit back out if you would rather review it separately.
Review round 1 (Codex, 2026-07-29)
All four findings were valid; all four are fixed here. Details are in the in-thread replies.
6d2936f— closes #389worktreeConfig3ff7097—git version>= 2.20 gate, ahead of every probeworktreeConfigonly in the shared repo configc6d5960— the one read here that went by no scopeab0cef9—--type=pathreturns them verbatimOne second-order find worth calling out:
--type=is itself git 2.18, so below that thecore.bareprobe exited non-zero and read as "not bare" — a safety gate degrading open.Putting the version gate above the probes rather than just before the write closes that too.
Tests
New (all against real linked worktrees via
verify.worktree_add):shield_never_touches_main_checkout(the regression: a file created in the main checkoutafter provisioning still stages, and the shared exclude is byte-identical),
excludes_only_inside_the_worktree(its necessary pair — a private file alone doesnothing),
dies_with_the_worktree,refuses_to_enable_extension_over_core_worktreeand…_over_core_bare,reuses_already_enabled_extension,seeds_users_excludesfile,seeds_xdg_default_when_unset,config_fault_skips_shield_entirely,main_checkout_degrades.Adapted in place (same names): the five provision shield tests — each now also asserting
the shared exclude is unchanged — and the #359 block (linked-worktree subjects, private-file
targets, arg-dispatching stub git).
From the review round:
refuses_to_enable_extension_over_old_git(stub reports 2.19.4;asserts the enable never fires and the repo config is untouched), a nine-case parse table
over
_git_version_at_least,enables_extension_when_only_the_global_config_claims_it(plants the key via
GIT_CONFIG_GLOBAL),seeds_a_relative_excludesfile_resolved_like_git,and a #389 block:
git_fault_before_answering_is_a_silent_skip(parametrized overGitError/GitSpawnError),git_fault_after_answering_degrades,survives_a_git_fault_while_reading_the_users_excludes,git_calls_carry_the_locale_pin(a stub git records
$LC_ALLfrom a real child), andlegacy_exclude_pollution_silent_when_git_never_answers.Per AGENTS.md, every negative assertion was ablated — 14 gates deleted one at a time. The
original nine (shield target, main-checkout refusal, both extension gates, the
already-enabled early return, the activation call, the excludesFile seed, the XDG branch,
plus an injected shared-exclude fallback), and five from this round: the version gate, the
probe's
--filescope, the relative-path resolution, and each of the re-derivedexcepttuples. All fail their tests; the shield-target ablation fails all nine of its.
Two ablations are worth naming because they changed the work. The first pass at the #389
guards left
_shield_inherited_excludes' newGitErroruntested — the ablation passed,which is the only way that would have surfaced — so
survives_a_git_fault_while_reading_the_users_excludesexists because of it. And therelative-excludesFile test only bites because it
chdirs away from the worktree first:verified by running the ablated code from inside the worktree, where it passes.
One engine test needed a fixture fix, and it is worth a look:
test_per_worktree_setup_then_gate_then_teardown_and_seedcreated an untracked.claude/skills/gameobject-create/in the main repo — its own comment calls it "agitignored MCP skill dir", but it never gitignored it. It passed only because the
repo-wide exclude hid that file in the main checkout, so the pre-merge cleanliness gate
(
verify.dirty_paths, which counts untracked files) never saw it. The fixture nowgitignores it, matching its comment and the documented real-world setup. Verified
non-vacuous: ablating the glob seed still fails the test. Note the user-visible
consequence — an untracked, non-ignored file under a tool dir in the main checkout now
blocks a merge with the existing "clean them, then resume" escalation, exactly as any
other untracked file there always has.
uv run pytest -q -n auto: 3730 passed, 24 skipped.trunk fmt+ fulltrunk checkclean;
uv run pyright0 errors.Review rounds 2–3 (Codex)
Single-finding rounds, both valid, both fixed and replied in thread:
cc9e338re-seeds aprivate exclude that a failed write left zero-length (read as authoritative, it skipped the
inherited-excludes copy and then activated a shadowing key), and
9fd0f73asksrev-parsefor one git dir per call — a newline is a legal POSIX path byte, so a repo directory carrying
one split two answers into four and degraded the shield away after provisioning had already
copied the files it exists to hide.
cf8648drecords the resulting git 2.20 floor and thechokepoint move in the docs.
Review round 4 (Codex, 2026-07-30)
The first round with a rejected finding. Two P1s; one is a live bug and is fixed, one is
invalid and its recommended remedy would have introduced the bug it claims to prevent. Both
verdicts were reached by measurement, not by reading docs — details in the in-thread replies.
1f4773b— exclude payload is bytes end-to-end; an unreadable excludes file now skips the shield--includesto the migration safety probes97288e1— the invariant and its cause recorded in the comment block; no behavior change, no testWhat changed for the valid one
The seed read the operator's
core.excludesFilewithread_text(encoding="utf-8")inside ahandler naming
UnicodeError, so one non-UTF-8 byte collapsed the whole seed to empty — andthe worktree-scoped
core.excludesFilethat follows shadows theirs, because git takes thekey from the most specific scope and never concatenates. Reproduced end-to-end: a path their
excludes file ignored came back untracked inside the worktree, so
git add -Awould commitit. The seed existed to prevent that leak and was causing it.
The payload is bytes end-to-end now —
read_bytesfor the seed and for the private file'sre-read, a bytes dedupe set,
os.fsencoded patterns, and a newplatform_util.atomic_write_bytes(the shared tail is factored;atomic_write_textisbehaviorally unchanged, since its newline translation is load-bearing for its other two
callers).
Three things the finding did not name, each of which needed its own fix or test:
OSErroron that same read was swallowed identically, so an excludes file that existsbut cannot be read produced the same silent empty seed and the same shadow. The seed's
handler narrows to
except GitErroralone — git unqueryable means nothing to copy, while afile we could not read degrades and skips the activation. Absent stays silent. Untested in
either direction before this.
str.splitlines()was the dedupe split, breaking on\x0b,\x0c,\x1c,\x1d,\x1eand\x85— none a git line boundary, all legal POSIX filename bytes — so alegitimate pattern fragmented into wrong keys.
bytes.splitlines()is git's own boundaryset, so this is a fidelity gain, not just type consistency.
existing.endswith("\n")left asstrraises
TypeError, which is not in the tail's tuple, so it escapes a function contractednever to propagate; and
presentas bytes againststrpatterns re-appends every patternon every re-provision, which no test could catch — the interrupted-creation case runs the
helper twice but sees a zero-byte file both times.
Behavior change to watch on Windows CI, its only oracle: the exclude is written verbatim,
so it carries LF where the text write produced CRLF, and seeded lines are byte-for-byte. Git
reads either (verified).
Docstring claims the fix falsifies are rewritten rather than left standing — the seed's
"a missing/unreadable/undecodable excludes file is not a reason to skip the shield" (measured
rule: absent is not; present but unreadable is), and the tail's
UnicodeErrorrationale, which no longer has a file-content codec fault to justify it. Two Unreleased
CHANGELOG entries (#384, #359) made claims this falsifies and are amended in place rather
than contradicted by a third entry.
Tests
New:
shield_preserves_a_non_utf8_excludesfile(the regression),shield_degrades_when_the_users_excludesfile_cannot_be_read,shield_reprovision_does_not_duplicate_patterns(the untested idempotency gap),shield_dedupe_does_not_split_on_non_git_line_breaks, plusatomic_write_bytesmirroredacross the eight
atomic_write_textpins and three cases that are only about this variant(verbatim bytes, no newline translation, staging inside the target's directory).
Repurposed rather than deleted, since the fix reverses what they asserted:
degrades_on_undecodable_excludebecomesappends_to_a_non_utf8_private_exclude(the bytessurvive and the shield applies — strictly better than the old policy of preserving the file
by giving up on shielding), and
degrades_on_unencodable_patternis re-pointed at the"\ud800"shape that still raises, POSIX-only, because a real non-UTF-8 filename nowround-trips. Three monkeypatch targets moved from
atomic_write_texttoatomic_write_bytes;left alone they become silent no-ops and fail on their degrade assertions.
Per AGENTS.md every negative assertion was ablated, including re-running the nine existing
shield ablations, since the payload path moved underneath them — 27 gates deleted one at a
time, all failing their tests. Two of those runs changed the work:
chmod(0o600)cannot tella preserved mode from
mkstemp's own 0600 default, and "leaves no temp behind" cannot see atemp staged outside the target's directory. Both had been passing for the wrong reason and are
now pinned properly.
One further comment hardened while in the area (
ab0cef9's relative-core.excludesFileresolution): reconfirmed on 2.20.4 and 2.49.1, but it is unspecified behavior, not a
promise — relative values for that key were deliberately never implemented, and the
worktree-top result is an artifact of git's setup
chdir. bmad-loop is not exposed (every gitcall goes through
_run_git'sgit -C <repo>, and the shield writes an absolute path), andthe comment now says "measured, not specified" so a later reader knows its standing.
uv run pytest -q -n auto: 3748 passed, 24 skipped (branch baseline 3732 + 16 new).uv run pyright0 errors;trunk fmt+ fulltrunk checkclean.Summary by CodeRabbit
core.excludesFilenow correctly disables excludes; degraded exclude writes are journaled and surfaced without unintended repo-wide changes.atomic_write_bytes.Review round 5
Codex reported two more channels reaching an empty seed and activating anyway. Both valid, both live, both silent, both reproduced end-to-end against real linked worktrees before any code changed. Everything below is in
d0e50ad— nothing deferred to a follow-up.3679637956GitErrorreadingcore.excludesFilewas swallowed as "no excludes", so the activation shadowed the file it failed to read — silently3679637962.strip()destroyed an excludesFile path with edge whitespace → same silent leak-zand split on NULworktree-open-failedgates.notifybeside the journal appendextensions.worktreeConfig(a permanent repo-format change) was written above the seed, so every degrade below it left an operator's repo marked for a shield that never applied_shield_enable_worktree_confignow only probes; the write moved to immediately above the activationThe last two are consequences of P1 rather than separate reports: the whole argument for skipping over shadowing rests on the skip being visible, and P1 adds a degrade path that previously did not exist. Both were reachable before P1; P1 is what made them matter.
The alternative architecture — evaluated, rejected, recorded in code. One design dissolves this entire finding class (an empty seed plus an activation that shadows what it failed to copy): drop the
core.excludesFilemechanism and instead unstage the tool paths in the orchestrator's own commit path, after theadd -Ainverify.py. No shadow, no seed, no permanent flag, no 2.20 floor — and it is mechanically reachable, since those adds are the orchestrator's own and no skill indata/skills/runs git. Rejected on two correctness grounds: it converts ambient protection into call-site protection (the shield has to hold for agit add -Abmad-loop never issued — the coding-CLI sessions have shell access and can stage on their own initiative), and it leaves the tool files permanently untracked-visible in everygit statusa session runs, inviting exactly the commit the shield prevents, from exactly the actor we cannot constrain. Recorded as a comment in the shield block the way round 4 recorded the rejected--includesfinding, not as an issue, so a later round doesn't re-propose it as a simplification.The shadow is also structural rather than an artifact of this implementation, so there was no third mechanism to look for: gitignore(5) documents four pattern sources and the per-repo one is
$GIT_COMMON_DIR/info/exclude— the common dir. There is no per-worktree exclude file,core.excludesFileis singular, and git never concatenates the key across scopes. git 2.55.0 is the newest release; there is nothing newer to adopt.Tests. Four new (net +4 cases):
test_shield_degrades_when_git_will_not_say_what_the_users_excludes_are— the repurposed test that used to pin the swallow, kept rather than deleted since the fix reverses what it asserted. Parametrized overGitError/GitSpawnError(the latter is the most plausible future re-narrowing, and aGitError-only test would pass against it). Plants the operator's excludes file first — the old version planted none, so it could not observe the shadow in either direction. Asserts"did not answer", not"timed out", which is a lie forGitSpawnErrorand is the string a neighbouring test already owns.test_shield_seeds_an_excludesfile_whose_path_has_edge_whitespace— POSIX-gated; one filename (" my-global-ignores ") covers both sides of the old strip.test_shield_degrade_leaves_no_permanent_repo_format_changeandtest_shield_enables_the_extension_on_the_path_that_activates— the pair that distinguishes the reordering from a deletion. Neither alone does.test_isolated_exclude_degrade_is_journaled_and_notifiedextended to both channels; its docstring's "journal-only by design" claim was made false by this round and is corrected in place.Ablations (AGENTS.md — every negative assertion), 14 gates deleted one at a time:
except GitError: return b""→ the repurposed test fails twice over, on the reason and independently on the harm (?? mine.log), verified by disabling the earlier assertions so the harm assertion had to bite on its own..strip()→ the whitespace test fails on the empty seed, and independently on?? mine.log.gates.notify→ the ATTENTION assertion fails, the journal one still passes.worktreeConfig = truebehind. Deleting it outright instead fails the happy-path test with git's own--worktree cannot be used …fatal, which independently confirms the comment's claim that the write cannot move below the activation.returncode != 0disjunct and left the version comparison intact, so the gate still held. Re-targeted at the whole condition it fails, and the failure surfaces through the fake's ownmade a permanent format change on git 2.19.4guard — which is the evidence that the deferred enable is still behind the version gate, i.e. that the split did not move a gate.-zwas checked at the 2.20 floor in Docker (git 2.20.4) as well as on 2.55.0, rather than inferred from git-config(1) — all six branches identical at both.Verification.
uv run pytest -q -n auto: 3752 passed, 24 skipped (branch baseline 3748 + 4).uv run pyright0 errors.trunk fmt+ fulltrunk checkclean.Docs amended in place rather than appended to, since both entries are under
[Unreleased]and nothing shipped is being reversed:CHANGELOG.mdanddocs/FEATURES.mdboth promised that "an excludes file that exists but cannot be read skips the shield" — widened to include a git that will not say which file applies, stating that only a definite absent answer stays silent, and the permanent-flag sentence now says it is set only when the shield actually activates.worktree_flow.py:118-123needed no edit: the P1 fix is what makes that existing promise true.Review round 6
One finding, on round 5's own fix. Valid, reproduced, fixed in
d4a1cc9.3679930476extensions.worktreeConfigenable down to just above the activation, closing every degrade path above it — but not the activation's own failure, one line later, which still returned a reason with the permanent format change madeThe claim in round 5's docstring — "nothing between here and the activation can degrade" — was true of everything between them and false of the activation itself. Reproduced with the injection Codex named (fail
--worktreewrites only, so the enable succeeds):worktreeConfig LEFT BEHIND: True. This falsified a guarantee round 5 had just written into the docstring,CHANGELOG.mdanddocs/FEATURES.md, which is why it's fixed here rather than filed.The
needs_enablegate on the rollback is the part worth reviewing. It is a safety property, not a micro-optimization: a repo that already carried the extension may haveconfig.worktreefiles other worktrees depend on, and unsetting it there would stop git reading them. The rollback only undoes a flag this call set.Measured, since "unset the key" and "the extension is off again" are separate claims:
--unsetreturns rc 0 and removes the key and the now-empty[extensions]section, andgit config --worktreerefuses again afterwards with git's own fatal — so the rollback is real, not cosmetic.Not made transactional in the stronger sense, and recorded so it isn't re-raised: two
git configinvocations writing two different files (.git/config,.git/config.worktree) cannot be made atomic. Compensating on failure is the available shape, and the residual window (enable ok → activation fails → unset fails) is now reported rather than silent — folded into the reason, which reacheson_degradedand as of round 5 both journals and notifies.Tests — 3 new, each with its own ablation, all confirmed to fail the right test:
test_shield_rolls_back_the_extension_when_activation_fails— flag gone after the degrade, and the reason does not claim a rollback failure that did not happen. Ablation: drop the--unset.test_shield_reports_a_rollback_it_could_not_make— both faults named in one reason (the coherent case: a read-only.gitfails both writes). Ablation: drop the report branch.test_shield_never_unsets_an_extension_the_repo_already_carried— the gate. Injected as a hard failure on the call, not an assertion on the config text, since the flag still being present would also hold if the unset had run and failed. Ablation: drop the gate → the fake raises.uv run pytest -q -n auto: 3755 passed, 24 skipped.uv run pyright0 errors.trunk fmt+ fulltrunk checkclean.Process note on the bot channels, since it affected this round. CodeRabbit's status check has been reporting
passwhile not reviewing at all — the body says "Reviews paused … this branch is under active development" (reviews.auto_review.auto_pause_after_reviewed_commits). That is a fourth distinct fake-green shape on this repo, after CodeRabbit's rate-limit, its reviews-disabled-for-this-base-branch, and Greptile's credit limit (still boilerplate here, not a review). An explicit@coderabbitai reviewis needed, and a push while one is queued appears to consume it.Review round 7
One finding, again on the previous round's fix. Valid, reproduced, fixed in
8494983.3680006079GitError/GitSpawnErrorwent to the tail with the permanent format change still madefaultstring before anything is decided, so there is a single rollback pointThree rounds landed on the same question, and the pattern is the finding. Worth stating plainly rather than burying, per CONTRIBUTING's note that review non-convergence is evidence about the approach:
The first two fixes each handled the failure mode they had been shown.
verify.git_byteshas exactly two (a non-zero rc, and a raise), so enumerating them one review round at a time was always going to take as many rounds as there were modes. The current shape funnels both into onefaultvalue before any decision, leaving one place the flag can be rolled back; a third shape would have to get past thetryitself. That is why this is a reshape rather than a third special case.Codex found it by reading a test, not the code — and that test is the second half of the story.
test_worktree_local_exclude_git_fault_after_answering_degradeshas been injecting exactly this raised activation fault since round 4 while asserting only the returned reason, so it sat on top of a live defect for two rounds without seeing it. It now asserts the flag is gone and is parametrized over both fault classes. The general lesson is in its docstring: a degrade assertion that stops at the reason string cannot tell a clean degrade from one that left state behind.Two details worth review:
GitErrorat the activation is a narrowing of what reaches the tail, not a bypass. The tail cannot roll the flag back, and a fault from that call means what a refusal means. The tail'sGitErrorguard stays load-bearing for the seed read (round 5's fix) and is still pinned by its own test._shield_undo_extension, which catches that and reports rather than raises, so the activation fault isn't lost behind it.Ablations — 16 gates, all failing their tests, including re-running the round-6 rollback ablations and the nine original shield ones since the arm was restructured underneath them. Two of the nine needed re-anchoring after the reshape, which is its own lesson worth recording: an ablation script's anchors rot silently when code is reformatted, and a
SKIP: anchor matched 0xreads almost like a pass. Both were re-targeted and fail correctly.uv run pytest -q -n auto: 3757 passed, 24 skipped.uv run pyright0 errors.trunk fmt+ fulltrunk checkclean. All 9 CI jobs were green ond4a1cc9including both Windows runs; re-running on8494983.Review round 18
One finding, valid, live and silent. Fixed in
b5f0834.3685528598git config --worktree core.excludesFileproves the value was WRITTEN, not that git READS it —commandscope outranksworktree, so an ambientcore.excludesFileleft a shield that reported success and never appliedfaultfunnelRound 17's shape one level up. That round was present is not effective (a pattern in the seeded file that a negation below it cancelled). This is written is not effective. Reproduced end to end on a real linked worktree before anything changed, with git's own output as the oracle:
reason=None,git status -uall→?? tool-dir/file.txt,git add -An→add 'tool-dir/file.txt'.The fix is a post-condition, not an origin check, and the measurement is why. Codex recommended detecting the higher-priority origin (
GIT_CONFIG_COUNT/GIT_CONFIG_KEY_*). Measured on real linked worktrees at both ends of the supported range:GIT_CONFIG_COUNT/KEY_n/VALUE_nGIT_CONFIG_PARAMETERS'k=v'GIT_CONFIG_PARAMETERS'k'='v'fatal: bogus formatgit -citself emits'k=v''k'='v'The channel the finding names does not exist at this shield's own 2.20 floor; the one that does exist there uses an encoding the newer git rewrote; and a
git -con a session's own command line is a third that never reaches our environment at all. So the fix asks git which excludes file it resolved and compares it to the path just written — one call, no environment-variable names, no new version floor (the read shape is the seed read's, already measured at both versions).--show-scopewould name the winning scope but is git 2.26, above the floor, and per round 1 a probe flag with its own version floor turns every rc-branch below it into a silent default.This extends rounds 5→7 rather than repeating them: those funnelled the ways a call can fail, and a complete funnel over that still said nothing about whether the call achieved what it was for. The verification sits inside the same
try, so its own two failure shapes reach the existing rollback instead of adding a second one.Three things the finding did not name:
--file <shared>/config, which is immune to command scope (measured: withcore.bare=trueinjected, the--fileread still answersfalse). No gate can be spoofed this way.config.worktree— but unsettingextensions.worktreeConfigmakes git stop reading that file at all, so the leftover key reads back unset while still on disk. Inert, not removed, and pinned that way. A second--unsetwould be a second write that can fail.install.py:749-781: sessions have shell access, andtmux new-window -ecan only overlay an env var, never unset one, so scrubbing cannot make the guarantee hold — and silently overriding config an operator deliberately set is provision_worktree's exclude patterns are repo-wide and permanent: new files under .claude/skills silently stop being staged in the main checkout #384 in mirror image.Tests — 6 new cases. The regression is parametrized over both env channels because an origin-detection fix would pass one and fail the other, with the COUNT case skipped below 2.31 and non-vacuity asserted inside the same environment. The fault-injection fakes key on state, not argv: the seed read and the verification read use byte-identical arguments and real git distinguishes them only by what was written in between, so an argv-keyed fake would fault the seed and test a different arm.
Ablations — 18/18 fail their tests, including the nine original shield gates re-run since the activation arm was restructured underneath them, and re-run again after
trunk fmt. Two changed the work:last_negationablation passed, which this PR has taught me to read as "untested gate" — but it was the ablation that was wrong (-1 and max(...)evaluates tomax(...), a no-op). Third instance of that trap here.b"", which compares unequal to the written path, so the mismatch arm one line down still degrades — it just returns the wrong reason, naming''as what git reads.assert reason is not Nonewould pass against that bug, so the test asserts the wording, and the test's own docstring was corrected to match the measurement.Filed, not fixed here: #401 (shield patterns are not escaped, so a seeded filename containing gitignore metacharacters shields the wrong set — measured: the real
foo[1].jsonstays untracked while an unrelatedfoo1.jsonis hidden) and #402 (declare a project-wide git floor and move the 2.20 gate intovalidate).uv run pytest -q -n auto: 3791 passed, 24 skipped.uv run pyright0 errors;trunk fmt+ fulltrunk checkclean.