diff --git a/CHANGELOG.md b/CHANGELOG.md index 97fbb2d9..1ad1e5fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -260,6 +260,94 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **The worktree git-add shield no longer hides new files in your own checkout (#384).** Worktree + provisioning shielded the tool files it writes (`.claude/skills`, the per-CLI hook config, seeded + configs) by appending to `.git/info/exclude` — a file shared with the main checkout and every + sibling worktree, permanent, and unversioned. Those paths are ones projects legitimately track, so + every **new** file under them silently stopped being staged by `git add -A` in the operator's own + checkout, long after the run: one report lost 51 files and three whole skills across two upgrade + commits, with nothing in either diff able to reveal why. The shield now writes a private exclude in + the worktree's own gitdir, activated with a worktree-scoped `core.excludesFile`, so it applies to + that unit alone and `git worktree remove` deletes it along with the worktree. The operator's + `core.excludesFile` is copied in **byte for byte** when the private file is created, since the + worktree-scoped key shadows it and git never concatenates across config scopes — so the copy has + to survive an exclude file in any legacy encoding at all (its patterns are paths, and POSIX paths + are arbitrary bytes). Relative values — of that key, or of an `XDG_CONFIG_HOME` + git falls back through — are resolved against the worktree, the way git does, rather + than against wherever the orchestrator was launched. Paths are read NUL-terminated, so an + excludes file whose path begins or ends in whitespace is copied rather than mangled. An excludes + file that **exists but cannot be read** — or a **git that will not say which file applies** (a + timeout, a failed spawn, or any answer to that query other than a definite "there is no such + key") — skips the shield with a journaled reason instead: + activating over patterns that could not be copied would shadow them exactly as an empty copy did, + and not knowing whether there is anything to copy has the same standing as failing to read it. + Only a definite **absent** answer stays a silent no-op — there is nothing to shadow. The skip is + also **notified**, not just journaled, since skipping is only defensible if you find out. The same + rule governs the pair of `rev-parse` probes that identify the repository: only a failure of the + first is the expected silent skip (a plain directory, no git at all); once git has answered it, a + fault on the second is journaled rather than swallowed. An + **explicitly empty** `core.excludesFile` is an answer too, and not the same one as unset: git + honors it as "no excludes file at all" and does **not** fall back to `$XDG_CONFIG_HOME/git/ignore`, + so patterns you switched off are no longer copied into the private file and switched back on inside + the worktree — the mirror image of the same file-loss bug, over-ignoring instead of under-ignoring. + Concurrent runs against one repository are **serialized** on a lock in `.git/`, and a failed + activation never rolls the repo-format flag back while another worktree's `config.worktree` still + depends on it: without either, one run's failed shield silently switched off a sibling run's + working one mid-story. A repository configured as **shared between OS users** + (`core.sharedRepository`) is refused with a journaled reason rather than shielded — but only + where the value really grants peer access: `umask`, a false boolean, and an octal filemode with + no group or other bits (`0600`, `0700`, `0711`) leave the repository private to you, and it is + shielded normally. + Three caveats: this enables `extensions.worktreeConfig`, a **permanent** + repo-format flag that is never removed — written at the last possible moment, so a run that + degrades away **above** it leaves your repo's format untouched, and wherever it could be left set + without a working shield (the enable failing, the activation failing, or the activation succeeding + without taking effect) the flag is rolled back. It outlives a failed shield only where the rollback was declined because a sibling + worktree depends on the flag, or could not be made at all — and both are named in the reason — the + lock leaves a + zero-length `.git/bmad-loop-shield.lock` behind (inside `.git`, so never in your working tree and + never stageable) — and where git requires that flag's + prerequisites be moved by hand first (`core.bare = true` or `core.worktree` in the shared config) + the shield is skipped with a journaled reason rather than widened back. It also needs **git 2.20**, + the release that introduced the flag and `git config --worktree`; below that the shield is skipped + and no repo-format change is made, since git that old refuses a repository carrying the flag. Lines + an older bmad-loop already wrote into `.git/info/exclude` are **not** removed for you — delete them + by hand (a `validate` warning lands separately). +- **The git-add shield's patterns survive an inherited negation (#384).** A pattern the seeded + excludes already carried was treated as done — but gitignore's rule is last match wins, so a + `!` line below it cancelled the shield's own pattern and the provisioned tool files stayed + stageable, with nothing reported because nothing had failed. A pattern is now re-appended + unless it already sits after the last negation in the file. +- **The git-add shield checks that it actually applies (#384).** Writing the worktree-scoped + `core.excludesFile` proved only that the value was stored, not that git reads it: config supplied + through the environment (`GIT_CONFIG_COUNT`, `GIT_CONFIG_PARAMETERS`) outranks the worktree scope, + so an operator carrying an ambient `core.excludesFile` got a shield that reported success and never + applied — the provisioned tool files stayed stageable, with nothing reported because nothing had + failed. The shield now asks git which excludes file it resolves and skips with a journaled reason + when that is not the one it just wrote. +- **The git-add shield finds the same global ignore file git does, on Windows too (#384).** With + `core.excludesFile` and `XDG_CONFIG_HOME` both unset, git falls back to `$HOME/.config/git/ignore` + — and it locates that with `HOME` on every platform, while Python's `Path.home()` reads + `USERPROFILE` on Windows and never consults `HOME` at all. Git for Windows derives `HOME` itself, + preferring a `HOMEDRIVE`+`HOMEPATH` home share over `USERPROFILE`, so the two disagreed for anyone + whose shell sets `HOME` (Git Bash and MSYS2 do) or whose home directory is a network drive. The + wrong path is simply not a file, so the copy came back empty and the shield then shadowed the very + global ignores it exists to preserve — silently, since nothing had failed. The shield now asks git + where its home is rather than guessing, and a home git will not resolve skips the shield with a + journaled reason instead of being assumed to mean "there is no ignore file". +- **The git-add shield no longer opens its own safety gates when git cannot answer (#384).** The + three probes that ask whether enabling `extensions.worktreeConfig` is safe read every non-zero + exit code as "that key is not set", so a git that could not answer — rather than one answering + "absent" — let a repository that really sets `core.worktree` cross the gate and take a permanent, + irreversible repo-format change. Only exit code 1 means "no such key" now; anything else skips the + shield with a reason. Separately, the write that enables the flag handled git _refusing_ but not + git _failing to reply_, so a timeout landing after the config was already replaced left the flag + set with no shield ever activated; that path now rolls the flag back too. A rollback of a flag + that was already absent is no longer reported as a rollback that failed. +- **The git-add shield's git calls run on the shared chokepoint (#389).** They were the last bare + `git` spawns outside `verify._run_git`, so they missed its `LC_ALL=C` pin — leaving a degrade + reason that quotes git's stderr in whatever language the box speaks — and used a hardcoded 120s + timeout instead of the configured `[limits] git_timeout_s`. Both now apply. - **A non-UTF-8 filename no longer crashes the run past every git guard (#377).** `verify._run_git` is the sole git spawn point and exists to give git's faults a type its ~50 `except GitError` guards can catch, but it decoded git's output strictly and translated only a timeout and a spawn @@ -296,9 +384,10 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories - **A worktree's local git exclude really is best-effort now (#359).** `_worktree_local_exclude` guarded only its `git rev-parse` call; the filesystem tail behind it (resolve, mkdir, read, write) - crashed the run — a symlink loop raises `RuntimeError` on 3.11/3.12, an exclude file that is not - UTF-8 raises `UnicodeDecodeError`, a seeded path whose filename is not UTF-8 raises - `UnicodeEncodeError` on the way back out, and a read-only `.git` raises `OSError`. The tail is guarded and + crashed the run — a symlink loop raises `RuntimeError` on 3.11/3.12, a read-only `.git` raises + `OSError`, and the codec faults of the day raised `UnicodeError` (an exclude file that is not UTF-8 + on the way in, a seeded path whose filename is not UTF-8 on the way back out — both since removed + as fault modes, because the exclude payload is now read, deduped and written as bytes). The tail is guarded and degrades to a reason string, and provisioning journals it as `worktree-exclude-degraded` on the story rather than swallowing it: without the exclude the unit's `git add -A` would commit the provisioned skill trees and tool configs into the story's merge. Git being unqueryable at all diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 1e0e7596..2c4407d8 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -75,7 +75,8 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Off by default (`[scm] isolation = "none"` — work in place on the checked-out branch, byte-for-byte the prior behavior). Set `isolation = "worktree"` and each story (and each sweep bundle) runs in its own `git worktree` on a `bmad-loop/[/]` branch cut from the target branch, then merges back **locally** — the main checkout stays free while a run is in flight. - Merge knobs: `merge_strategy` (`ff` / `merge` / `squash`), `target_branch` (default = branch checked out at run start; created if missing — a detached HEAD or unborn repo pauses the run instead of merging onto an unreferenced commit), `branch_per` (`story` or a shared `run` branch; `run` forces `delete_branch = false`), and `delete_branch`. - Failed-unit forensics: a deferred/escalated unit's worktree + branch stay mounted (`keep_failed`, default on) and its full diff is preserved to `run_dir/failed//changes.patch`; `failed_diff_max_mb` caps per-file untracked-file size (oversized skipped with a marker), `failed_diff_unlimited` lifts the cap. -- Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches; `worktree_seed` adds extra paths. Copy-when-absent at file granularity — a directory entry whose destination already exists (a worktree checkout carries its tracked children) still seeds the children that are missing — seeded before the hook-merge (a seeded `settings.json` keeps its content and just gains the Stop hook), and shielded from the unit's `git add -A`. +- Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches; `worktree_seed` adds extra paths. Copy-when-absent at file granularity — a directory entry whose destination already exists (a worktree checkout carries its tracked children) still seeds the children that are missing — seeded before the hook-merge (a seeded `settings.json` keeps its content and just gains the Stop hook), and shielded from the unit's `git add -A` — in a private exclude scoped to that worktree alone (see below), never repo-wide. +- The git-add shield is scoped to the worktree and expires with it (#384). The provisioned tool files (skill trees, the per-CLI hook config, seeded configs) are excluded through a private `.git/worktrees//info/exclude`, activated by a worktree-scoped `core.excludesFile` — so the shield applies to that unit only, and `git worktree remove` deletes it along with the worktree. Storing that key is not the same as git reading it: config supplied through the environment (`GIT_CONFIG_COUNT`, `GIT_CONFIG_PARAMETERS`, or a `git -c` above us) is **command** scope, which outranks **worktree** scope, so an ambient `core.excludesFile` of your own would leave a shield that reported success and never applied. After activating, bmad-loop asks git which excludes file it actually resolves; if that is not the one just written, the shield is skipped with a journaled reason rather than reported as working. The repository-wide `.git/info/exclude` is never written: it is shared with your own checkout and permanent, so shielding through it made every **new** file under a tracked tool dir (`.claude/skills`, `.claude/settings.json`) silently invisible to your `git add -A`, long after the run. Because that key shadows your own `core.excludesFile` (git reads it from the most specific scope and never concatenates), your excludes file is copied into the private one **byte for byte** when it is created — any encoding, since exclude patterns are paths and POSIX paths are arbitrary bytes — and any path, read NUL-terminated so leading or trailing whitespace in the filename survives. An excludes file that exists but **cannot be read**, or a **git that will not say which file applies** (a timeout, a failed spawn, or any answer to that one query other than a definite "there is no such key"), skips the shield with a journaled and notified reason instead of shadowing patterns it could not copy: not knowing whether there is anything to copy has the same standing as knowing there is and failing to read it. Only a definite **absent** answer is a silent no-op. The same rule governs the two `rev-parse` probes that identify the repository: only a failure of the first is the expected silent skip (you handed it a plain directory, or git is missing) — once git has answered that one, a fault on the second is journaled rather than swallowed. An **explicitly empty** `core.excludesFile` is not an unset one: git reads that as "no excludes file at all" and does **not** fall back to `$XDG_CONFIG_HOME/git/ignore`, so neither does the shield — patterns you deliberately switched off stay off instead of being copied into the private file and re-applied inside the worktree. Two runs against one repository are **serialized**: the probe → enable → activate → rollback sequence is taken under an exclusive lock, and a failed activation never rolls the repo-format flag back while another worktree's `config.worktree` still depends on it — otherwise one run's failure would silently switch off a sibling run's live shield. Three residues are worth knowing about: enabling this needs `extensions.worktreeConfig`, a permanent repo-format flag bmad-loop sets once and never removes — written at the last possible moment, so a degrade **above** it never leaves your repo marked for a shield that did not apply, and wherever it could be left set without a working shield — the enable failing, the activation failing, or the activation succeeding without taking effect — the flag is rolled back. It outlives a failed shield only where that rollback was declined because a sibling worktree depends on the flag, or could not be made at all, and the reason says which — the lock leaves a zero-length `.git/bmad-loop-shield.lock` behind (inside `.git`, so never in your working tree and never stageable), and lines an older bmad-loop already wrote into `.git/info/exclude` are **not** removed for you — delete them by hand. Where the flag cannot be set safely (`core.bare = true` or `core.worktree` in the shared config, which git requires you move first), the shield is skipped with a journaled reason rather than widened back — and a git that cannot **answer** those two questions is treated the same way rather than as a "no", since reading "git failed" as "that key is unset" is what would open the gate. It also needs **git 2.20 or newer** — the release that added both the flag and `git config --worktree`; on anything older (or a git that will not report its version) the shield is skipped the same way, and the repo-format flag is deliberately _not_ written, since git that old refuses a repository carrying it. - Run state never moves into a worktree — `.bmad-loop/` always lives in the main repo; spec paths are persisted relative to the worktree so a kept-failed run stays portable. - Merge-back is serialized; `max_parallel` is a validated knob clamped to `1` until parallel fan-out is built. The `repo_root` key in `_bmad/bmm/config.yaml` (defaults to the project dir) decouples where git/code work happens from where run state lives (monorepos). - `commit_message_template` (`{story_key}` / `{run_id}` substituted) customizes story/bundle commit messages. diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 6af7c917..0a548fe7 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -23,15 +23,17 @@ import subprocess import tomllib from collections.abc import Iterable, Sequence +from contextlib import ExitStack from importlib import resources from pathlib import Path from typing import Any, NamedTuple from .adapters.profile import ALIASES, CLIProfile, ProfileError, load_profiles from .checks import Finding -from .platform_util import atomic_write_text +from .platform_util import atomic_write_bytes, file_lock from .policy import POLICY_TEMPLATE from .process_host import get_process_host +from .verify import GitError, git_bytes HOOK_SCRIPT_REL = ".bmad-loop/bmad_loop_hook.py" # Markers for bmad-loop-managed hook commands. RELAY_MARKER is shared by @@ -731,113 +733,1526 @@ def _copy_traversable(src, dst: Path, *, skip_existing: bool = False) -> bool: return True -def _worktree_local_exclude(worktree: Path, patterns: Sequence[str]) -> str | None: - """Add anchored ignore patterns to the worktree's local git exclude so the - provisioned tool files are never staged by the unit's `git add -A`. Uses - git's standard local-only exclude (never committed or pushed); it does not - affect already-tracked files. +# The git that introduced `extensions.worktreeConfig` and `git config --worktree`, +# both of which the worktree-scoped shield is built out of (git-worktree(1)). +_WORKTREE_CONFIG_GIT = (2, 20) + +# The shield's mutual exclusion, held in the repository's COMMON dir so every +# worktree of a repo contends on one file — a per-worktree gitdir would give each +# concurrent run its own lock and exclude nothing. A dedicated file, never config +# or the exclude itself, per `file_lock`'s contract: the lock rides an open fd's +# inode, so anything written through `atomic_replace` swaps that inode out from +# under later acquirers. Zero bytes, and inside `.git` — never in the working tree, +# so it is not something the operator's `git add -A` can see. +_SHIELD_LOCK_NAME = "bmad-loop-shield.lock" + +# THE ALTERNATIVE ARCHITECTURE, EVALUATED AND REJECTED — recorded here the way the +# `--includes` finding below is, so a later reader or bot round does not re-propose +# it as a simplification of the whole block. +# +# Every finding in this family (an empty seed plus an activation that shadows what +# the seed failed to copy) exists because the shield SHADOWS a config key. One +# design dissolves the class outright: drop `core.excludesFile` entirely and instead +# unstage the tool paths in the orchestrator's own commit path — `git reset -- +# ` after the `add -A` at `verify.py:1891` and `:1932`. No shadow, so no +# seed, no `_shield_inherited_excludes`, no permanent `extensions.worktreeConfig`, +# no 2.20 floor. It is MECHANICALLY REACHABLE: those adds are the orchestrator's +# own, not an LLM's (no skill in `data/skills/` runs git). +# +# Rejected on the merits, for two reasons that are about correctness rather than +# scope: +# +# - It converts AMBIENT protection into CALL-SITE protection. An ignore binds +# whoever runs the add; a reset covers only the call sites we remember to patch. +# These adds run around disposable coding-CLI sessions that have shell access and +# can stage and commit on their own initiative, so the shield has to hold for a +# `git add -A` bmad-loop never issued. That actor is the one we cannot constrain, +# and the reset design is the one that stops protecting against it. +# - It leaves the tool files permanently UNTRACKED-VISIBLE in the worktree. Every +# `git status` a session runs would list the skill trees, the per-CLI hook config +# and the seeded configs as untracked — inviting exactly the commit the shield +# exists to prevent, from exactly that actor. +# +# The shadow is also structural, not an artifact of this implementation, so there is +# no third way to look for: gitignore(5) documents four pattern sources and the +# per-repo one is `$GIT_COMMON_DIR/info/exclude` — the COMMON dir, shared by every +# worktree. There is no per-worktree exclude file, `core.excludesFile` is singular, +# and git never concatenates the key across scopes. Checked against current sources +# at git 2.55.0, the newest release. + + +def _git_version_at_least(reported: str, want: tuple[int, int]) -> bool: + """Is this `git version …` line at least `want`? Anything unreadable is NO. + + Only `major.minor` is compared, and only from a line that actually starts + `git version` — the tail is vendor soup (`2.44.0.windows.1`, + `2.39.5 (Apple Git-154)`) and searching the whole string for the first two + dotted numbers would happily read a version out of a build tag. + + Refusing an unparseable answer is the point rather than a fallback. The only + caller uses this to decide whether to make a PERMANENT repo-format change, so + the failure it must not have is the optimistic one; a git that will not say + what it is does not get that change made on its behalf. + """ + match = re.match(r"git version (\d+)\.(\d+)", reported.strip()) + return match is not None and (int(match[1]), int(match[2])) >= want + + +def _shield_shared_config(worktree: Path, shared: str, key: str, *opts: str) -> bytes | None: + """`key`'s raw value in the SHARED config, or None when it is definitely ABSENT. + + Raises `GitError` when git did not answer. **rc 1 is the only non-zero rc that + means "no such key"** — the funnel `_shield_inherited_excludes` already applies to + its own read (round 9), applied here to the three probes that were still reading + every non-zero rc as an absence (#384, Codex round 10). Routing those to ABSENT + degrades the `core.bare` / `core.worktree` gates OPEN, and what they guard is + irreversible: enabling `extensions.worktreeConfig` drops the exception that + confined those keys to the main worktree, after which a genuine `core.worktree` + starts applying to every linked worktree. + + Measured at git 2.20.4 AND 2.55.0, identical at both ends of the supported range: + + absent key ......................... 1 malformed config file ..... 128 + missing file ....................... 1 --type=bool on a non-bool . 128 + `--file` naming a directory ........ 1 key present ................ 0 + unreadable file (EACCES, non-root) . 1 empty or valueless key ..... 0 + + git-config(1) prefaces its own list with "Some exit codes are:", i.e. the list is + not closed, and it documents a malformed file as ret=3 while both versions + actually answer 128 — so a `{0,1}`-closed reading is wrong on git's documentation + AND on its behavior. Hence a funnel rather than a per-rc taxonomy (round 7). + + THE RESIDUAL THIS CANNOT CLOSE, stated rather than left for another round: a + momentarily missing or unreadable `.git/config` answers **rc 1**, byte-identical + to "no such key" — all four rc-1 rows above are the same answer from here. git + exposes no way to separate them, so this is a bound on the interface rather than a + defect. Deliberately NOT narrowed with a pre-`--get` stat: that is TOCTOU, which + shrinks the window instead of closing it, adds a syscall to every probe, and + trades a very rare silent-wrong for a less rare noisy-degrade. Bounded in practice + because git rewrites config atomically (lock + rename), so only a non-git tool + truncating in place can produce it. + + Returns the value RAW and undecoded — `core.worktree` is a path — so callers test + presence with `is not None`, never truthiness. See the call sites for why. + """ + answer = git_bytes(worktree, "config", "--file", shared, *opts, "--get", key) + if answer.returncode == 0: + return answer.stdout + if answer.returncode == 1: + return None + detail = os.fsdecode(answer.stderr).strip() or f"git exited {answer.returncode}" + raise GitError(f"git could not read {key} from the shared config: {detail}") + + +_SHARED_REPOSITORY_OCTAL = re.compile(r"[0-7]+") + + +def _shared_repository_is_private(value: str) -> bool: + """Does `core.sharedRepository = value` leave the repository private to its owner? + + git's parse, mirrored (`setup.c::git_config_perm`): the keyword `umask` and the + false booleans are private, `group`/`all`/`world`/`everybody` and the true + booleans are not, and an octal value is a FILEMODE with the legacy 0/1/2 + special-cased ahead of it. + + Measured at git 2.20.4 AND 2.55.0, byte-identical at both ends of the supported + range, by the mode of a loose object git writes under `umask 077` — git's OWN + answer, not a reading of `setup.c`: + + 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 + + So the mask is `value & 0666` and private is `& 0066 == 0` — NOT "no execute + bits" and NOT "equals 0600". `0711` IS private, because the mask discards the + execute bits; `0755` is NOT, because its group/other READ bits survive it. Those + two rows are why this is a mask test and not a literal set, and both were + mispredicted before they were measured. + + `(mode & 0600) != 0600` is git's own `die()` condition, so such a value reaches + us only in a repository git already refuses to operate on. It stays off the + accept side with every other value git rejects. + + THE PATTERN IS DELIBERATELY STRICTER THAN `strtol`, in the refusing direction. + git accepts leading whitespace and a leading `+` (measured: `+0600` is private) + which `[0-7]+` refuses — a false refusal, which this gate is contracted to + prefer. Strictness in the other direction is the load-bearing half: Python's + `int(value, 8)` accepts `0o600` and `0_600`, which git REJECTS, so parsing with + Python's own rules would accept a repository git cannot open. + + THE EMPTY STRING IS EXCLUDED ON PURPOSE, and the `+` quantifier is what excludes + it. `strtol("")` converts nothing, leaves `*end == 0` and yields 0, so git reads + an explicitly empty value as PERM_UMASK, i.e. private. It is refused anyway, + because `--get` answers an empty value and a VALUELESS `sharedRepository` line + (PERM_GROUP, shared) with the same rc 0 and the same lone newline. What is + refused there is the ambiguity, not the value — see the caller. + """ + if value == "umask" or value.lower() in ("false", "no", "off"): + return True + if not _SHARED_REPOSITORY_OCTAL.fullmatch(value): + return False + mode = int(value, 8) + if mode == 0: # PERM_UMASK + return True + # The legacy 1 (PERM_GROUP) and 2 (PERM_EVERYBODY) need NO special case here even + # though git special-cases them ahead of its filemode branch: neither satisfies + # `& 0600`, so both land on the same refusal this line already returns. Written as + # one test rather than three because a `mode in (1, 2)` arm would be indistinguish- + # able from dead code — no test could fail without it (#384, round 10's rule). + return mode & 0o600 == 0o600 and mode & 0o066 == 0 + + +def _shield_shared_repository(worktree: Path, common_dir: Path) -> str | None: + """Refuse a repository configured to be SHARED BETWEEN OS USERS, else None. + + A repository shared between OS users is not a supported configuration + (maintainer decision, #384): the shield creates files of its own inside `.git`, + and making those work across OS users is out of scope. Refusing the + configuration up front is what makes that decision visible — it fails LOUDLY and + identically for every user, with a reason that is journaled and notified, rather + than behaving differently depending on which user provisioned first and leaving + state behind for the next run to meet. + + This gate runs ABOVE the shield's lock, which is the point of it: nothing is + created in a shared repository at all. That is safe here and would not be for + `extensions.worktreeConfig` — this key is static configuration the tool never + writes, so there is no shared mutable state to serialize. + + It adds no version floor: a plain `--get` with no `--type=`, so it does not + reintroduce the 2.18 trap that keeps the version gate above the probes in + `_shield_enable_worktree_config`. + + AN ALLOWLIST, NOT AN ENUMERATION OF SHARED SHAPES (round 7's funnel). git accepts + keywords, booleans, the legacy 0/1/2, and bare octal modes; enumerating the + shared ones leaves every value git adds later, and every value git rejects, + silently opening the gate. Measured at git 2.20.4 AND 2.55.0, byte-identical at + both ends of the supported range. The verdict column is git's OWN answer — the + mode of a loose object git writes under `umask 077` — not a reading of `config.c`: + + absent .................. rc 1 (no key) + umask / false/no/off / 0 rc 0 r-------- PRIVATE + FALSE, Off .............. rc 0 r-------- PRIVATE + empty (`= ""`) ..... rc 0 r-------- PRIVATE <- stdout is b"\\n" + valueless (no `=`) ..... rc 0 r--r----- group <- stdout is b"\\n" TOO + true/yes/on / group / 1 . rc 0 r--r----- group + all/world/everybody / 2 . rc 0 r--r--r-- everybody + octal filemodes ......... rc 0 varies `_shared_repository_is_private` + + THE MIDDLE TWO ROWS ARE INDISTINGUISHABLE AND MEAN OPPOSITE THINGS: an explicitly + empty value is PERM_UMASK (private) and a VALUELESS `sharedRepository` line is + PERM_GROUP (shared), while `--get` answers both rc 0 with a lone b"\\n". git + exposes nothing that separates them, so the empty answer REFUSES — a false refusal + is a reported skip, a false accept is the bug this gate exists to prevent. + + `removesuffix("\\n")`, NEVER `.strip()`, and that is live rather than defensive + here: `--get` returns edge whitespace VERBATIM (a quoted `" umask"` comes back as + b" umask\\n", measured at both versions), so stripping would widen a value git + itself rejects straight into the accept set below. Round 15's lesson at a second + site in this same function. + + The case handling is measured rather than inferred from the accept list's shape: + `FALSE` and `Off` really are not shared at both versions and are accepted, while + `UMASK` and `GROUP` are values git REJECTS — git comparing the keywords with + strcmp and the booleans with strcasecmp. Values git rejects DO reach this gate: + `rev-parse --absolute-git-dir` answers rc 0 for every one of them, `banana` + included, so the "it would have fatalled earlier" reasoning that fits some other + probes in this file does not hold here. They fall off the allowlist into a + refusal deliberately — their repository is already unusable (`git status` exits + 128, `fatal: bad boolean config value ... for 'core.sharedrepository'`), so + refusing costs nothing while guessing at a malformed value would. + + SCOPE, stated because this is NARROWER than git's own resolution: the read names + the repository's SHARED CONFIG, as every probe in `_shield_enable_worktree_config` + does. Measured, git honors `core.sharedRepository` from a GLOBAL config too (an + object written under a global `= group` came back `r--r-----` with the repo config + empty), so an operator's global setting is not consulted here and such a + repository is still shielded — unchanged from the behavior before this gate + existed, and deliberately so, since a global preference is not a statement that + THIS repository is shared. What is refused is a repository CONFIGURED as shared, + which is the shape `git init --shared` writes: measured, `--shared=group` records + `sharedrepository = 1`, the legacy numeric form the table above lists as group. + + WORKTREE SCOPE is unread too, and unlike the global case that omission rests on a + barrier two modules away rather than on anything this function does. git DOES honor + `core.sharedRepository` from a linked worktree's own `config.worktree` — measured at + 2.20.4 and 2.55.0, an object written from such a worktree under `umask 077` came back + `r--r-----` against a `r--------` control, while the `--file` read below answers rc 1 + for it. What makes that unreachable is the provisioning flow: the shield only ever + runs on a worktree this process just created (`workspace.open_unit_workspace` -> + `verify.worktree_add` -> `worktree_flow.provision_worktree`), `git worktree add` + creates the admin dir with NO `config.worktree` at either version, and a re-mount + prunes `.git/worktrees/` whole. The MAIN worktree's `.git/config.worktree` does + not reach a linked one either (measured, `r--------`), and the tree's only + worktree-scoped write is the shield's own `core.excludesFile`, further down this + file. So the value has no writer and no window. 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, this read must grow a second + `--file /config.worktree` probe in the same change. + """ + raw = _shield_shared_config(worktree, str(common_dir / "config"), "core.sharedRepository") + if raw is None: + return None + value = os.fsdecode(raw).removesuffix("\n") + if _shared_repository_is_private(value): + return None + # {value!r}: operator-authored text going into a journaled reason, and repr + # neutralizes a newline in it. The version gate above renders git's own answer + # the same way. + return ( + f"skipped the git-add shield ({worktree}): the repository's shared config sets " + f"core.sharedRepository = {value!r}, and a repository shared between OS users is " + "not a supported configuration — the provisioned tool files are not shielded " + "from the unit's `git add -A`" + ) + - Best-effort in two arms that must stay separate, because `OSError` means the +def _shield_enable_worktree_config(worktree: Path, common_dir: Path) -> tuple[str | None, bool]: + """May `git config --worktree` be made writable in this repo? `(reason, needs_enable)`. + + PROBE ONLY — it does not write. `needs_enable` is True when every gate passed + and the caller must still set `extensions.worktreeConfig` itself; False when the + repo already carries it (or when `reason` is set and nothing may be written). + Splitting the answer from the write is round 5's fix: enabling the extension is + a PERMANENT repo-format change, and performed here it landed BEFORE the seed, + the mkdir, the write and the activation — so every degrade below it, including + the `_shield_inherited_excludes` one, left the operator's repo permanently + marked for a shield that never applied. The caller now enables it immediately + before the activation, and rolls the flag back when the shield then fails to + hold — including when the ENABLE ITSELF fails in a way that leaves the outcome + unknown, which is round 10. The flag outlives a failed shield only where the + rollback was DECLINED because a sibling worktree depends on it, or could not be + made at all; both are named in the returned reason. + + `extensions.worktreeConfig` is what makes a per-worktree config file exist at + all; without it `git config --worktree` either refuses outright (a repo with + linked worktrees) or silently writes the SHARED config. Already-true is the + common case after the first isolated run — reported as `needs_enable` False, so + the caller never rewrites it. + + Enabling it is a PERMANENT, repo-wide format change this code never reverses, + so it is refused in the two shapes git's own documentation calls out + (git-worktree(1), CONFIGURATION FILE): with `core.bare = true` or + `core.worktree` in the shared config, enabling the extension drops the + exception that confined those keys to the main worktree, and they would start + applying to every worktree. Git says to move them into the main worktree's + `config.worktree` first — a repo-layout edit the installer has no business + making behind an operator's back — so the shield degrades instead, and the + run continues with the tool files unshielded rather than the repo rearranged. + + The version gate and both refusal probes STAY HERE, ahead of everything, and + deferring them the way the write was deferred would be a bug. `git config + --worktree` does not exist before 2.20, so on an older git the enable buys a + permanent format change with nothing to show for it — and git-worktree(1) says + older git refuses a repository carrying the extension. And `--type=` is itself + git 2.18: on an older git the two probes below exit non-zero, which this + function USED to read as "not enabled" and, worse, as "not bare" — silently + opening the core.bare gate the paragraph above exists to hold shut. That is the + safety-gate-degrading-open trap, and it had a second door: ANY unanswerable rc + did the same thing, not just an old git's. `_shield_shared_config` now closes + that one by raising on every rc but 0 and 1 (round 10). + + THAT IS DEFENCE IN DEPTH BEHIND THIS GATE, NOT A REPLACEMENT FOR IT, and the + distinction is worth stating because the funnel makes the 2.18 pair fail closed + on its own and a later round could read that as licence to relax the floor. It is + not: this gate rests on two arguments the funnel does not touch — a permanent + format change bought on a git that cannot use it, and git-worktree(1)'s older-git + refusal — and it keeps the 2.18 pair UNREACHABLE rather than merely survivable. + The caller's own `--type=path` excludesFile read depends on the same floor. + So: the GATES run first and early, the WRITE runs last and late. + """ + # Polarity note, deliberately recorded: this gate reads `returncode != 0` as + # REFUSE, while the funnel below reads it as "git did not answer" and raises. + # Both fail closed, by opposite-looking means, and unifying them would reverse + # one of them — an unanswerable `git version` must refuse here, not degrade into + # a question about a key. Nor can this gate catch a bad repo config on git's + # behalf: `{ "version", cmd_version }` carries no flags at all in git.c, so it + # never does repository setup, and measured at 2.20.4 and 2.55.0 `git version` + # exits 0 under both a malformed `.git/config` and a non-bool `core.bare` while + # `rev-parse` fatals 128 on each. + version = git_bytes(worktree, "version") + if version.returncode != 0 or not _git_version_at_least( + os.fsdecode(version.stdout), _WORKTREE_CONFIG_GIT + ): + want = f"{_WORKTREE_CONFIG_GIT[0]}.{_WORKTREE_CONFIG_GIT[1]}" + found = os.fsdecode(version.stdout).strip() or f"git exited {version.returncode}" + return ( + f"skipped the git-add shield ({worktree}): needs git {want} for " + f"extensions.worktreeConfig and `git config --worktree`, but git answered " + f"{found!r} — enabling the extension on an older git is a permanent " + "repo-format change that would shield nothing" + ), False + # EVERY read below names the SHARED config as a file rather than going by + # scope. `--local` from a linked worktree already resolves to this same file, + # but naming it keeps the checks honest if that ever stops being true — and, + # load-bearing for the first of them, it stops a value the operator set + # GLOBALLY from answering a question about this repository's format. + # + # `extensions.*` is repo-format state git honors only from the repo's own + # config, so an unscoped `--get extensions.worktreeConfig` asks the wrong file: + # a `worktreeConfig = true` in ~/.gitconfig answers it, this returns "already + # enabled", the write below never happens, and the activation dies with + # `fatal: --worktree cannot be used with multiple working trees unless the + # config extension worktreeConfig is enabled` — the shield skipped over a repo + # that was one write away from supporting it. + # + # And NO `--includes` on any of the three, which is the asymmetry a later + # reader (or bot round) will want to "fix" — `--file` without it does miss a + # value an `include.path` supplies. Adding it would break both gates: + # + # - The two REFUSAL probes ask whether enabling the extension would break the + # operator's worktrees, and git's worktree SETUP reads `core.bare` / + # `core.worktree` only from the common config literally. It reaches them + # through `git_config_from_file` (setup.c's `check_repo_format`), and include + # expansion is not a parser feature but a callback wrapper installed in + # `config_with_options()` alone — so `git_config_from_file` does not respect + # includes, in git's own header's words (config.h). An `include.path` arrives + # there as an ordinary unmatched key. Measured with the extension enabled at + # git 2.20.4 / 2.32.7 / 2.43.7 / 2.49.1 / 2.55.0, identical at all five: a + # LITERAL `core.worktree` moves a linked worktree's toplevel to the main + # checkout and a literal `core.bare = true` fatals every command, while + # include-supplied values of either do nothing at all. `--includes` here + # would therefore refuse repos git is perfectly happy to shield — leaving the + # provisioned tool files unshielded, the harm this shield exists to prevent. + # - The ENABLED probe is worse: an include-supplied `extensions.worktreeConfig` + # is not honored either (`git config --worktree` still fatals at all five + # versions), because the `config.worktree` read is gated on the flag harvested + # by that same include-blind setup read. `--includes` there is a false + # "already enabled" → the write is skipped → the activation dies → nothing is + # shielded. That is commit c6d5960's bug re-entering through the include door. + # + # Two bounds on the claim, so a later reader knows what would invalidate it. + # It is an undocumented IMPLEMENTATION DETAIL, not a compatibility contract: + # git has deliberately converted reads to respect includes before (ecec57b3c973, + # git 2.39, "config: respect includes in protected config"), and a future + # `config_with_options()` conversion could flip it with no release note. And + # `core.bare`'s inertness is a CONJUNCTION, not an absence — an include-supplied + # `core.bare = true` does reach `repo->bare_cfg` on the normal + # include-respecting path; what keeps it harmless is `is_bare_repository()` + # requiring `bare_cfg && !repo_get_work_tree(repo)`, and every worktree this + # shield touches has a work tree. `core.worktree` needs no such caveat: it is + # read as a config key only in setup.c. Flag availability was never the + # question either way — `--includes` dates to git 1.7.10, far below the 2.20 + # floor the gate above enforces. + shared = str(common_dir / "config") + carried = _shield_shared_config(worktree, shared, "extensions.worktreeConfig", "--type=bool") + if carried is not None and os.fsdecode(carried).strip() == "true": + return None, False # already carried: nothing for the caller to write + bare = _shield_shared_config(worktree, shared, "core.bare", "--type=bool") + if bare is not None and os.fsdecode(bare).strip() == "true": + refused = "core.bare = true" + elif _shield_shared_config(worktree, shared, "core.worktree") is not None: + # `is not None`, NEVER truthiness: any value refuses, including an empty one. + # Measured at 2.20.4 and 2.55.0 — `core.worktree = ""` and a valueless + # `worktree` line BOTH answer rc 0 with a lone b"\n", so raw truthiness + # happens to hold today, but it is one `.strip()` away from b"", which is + # falsy, and that would re-open this gate for a key that IS set. The rc is the + # only thing separating present-but-empty from absent, and returning it as + # None-or-value is exactly what the helper exists to do. Never decoded, and + # the value never inspected: this key is a path, and its mere PRESENCE is the + # refusal — unlike `core.bare`, where only a true value disqualifies. + refused = "core.worktree" + else: + # Cleared, not enabled. The write itself is the caller's, deferred to the + # last moment before the activation (see the docstring). + return None, True + return ( + f"skipped the git-add shield ({worktree}): the repository's shared config sets " + f"{refused}, which git requires moving into the main worktree's config.worktree " + "before extensions.worktreeConfig may be enabled" + ), False + + +def _shield_undo_extension(worktree: Path, git_dir: Path, common_dir: Path) -> str: + """Undo the `extensions.worktreeConfig` THIS provisioning just enabled. + + Returns `""` when the repository is back in the state it was found in, or a + clause naming the fault when it is not — never raises, because every caller is + already reporting a degrade and this exists to say what it could not undo. + + Callers MUST gate this on having enabled the flag themselves — or, since round + 10, on not having been able to FIND OUT whether they did, which is what the + enable's own raise means. Unsetting one the repository already carried would stop + git reading the `config.worktree` files other worktrees may depend on — the + reason the shield's rollback is conditional rather than unconditional cleanup. + + That caller-side gate is necessary and NOT sufficient, which is round 8 (#384, + Codex P2). `needs_enable` records what the probe saw, and the enable itself is an + idempotent rc-0 no-op against a flag that is already `true` — so two concurrent + provisionings that both probed an absent flag both believe they own it, and + whichever one's activation fails then unsets a flag the other's LIVE shield + depends on: git stops reading its `config.worktree`, its `core.excludesFile` + goes inert, and its shielded tool paths become stageable again mid-run. The + caller serializes the whole transaction under a repository lock, which closes + that window for processes taking the lock. This scan is the fallback for the + flag having been enabled OUTSIDE that discipline — an older bmad-loop, a + concurrently running different version, an operator, or another tool entirely + (`git sparse-checkout` enables this same extension) — so it is not redundant + with the lock and must not be deleted as such. A lock binds only its holders. + + EXCLUDING OUR OWN `git_dir` IS LOAD-BEARING, not tidiness: a `config.worktree` + in it is the half-written product of the very activation whose failure prompted + this rollback (a timeout can leave one behind), and treating that as a dependent + would suppress every rollback the round-6/7 fixes exist to make. + + WHAT THIS DOES NOT COVER, stated rather than left for another round to find (the + round-6 lesson: a rationale of "now nothing can go wrong" has to enumerate its own + remaining cases). The scan reads an EXISTING file, so a non-lock-taking party + sitting between its own enable and its own activation owns the flag while having + written nothing yet, and this rollback will unset it. The consequence is bounded + and different in kind from the finding above: that party's `git config --worktree` + then fatals for want of the extension, so its shield degrades with a REPORTED + reason instead of silently going inert mid-run. That bound depends on a + precondition this scenario supplies for free, and it is worth naming because the + other branch is #384 itself: `git config --worktree` only REFUSES (rc 128) once a + repository has more than one worktree, and in a single-worktree repo it exits 0 + and writes the SHARED config instead. Here a sibling exists by construction — it + is the other party — so the refusal is the one git gives. The reverse asymmetry is also + accepted — a stale admin dir git has not pruned yet counts as a dependent, which + costs a flag left enabled and nothing else. + + Round 10 widened that window's APERTURE without changing its bound: this rollback + now also runs from the enable's own raise, one call earlier, so there are two + occasions on which a non-lock-taking party can be caught mid-transaction rather + than one. Same class and same bounded consequence, twice as often. + + Measured, because "the key is gone" and "the extension is off" are two claims: + the unset exits 0, removes the key AND the now-empty `[extensions]` section (the + config file returns to its original bytes), and `git config --worktree` refuses + again afterwards with git's own fatal — so this is a real rollback, not cosmetic. + An ABSENT key exits 5, which used to be unreachable ("the gate above means we + never do") and stopped being so in round 10: the enable's raise routes here + without knowing whether anything was written. See the call below for why that + makes rc 5 a success and why the spelling is `--unset-all`. + """ + try: + # The main worktree's own, then every SIBLING's — never ours (above). + others = [common_dir / "config.worktree"] + others += [ + d / "config.worktree" + for d in (common_dir / "worktrees").iterdir() + if d.resolve() != git_dir + ] + dependent = next((str(p) for p in others if p.exists()), None) + except (OSError, RuntimeError, UnicodeError) as e: + # Conservative on purpose, and the asymmetry is the argument: skipping + # leaves a reported flag this call did not earn, while unsetting one that + # IS depended on silently un-shields a live worktree. A cosmetic residue + # beats silent file loss, so an unreadable scan counts as a dependent. + # + # The tuple mirrors the caller's tail minus `GitError` (nothing here spawns + # git), and the two beyond `OSError` are what keeps this function's "never + # raises" promise true rather than merely stated — round 9 (#384, + # CodeRabbit), and measured, not reasoned: with `resolve()` made to fail the + # way a symlink loop does, the bare `OSError` tuple let it escape. On the + # Python 3.11/3.12 floor `Path.resolve()` raises `RuntimeError` for a + # symlink loop rather than `OSError` (the caller's own tail already carries + # it for exactly that), and `UnicodeError` covers the fsdecode of a + # directory name on Windows. Escaping here is not a crash — the caller's + # tail catches all three — but it replaces the activation fault AND the + # retained-flag disclosure with a generic reason, losing precisely what + # rounds 6 and 7 added. + dependent = f"the scan for sibling worktrees failed ({e})" + if dependent is not None: + return ( + "; extensions.worktreeConfig was deliberately LEFT enabled — " + f"{dependent} exists, so another worktree's shield depends " + "on the flag and unsetting it would stop git reading that file" + ) + try: + # `--unset-all`, and rc 5 counts as SUCCESS. Both halves are round 10's, and + # neither is cosmetic. Routing an uncertain ENABLE into this rollback makes an + # absent flag reachable here for the first time, and `--unset` of an absent + # key exits 5 — which rendered as "could NOT be rolled back (git exited 5)" + # for a repository whose flag is in fact gone: a false disclosure. + # + # `--unset` alone could not simply treat 5 as success, because git-config(1) + # gives that code TWO meanings — "unset an option which does not exist" and + # "unset/set an option for which multiple lines match" — so a doubled key that + # SURVIVED would report a clean rollback. Measured at 2.20.4 and 2.55.0, + # identical: `--unset` against a doubled key exits 5 and removes NOTHING, + # while `--unset-all` exits 0 and removes both lines. `--unset-all` therefore + # collapses rc 5 to the single meaning "no line matched", which makes the + # guarantee structural instead of resting on a four-step argument about which + # doubled values can reach here. It is also what this function actually + # promises: a statement about the KEY, not about one line. + undone = git_bytes(worktree, "config", "--unset-all", "extensions.worktreeConfig") + if undone.returncode in (0, 5): + return "" + detail = os.fsdecode(undone.stderr).strip() or f"git exited {undone.returncode}" + except GitError as e: + # the rollback's OWN git can time out or fail to spawn. Both writes failing + # is the coherent case rather than a contrived one: a read-only `.git` or a + # dead git fails the unset for the same reason it failed the activation. + detail = str(e) + # Both clauses HEDGE whether this shield set the flag, which round 10 made + # necessary: reached from the enable's own raise, a spawn failure can kill the + # enable and this unset for the same reason, having written nothing at all — and + # the old wording ("was enabled for this shield and could NOT be rolled back") + # then asserted a format change the repository does not carry. No real precision + # is lost: `needs_enable` is probe-derived and already cannot tell "we enabled it" + # from "we and a concurrent run both thought we did". + return ( + "; extensions.worktreeConfig could NOT be " + f"rolled back ({detail}) — if this shield set the flag, the repository keeps " + "a permanent format change that shields nothing" + ) + + +def _shield_home_git_ignore(worktree: Path) -> Path: + """`$HOME/.config/git/ignore` — git's XDG fallback — asked of GIT, not of Python. + + `Path.home()` is the obvious spelling here and it is WRONG on Windows, the one + platform where git's home and Python's home are derived differently. Git locates + this fallback with `getenv("HOME")` on every platform (`path.c::xdg_config_home`, + semantics unchanged 2.20.0 → 2.55) — but on Windows + `compat/mingw.c::setup_windows_environment()` DERIVES `HOME` inside the git + process before that runs, preferring `HOMEDRIVE`+`HOMEPATH` (and only when that + names a real directory) over `USERPROFILE`. Python's `ntpath.expanduser` has the + OPPOSITE precedence — `USERPROFILE` first — and never consults `HOME` at all. Two + ordinary setups therefore disagree: + + * anything that sets `HOME` (Git Bash and MSYS2 do), where git obeys it and + Python cannot see it; and + * a domain-joined machine whose `HOMEDRIVE`+`HOMEPATH` is a network home share, + where git reads `H:\\…\\.config\\git\\ignore` and Python reads `C:\\Users\\…`. + + The miss is SILENT, and it is #384's own harm reached through the platform split: + the wrong path is simply not a file, the seed comes back empty, and the caller's + activation then SHADOWS the global ignore file git really reads — un-ignoring + inside the worktree exactly what this branch exists to preserve. + + So ask git. `--type=path` interpolates a leading `~/` through the same + `getenv("HOME")` git uses for the fallback itself, which makes the answer correct + by construction on every platform and names no environment variable — round 18's + lesson (ask what git RESOLVED, not how the environment might have told it) at the + sibling site. `-c` is COMMAND scope, the most specific git has, so the probe's + value cannot be outranked by the operator's config: the very precedence rule that + caused round 18 is what makes this reliable. + + MEASURED at git 2.20.4 (docker `alpine:3.9`, non-root, `id -u` checked) and + 2.55.0, byte-identical: the probe returns `$HOME/.config/git/ignore` NUL- + terminated at rc 0, and that is the file git really loads patterns from — proven + through `git status` hiding a name the file lists, not inferred from the path. + With `HOME` unset it exits 128 and git applies no fallback at all. + + KNOWN GAP, recorded so this is not read as complete: this answers "where is + git's `$HOME`", which is the whole of the fallback UPSTREAM but not on **Git for + Windows >= 2.46**, whose fork patches `xdg_config_home_for` to prefer + `%APPDATA%/Git/` whenever that file EXISTS — it even warns that the `$HOME` + one "was ignored because" the APPDATA one is there. Verified by reading the + fork's `path.c` and bounded by version: 1 occurrence of `APPDATA` at + `v2.46.0.windows.1`, 0 at `v2.45.0.windows.1`, `v2.20.0.windows.1`, and 0 + upstream at master. So an operator on a current Git for Windows who keeps their + global ignores at `%APPDATA%\\Git\\ignore` still gets the wrong file seeded here. + Deliberately NOT fixed in this pass: it is a downstream fork's behavior, gated on + a version well above this shield's floor, and there is no way to MEASURE it from + a POSIX box — which is this branch's standing bar for a git claim. + + Raises `GitError` when git did not answer, INCLUDING that `HOME`-unset rc. + Proceeding on a non-zero rc would be a guess, and a guess here is silent: the + caller seeds nothing and activates over whatever git does read. git's message is + deliberately NOT matched to tell "no HOME" apart from a transient fault, because + that wording is version-dependent (it differs between 2.20.4 and 2.55.0 for other + config faults on this same branch). The cost is that a genuinely `HOME`-less + environment now skips the shield with a journaled reason instead of proceeding + with an empty seed — and that direction is REPORTED, which is the whole taxonomy + of the caller: only a definite absent may be silent. + """ + key = "bmadloop.xdghomeprobe" + probe = git_bytes( + worktree, "-c", f"{key}=~/.config/git/ignore", "config", "-z", "--type=path", "--get", key + ) + if probe.returncode != 0: + detail = os.fsdecode(probe.stderr).strip() or f"git exited {probe.returncode}" + raise GitError(f"git could not resolve its own home directory: {detail}") + return Path(os.fsdecode(probe.stdout.split(b"\0", 1)[0])) + + +def _shield_inherited_excludes(worktree: Path) -> bytes: + """The BYTES of whatever `core.excludesFile` this worktree resolves to now. + + A worktree-scoped `core.excludesFile` SHADOWS the operator's own: git reads + the key from the most specific scope that sets it and does not concatenate + across scopes (verified — with a worktree value set, a path listed in the + global excludes file shows up as untracked inside the worktree and stays + ignored in the main checkout). Activating the shield without carrying their + patterns over would therefore un-ignore, inside the worktree, everything they + ignore globally — and the unit's `git add -A` would commit it. + + Seeded once, at creation, so the copy never fights later edits to the private + file. + + BYTES, never decoded text, and the return type is the fix: an exclude file is + a list of path patterns, POSIX paths are arbitrary bytes, and a `read_text` + here made one non-UTF-8 byte anywhere in the operator's file collapse the whole + seed to empty — after which the activation SHADOWED the excludes it had failed + to copy, and `git add -A` staged what they told git to ignore (measured + end-to-end). Copying verbatim also keeps every pattern intact for the caller's + dedupe, which a text round trip could not promise. Git reads the result either + way: it strips a trailing `\\r` from an exclude line and skips a UTF-8 BOM. + + FOUR outcomes, and which answer means what is the whole subtlety. The first + two are silent; this function RAISES for the other two, and the caller's tail + turns that into the degrade reason that skips the activation. Only ABSENT has a + fallback, and routing DISABLED into it was round 8's bug. + + - ABSENT is silent and empty, and the only outcome that falls back. The common + case — no `core.excludesFile`, no XDG fallback file — and not a reason to skip + the shield. It is an ANSWER: `--get` of an unset key exits 1, and `is_file()` + false says the file is not there. One caveat added with the `$HOME` probe: + REACHING the fallback path can itself fail, and `_shield_home_git_ignore` + raises when it does. That is not this outcome — an unresolvable home is + UNKNOWN, not absent, and gets the loud arm like every other unknown here. + - DISABLED is silent and empty too, and it is NOT absent (#384, Codex round 8). + An explicitly EMPTY `core.excludesFile` is the operator saying "no excludes + file at all", and git honors that literally rather than treating it as unset: + measured at 2.55.0, `-z --get` answers rc 0 with a lone NUL, after which git + loads no patterns and does NOT reach for XDG (`git check-ignore` exits 1 + against a name the XDG file lists). The mechanism is in git's source, not + inferred: `dir.c::setup_standard_excludes` guards the XDG fallback on + `if (!excludes_file)` — a NULL POINTER — while `config.c` resolves an empty + value through `interpolate_path("")`, which returns a non-NULL empty string. + Byte-identical guard at v2.20.0, this shield's own floor, and at master. + Reading that as unset (the pre-round-8 `and raw`) seeded the XDG file's + patterns in and activated them, which is the MIRROR IMAGE of every other + fault here: instead of shadowing patterns it failed to copy it OVER-ignores, + so session-created files the operator deliberately stopped ignoring go + silently missing from the unit's `git add -A` and from the story's commit. + Same silent-file-loss class as #384 itself, in the other direction. + - PRESENT BUT UNREADABLE propagates. A read `OSError` (EACCES, EIO) reaches the + caller's degrade arm, which skips the activation — the only safe answer, since + activating over patterns that could not be copied shadows them exactly as an + empty seed did. + - UNKNOWN propagates too, and it arrives in TWO shapes that this function + deliberately funnels rather than enumerates — a raised `GitError`, and any + returncode that is neither 0 nor 1. + + The raise is round 5's fix (#384, Codex P1). A `GitError` out of the read below + used to be swallowed here on the premise that "git unqueryable ⇒ no key to + resolve ⇒ nothing to copy". The premise is a false inference: a raised fault + means we did not FIND OUT — so the code mapped UNKNOWN onto ABSENT and then + activated a key that shadowed the file it had failed to read. Measured, + and pinned by the ablation on + `test_shield_degrades_when_git_will_not_say_what_the_users_excludes_are`: + inject a fault on this one call and a file the operator ignores globally comes + back `?? ` inside the worktree, ready for the unit's `git add -A`, with + no reason returned at all. Nor is a permanently + unqueryable git reachable here — by the time this runs `git_bytes` has + answered at least four times, so such a git already left through the caller's + silent `rev-parse` arm. What arrives is a TRANSIENT fault, and for those the + premise is false by construction: the key is there, we just cannot see it. + The accepted cost is that such a fault now loses the shield for that run, + which the caller surfaces; the harm it buys off is unbounded and silent. + + The rc shape is round 9's (#384, CodeRabbit). Round 5's bullet used to justify + itself by saying the swallow "holds for `rc != 0`, which is an answer" — and + that generalization was too broad, which is the correction worth recording + here rather than quietly dropping. Only **rc 1** means "no such key"; every + other non-zero rc is git reporting that it could not answer, and routed to + ABSENT it seeded and activated the XDG file over patterns it had not read. + What can actually occupy that branch is narrower than it looks, and the + comment at the call below carries the measurement: not a static config value + — those fatal the `rev-parse` in the caller first, which skips the shield + silently — but a fault arriving inside the window the caller holds its lock. + """ + # -z, not a bare read plus `.strip()` — round 5, Codex P2. A `.strip()` here + # DESTROYED a legal POSIX path: `--type=path --get` returns leading and + # trailing whitespace VERBATIM, because git writes such a value quoted and it + # round-trips. So an operator whose excludesFile ended in a space had the strip + # mangle the path, `is_file()` read absent, the seed come back empty, and the + # activation shadow the file it never copied — the same leak as the encoding bug + # above, through a different door. `-z` terminates the value with NUL instead, + # which git-config(1) recommends for exactly this ("allows for secure parsing of + # the output without getting confused by values that contain line breaks"). + # + # Everything below is measured at git 2.20.4 AND 2.55.0 — both ends of the + # supported range, the flag checked AT the floor rather than inferred from + # git-config(1), since the 2.20 refusal in `_shield_enable_worktree_config` has + # already run by the time this does and 2.20 is therefore the oldest git that + # can reach this line. `-z` still expands `~`, gives an unset key rc 1 + empty + # stdout, an empty value a lone NUL at rc 0, a multi-valued key the LAST value + + # NUL at rc 0, and a relative value verbatim. + # + # That empty-value branch is the one round 5 got WRONG, and how is worth + # recording: this comment used to say the lone NUL "falls to XDG the way it + # did" — a measurement of OUR READER's classification and of behavior + # preservation, which never asked what GIT does with the same value. Preserving + # a behavior is not the same as that behavior being correct. Git treats it as + # "no excludes file", so the value is an ANSWER and gets its own branch below; + # see the docstring's DISABLED outcome for git's own source. + # + # The sibling `rev-parse` strips below are deliberately NOT getting the same + # treatment, and this is the note that stops a later round re-raising it: + # `rev-parse` has no `-z` (it echoes one back as a literal argument), and it + # cannot answer with edge whitespace anyway — git SANITIZES the worktree admin + # id, so a worktree at `…/wt ` gets `.git/worktrees/wt-` (measured), and + # `--git-common-dir` ends in `.git`. + answer = git_bytes(worktree, "config", "-z", "--type=path", "--get", "core.excludesFile") + raw = answer.stdout.split(b"\0", 1)[0] + # rc FIRST, and the value only once rc says there IS one — the condition used to + # be `returncode == 0 and raw`, which collapsed DISABLED onto ABSENT. Round 8's + # fix is the branch below, not this reordering; the reordering is what makes the + # two answers nameable apart. + if answer.returncode == 0: + if not raw: + # DISABLED: an explicit "no excludes file", so there is nothing to copy + # and NO fallback to reach for — see the docstring. Note for a later + # reader (or ablation): deleting this arm does not restore the bug, it + # only hides it, because `Path(os.fsdecode(b""))` is `.`, which resolves + # to the worktree directory and reads `is_file()` false. The bug is the + # ROUTING of this answer into the XDG branch, so the only faithful + # ablation is restoring `and raw` on the condition above. + return b"" + source = Path(os.fsdecode(raw)) + if not source.is_absolute(): + # `--type=path` expands `~` and stops there: a RELATIVE value comes + # back verbatim. Git resolves such a value against the worktree's + # top level (MEASURED — git 2.20.4, 2.49.1, 2.55.0 — and NOT + # specified: relative values for this key were deliberately never + # implemented, and the worktree-top result is an artifact of git's + # setup chdir, so a consumer reached through RUN_SETUP alone with an + # explicit git dir and an outside cwd resolves it against the + # CALLER's cwd instead. bmad-loop is not exposed to that: every git + # call here goes through `_run_git`'s `git -C `, and the + # activation below writes an absolute path.) Python would resolve it + # against this process's cwd, which is wherever the orchestrator was + # launched. Left alone that reads the wrong file or, far more often, + # none — and `is_file()` false is silent, so the operator's patterns + # would simply not be carried and the activation below would shadow + # them anyway. Un-ignoring inside the worktree exactly what this + # function exists to preserve. + source = worktree / source + elif answer.returncode != 1: + # UNKNOWN, through git's OTHER failure shape. rc 1 is the ONLY non-zero rc + # that means "no such key"; every other one is git saying it could not answer, + # and routing those to the fallback below is the same UNKNOWN-as-ABSENT + # mistake round 5 fixed for the RAISED shape (#384, CodeRabbit round 9). + # + # WHAT CAN REACH HERE is narrower than the finding's own example, and the + # measurement is worth keeping because the plausible version of it is wrong. + # `core.excludesFile = ~nosuchuser/ignore` — an ordinary thing to inherit from + # a `.gitconfig` synced between machines — does make THIS read exit 128 + # (`fatal: failed to expand user dir`). But git expands that same value while + # parsing core config for any command that sets the repository up, so + # `rev-parse --absolute-git-dir` in the caller fatals identically and the + # shield SILENTLY SKIPS there, above this line. Measured at git 2.20.4 and + # 2.55.0, both ends of the supported range. No static config value reaches + # this branch: its occupants are faults that appear BETWEEN that rev-parse + # and this read. + # + # That window is structural rather than theoretical, and the caller sets its + # width: the rev-parse runs before the repo-scoped lock, this read runs after + # it, and POSIX `flock` blocks INDEFINITELY — so a run waiting on a sibling + # worktree's shield sits between the two calls for that sibling's whole git + # transaction. A dotfile manager writing `~otheruser/ignore` in, or an + # `~alice` whose NSS/LDAP lookup fails for a moment, lands here. + # + # "The repo is broken anyway, so the harm is bounded" does not survive that: + # the fault can CLEAR (the sync finishes, LDAP answers) while an activation + # performed over it lasts as long as the worktree — a `core.excludesFile` + # shadowing the operator's real excludes with the XDG file's patterns. #384's + # own harm, reached through a transient. + # + # Deliberately shaped as a FUNNEL rather than a per-rc taxonomy, which is + # round 7's lesson: git-config(1) allocates several failure codes and + # enumerating them one per review round is how the activation arm took three. + # Anything that is not "answered" or "no such key" is not an answer. + detail = os.fsdecode(answer.stderr).strip() or f"git exited {answer.returncode}" + raise GitError(f"git could not resolve core.excludesFile: {detail}") + else: + # ABSENT (rc 1, an unset key): git's documented fallback (gitignore(5)), and + # the ONLY outcome that gets one. Not reachable for an empty value any more. + # + # `XDG_CONFIG_HOME` this process may read for itself: git reads the same + # variable from the same environment (`_run_git` passes `os.environ` + # through), and set-but-empty counts as unset for BOTH — git's guard is + # `if (config_home && *config_home)`, the falsy check below is the same + # test. The `$HOME` limb is the one Python cannot answer; see + # `_shield_home_git_ignore` for why it is asked of git instead. + xdg = os.environ.get("XDG_CONFIG_HOME") + source = Path(xdg) / "git" / "ignore" if xdg else _shield_home_git_ignore(worktree) + if not source.is_absolute(): + # THE SAME DEFECT AS THE RELATIVE `core.excludesFile` ABOVE, at the + # sibling site (#384, Codex round 12) — this branch reproduces git's + # fallback, so it has to reproduce git's RESOLUTION too, and it did not. + # + # A relative XDG_CONFIG_HOME is invalid per the XDG base-directory spec + # ("All paths ... must be absolute. If an implementation encounters a + # relative path ... it should consider the path invalid and ignore it"), + # but git does not ignore it: measured at 2.20.4 and 2.55.0, git HONORS + # the value and resolves it against the worktree's TOP LEVEL. Measured + # from a SUBDIR to tell top-level from cwd, the same way the branch above + # was — `git -C /sub` with `XDG_CONFIG_HOME=rel` read + # `/rel/git/ignore`, not `/sub/rel/git/ignore`. + # + # Python resolves it against the orchestrator's launch cwd instead, and + # the miss is SILENT: `is_file()` false, an empty seed, and then the + # activation shadows the file git really reads. Un-ignoring inside the + # worktree exactly what this function exists to preserve — #384's harm + # reached through a misconfigured environment rather than a fault. + source = worktree / source + # `is_file()` is the ABSENT test and swallows its own OSError; `read_bytes` + # on a file that exists but cannot be read raises, deliberately (docstring). + return source.read_bytes() if source.is_file() else b"" + + +def _shield_verify_activation(worktree: Path, exclude: Path) -> str | None: + """Why the just-written `core.excludesFile` is NOT the one git reads, or None. + + A successful `git config --worktree core.excludesFile` proves the value was + WRITTEN, not that it is in FORCE. git resolves the key from the most specific + scope that carries it, and `worktree` is not the most specific one — `command` + outranks it, and that scope is fed by the ENVIRONMENT the orchestrator was + launched with. So an operator carrying an ambient `core.excludesFile` gets a + shield that reports success, writes the private file, and never has it read: + the provisioned tool files stay stageable by the unit's `git add -A`, with no + degrade reason to journal. Round 17 was "a pattern PRESENT in the file is not + EFFECTIVE"; this is the same gap one level up — WRITTEN is not EFFECTIVE. + + So the post-condition is asked of git rather than assumed, and the shape is the + point. The obvious fix is to detect the ORIGIN — look for `GIT_CONFIG_COUNT` + and friends in `os.environ` — and it is the enumeration this function exists to + avoid, because the channels are neither one nor stable. Measured end to end, on + a real linked worktree with the shield fully installed, at BOTH ends of the + supported range: + + 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 mutually incompatible encodings, and the one the finding + that prompted this named does not exist at this shield's own floor. A `git -c` + on a session's own command line is a fourth and is not visible in our + environment at all. Asking git what it RESOLVED costs one call and covers every + one of them, including whatever git adds next. + + `--show-scope` would name the winning scope and make the reason better, and is + deliberately not used: it is git 2.26, above the 2.20 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, one round-1 finding away from this one). + + The read shape is the seed read's, already measured at 2.20.4 and 2.55.0: `-z` + because a legal POSIX path may carry edge whitespace, `--type=path` because + that is how git itself resolves the key. Any non-zero rc is a fault here rather + than an ABSENT answer — this call asks about a key we have just written, so + "there is no such key" is not good news about it. That is a DIFFERENT taxonomy + from `_shield_inherited_excludes`, which reads a key the operator may simply + not have set; the two must not be unified. + + The comparison is byte-exact and stays that way. Round-tripping a path through + `git config` and back was measured for every hazard this shield has already + been burned by — a space, leading and trailing whitespace, an embedded newline, + a non-UTF-8 byte, an interior `~` — all seven exact at both versions, so an + inexact comparison would be buying nothing and could only mask a real + mismatch. Windows CI is the oracle for the one platform not measured here: git + escapes a backslash path it is handed as an ARGUMENT and reads it back intact, + which is why the shield's other tests pass there, but that claim is about the + config file rather than about `--type=path`. + """ + resolved = git_bytes(worktree, "config", "-z", "--type=path", "--get", "core.excludesFile") + if resolved.returncode != 0: + detail = os.fsdecode(resolved.stderr).strip() or f"git exited {resolved.returncode}" + return f"git would not confirm which excludes file now applies: {detail}" + effective = resolved.stdout.split(b"\0", 1)[0] + if effective == os.fsencode(str(exclude)): + return None + return ( + "the write succeeded but another configuration scope outranks it, so git " + f"reads {os.fsdecode(effective)!r} instead and the shield's patterns never apply" + ) + + +def _worktree_local_exclude(worktree: Path, patterns: Sequence[str]) -> str | None: + """Shield the just-provisioned tool files from the unit's `git add -A`, in a + private exclude file scoped to THIS worktree alone. + + Two properties, both load-bearing, and both of them were wrong before #384: + + - SCOPE — the patterns land in `/info/exclude` + (`.git/worktrees//info/exclude`), activated by + `git config --worktree core.excludesFile `. Git only ever reads + `$GIT_COMMON_DIR/info/exclude`, so the private file does nothing on its own; + the worktree-scoped config key is what makes it apply, and it applies to + this worktree only. The main checkout and every sibling worktree are + untouched. Writing that key is not the same as it being in FORCE — `command` + scope outranks `worktree`, and it is fed by the environment the orchestrator + was launched with — so the activation is VERIFIED against what git actually + resolves rather than trusted; see `_shield_verify_activation`. + - LIFETIME — `git worktree remove`/`prune` deletes the whole per-worktree + gitdir, taking the private exclude AND the `config.worktree` that points at + it. The shield expires exactly when the thing it shields does; there is no + remover to keep in step with it. + + The repository-wide `.git/info/exclude` is NEVER written again, on any path. + That file is what #384 reported: shared by every worktree, permanent, and + unversioned, so patterns naming directories a project legitimately tracks + (`.claude/skills`, `.claude/settings.json`) went on hiding every NEW file + under them from `git add -A` in the operator's own checkout, long after the + run that wrote them. The old docstring's defense — that an exclude "does not + affect already-tracked files" — is precisely what made it invisible: the + tracked files kept diffing normally while their new siblings vanished. So a + refusal below SKIPS the shield and reports; falling back to the shared file + would reintroduce the bug this function exists to fix. + + The price is a permanent one: `extensions.worktreeConfig` is repo-wide format + state, enabled once and never removed here, and git versions older than 2.20 + refuse to access a repository carrying it (git-worktree(1)). See + `_shield_enable_worktree_config` for the two shapes where enabling it is + refused outright. It is paid at the LAST possible moment, so no degrade above it + can charge it: that function probes, and the write itself happens here, one line + above the activation. Enabling it up front — where it used to be — charged the operator's + repo that permanent price on every path that then degraded away without + shielding anything. Moving it down leaves exactly two writes able to set it + without shielding anything — the enable itself and the activation — and each + classifies both of the ways a chokepoint call can fail (a non-zero rc and a + raise). The ACTIVATION rolls the flag back on either shape, and on a third + outcome that is not a failure at all — a write that succeeds without taking + effect, which a later round showed is reachable whenever the environment + supplies the same key at `command` scope. The ENABLE rolls it + back on the raise ONLY, because an rc from that write is git declining to make + it: nothing was written, so there is nothing to undo, and undoing it anyway + would unset a concurrent writer's flag. That asymmetry is load-bearing, not an + unfinished enumeration. Getting here took five review rounds, because the early + attempts enumerated failure shapes instead of funnelling them, the funnel was + then applied to the activation but not to the enable one line above it, and even + a complete funnel over the ways a CALL can fail said nothing about whether the + call achieved what it was for. Rollback + only when this call is what set the flag, and only when no other worktree's + `config.worktree` depends on it; see `_shield_undo_extension`. + + CONCURRENCY — the probe→enable→activate→rollback sequence runs under an exclusive + `file_lock` on `/bmad-loop-shield.lock`, because that flag is the one + piece of state two simultaneous runs in one repository share (round 8). Without + it both probe an absent flag, both believe they own it, and a failed activation + in one rolls the flag back out from under the other's live shield. The lock is + per REPOSITORY, not per worktree, and it leaves a residue: a zero-length file in + `.git/` — never in the working tree, so nothing the operator's `git add -A` can + see, and nothing that needs a stale-lock scheme, since the kernel drops the lock + when a holder dies. Its wait is platform-asymmetric by inheritance: POSIX `flock` + blocks indefinitely (the transaction is ~7 git spawns, each bounded by + `limits.git_timeout_s`), while `msvcrt.locking` gives up after ~10 s and raises + `OSError`, which this function reports as a skipped shield naming the lock. + + Every git call goes through `verify.git_bytes`, the chokepoint accessor whose + two properties this function is built on: the returncode comes back as an + ANSWER rather than a raised fault (`config --get` of an unset key exits 1, and + that reply is what tells the shield to enable the extension or fall through to + the XDG default), and stdout arrives as BYTES, since POSIX filenames are bytes + and a strict decode would raise `UnicodeDecodeError` out of the silent arm + below, which promises never to propagate (#374). Standing inside it also buys + the `LC_ALL=C` pin and the `limits.git_timeout_s` bound the engine configures. + + Best-effort in two arms that must stay separate, because a fault means the opposite thing in each: - - git unqueryable (not a repo, git missing) is an EXPECTED skip — returns - None silently. Callers hand this plain temp dirs routinely. Nothing is - DECODED in this arm: git's stdout is captured as bytes and decoded in the - tail below, because a decode fault is a degrade (git answered; we could - not read it), not the silent skip this arm means (#374). - - a filesystem fault AFTER git answered degrades to a returned reason string - the caller can surface: `.resolve()` (pre-3.13 a symlink loop raises - RuntimeError, not OSError), `mkdir`, read/write OSError, and either - direction of a codec fault (`UnicodeError`) — an exclude file that is not - UTF-8 fails the READ, and a pattern carrying a surrogate fails the WRITE, - since a seed_globs pattern is built from a real filename and a non-UTF-8 - one arrives surrogate-escaped. Silence here is not cosmetic: without the - exclude the unit's `git add -A` commits the tool files this provisioning - just wrote into the story's merge. + - git unqueryable BEFORE GIT HAS ANSWERED AT ALL (not a repo, git missing, a + timeout or spawn failure on the first `rev-parse` — both `GitError`) is an + EXPECTED skip — returns None silently. That scope is the whole of it, and the + qualifier is load-bearing: read loosely as "any `GitError` is silent" it + licensed the swallow round 5 deleted from `_shield_inherited_excludes`, which + is BELOW this arm and therefore in the one below. Callers hand this + plain temp dirs routinely. Nothing is DECODED in this arm: git's stdout is + captured as bytes and decoded in the tail below, because a decode fault is a + degrade (git answered; we could not read it), not the silent skip this arm + means (#374). A git too old for a flag is NOT here: rev-parse echoes an + option it does not know and exits 0, so that lands in the arm below, via + the version refusal in `_shield_enable_worktree_config`. + - anything AFTER git answered degrades to a returned reason string the caller + can surface: the `--git-common-dir` probe failing once `--absolute-git-dir` + has already answered (round 11 — it sat in the silent arm above from round 3, + when one combined `rev-parse` became two calls, until this list was read as + the specification it is), a main checkout passed in (nothing to scope to), a + repository configured to be shared between OS users (refused above the lock — + see `_shield_shared_repository`), a + shield lock that could not be taken (contention on Windows, or an `os.open` + fault in the common dir), a refused or + failed extension enable, a failed activation, `.resolve()` (pre-3.13 a + symlink loop raises RuntimeError, not OSError), `mkdir`, read/write + `OSError` — including the operator's own excludes file existing but being + unreadable, which must NOT be silent, because activating over patterns that + could not be copied shadows them — a later git call timing out or failing to + spawn (`GitError`), and a `UnicodeError` (below). Since round 5 that + `GitError` case explicitly covers a git that will not say WHICH excludes file + applies: not knowing whether there is anything to copy is the same standing as + knowing there is and failing to read it, because the activation shadows it + either way. Only a definite ABSENT answer stays silent. Silence here is not + cosmetic: without the exclude the unit's `git add -A` commits the tool files + this provisioning just wrote into the story's merge. + + The exclude PAYLOAD is bytes end-to-end — read, dedupe and write — so nothing + on this path decodes or encodes an exclude file at all. That leaves two real + sources for the `UnicodeError` in the tail's tuple, neither of them a + file-content codec fault: the `os.fsdecode` of git's own stdout, which can raise + on Windows only (#374/#377), and `os.fsencode` of a caller's pattern when that + pattern carries a surrogate POSIX surrogateescape never produced — a + hand-authored `"\\ud800"` in a config string rather than a real filename, which + fsencode's surrogateescape rejects. The seed_globs case the tuple was originally + written for is GONE: a pattern built from a genuinely non-UTF-8 filename arrives + surrogate-escaped and `os.fsencode` returns its exact original bytes (POSIX); + Windows' utf-8/surrogatepass encodes such a name to WTF-8 without raising. """ # Callers pass POSIX-slash patterns (glob rels via as_posix; config strings as # authored); git's exclude is POSIX-slash on every platform, so nothing to fix here. try: - # bytes, not text=True: strict decoding here would raise UnicodeDecodeError - # out of an arm whose whole contract is "skip silently", and that type is - # in neither arm's tuple (#374). POSIX filenames are bytes, so a repo path - # with bytes invalid in the locale encoding reaches this on a normal box. - # - # Deliberately NOT routed through verify._run_git, the chokepoint AGENTS.md - # otherwise requires. Nothing structural forbids it — neither module imports - # the other — but _run_git hands back CompletedProcess[str], i.e. the strict - # decode this call exists to avoid, and it raises GitError rather than the - # (SubprocessError, OSError) this arm treats as an expected skip. Adopting it - # needs a bytes mode on the chokepoint first; that is #377, which also covers - # the decode fault escaping _run_git's own taxonomy. The cost of standing - # outside it meanwhile is this call's missing timeout. - raw = subprocess.run( - ["git", "-C", str(worktree), "rev-parse", "--git-common-dir"], - capture_output=True, - check=True, - ).stdout - except (subprocess.SubprocessError, OSError): + # ONE path per call, never both in one answer. `rev-parse` separates its + # answers with a newline, which is a legal BYTE IN A POSIX PATH — asking + # for both at once made the split of a repo directory carrying one produce + # four "paths" instead of two, and the length check below then degraded the + # shield away *after* provisioning had copied the very files it exists to + # hide (verified, git 2.55: the newlines come back raw, unquoted). No + # NUL-delimited mode to reach for instead — `rev-parse` has no `-z`; it + # echoes one back as a literal argument. A path that ENDS in a newline is + # still beyond this parse, and beyond rev-parse's output format itself. + answered = git_bytes(worktree, "rev-parse", "--absolute-git-dir") + if answered.returncode != 0: + # Not a repo (rc 128) — the expected skip the previous `check=True` raised + # its way into. NOT "a git too old for `--absolute-git-dir`", which this + # comment used to also claim: rev-parse ECHOES an option it does not know + # and exits 0 (measured), so a git predating the flag (2.13) answers with + # the flag's own name. That lands in the git 2.20 refusal in + # `_shield_enable_worktree_config`, which returns above any write — a + # degrade, not this silent skip. + return None + except GitError: + # GitError alone, and it is not a narrowing: this `try` wraps ONE git call + # and nothing else that can raise, and every fault the chokepoint raises out + # of it is a GitError — a timeout directly, a spawn failure as + # GitSpawnError, which subclasses it. The `OSError` that used to belong here + # was the spawn fault's raw form and is now translated before it arrives. return None try: + # The COMMON-DIR probe lives in THIS try, not the silent one above, and that + # placement IS the fix (round 11). Once `--absolute-git-dir` has answered, + # git has identified the repository, so every later fault is on the degrade + # side of the two-arm taxonomy — which the silent arm's own docstring already + # said ("a timeout or spawn failure on the FIRST rev-parse ... that scope is + # the whole of it, and the qualifier is load-bearing"), and which + # `worktree_flow`'s contract says too ("any fault after that ... is reported + # to on_degraded rather than swallowed"). Round 3 split one combined + # `rev-parse` into two calls to survive a newline in a repo path, and the + # second call inherited the first's silent arm — leaving a timeout on it to + # skip the shield with NO reason, after provisioning had already copied the + # files the shield exists to hide. + # + # Both failure shapes are handled without an `except` of its own, per round + # 7's funnel lesson: a non-zero rc returns the specific reason below, and a + # raise is caught by this try's tail, which reports a reason too. The tail + # also catches the `UnicodeError` this `fsdecode` of git's stderr can raise + # on Windows — the same defect #394 records at a sibling block, so the stderr + # read is deliberately INSIDE a guarded region rather than above one. + shared_answer = git_bytes(worktree, "rev-parse", "--git-common-dir") + if shared_answer.returncode != 0: + detail = ( + os.fsdecode(shared_answer.stderr).strip() + or f"git exited {shared_answer.returncode}" + ) + return ( + f"skipped the git-add shield ({worktree}): git identified the " + f"repository but could not name its common dir: {detail} — the " + "provisioned tool files are not shielded from the unit's `git add -A`" + ) # fsdecode, so a non-UTF-8 repo path round-trips back to the filesystem # instead of faulting. It can still raise on Windows (utf-8/surrogatepass # rejects a lone invalid byte), which is why it sits inside this try — # defensive placement rather than a path under test: the regression test # is POSIX-only because git on Windows has no such path to hand back. - common_dir = Path(os.fsdecode(raw).strip()) + # + # removesuffix("\n"), NOT strip() — rev-parse terminates its answer with one + # newline and every other byte belongs to the path (#384, Codex round 15). + # + # This comment used to argue strip() was harmless because "a final component + # is `.git` or `worktrees/`". That is true for a NON-bare repo and false + # for a bare one, whose common dir IS the repository directory. Measured at + # 2.55.0, a worktree of a bare repo at `…/common ` answers + # `--git-common-dir` = `…/common ` — trailing space and all — while + # `--absolute-git-dir` stays safe at `…/common /worktrees/`, since git + # sanitizes the admin id (round 5). So exactly one of the two answers was + # exposed, and stripping it pointed every later step at `…/common`, which does + # not exist: `--file /config` reads rc 1, i.e. ABSENT (round 10), so the + # `core.bare = true` refusal was MISSED and the shield went on to make the + # permanent format change on a bare repository. The lock's own + # `mkdir(parents=True)` then CREATED the stripped directory as a side effect. + # A safety gate defeated by whitespace in a path, which is why this is the + # parse and not a cosmetic. + # + # The dropped CRLF absorption is deliberate: git writes LF here on every + # platform, and a path legitimately ending in `\r` is indistinguishable from a + # CRLF terminator — so guessing would trade this bug for a rarer one. Windows + # CI is the oracle: were git to emit CRLF there, every shield test would fail + # on the malformed path rather than degrade quietly. + raw_git_dir = os.fsdecode(answered.stdout).removesuffix("\n") + raw_common = os.fsdecode(shared_answer.stdout).removesuffix("\n") + if not raw_git_dir or not raw_common: + # Defensive, and deliberately still a REASON rather than a silent skip: + # git answered, so the two-arm taxonomy in the docstring puts an + # unreadable answer on the degrade side. Unreachable with real git — an + # rc of 0 from `rev-parse` always prints a path — which is the same + # standing the fsdecode placement above has. + return ( + f"could not read this worktree's git dirs ({worktree}): " + f"{raw_git_dir!r}, {raw_common!r}" + ) + git_dir = Path(raw_git_dir) + common_dir = Path(raw_common) if not common_dir.is_absolute(): # a PLAIN checkout answers with a relative ".git"; a linked worktree - # answers with the main repo's absolute .git (verified, git 2.55), so - # this branch is the plain-checkout case, not the worktree one. - common_dir = (worktree / common_dir).resolve() - exclude = common_dir / "info" / "exclude" - exclude.parent.mkdir(parents=True, exist_ok=True) - existed = exclude.is_file() - existing = exclude.read_text(encoding="utf-8") if existed else "" - present = set(existing.splitlines()) - new = [p for p in patterns if p not in present] - if not new: - return None - prefix = existing if not existing or existing.endswith("\n") else existing + "\n" - # atomic_write_text, never write_text onto `exclude` directly (#375). - # write_text opens "w", which TRUNCATES before writing, and this is a - # read-modify-REWRITE carrying the operator's own excludes in `prefix` — - # so a short write (ENOSPC) left the file truncated mid-content while the - # degrade reason still said "could not update". Worse, the surviving tail - # is a valid git pattern and a prefix of the intended one: cut one char - # in and the last line is "/", which excludes the entire worktree. + # answers with the main repo's absolute .git (verified, git 2.55). + common_dir = worktree / common_dir + # resolve BOTH before comparing: --absolute-git-dir is absolute but not + # canonical, so a symlinked repo path would otherwise read as two + # different dirs and a main checkout would pass for a linked worktree. + git_dir, common_dir = git_dir.resolve(), common_dir.resolve() + if git_dir == common_dir: + # The main checkout itself: its gitdir IS the common dir, so the only + # exclude file to write would be the shared one — the #384 bug. + # Nothing here is transient enough to shield, so say so and stop. + return ( + f"skipped the git-add shield ({worktree}): not a linked worktree, and the " + "shield never writes the repository-wide exclude (#384)" + ) + # ABOVE THE LOCK DELIBERATELY, and the placement is the whole value of this + # gate: a repository shared between OS users is refused before anything is + # created in it, so every user gets this same reason rather than the second + # one meeting a lock file the first left behind. Safe above the lock because + # `core.sharedRepository` is static config the tool never writes — unlike + # `extensions.worktreeConfig` there is no shared mutable state to serialize. + # It is one deletion point if shared repositories are ever supported for real. + # A `GitError` from the read lands in this try's tail, which returns a reason. + shared_repository = _shield_shared_repository(worktree, common_dir) + if shared_repository is not None: + return shared_repository + # EVERYTHING BELOW IS ONE TRANSACTION, serialized per repository — round 8 + # (#384, Codex P2). `_shield_enable_worktree_config` PROBES the flag and the + # enable further down is an idempotent no-op when it is already `true`, so two + # concurrent provisionings in one repo both read an absent flag, both believe + # they enabled it, and whichever one's activation then fails rolls the flag + # back out from under the other's LIVE shield — git stops reading its + # `config.worktree`, its `core.excludesFile` goes inert, and its shielded tool + # files become stageable again mid-run. Nothing else collides first: every + # other per-run name is keyed on the run id (worktree path, branch, tmux + # session, run dir), so the flag is the one piece of state two runs share. + # `cmd_run` has no liveness check and the TUI's warning modal offers "launch + # anyway", so concurrent runs against one repo are permitted by design. # - # The helper rather than a hand-rolled tmp+replace: its temp name is - # unique, and EVERY linked worktree of a repo resolves to this same - # common dir, so a fixed `.tmp` sibling is a collision between two runs - # provisioning against one repo. It also fsyncs before the replace and - # carries the target's mode over, which a bare replace resets. + # The lock also removes a benign second consequence: `git config` writes take + # `.git/config.lock`, so a concurrent writer got an rc≠0 the enable read as a + # degrade and skipped the shield for that unit. # - # Atomicity per write is NOT isolation across the read above and this - # write: they are unserialized, so two such runs interleaving lose one - # side's patterns outright, both returning None. That race predates - # #375 and survives this fix — it is #381. - if not existed: - # Create it first, the way the write_text this replaced did: O_CREAT - # under the umask, giving the helper a mode to carry. Left to itself - # it creates a new target at mkstemp's private 0600 — and an exclude - # git cannot READ is one git silently IGNORES. Measured: git warns, - # exits 0, and `git add -A` stages the files the exclude was written - # to shield. That is the exact harm the docstring above says silence - # must not cause, and here nothing would even be reported, because - # the write SUCCEEDS. Reachable wherever the common dir is read - # under another UID (core.sharedRepository, a shared checkout) — - # which is why atomic_write_text's own docstring says callers of - # genuinely shared state need more than the helper alone. + # It spans the probe→enable→activate→rollback sequence and cannot be narrowed + # to just those: the enable must stay immediately above the activation (round + # 5), the private exclude must exist before the activation, and the version + + # `core.bare`/`core.worktree` gates inside the probe must stay above the seed + # read, which depends on the same `--type=` floor. So the file write sits in + # the middle of the locked span rather than outside it. + # + # The acquisition's own `OSError` is caught HERE rather than left to the tail, + # only so the operator is told which step failed: POSIX `flock` blocks + # indefinitely, but `msvcrt.locking` bounds the wait at ~10 s and then raises, + # so on Windows contention is a real, reportable outcome. Behaviorally the + # tail would also return a reason — this arm names the lock in it. + held = ExitStack() + try: + held.enter_context(file_lock(common_dir / _SHIELD_LOCK_NAME)) + except OSError as e: + return ( + f"skipped the git-add shield ({worktree}): could not take this " + f"repository's shield lock ({e}) — another bmad-loop run may hold it" + ) + with held: + refusal, needs_enable = _shield_enable_worktree_config(worktree, common_dir) + if refusal is not None: + return refusal + exclude = git_dir / "info" / "exclude" + exclude.parent.mkdir(parents=True, exist_ok=True) + # Zero bytes is an INTERRUPTED creation, not an initialized exclude. The + # `touch()` below creates the target before `atomic_write_bytes` fills it, and + # a fault in between (ENOSPC, or a kill) leaves that placeholder behind — + # harmless on its own, since the degrade arm returns above the activation and + # an unactivated private exclude does nothing. The harm would be deferred: + # read as authoritative, it skips the seed below and then points a shadowing + # `core.excludesFile` at a file missing the operator's own excludes, + # un-ignoring inside the worktree everything they ignore globally. Size + # rather than unlinking the placeholder on the way out, because no handler + # runs when the process is KILLED in that window. Re-seeding costs nothing + # either way: git treats an empty exclude and an absent one alike. + existed = exclude.is_file() and exclude.stat().st_size > 0 + # On creation the operator's own excludes are copied in, because the + # activation below shadows them (see _shield_inherited_excludes). On a + # re-provision a non-empty file's content is authoritative and the copy is + # not repeated, so the two stay idempotent together. # - # Safe against the tail below: an empty exclude is equivalent to an - # absent one for git, so a fault after this leaves no more damage - # than the missing file it replaced. - exclude.touch() - atomic_write_text(exclude, prefix + "\n".join(new) + "\n") - # UnicodeError, not UnicodeDecodeError: the write raises the ENCODE sibling. - # Still far short of ValueError-broad, so a programming error still escapes. - except (OSError, RuntimeError, UnicodeError) as e: + # A COPY FAULT SKIPS THE ACTIVATION. `_shield_inherited_excludes` raises on + # anything but a definite "absent" — an unreadable file, and since round 5 a + # git that will not say which file applies — and the raise lands in this + # function's tail, which returns a reason. That is the whole point of it + # raising rather than returning empty: an empty seed plus the activation + # below is not "the seed was lost", it is the operator's global ignores + # SHADOWED inside this worktree, and `git add -A` staging what they told git + # to ignore. There is nothing to clean up on that path — the raise is above + # `exclude.touch()`, so no placeholder is left for a later provisioning to + # read as authoritative, and the `mkdir` above leaves an inert empty `info/` + # that dies with the worktree. + # + # BYTES on both sides of that choice, and never decoded text: an exclude + # file holds path patterns, POSIX paths are arbitrary bytes, and a legacy + # 8-bit encoding is a perfectly ordinary thing for an operator's own file to + # be in. `bytes.splitlines()` is also the git-CORRECT line split, not merely + # the type-consistent one — `str.splitlines()` breaks on \x0b, \x0c, \x1c, + # \x1d, \x1e and \x85, none of which git treats as a line boundary, so a + # legitimate pattern containing one fragmented into wrong dedupe keys. + existing = exclude.read_bytes() if existed else _shield_inherited_excludes(worktree) + lines = existing.splitlines() + # PRESENT IS NOT THE SAME AS EFFECTIVE (#384, Codex round 17). gitignore's + # rule is LAST MATCH WINS, so a pattern this file already contains can be + # cancelled by a `!` line below it — and a plain set-membership dedupe then + # declined to append the shield's own pattern, leaving the provisioned tool + # files stageable with NO degrade reason at all, because nothing failed. + # Measured end-to-end through this function: seeded from an operator's + # `core.excludesFile` holding `/.claude/skills` then `!/.claude/skills`, + # `git status --porcelain -uall` in the worktree answers + # `?? .claude/skills/tool.md` and `git add -A` stages it. + # + # So a pattern may be skipped only where it is already GUARANTEED effective: + # it sits after the LAST negation in the file. That is airtight without + # reimplementing git's matching — no later line can negate it, so the last + # pattern matching any path it covers is either this one or a positive below + # it, and both ignore. Anything earlier is appended instead, which is always + # safe: the appended copy is last, so it is the one that decides. + # + # Deliberately CONSERVATIVE rather than exact. Measured, the only negation + # that actually defeats the shield is one re-including the pattern's own + # directory (`!.claude/skills` does it too — the negation need not repeat the + # positive's spelling); a `!*.md` below it does NOT, since git never descends + # into an excluded directory, and neither does `!/.claude`, since re-including + # a parent leaves the child matched. Telling those apart is git's matcher, so + # this appends a duplicate line in the harmless cases instead of guessing. + # + # `startswith(b"!")` with NO lstrip, which is git-correct twice over: git does + # not strip leading whitespace from a pattern, and `\!` escapes a literal + # leading `!` — both then read as positives here, as they do in git. + last_negation = max( + (i for i, line in enumerate(lines) if line.startswith(b"!")), default=-1 + ) + settled = set(lines[last_negation + 1 :]) + # fsencode, the inverse of the fsdecode above: a pattern derived from a real + # filename round-trips to its exact original bytes. Both sides of the `in` + # 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 settled] + # `not existed` keeps a re-provision that adds no pattern from leaving the + # config below pointing at a file that was never created. + if new or not existed: + # b"\n", not "\n": `bytes.endswith(str)` is a TypeError, and TypeError is + # not in the tail's except tuple — it would escape a function contracted + # never to propagate. + prefix = existing if not existing or existing.endswith(b"\n") else existing + b"\n" + # atomic_write_bytes, never write_bytes onto `exclude` directly (#375). + # write_bytes opens "wb", which TRUNCATES before writing, and this is a + # read-modify-REWRITE carrying the operator's own excludes in + # `prefix` — so a short write (ENOSPC) left the file truncated + # mid-content while the degrade reason still said "could not update". + # Worse, the surviving tail is a valid git pattern and a prefix of the + # intended one: cut one char in and the last line is "/", which + # excludes the entire worktree. + # + # The helper rather than a hand-rolled tmp+replace: it fsyncs before + # the replace and carries the target's mode over, which a bare replace + # resets. + # + # #381's interleaving race is mooted for NEW writes rather than fixed: + # this file lives in one worktree's private gitdir, so the two runs + # that used to race here — every linked worktree of a repo resolved to + # the same shared common dir — no longer share a target at all. Only a + # second provisioning of the SAME worktree can reach this file, which + # the engine does serially. The atomic write stays for #375, whose + # fault (a short write) needs no second writer. + if not existed: + # Create it first, the way the write_text this replaced did: + # O_CREAT under the umask, giving the helper a mode to carry. + # BEHAVIOR PRESERVATION is the whole of the rationale (#375). + # `atomic_write_bytes` carries a mode over only when the target + # already EXISTS — its own docstring says a fresh one gets + # mkstemp's private 0600 — so dropping this line would narrow + # every private exclude's mode as an unannounced side effect of + # that refactor. + # + # The mode is load-bearing only where another OS user reads this + # gitdir, and that configuration no longer reaches here: + # `_shield_shared_repository` refuses a repository configured + # `core.sharedRepository` above the lock (#384). What keeps the + # line anyway is the SHAPE the mode's failure takes — measured, + # git treats an exclude it cannot read as one that is not there: + # it warns, exits 0, and `git add -A` stages the very files the + # exclude was written to shield, with no degrade reason at all + # because the write SUCCEEDED. Silence is the one outcome this + # function's docstring rules out, so a line that cannot produce + # it is worth its cost. + # + # Safe against the tail below: an empty exclude is equivalent to + # an absent one for git, so a fault after this leaves no more + # damage than the missing file it replaced. + exclude.touch() + atomic_write_bytes(exclude, prefix + b"".join(p + b"\n" for p in new)) + # The PERMANENT repo-format change, deliberately the last-but-one thing + # this function does. `_shield_enable_worktree_config` probed the gates + # above and reported that the write is still owed; performing it there — + # where it used to live — put a format change the operator's repo carries + # forever AHEAD of the seed, the mkdir and the write, so every degrade + # below it (including the `_shield_inherited_excludes` fault that round 5 + # turned from a swallow into a degrade) left the repo marked for a shield + # that never applied. Nothing BETWEEN this write and the activation can + # degrade — and that sentence used to stop right there, which is what + # round 10 caught: "nothing between here and X" never covered the ENABLE + # ITSELF, precisely as round 6 found it did not cover the activation + # itself. This call has its own two failure shapes. So the paths that can + # still set the flag without shielding anything are the enable's own raise + # (handled immediately below) and the activation's failure (handled after + # it), and each rolls the flag back. + # + # It cannot move BELOW the activation: `git config --worktree` is what the + # extension unlocks, so the activation fatals without it. + if needs_enable: + # BOTH of the chokepoint's failure shapes, classified into one value + # before anything is decided — round 7's funnel, applied to the one + # call that never received it (#384, Codex round 10). Handling only + # the rc left a raise (a timeout, a spawn failure) to the tail, which + # cannot roll anything back: `git config` can be killed AFTER it has + # renamed the new config into place, so the flag survived with no + # shield ever activated. + # + # THE ROLLBACK FIRES ON THE RAISE ONLY, and that asymmetry is + # deliberate — it is NOT the enumeration defect it resembles, so do + # not "finish" it by rolling back the rc too. Round 5's taxonomy draws + # the line: an rc IS an answer. git refused, which means the lock file + # was never renamed into place and there is nothing to undo; the + # likeliest cause is `.git/config.lock` contention with a concurrent + # writer, and that writer is exactly the party whose flag an `--unset` + # would then remove. A raise is NOT an answer — we did not find out — + # and only the uncertain case may trigger repair. Both shapes are + # still CLASSIFIED in one place, which is the part round 7 was about. + # + # `needs_enable` is True on this branch, so `_shield_undo_extension`'s + # caller-side gate is satisfied by construction; the callee applies + # its own sibling-ownership gate on top. + rolled_back = "" + try: + enabled = git_bytes(worktree, "config", "extensions.worktreeConfig", "true") + enable_fault = ( + None + if enabled.returncode == 0 + else os.fsdecode(enabled.stderr).strip() + or f"git exited {enabled.returncode}" + ) + except GitError as e: + enable_fault = str(e) + rolled_back = _shield_undo_extension(worktree, git_dir, common_dir) + if enable_fault is not None: + return ( + f"could not enable extensions.worktreeConfig ({worktree}): " + f"{enable_fault} — the provisioned tool files are not shielded " + f"from the unit's `git add -A`{rolled_back}" + ) + # LAST, after the file it names exists: git reads core.excludesFile lazily, + # but a config pointing at a file that a fault above left unwritten would + # be a shield that silently excludes nothing while reporting a reason. + # ONE rollback covering EVERY way this step can fail to leave a working + # shield, and the shape of this arm is the lesson rather than an incidental + # style choice. `git_bytes` can fail two ways — a non-zero rc (git refused) + # and a RAISE (a timeout, or a spawn failure as its GitSpawnError subclass) + # — and two consecutive rounds each fixed ONE of them, because each fix + # ENUMERATED the failure it had been shown. Enumeration is what kept being + # incomplete. So every shape converges on one `fault` string BEFORE + # anything is decided. + # + # A later round then supplied the shape that sentence could not have + # anticipated, because it is not a FAILURE at all: the write SUCCEEDS and + # the shield still does not apply, since `command` scope outranks + # `worktree` and git resolves the key from the most specific scope that + # carries it. So the `try` now spans the write AND the verification that + # it took effect, and rc-0 is no longer an exit from this block — see + # `_shield_verify_activation`. The rule that survived all of it: ask what + # the POST-CONDITION is and confirm it, rather than enumerating the ways + # the call in front of you can go wrong. + # + # What that first paragraph used to add — "and there is a single place + # where the flag can be rolled back" — was true of the failure SHAPES and + # false of the WRITES: round 10 added a second rollback point at the + # enable above, because rollback sites track the number of writes that can + # fail, not the number of ways each one fails. + # + # The rollback exists because the enable one line above is a PERMANENT + # repo-format change, and the guarantee this code owes (docstring, CHANGELOG, + # docs/FEATURES.md) is that the flag outlives a failed shield only where a + # rollback was declined or could not be made, and the reason says which. + # That wording is round 10's: it used to read "survives only a shield that + # actually activated", which the enable's own raise falsified. When a + # fix's value proposition is a GUARANTEE, the same edit has to reach every + # copy of it — these three, plus this function's own docstring. + # `needs_enable` gates it — see `_shield_undo_extension` for why + # that condition is a safety property and not a micro-optimization, and for + # the second gate it applies itself: `needs_enable` records what the PROBE + # saw, so it cannot tell "we enabled it" from "we and a concurrent run both + # thought we did", and the callee declines when a sibling worktree's + # `config.worktree` would stop being read. + try: + activated = git_bytes( + worktree, "config", "--worktree", "core.excludesFile", str(exclude) + ) + fault = ( + None + if activated.returncode == 0 + else os.fsdecode(activated.stderr).strip() + or f"git exited {activated.returncode}" + ) + if fault is None: + # An rc 0 here means the value was WRITTEN, not that it is in + # FORCE — `command` scope outranks `worktree` and is fed by the + # environment we were launched with. Verified rather than + # assumed, INSIDE this same `try` and above the one decision + # below, so the verification's own two failure shapes reach the + # existing rollback instead of growing a second one. See + # `_shield_verify_activation` for why this asks git what it + # resolved instead of looking for the override's origin. + fault = _shield_verify_activation(worktree, exclude) + except GitError as e: + # Deliberately NOT left to the tail: the tail cannot roll the flag back, + # and a GitError from EITHER call in this block means exactly what a + # refusal means — the shield is not in force. That now covers the + # verification's own fault as well as the write's, which is the point of + # putting it inside this `try` rather than after it. The tail's own + # GitError guard stays load-bearing for the seed read above, which round + # 5 turned into a degrade, so this is a narrowing of what reaches it, + # not a bypass. + fault = str(e) + if fault is not None: + if needs_enable: + fault += _shield_undo_extension(worktree, git_dir, common_dir) + return f"could not activate the worktree git exclude ({worktree}): {fault}" + # GitError is every fault a chokepoint git call can raise here — a timeout, or + # a spawn failure as its GitSpawnError subclass. It replaces the + # `subprocess.SubprocessError` this tuple used to carry for exactly that pair, + # which the chokepoint now translates before it ever reaches this frame. + # + # UnicodeError, not UnicodeDecodeError, and NOT for the exclude file any more: + # the payload is bytes end-to-end, so neither its read nor its write can raise a + # codec fault at all. What is left for it is stated in the docstring — the + # `os.fsdecode` of git's stdout (Windows-only) and `os.fsencode` of a pattern + # carrying a surrogate that POSIX surrogateescape never produced. Both are + # ENCODE-or-DECODE depending on which, hence the shared base class. Still far + # short of ValueError-broad, so a programming error still escapes. + # + # TypeError is deliberately absent: a str/bytes mixup on the payload path is a + # programming error, and letting it crash beats reporting it as an operator's + # degraded shield once per run. + except (GitError, OSError, RuntimeError, UnicodeError) as e: return f"could not update the worktree-local git exclude ({worktree}): {e}" return None diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index ba19b38c..edb0a53f 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -6,9 +6,9 @@ configuration, not a process-lifecycle primitive, so it does not belong on the host. On Linux/macOS — and WSL, which *is* Linux — these preserve today's exact behavior. The file-replace and segment helpers below (``atomic_replace``, -``atomic_write_text``, ``safe_segment``, ``safe_ref_segment``) are exercised by the -platform tests; the pid kill/liveness Windows branch degrades gracefully and is not -yet exercised. +``atomic_write_text``, ``atomic_write_bytes``, ``safe_segment``, +``safe_ref_segment``) are exercised by the platform tests; the pid kill/liveness +Windows branch degrades gracefully and is not yet exercised. ``safe_segment`` and ``safe_ref_segment`` share a contract but not a rule set: the first coerces a Windows *filename* segment, the second a *git ref* component, and @@ -201,15 +201,44 @@ def atomic_write_text(path: Path, text: str) -> None: Ordering the flush before the rename means a crash yields either the old file or the complete new one. The directory itself is deliberately not synced: that would make the *rename* durable, and losing the rename just leaves the old - contents in place — stale, never corrupt.""" + contents in place — stale, never corrupt. + + The bytes sibling is :func:`atomic_write_bytes`; the two share every property + above and differ only in the ``os.fdopen`` mode. Text mode's *newline* + default (translating) is deliberate here — it matches the ``Path.write_text`` + this replaced, so a ledger's line endings do not change under Windows.""" + _atomic_write(path, text, mode="w", encoding="utf-8") + + +def atomic_write_bytes(path: Path, data: bytes) -> None: + """Replace ``path``'s contents with ``data`` atomically — the byte-exact + sibling of :func:`atomic_write_text`, whose docstring carries the shared + contract (symlinks followed, mode and xattrs preserved when the target already + exists, a fresh target left at ``mkstemp``'s private ``0600``, fsync before the + replace, temp removed on any failure). + + The one difference is the whole point: ``data`` lands byte-for-byte. No encode + and no newline translation, so a payload carrying LF keeps LF on Windows and + bytes that are not valid text in any codec survive the round trip. Callers + handling filesystem-derived content want this variant — a POSIX filename is + arbitrary bytes, and an operator's git exclude file may be in any legacy + encoding at all.""" + _atomic_write(path, data, mode="wb", encoding=None) + + +def _atomic_write(path: Path, payload: str | bytes, *, mode: str, encoding: str | None) -> None: + """The shared body of the two public helpers above — see + :func:`atomic_write_text` for the contract every step here implements. + + Written through ``os.fdopen`` rather than a raw ``os.write`` loop on purpose: + it routes to ``io.open``, the one seam a test can inject a short write at for + both variants at once (tests/test_install.py's #375 case).""" target = path.resolve() fd, tmp_name = tempfile.mkstemp(dir=str(target.parent), prefix=target.name + ".", suffix=".tmp") tmp = Path(tmp_name) try: - # newline default (translating) deliberately: matches the Path.write_text - # this replaced, so a ledger's line endings do not change under Windows. - with os.fdopen(fd, "w", encoding="utf-8") as fh: - fh.write(text) + with os.fdopen(fd, mode, encoding=encoding) as fh: + fh.write(payload) fh.flush() # userspace buffer -> kernel, so there is something to sync os.fsync(fh.fileno()) if target.exists(): @@ -243,12 +272,42 @@ def file_lock(path: Path, *, blocking: bool = True) -> Iterator[None]: Lock a dedicated sibling file, never data that is swapped via :func:`atomic_replace` — the lock rides the open fd's inode, and a replace would swap that inode out from under later acquirers. ``fcntl.flock`` on - POSIX (blocks indefinitely; holders only do brief file I/O); ``msvcrt.locking`` - on Windows, where the blocking mode's built-in ~10 s retry bounds the wait - and surfaces contention as ``OSError``. + POSIX; ``msvcrt.locking`` on Windows, where the blocking mode's built-in + ~10 s retry bounds the wait and surfaces contention as ``OSError``. + + THE WAIT IS PLATFORM-ASYMMETRIC, and a caller has to decide what that means + for it: POSIX blocks indefinitely, Windows gives up after ~10 s and raises. + This docstring used to add "holders only do brief file I/O" — true when the + only consumers were tests, and falsified by the first production caller + (``install._worktree_local_exclude``, #384), which holds it across a + multi-step git transaction of roughly seven ``git`` spawns, each bounded by + ``[limits] git_timeout_s``. So a Windows acquirer can genuinely time out + under contention rather than only under a deadlock. Hold it for as short a + span as correctness allows, and handle the ``OSError`` from acquisition. + + OWNER-ONLY, AND DELIBERATELY NOT MADE TO WORK ACROSS OS USERS. A repository + shared between OS users is not a supported configuration (maintainer + decision, #384); ``install._shield_shared_repository`` refuses one up front, + above the shield's acquisition of this lock, so no lock file is created there + at all. That gate is where the case is handled — not here. Three "fixes" + belong to it rather than to this function, and each is worse than it looks: + widening the mode (from the umask, from ``.git``'s own bits, from a + ``core.sharedRepository`` read) hands a peer write access to a file this + process is relying on; falling back to ``O_RDONLY`` when the open fails + rescues only the modes that happen to grant group read; and unlinking a + badly-moded lock to recreate it is the actively dangerous one — a lock + another process currently HOLDS would be replaced by a new inode, ``flock`` + exclusion rides the inode, and both processes would then believe they hold + it, rebuilding the concurrency bug this lock was added to prevent. + + The explicit ``0o600`` states that policy rather than leaving it to the + caller's umask, which would make the mode a property of whoever provisioned + first (measured: umask 022 gives 0o755, umask 077 gives 0o700 — both + arbitrary). It is a ceiling, not a floor: ``os.open``'s mode is still masked, + so an unusual umask can only narrow it further, never widen it. """ path.parent.mkdir(parents=True, exist_ok=True) - fd = os.open(path, os.O_RDWR | os.O_CREAT) + fd = os.open(path, os.O_RDWR | os.O_CREAT, 0o600) try: if sys.platform == "win32": import msvcrt diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 4bbb9e1b..30504fbe 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -110,11 +110,18 @@ def provision_worktree( - the hook points at the MAIN repo's already-installed relay via an absolute path (the relay locates the run dir from $BMAD_LOOP_RUN_DIR, not its own location), so nothing is written into the worktree's .bmad-loop/; - - everything we wrote is added to the worktree's local git exclude. That write - is best-effort: when git can't be queried at all it is skipped silently, but - a filesystem fault after that is reported to `on_degraded` (once, with the - reason) rather than swallowed — an unwritten exclude is what lets `git add -A` - stage the tool files, so it must not fail invisibly. + - everything we wrote is excluded from git, in a file private to THIS worktree + (`.git/worktrees//info/exclude`, activated per-worktree) that dies with it + when the worktree is removed. It is never the repository-wide + `.git/info/exclude`: that file is shared with the operator's own checkout and + permanent, so shielding through it hid every new file under a tool dir from + their `git add -A` forever (#384). That write is best-effort: when git can't + be queried at all it is skipped silently, but any fault after that — including + a refusal to scope the shield, and a shield that was written but which git does + not resolve to — is reported to `on_degraded` (once, with the reason) rather + than swallowed, and the shield is skipped rather than widened back to the + shared file. An unshielded worktree lets `git add -A` stage the tool files, so + it must not fail invisibly. Skill trees, the per-CLI hook config, and the seeded configs all live in dirs projects gitignore — but the exclude shields them even when a project doesn't. @@ -282,7 +289,9 @@ def provision_worktree( # Shield exactly the paths we wrote (skill trees + hook configs + seeded # configs) from the unit's `git add -A`, in case a project doesn't gitignore - # its tool dirs. + # its tool dirs. Scoped to this worktree and expiring with it — these are + # paths projects legitimately TRACK, so a repo-wide exclude here went on + # hiding their new files from the operator's own checkout (#384). patterns = {f"/{p.skill_tree}" for p in profiles} # hookless profiles have no config_path — and their empty string would render # as the pattern "/", git-excluding the entire worktree. @@ -483,9 +492,7 @@ def run_isolated(self, task: StoryTask, drive: Callable[[StoryTask], None]) -> N self.paths.repo_root, seed_files=list(dict.fromkeys(seeds)), # dedupe, preserve order seed_globs=self._registry.seed_globs(), - on_degraded=lambda msg: self.journal.append( - "worktree-exclude-degraded", story_key=task.story_key, error=msg - ), + on_degraded=lambda msg: self._exclude_degraded(task.story_key, msg), ) if skipped_seeds: # A seed entry whose destination already exists is a no-op. Harmless for @@ -523,6 +530,23 @@ def run_isolated(self, task: StoryTask, drive: Callable[[StoryTask], None]) -> N # spec gate or an escalation propagates past here, leaving the worktree up. self.integrate_unit(task, unit) + def _exclude_degraded(self, story_key: str, msg: str) -> None: + """The git-add shield was owed for this unit's worktree and did not happen. + + Journaled AND notified, the way `worktree-open-failed` is above. The notify + is the half that was missing: the shield's whole degrade policy is to SKIP + rather than widen — activating over patterns it could not copy would shadow + the operator's own excludes — and skipping is only defensible if the operator + finds out. A run that ends "finished" with a journal line nobody reads is how + the provisioned tool files reach a story's merge unnoticed. + + `gates.notify` is best-effort and never raises, and is inert unless + `notify.file`/`notify.desktop` is configured, so this cannot break a run — + which matters, because it is called from inside provisioning. + """ + self.journal.append("worktree-exclude-degraded", story_key=story_key, error=msg) + gates.notify(self.policy, self.run_dir, f"worktree exclude degraded: {story_key}", msg) + def failed_diff_max_bytes(self) -> int | None: """Per-untracked-file size cap for a failed unit's forensic patch, in bytes — or None when the operator lifted the cap (scm.failed_diff_unlimited).""" diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 2b5df30e..57ea3726 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -902,6 +902,15 @@ def test_per_worktree_setup_then_gate_then_teardown_and_seed(project): """Happy path: the worktree is seeded, the setup hook runs, the ready gate waits (and only passes because setup ran first), the agent runs, teardown fires. Ordering is proven by the gate depending on a setup marker.""" + # The MCP-generated skill tree really is gitignored in a per_worktree project + # (docs/FEATURES.md), and this fixture has to say so itself: until #384 the + # git-add shield wrote its patterns into the repo-wide `.git/info/exclude`, so + # the untracked dir below was hidden in the MAIN checkout too and the pre-merge + # cleanliness gate never saw it. The shield is per-worktree now, and an + # untracked file in the operator's own checkout blocks a merge as it always has + # (`verify.dirty_paths` counts untracked). Committed before the dir exists. + gitignore = project.project / ".gitignore" + gitignore.write_text(gitignore.read_text() + ".claude/skills/\n", encoding="utf-8") commit_sprint(project, {"1-1-a": "ready-for-dev"}) # a gitignored MCP skill dir present in the main repo (untracked) to be seeded skill = project.project / ".claude" / "skills" / "gameobject-create" @@ -1514,18 +1523,29 @@ def always_fail(*a, **k): assert "worktree-teardown-degraded" in journal_kinds(engine) -def test_isolated_exclude_degrade_is_journaled(project, monkeypatch): +def test_isolated_exclude_degrade_is_journaled_and_notified(project, monkeypatch): """#359: `provision_worktree`'s local-exclude write is best-effort, so the sole caller has to give the degrade somewhere to land — otherwise a swallowed fault silently lets the unit's `git add -A` commit the provisioned tool files. The helper is patched at `worktree_flow`'s own `from .install import` binding (worktree_flow.py:35) — patching `install._worktree_local_exclude` would not be - seen here. Journal-only by design (the run is unharmed and continues), matching - `worktree-teardown-degraded` rather than the notify-worthy `worktree-open-failed`. - - Ablation: delete the `on_degraded=` lambda at the `provision_worktree` call in - `run_isolated` and this fails — no `worktree-exclude-degraded` is journaled.""" + seen here. + + BOTH CHANNELS, and round 5 added the second. This was journal-only, on the + reasoning that the run is unharmed and continues — so it matched + `worktree-teardown-degraded` rather than the notify-worthy + `worktree-open-failed`. What that missed is that the shield's entire degrade + policy is to SKIP rather than widen (activating over patterns it could not copy + would shadow the operator's own excludes), and a skip is only defensible if the + operator finds out. Round 5 also turned a swallowed `GitError` into a degrade, + so the path is now reachable from a transient git fault rather than only from a + write fault. A journal line nobody reads is how the provisioned tool files reach + a story's merge unnoticed. + + Ablations, one per channel: delete the `on_degraded=` lambda at the + `provision_worktree` call in `run_isolated` and both assertions fail; drop the + `gates.notify` from `_exclude_degraded` and only the ATTENTION one does.""" repo = project.project gitignore = repo / ".gitignore" gitignore.write_text(gitignore.read_text() + ".mcp.json\n", encoding="utf-8") @@ -1549,6 +1569,9 @@ def test_isolated_exclude_degrade_is_journaled(project, monkeypatch): assert summary.done == 1 and not summary.paused and not summary.crashed entry = next(e for e in engine.journal.entries() if e["kind"] == "worktree-exclude-degraded") assert entry["story_key"] == "1-1-a" and entry["error"] == "boom: read-only .git" + attention = (engine.run_dir / "ATTENTION").read_text(encoding="utf-8") + assert "worktree exclude degraded: 1-1-a" in attention + assert "boom: read-only .git" in attention # the degrade is a warning, not a stop: the unit still merged back assert "unit-merged" in journal_kinds(engine) diff --git a/tests/test_install.py b/tests/test_install.py index bfc02a70..eede0ee6 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -1,8 +1,11 @@ +import contextlib import io import json import os import re +import shlex import stat +import subprocess import sys import zipfile from pathlib import Path @@ -18,6 +21,8 @@ DEV_BASE_SKILLS, MODULE_SKILLS, _copy_traversable, + _git_version_at_least, + _shield_undo_extension, _worktree_local_exclude, install_into, merge_hooks, @@ -45,6 +50,29 @@ def _install_base_skills(root, tree=".claude/skills"): _install_skills(root, tree, BASE_SKILLS) +def _wt_private_exclude(wt): + """The file the git-add shield writes: the exclude in the worktree's OWN + gitdir (`.git/worktrees//info/exclude`), never the repo-wide one (#384). + + Asked of git rather than composed from `.git/worktrees/`: git + appends a disambiguating number when the basename is already taken, so a + hand-built path is right only by luck.""" + return Path(git(wt, "rev-parse", "--absolute-git-dir")) / "info" / "exclude" + + +def _is_unset(args): + """Does this `git_bytes` argv unset a key, in ANY of git's spellings? + + A PREFIX test rather than `"--unset" in args`, and the reason is a trap this + suite walked into: `args` is a tuple, so membership is exact-token, and round + 10's switch from `--unset` to `--unset-all` silently stopped four fakes from + matching. Two were assertions and failed loudly; the other two were TRIPWIRES, + which simply went quiet — a tripwire that no longer matches passes exactly like + a gate that works, and it takes the ablations built on it down with it. + """ + return any(a.startswith("--unset") for a in args) + + def _registrations(profile, command="python3 /x/.bmad-loop/bmad_loop_hook.py {event}"): return { native: command.format(event=canonical) @@ -1161,17 +1189,23 @@ def test_provision_worktree_bmad_custom_does_not_clobber_checkout(tmp_path): def test_provision_worktree_bmad_custom_shielded_in_local_exclude(project, tmp_path): """Seeded customization must stay out of the unit's `git add -A` — a project - that doesn't gitignore `_bmad/` would otherwise merge it back on every story.""" + that doesn't gitignore `_bmad/` would otherwise merge it back on every story. + + The pattern lands in the worktree's PRIVATE exclude, and the repo-wide one is + byte-identical afterwards (#384).""" repo = project.project _write_override(repo, _layer("house-style", "bmad-review-company"), user=True) wt = tmp_path / "wt" verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() provision_worktree(wt, [get_profile("claude")], repo) assert (wt / "_bmad" / "custom" / "bmad-dev-auto.user.toml").is_file() - exclude = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8") + exclude = _wt_private_exclude(wt).read_text(encoding="utf-8") assert "/_bmad/custom" in exclude.splitlines() + assert shared.read_bytes() == before def test_missing_stories_support_findings_split_absent_from_stale(tmp_path): @@ -1421,18 +1455,22 @@ def test_provision_worktree_seed_then_hook_merge_preserves_settings(tmp_path): def test_provision_worktree_seed_shielded_in_local_exclude(project, tmp_path): - """Seeded configs are added to the worktree's local git exclude so a project - that doesn't gitignore them won't have the unit's `git add -A` stage them.""" + """Seeded configs are added to the worktree's private git exclude so a project + that doesn't gitignore them won't have the unit's `git add -A` stage them — + without a line reaching the repo-wide exclude the main checkout shares (#384).""" repo = project.project (repo / ".mcp.json").write_text("{}", encoding="utf-8") wt = tmp_path / "wt" verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() provision_worktree(wt, [get_profile("claude")], repo, seed_files=[".mcp.json"]) assert (wt / ".mcp.json").is_file() - exclude = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8") + exclude = _wt_private_exclude(wt).read_text(encoding="utf-8") assert "/.mcp.json" in exclude.splitlines() + assert shared.read_bytes() == before def test_provision_worktree_partial_seed_dir_shielded_in_local_exclude(project, tmp_path): @@ -1449,11 +1487,2567 @@ def test_provision_worktree_partial_seed_dir_shielded_in_local_exclude(project, (wt / "cfg").mkdir(parents=True) (wt / "cfg" / "tracked.yaml").write_text("FROM_REPO", encoding="utf-8") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() + provision_worktree(wt, [get_profile("claude")], repo, seed_files=["cfg"]) assert (wt / "cfg" / "ignored.yaml").read_text() == "SEED ME" - exclude = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8") + exclude = _wt_private_exclude(wt).read_text(encoding="utf-8") assert "/cfg" in exclude.splitlines() + assert shared.read_bytes() == before + + +# ------------------------------------------- the shield is scoped to the worktree (issue #384) +# +# Real linked worktrees throughout (`verify.worktree_add` on the `project` fixture), +# never a stand-in: every property under test is git's, not the filesystem's — which +# exclude file git reads, which config scope wins, what `git worktree remove` deletes. +# A mocked git would assert the test author's model of those instead of git's answer. + + +def test_shield_never_touches_main_checkout(project, tmp_path): + """THE #384 regression: after a worktree is provisioned, a NEW file under a + TRACKED tool dir must still be staged by `git add -A` in the main checkout. + + The reported harm exactly. `.claude/skills` is a directory projects legitimately + track, the shield names it, and the repo-wide `.git/info/exclude` the helper + used to append to is shared with the operator's own checkout, permanent, and + unversioned. Their next `git add -A` then captured only diffs to files git + already tracked, while every newly created sibling silently vanished — 51 files + and three whole skills across two upgrade commits in the reporter's repo, with + nothing in either diff able to reveal why. + + Both assertions are needed. Git's own answer (the file stages) is the harm; the + shared file's BYTES are the mechanism, and pin that nothing was appended even in + a form that happens not to match this fixture's paths. + + Ablation: target `common_dir / "info" / "exclude"` again and both fail.""" + repo = project.project + tracked = repo / ".claude" / "skills" / "committed-skill" + tracked.mkdir(parents=True) + (tracked / "SKILL.md").write_text("# tracked\n", encoding="utf-8") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "track the skill tree") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + + provision_worktree(wt, [get_profile("claude")], repo) + + # what the operator does next, in their own checkout: add a skill + fresh = repo / ".claude" / "skills" / "written-after-the-run" + fresh.mkdir(parents=True) + (fresh / "SKILL.md").write_text("# new\n", encoding="utf-8") + git(repo, "add", "-A") + staged = git(repo, "diff", "--cached", "--name-only").splitlines() + assert ".claude/skills/written-after-the-run/SKILL.md" in staged + assert shared.read_bytes() == before + + +def test_shield_excludes_only_inside_the_worktree(project, tmp_path): + """The shield still shields: the very path the main checkout must keep seeing + is invisible to the WORKTREE's `git add -A`. + + Paired with the regression above deliberately. Issue #384 measured that a + private exclude alone does NOTHING — git reads only `$GIT_COMMON_DIR/info/exclude` + — so writing the file and skipping the `config --worktree core.excludesFile` + activation would satisfy that test while quietly committing the tool files here. + Neither test is meaningful without the other. + + Ablation: drop the activation call and this fails — the provisioned skills and + settings.json show up staged.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + + provision_worktree(wt, [get_profile("claude")], repo) + + assert (wt / ".claude" / "skills" / "bmad-loop-sweep" / "SKILL.md").is_file() + assert (wt / ".claude" / "settings.json").is_file() + git(wt, "add", "-A") + assert git(wt, "diff", "--cached", "--name-only") == "" + + +@pytest.mark.parametrize("channel", ["GIT_CONFIG_COUNT", "GIT_CONFIG_PARAMETERS"]) +def test_shield_degrades_when_a_command_scope_excludesfile_outranks_it( + project, tmp_path, monkeypatch, channel +): + """A successful `git config --worktree core.excludesFile` proves the value was + WRITTEN, not that git READS it. `command` scope outranks `worktree`, and it 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, with no reason to journal. + + Round 17 was "a pattern PRESENT in the file is not EFFECTIVE"; this is that gap one + level up — WRITTEN is not EFFECTIVE. + + PARAMETRIZED BECAUSE THE ENUMERATION FIX WOULD PASS ONE AND FAIL THE OTHER, which + is the whole argument for verifying the post-condition instead of detecting the + override's origin. Measured end to end on real linked worktrees at both ends of the + supported range: + + 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' + + So the channel the finding named 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 -c` + on a session's own command line is a third that never appears in our environment at + all. The fix names none of them. + + `'k=v'` is the encoding used below because it is the only one honored at BOTH ends; + the newer `'k'='v'` form is `fatal: bogus format in GIT_CONFIG_PARAMETERS` at 2.20.4. + + The reason string is the discriminator here, and deliberately so — unlike round 17, + where it could not bite. `git status` shows the tool file with the bug AND with the + fix; what changes is whether the operator is told. Non-vacuity is pinned separately, + by asserting the override really is in force before trusting the degrade. + + The sibling that keeps this from being a blanket refusal already exists: + `test_shield_seeds_users_excludesfile` sets `core.excludesFile` at LOCAL scope and + asserts the shield still activates. Only a scope ABOVE worktree may degrade. + + Ablation: drop the `_shield_verify_activation` call and both cases fail on + `reason is not None` — the shield reports success while `git status` in the worktree + still shows `probe-384`.""" + repo = project.project + if channel == "GIT_CONFIG_COUNT" and not install_mod._git_version_at_least( + git(repo, "version"), (2, 31) + ): + pytest.skip("GIT_CONFIG_COUNT is git 2.31; the GIT_CONFIG_PARAMETERS case covers older git") + if channel == "GIT_CONFIG_PARAMETERS" and sys.platform == "win32": + # POSIX-only: the pre-2.31 encoding is sq-quoted, so a Windows path's + # backslashes would be exercising git's own quoting rules rather than this + # funnel. The GIT_CONFIG_COUNT case covers Windows, and needs no quoting. + pytest.skip("POSIX-only: sq-quoted encoding vs. backslash paths is not what this pins") + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + operator = tmp_path / "operator-ignores" + operator.write_text("/something-else\n", encoding="utf-8") + + with monkeypatch.context() as env: + if channel == "GIT_CONFIG_COUNT": + env.setenv("GIT_CONFIG_COUNT", "1") + env.setenv("GIT_CONFIG_KEY_0", "core.excludesFile") + env.setenv("GIT_CONFIG_VALUE_0", str(operator)) + else: + env.setenv("GIT_CONFIG_PARAMETERS", f"'core.excludesFile={operator}'") + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + # Non-vacuity, inside the same environment: git really does resolve the + # operator's file rather than ours. Without this the test would also pass on a + # git where the channel is inert, for entirely the wrong reason. + assert git(wt, "config", "--type=path", "--get", "core.excludesFile") == str(operator) + + assert reason is not None + assert "another configuration scope outranks it" in reason + # The reason renders the path with `!r`, deliberately — a legal POSIX path can + # carry edge whitespace or control bytes, and only the repr discloses them. So + # compare against the REPR, not the raw string: on Windows `repr()` doubles every + # backslash, and `str(operator) in reason` fails there while the shield is behaving + # perfectly. Windows CI was the oracle for this, as it keeps being for this shield. + assert repr(str(operator))[1:-1] in reason + # and the shield really is skipped rather than half-applied + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + assert "probe-384" in git(wt, "status", "--porcelain", "-uall") + + +def test_shield_outranked_degrade_leaves_no_permanent_repo_format_change( + project, tmp_path, monkeypatch +): + """The outranked degrade is the first one reachable AFTER a write that SUCCEEDED, + so the rollback runs in a repo state none of its other tests produce: the + worktree-scoped `core.excludesFile` is really there in `config.worktree`. + + Two claims, and they are separate: the permanent flag must be gone, and the key the + activation did land must be harmless. Measured rather than assumed — unsetting + `extensions.worktreeConfig` makes git stop reading `config.worktree` at all, so the + leftover key is inert and needs no second `--unset`. That is why this arm has one + rollback rather than two; a second write would be a second failure shape and a + second rollback site, which is the enumeration this block exists to avoid. + + Ablation: drop the `needs_enable` rollback and the first assertion fails — + `worktreeConfig` survives a degrade that shielded nothing.""" + repo = project.project + if not install_mod._git_version_at_least(git(repo, "version"), (2, 31)): + pytest.skip("GIT_CONFIG_COUNT is git 2.31") + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "config" + assert "worktreeConfig" not in shared.read_text(encoding="utf-8") + operator = tmp_path / "operator-ignores" + operator.write_text("/something-else\n", encoding="utf-8") + + with monkeypatch.context() as env: + env.setenv("GIT_CONFIG_COUNT", "1") + env.setenv("GIT_CONFIG_KEY_0", "core.excludesFile") + env.setenv("GIT_CONFIG_VALUE_0", str(operator)) + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None + assert "worktreeConfig" not in shared.read_text(encoding="utf-8") + # the rollback is silent when it works: the operator is told the shield did not + # apply, not handed a second fault that never happened + assert "could NOT be rolled back" not in reason + # And with the extension off, the key the activation DID land is inert: git stops + # reading `config.worktree` entirely, so the read comes back UNSET (rc 1) even + # though the value is still sitting in that file. Spawned directly rather than + # through the `git` helper, whose `check=True` turns git's own "no such key" into + # an error — the rc IS the assertion here. + wt_config = Path(git(wt, "rev-parse", "--absolute-git-dir")) / "config.worktree" + assert "excludesFile" in wt_config.read_text( + encoding="utf-8" + ), "the activation's key should still be on disk — this pins INERT, not removed" + left_behind = subprocess.run( + ["git", "-C", str(wt), "config", "--type=path", "--get", "core.excludesFile"], + capture_output=True, + text=True, + check=False, + ) + assert left_behind.returncode == 1 and left_behind.stdout == "" + + +@pytest.mark.parametrize("fault", [verify.GitError, verify.GitSpawnError]) +def test_shield_degrades_when_git_will_not_confirm_the_activation( + project, tmp_path, monkeypatch, fault +): + """The verification is itself a chokepoint call, so it inherits both of + `git_bytes`' failure shapes. Neither may be read as "the shield is fine": not + knowing whether the written key is the one git resolves has exactly the standing of + knowing it is not. + + THE FAKE MUST TELL THE TWO READS APART BY STATE, not by argv, and that is the trap + this test exists on top of. The seed read and the verification read use byte-identical + arguments — real git distinguishes them only by what has been written in between — so + a fake keyed on the arguments alone would fault the SEED instead and this would pass + while testing a completely different arm. + + Parametrized over both classes because `GitSpawnError` is a subclass, so a + `GitError`-only test would keep passing against a handler narrowed to the parent. + + Ablation: move the verification call out of the `try` and both cases fail — the + fault reaches the tail, which returns a reason but cannot roll the flag back.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + activated = [] + + def fault_after_activation(worktree, *args): + if args[:2] == ("config", "--worktree"): + activated.append(args) + return real(worktree, *args) + if activated and "--get" in args and "core.excludesFile" in args: + raise fault("git did not answer") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fault_after_activation) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert activated, "the fake never saw the activation — it faulted the seed read instead" + assert reason is not None and "did not answer" in reason + # the permanent format change does not outlive a shield that never confirmed + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + + +def test_shield_degrades_when_the_activation_read_back_is_unreadable( + project, tmp_path, monkeypatch +): + """The rc half of the same call. Any non-zero rc here is a fault rather than an + ABSENT answer, and the difference from `_shield_inherited_excludes` is the point: + that helper reads a key the operator may simply never have set, so rc 1 is real + news. This one asks about a key we have just written, where "there is no such key" + is not good news about it. + + The taxonomies must not be unified, and this test is what makes that concrete — + routing rc 1 here through the seed's ABSENT branch would report a working shield. + + Ablation: treat any rc as an answer. THE DELETION DOES NOT FALL THROUGH TO SUCCESS, + which is why the assertions below are on the wording rather than on + `reason is not None` — measured, not predicted: an unread stdout is `b""`, 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, claiming another + scope outranks us and naming `''` as what git reads. A reason-is-not-None assertion + would pass against that bug.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + activated = [] + + def refuse_after_activation(worktree, *args): + if args[:2] == ("config", "--worktree"): + activated.append(args) + return real(worktree, *args) + if activated and "--get" in args and "core.excludesFile" in args: + return subprocess.CompletedProcess( + args=["git", *args], + returncode=128, + stdout=b"", + stderr=b"fatal: bad config line 1\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", refuse_after_activation) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert activated, "the fake never saw the activation — it refused the seed read instead" + assert reason is not None + assert "would not confirm which excludes file now applies" in reason + assert "bad config line 1" in reason + + +def test_shield_dies_with_the_worktree(project, tmp_path): + """Lifetime, the other half of #384: `git worktree remove` deletes the whole + per-worktree gitdir, taking the private exclude AND the `config.worktree` that + points at it. The shield expires exactly when the thing it shields does. + + That is why this fix needs no remover. The old design had none either, and that + was the bug: `gc_run_worktrees` reclaimed the worktree and left the patterns in + the shared exclude forever — surviving `isolation` going back to `"none"`, and + the run, and the release. + + Ablation is git's own behavior, so the inverse is the check that matters: a + shield written to the shared exclude survives this removal by construction, and + this test would fail against it.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + + provision_worktree(wt, [get_profile("claude")], repo) + + private = _wt_private_exclude(wt) + assert private.is_file() # it really was there to be lost + gitdir = private.parent.parent + + verify.worktree_remove(repo, wt, force=True) + + assert not private.exists() + assert not gitdir.exists() # the whole .git/worktrees/, config.worktree too + + +def test_shield_refuses_to_enable_extension_over_core_worktree(project, tmp_path): + """`core.worktree` in the shared config is one of the two shapes git's own docs + (git-worktree(1), CONFIGURATION FILE) say must be moved into the main worktree's + `config.worktree` BEFORE `extensions.worktreeConfig` is enabled: enabling drops + the exception that confines those keys to the main worktree, so they would start + applying to every worktree. + + Rearranging an operator's repo layout is not something an installer may do + behind their back, so the shield degrades and the run continues unshielded. + Enabling anyway is the loud failure this refuses to risk. + + Ablation: delete the `core.worktree` branch in `_shield_enable_worktree_config` + and this fails — `worktreeConfig` appears in the shared config.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "core.worktree", str(repo)) + shared_exclude = repo / ".git" / "info" / "exclude" + before = shared_exclude.read_bytes() + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "core.worktree" in reason + # read as TEXT, not via `git config --get`: conftest's git() is check=True and + # an unset key exits 1, which would error the test rather than assert it. + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert not _wt_private_exclude(wt).exists() + assert shared_exclude.read_bytes() == before + + +def test_shield_refuses_to_enable_extension_over_core_bare(project, tmp_path): + """The second refused shape: `core.bare = true` in the shared config. Same + reasoning as its `core.worktree` sibling, different key, and it needs its own + case because only a TRUE value is disqualifying — `core.bare = false` is written + into every ordinary repo by `git init` and must not block the shield. + + Set after the worktree is mounted, and nothing below asks git about the repo + itself: a repo declaring itself bare answers most commands with a refusal, + which is precisely why git wants the key moved. + + Ablation: delete the `core.bare` branch and this fails; narrow the value check + to "is the key present" and every ordinary repo (`bare = false`) stops being + shielded, which the sibling tests above catch.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + shared_exclude = repo / ".git" / "info" / "exclude" + before = shared_exclude.read_bytes() + git(repo, "config", "core.bare", "true") + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "core.bare" in reason + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert shared_exclude.read_bytes() == before + + +def test_shield_refuses_a_repository_shared_between_os_users(project, tmp_path): + """A repository configured as shared between OS users is not supported (#384), and + is refused UP FRONT rather than shielded — loudly, and identically for every user. + + The refusal sits ABOVE the shield's lock, which is why the last four assertions + matter more than the first. A reason string alone cannot distinguish a clean + refusal from one that left state behind: that is exactly the gap that let round 7's + defect sit under a passing fault-injection test for two review rounds. So this pins + that NOTHING was created — no lock file for a peer's run to meet, no permanent + repo-format change, no private exclude, and the repository-wide exclude untouched. + + Set after the worktree is mounted, as the `core.bare` sibling above is: git honors + the key for files it creates, and nothing here needs it applied during the mount. + + Ablation: delete the `_shield_shared_repository` call in `_worktree_local_exclude` + and this fails. What the deletion falls through to was checked by RUNNING the + ablated helper rather than inferred from the first failing assertion, because + twice on this PR a deleted arm landed on an adjacent guard that also returned a + reason: it falls through to a clean SUCCESS — `reason is None`, the lock file + created, `worktreeConfig` written — so every assertion below is load-bearing and + none of them can pass against the bug.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + shared_exclude = repo / ".git" / "info" / "exclude" + before = shared_exclude.read_bytes() + git(repo, "config", "core.sharedRepository", "group") + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + # "shared between OS users" is wording ONLY this gate produces. The key name alone + # is not enough to identify the refusal: git's own rejection message names the key + # too (lowercased, `'core.sharedrepository'`), and it reaches a degrade reason from + # further down this function — see the sibling test on the parse. + assert reason is not None and "shared between OS users" in reason + assert "core.sharedRepository" in reason and "group" in reason + assert not (repo / ".git" / "bmad-loop-shield.lock").exists() + # read as TEXT, not via `git config --get`: conftest's git() is check=True and an + # unset key exits 1, which would error the test rather than assert it. + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert not _wt_private_exclude(wt).exists() + assert shared_exclude.read_bytes() == before + + +def test_shield_runs_in_a_repository_that_is_not_shared(project, tmp_path): + """The allowlist's accept side, which nothing else covers: `core.sharedRepository` + being PRESENT is not the refusal — being present with a value git resolves to + something other than "private" is. + + `umask` rather than `false` on purpose. Measured at git 2.20.4 and 2.55.0, git + compares this keyword with strcmp while it compares the booleans with strcasecmp, + so the two sides of the accept test are separate code paths and this is the one a + bool-only reading would drop. + + Ablation: refuse whenever the key is present (drop the value test) and this + fails — an ordinary repository stops being shielded.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "core.sharedRepository", "umask") + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert "/probe-384" in _wt_private_exclude(wt).read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + ("value", "private"), + [ + # keywords and booleans — `umask` by strcmp, the booleans by strcasecmp + ("umask", True), + ("false", True), + ("FALSE", True), + ("no", True), + ("off", True), + ("Off", True), + ("group", False), + ("all", False), + ("world", False), + ("everybody", False), + ("true", False), + ("yes", False), + ("on", False), + # PERM_UMASK, in every octal spelling of zero + ("0", True), + ("00", True), + ("0000", True), + # owner-only filemodes. 0711 is the row a "has no execute bits" reading gets + # wrong: git masks the value with 0666, which discards the execute bits. + ("0600", True), + ("0700", True), + ("0711", True), + # NOT owner-only. 0755 is the mirror row — its group/other READ bits survive + # the 0666 mask, so it is `everybody` despite looking like a private dir mode. + ("0755", False), + ("0640", False), + ("0660", False), + ("0666", False), + ("0604", False), + ("07777", False), + # the legacy 0/1/2 compatibility values, which git special-cases ahead of its + # filemode branch. 1 and 2 need no arm of their own here — neither satisfies + # `& 0600`, so they land on the same refusal. + ("1", False), + ("01", False), + ("2", False), + ("02", False), + # values git itself REJECTS (`git add` exits 128 at both versions). They are + # refused, like every other value that makes the repository unusable. + ("0400", False), + ("0200", False), + ("0600x", False), + ("banana", False), + (" umask", False), + # Python's `int(value, 8)` accepts these two and git REJECTS them, which is why + # the pattern is applied BEFORE the conversion rather than relying on the + # conversion to raise. This is the strictness that is load-bearing. + ("0o600", False), + ("0_600", False), + # deliberate FALSE REFUSALS: git's `strtol` accepts a leading `+` and leading + # whitespace, and measured, `+0600` really is private to git. The gate refuses + # in that direction on purpose — a false refusal is a reported skip. + ("+0600", False), + (" 0600", False), + # the empty value is PERM_UMASK, i.e. private, and is refused anyway: `--get` + # cannot distinguish it from a VALUELESS key, which is PERM_GROUP. Refusing + # the ambiguity is the caller's documented policy. + ("", False), + ], +) +def test_shared_repository_private_verdicts(value, private): + """git's `core.sharedRepository` verdicts, mirrored value by value. + + The pure-core layer for the octal support: `0600` is an owner-only filemode, not + a shared repository, so refusing it skipped the shield for a single-user repo + (#384, Codex round 20). Every row was measured at git 2.20.4 AND 2.55.0 — + byte-identical at both ends — by the mode of a loose object git writes under + `umask 077`, which is git's own answer rather than a reading of `setup.c`. + + Ablation, per row group: restore the old literal accept set + (`value in ("umask", "0") or value.lower() in ("false", "no", "off")`) and the + five owner-only octal rows fail; drop the `& 0066` mask and the six shared-octal + rows fail; drop the `& 0600` test and `0400`/`0200` fail; relax `fullmatch` to + `match` and `0600x` fails; drop the pattern and let `int(value, 8)` raise instead + and `0o600`/`0_600` fail.""" + assert install_mod._shared_repository_is_private(value) is private + + +def test_shield_runs_in_a_repository_with_an_owner_only_octal_mode(project, tmp_path): + """`core.sharedRepository = 0600` is a filemode granting no peer access at all — + a repository private to its owner, not one shared between OS users — so the + shield runs (#384, Codex round 20). + + The refusal is deliberately coarse everywhere else in this gate, but it may not + be coarse HERE: an owner-only octal mode is exactly the shape the refusal exists + to let through, and refusing it left a single-user repository's provisioned tool + files eligible for the unit's `git add -A` — the bug this whole branch is about, + reintroduced by the guard against a different one. + + `0600` rather than `0700`/`0711` because it is the value `git init --shared=0600` + stores verbatim, i.e. the one an operator actually ends up with. The sibling + parametrized test carries the rows that pin the MASK. + + Ablation: restore the old literal accept set and this fails — at the status + assertion as well as the reason, which is checked below rather than assumed.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "core.sharedRepository", "0600") + # non-vacuity: git stores the octal verbatim rather than normalizing it to a + # keyword, so the gate really is handed the string this test is about. + assert git(repo, "config", "--get", "core.sharedRepository") == "0600" + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert "/probe-384" in _wt_private_exclude(wt).read_text(encoding="utf-8") + # THE HARM, through git's own answer: with the shield skipped this file is + # untracked-and-visible, which is what reaches the unit's `git add -A`. + (wt / "probe-384").write_text("generated\n", encoding="utf-8") + assert "probe-384" not in git(wt, "status", "--porcelain", "-uall") + + +def test_shield_refuses_a_group_readable_octal_mode(project, tmp_path): + """`core.sharedRepository = 0640` is a filemode granting GROUP access, so it is + refused exactly like the `group` keyword — the octal support added for round 20 + accepts owner-only modes and must not widen past them. + + `0640` rather than `0666` on purpose: it is the row immediately across the + boundary from the accepted `0600`, so it fails first if the mask is loosened. + + Ablation: drop the `& 0066` mask from `_shared_repository_is_private` (accept any + octal git does not reject) and this fails — the shield proceeds in a repository + shared between OS users.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "core.sharedRepository", "0640") + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "shared between OS users" in reason + assert "0640" in reason + assert not (repo / ".git" / "bmad-loop-shield.lock").exists() + assert not _wt_private_exclude(wt).exists() + + +def test_shield_refuses_a_valueless_shared_repository_key(project, tmp_path): + """`sharedRepository` with NO value is git's `PERM_GROUP` — a shared repository — + and `--get` answers it rc 0 with a lone newline, byte-identical to an explicitly + EMPTY value, which git reads as `PERM_UMASK`, i.e. private. Measured at 2.20.4 and + 2.55.0 by the mode of a loose object git writes under `umask 077`: valueless gives + `r--r-----`, empty gives `r--------`. + + git exposes nothing that separates the two answers, so the gate refuses the + ambiguous one. That is the deliberate direction of error — a false refusal is a + reported skip, a false accept is the bug the gate exists to prevent — and it is + recorded here because the obvious "simplification" is to read the empty answer as + "not shared" and let the shared case through. + + Hand-written because `git config` cannot express a key with no value. Appended as + a fresh `[core]` section rather than edited into the existing one: a repeated + section is legal in git config, and it carries no path, so none of this PR's + Windows fixture hazards (a backslash is a config ESCAPE) apply. + + Ablation: add "" to the accepted values and this fails — the shield proceeds.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + config = repo / ".git" / "config" + text = config.read_text(encoding="utf-8") + config.write_text( + (text if text.endswith("\n") else text + "\n") + "[core]\n\tsharedRepository\n", + encoding="utf-8", + ) + # non-vacuity, and it names this test's own precondition: git must answer the key + # rc 0 with an EMPTY value. Were the hand-written line unparsed it would answer + # rc 1 (absent) and the refusal below would be testing nothing it claims to. + assert git(repo, "config", "--get", "core.sharedRepository") == "" + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "shared between OS users" in reason + assert not (repo / ".git" / "bmad-loop-shield.lock").exists() + assert not _wt_private_exclude(wt).exists() + + +def test_shield_reads_shared_repository_without_stripping_it(project, tmp_path): + """The gate removes `--get`'s TERMINATOR and nothing else. `git config --get` + returns edge whitespace VERBATIM (measured at 2.20.4 and 2.55.0: a quoted + `" umask"` comes back as b" umask\\n"), so `.strip()` would widen a value into the + accept set — round 15's defect at a second site in this same function, and live + rather than defensive: `rev-parse --absolute-git-dir` answers rc 0 for such a + repository, so the gate really is reached with this value in hand. + + The value is one git itself REJECTS (`fatal: bad boolean config value ... for + 'core.sharedrepository'`, and `git status` exits 128), which is why refusing is + the only defensible reading of it — but what is pinned here is the PARSE, not the + verdict on that value. + + No Windows skip: the fixture is a config value, not a path, and it carries no + backslash — the escape that made a hand-written config fixture unparseable on + Windows CI earlier in this PR. + + Ablation: use `.strip()` instead of `removesuffix("\\n")` and this fails — but NOT + in the shape the obvious note would claim, which is why the assertions below are + written the way they are. Measured with the ablation applied: the value reads as + `umask`, the gate lets it through, the shield takes the lock and then degrades one + step later with git's OWN rejection of the value ("could not enable + extensions.worktreeConfig ... fatal: bad boolean config value ' umask' for + 'core.sharedrepository'"). So `assert reason is not None` PASSES against the bug, + and so would a check for the key name — git lowercases it in that message, but a + test may not rest on that. What bites is the wording only this gate emits, plus + the lock file the refusal must never create.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + config = repo / ".git" / "config" + text = config.read_text(encoding="utf-8") + config.write_text( + (text if text.endswith("\n") else text + "\n") + '[core]\n\tsharedRepository = " umask"\n', + encoding="utf-8", + ) + # non-vacuity, through the same chokepoint the gate reads with, because conftest's + # git() strips its stdout and so cannot testify about edge whitespace at all + answer = install_mod.git_bytes( + repo, "config", "--file", str(config), "--get", "core.sharedRepository" + ) + assert answer.returncode == 0 and answer.stdout == b" umask\n" + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "shared between OS users" in reason + assert not (repo / ".git" / "bmad-loop-shield.lock").exists() + assert not _wt_private_exclude(wt).exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="a trailing space is not a legal Windows path") +def test_shield_keeps_edge_whitespace_in_the_common_dir(tmp_path): + """`rev-parse` terminates its answer with one newline; every other byte belongs to + the path — so the parse may remove only that terminator (#384, Codex round 15). + + A comment here used to justify `.strip()` on the grounds that "a final component is + `.git` or `worktrees/`". True for a NON-bare repo, false for a BARE one, whose + common dir IS the repository directory. Measured at 2.55.0, a worktree of a bare + repo at `…/common ` answers `--git-common-dir` = `…/common `, while + `--absolute-git-dir` stays safe at `…/common /worktrees/` because git sanitizes + the admin id — so exactly ONE of the two answers was exposed. + + THE HARM IS A DEFEATED SAFETY GATE, which is why this is a P1 and not a cosmetic + path bug: stripped, every later step points at `…/common`, which does not exist, + and `config --file /config --get core.bare` answers rc 1 — ABSENT, not + "unreadable" (round 10). So `core.bare = true` is MISSED and the shield proceeds to + make its permanent repo-format change on a bare repository. The lock's own + `mkdir(parents=True)` then creates the stripped directory as a side effect, which + is the visible symptom. + + The three assertions are one per consequence, and the sibling-directory one is what + makes the others non-vacuous: a refusal for the WRONG reason would still satisfy + the first two. + + Bare repo built by hand rather than through the `project` fixture, since the whole + point is a common dir that is not `/.git`. + + Ablation: restore `.strip()` on the two rev-parse answers and this fails — the + refusal is gone, `extensions.worktreeConfig` lands in the real config, and + `/common` exists.""" + bare = tmp_path / "common " # trailing space is the fixture + seed = tmp_path / "seed" + subprocess.run(["git", "init", "-q", "--bare", str(bare)], check=True) + subprocess.run(["git", "init", "-q", str(seed)], check=True) + for k, v in (("user.email", "t@e"), ("user.name", "t")): + subprocess.run(["git", "-C", str(seed), "config", k, v], check=True) + (seed / "f").write_text("x", encoding="utf-8") + subprocess.run(["git", "-C", str(seed), "add", "f"], check=True) + subprocess.run(["git", "-C", str(seed), "commit", "-qm", "c"], check=True) + subprocess.run( + ["git", "-C", str(seed), "push", "-q", str(bare), "HEAD:refs/heads/main"], check=True + ) + wt = tmp_path / "wt" + subprocess.run(["git", "-C", str(bare), "worktree", "add", "-q", str(wt), "main"], check=True) + # non-vacuity: git really does hand back the trailing space, so this fixture can + # express the bug at all + answered = subprocess.run( + ["git", "-C", str(wt), "rev-parse", "--git-common-dir"], + capture_output=True, + text=True, + check=True, + ) + assert answered.stdout.endswith(" \n"), repr(answered.stdout) + + reason = _worktree_local_exclude(wt, ["/probe-r15"]) + + assert reason is not None and "core.bare" in reason + assert "worktreeConfig" not in (bare / "config").read_text(encoding="utf-8") + assert not (tmp_path / "common").exists() # no stripped sibling was created + + +@pytest.mark.parametrize( + ("reported", "supported"), + [ + ("git version 2.20.0\n", True), # the boundary itself + ("git version 2.19.4\n", False), # one minor below it + ("git version 2.9.5\n", False), # numeric, not lexicographic: "9" > "20" as text + ("git version 2.44.0.windows.1\n", True), + ("git version 2.39.5 (Apple Git-154)\n", True), + ("git version 3.0\n", True), + ("", False), # nothing at all: a spawn that produced no stdout + ("fatal: not a git repository\n", False), + # No `git version` prefix. Refused deliberately: a bare-number answer is + # not this program's output, and the caller is about to make a permanent + # repo-format change on the strength of it. + ("2.55.0\n", False), + ], +) +def test_git_version_at_least_reads_only_a_git_version_line(reported, supported): + """The parse behind the shield's 2.20 gate. Unreadable answers must come back + False, because the caller reads False as "do not touch this repository" — an + optimistic parse is the only failure mode that costs anything.""" + assert _git_version_at_least(reported, (2, 20)) is supported + + +def test_shield_refuses_when_the_core_worktree_probe_cannot_answer(project, tmp_path, monkeypatch): + """A safety probe that could not be ANSWERED must not read as "that key is unset". + `core.worktree` is genuinely set here, and the shield must refuse exactly as it + does when it can see the key — because what it guards is irreversible: enabling + `extensions.worktreeConfig` drops the exception confining `core.worktree` to the + main worktree, after which it applies to every linked one (git-worktree(1)). + + THE ANSWER IS INJECTED, NOT THE ILLNESS — round 9's lesson, and the reason this + fixture is not a repo with a broken config. A genuinely malformed `.git/config` + fatals the caller's own earlier `rev-parse --absolute-git-dir` (measured 128 at + both 2.20.4 and 2.55.0), which takes the SILENT-SKIP arm above this branch, so + such a fixture would be vacuous. Keeping git healthy is also what leaves the harm + assertable through git itself once the gate is ablated. + + rc 128 is what git really answers for a config it cannot parse (measured at 2.20.4 + and 2.55.0); git-config(1) documents that case as ret=3 and prefaces its whole list + with "Some exit codes are:", so neither the docs nor the behavior support treating + every non-1 rc as an absence. + + Ablation: in `_shield_shared_config`, replace the raise with `return None` (the old + `else` semantics) and this fails — the gate opens and `extensions.worktreeConfig` + lands in the shared config of a repo that sets `core.worktree`.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "core.worktree", str(repo)) + real = install_mod.git_bytes + + def unanswerable_worktree_probe(worktree, *args): + # onto the core.worktree READ alone: everything else, including the sibling + # core.bare probe, runs for real so this pins THIS branch + if "--get" in args and "core.worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], + returncode=128, + stdout=b"", + stderr=b"fatal: config file was replaced mid-probe\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", unanswerable_worktree_probe) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "replaced mid-probe" in reason + assert "core.worktree" in reason # the reason names the question git could not answer + # read as TEXT, not `git config --get`: conftest's git() is check=True + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert not _wt_private_exclude(wt).exists() + + +def test_shield_refuses_when_the_core_bare_probe_cannot_answer(project, tmp_path, monkeypatch): + """The `core.bare` sibling of the probe above, and it needs its own case because + the two branches read their answers differently: `core.bare` refuses only on a + TRUE value, `core.worktree` on mere presence. An unanswerable rc must refuse in + both, and a fix applied to one arm only would leave this one open. + + `--type=bool` gives this probe a second way to fail that the plain read has not: + measured at 2.20.4 and 2.55.0, `--type=bool` over a non-bool value exits 128 + (2.20.4 words it "bad numeric config value", 2.55.0 "bad boolean config value" — + same rc). Note that no STATIC config value can reach that: a repo whose + `core.bare` is a non-bool fatals the caller's earlier `rev-parse` first (measured + 128 at both). What reaches here is a transient fault inside the caller's lock. + + Ablation: same one-line change in `_shield_shared_config` — the gate opens over a + repository that declares itself bare.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + shared_exclude = repo / ".git" / "info" / "exclude" + before = shared_exclude.read_bytes() + git(repo, "config", "core.bare", "true") + real = install_mod.git_bytes + + def unanswerable_bare_probe(worktree, *args): + if "--get" in args and "core.bare" in args: + return subprocess.CompletedProcess( + args=["git", *args], + returncode=128, + stdout=b"", + stderr=b"fatal: bare probe could not be answered\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", unanswerable_bare_probe) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "bare probe could not be answered" in reason + assert "core.bare" in reason + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert shared_exclude.read_bytes() == before + + +def test_shield_refuses_when_the_extension_probe_cannot_answer(project, tmp_path, monkeypatch): + """The third probe, whose unanswerable rc is harmful in the opposite direction to + its siblings'. Read as "not enabled" it does not open a safety gate — it makes the + shield WRITE, re-asserting a repo-format change over a repository whose flag is + already `true` and whose state we failed to read. + + The flag is genuinely on here, so the correct outcome is a degrade that touches + nothing: not knowing is not the same as knowing it is off. The write is forbidden + by tripwire rather than asserted on the config text, for the reason + `test_shield_reuses_already_enabled_extension` gives — `git config` rewriting + `true` over `true` leaves the file byte-identical, so a bytes comparison passes + with the fix removed. + + Ablation: same one-line change in `_shield_shared_config` — the probe's 128 reads + as "not enabled", the enable fires, and the fake raises.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "extensions.worktreeConfig", "true") + real = install_mod.git_bytes + + def unanswerable_extension_probe(worktree, *args): + if args[:1] == ("config",) and "extensions.worktreeConfig" in args and "--get" not in args: + raise AssertionError(f"wrote a repo-format change it could not read first: {args}") + if "--get" in args and "extensions.worktreeConfig" in args: + return subprocess.CompletedProcess( + args=["git", *args], + returncode=128, + stdout=b"", + stderr=b"fatal: extension probe could not be answered\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", unanswerable_extension_probe) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "extension probe could not be answered" in reason + assert "extensions.worktreeConfig" in reason + # the flag the repo already carried is untouched, and no rollback was attempted + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + assert "could NOT be rolled back" not in reason + assert not _wt_private_exclude(wt).exists() + + +def test_shield_refuses_to_enable_extension_over_old_git(project, tmp_path, monkeypatch): + """`extensions.worktreeConfig` and `git config --worktree` are both git 2.20. + Below that the write buys a PERMANENT repo-format change that shields nothing — + and git-worktree(1) says older git refuses a repository carrying the extension. + So the version is checked before any of it. + + The gate also sits above the two probes underneath it on purpose: `--type=` is + git 2.18, so on an older git the `core.bare` read exits non-zero and is read as + "not bare" — the safety gate opening silently. Refusing here keeps that pair + unreachable. + + Only the `version` call is faked; every other call runs against the real repo, + so the assertion below reads the actual shared config rather than a stub's log. + The enable is ALSO made to raise, because "the key is absent afterwards" would + hold if the write merely failed. + + Ablation: delete the version gate in `_shield_enable_worktree_config` and this + fails — the enable fires, the fake raises, and the key lands in the config.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + shared_before = (repo / ".git" / "info" / "exclude").read_bytes() + real = install_mod.git_bytes + + def ancient(worktree, *args): + if args == ("version",): + return subprocess.CompletedProcess( + args=["git", "version"], returncode=0, stdout=b"git version 2.19.4\n", stderr=b"" + ) + if args[:1] == ("config",) and "extensions.worktreeConfig" in args and "--get" not in args: + raise AssertionError(f"made a permanent format change on git 2.19.4: {args}") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", ancient) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "2.19.4" in reason and "git 2.20" in reason + # the repo's own format is untouched. Read as TEXT for the reason the sibling + # refusal tests give: conftest's git() is check=True and an unset key exits 1. + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert not _wt_private_exclude(wt).exists() + assert (repo / ".git" / "info" / "exclude").read_bytes() == shared_before + + +def test_shield_degrade_leaves_no_permanent_repo_format_change(project, tmp_path, monkeypatch): + """`extensions.worktreeConfig` is a PERMANENT repo-format change bmad-loop never + reverses, so it must not be paid for a shield that then degrades away. Round 5: + `_shield_enable_worktree_config` used to perform the enable itself, ahead of the + seed, the mkdir, the write and the activation — so every degrade below it left + the operator's repo marked forever for a shield that never applied. It now only + PROBES; the write happens one line above the activation. + + Driven through the seed fault the round-5 GitError fix introduced, because that + is the degrade path this reordering exists for — a fault the old code could not + even reach, since it swallowed instead of degrading. + + Read as TEXT rather than via `git config --get`, for the reason the sibling + refusal tests give: conftest's git() is check=True and an unset key exits 1. + + Ablation: move the enable back above the seed (into + `_shield_enable_worktree_config`, where it was) and this fails — + `worktreeConfig` is in the shared config after a degrade that shielded nothing.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", str(users)) + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + real = install_mod.git_bytes + + def unanswerable_excludes_read(worktree, *args): + if "core.excludesFile" in args and "--get" in args: + raise verify.GitError("git config did not answer") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", unanswerable_excludes_read) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None # it really did degrade + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + + +def test_shield_enables_the_extension_on_the_path_that_activates(project, tmp_path): + """The other half of the deferral: moving the enable down must not lose it. A + happy path still leaves the repo carrying the extension — that write is what + `git config --worktree` needs to exist at all, so without it the activation + fatals and the shield does nothing. + + The sibling above proves the flag is absent after a degrade; this proves the + reordering did not simply stop setting it. Neither alone distinguishes the fix + from a deletion.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + # ...and it bought a shield that actually holds + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + + +def test_shield_rolls_back_the_extension_when_activation_fails(project, tmp_path, monkeypatch): + """One of the two degrades that can still have made a permanent repo-format + change: the ACTIVATION's own failure, which happens one line after the enable. + Round 5 moved the enable down to close every path above it; Codex then pointed out + this one, and it is real — read-only `.git` and a lock on `config.worktree` both + reach it. So the arm rolls the flag back. (This docstring said "the one degrade + left" until round 10, which found the other: the ENABLE's own raise, covered by + `test_shield_rolls_back_an_enable_whose_git_faulted`.) + + Measured, not assumed: `--unset` removes the key *and* the now-empty + `[extensions]` section, and `git config --worktree` refuses again afterwards, so + the repo is genuinely returned to the state we found rather than cosmetically + tidied. + + Only `--worktree` writes are failed, so the enable itself succeeds — which is + what makes this the enable-then-fail ordering rather than a short-circuit. + + Ablation: drop the `--unset` rollback and this fails — `worktreeConfig` is left + in the shared config after a degrade that shielded nothing.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + real = install_mod.git_bytes + + def fail_worktree_writes(worktree, *args): + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_worktree_writes) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "read-only .git" in reason + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + # the rollback is silent when it works: the operator is told the shield failed, + # not handed a second fault that did not happen + assert "could NOT be rolled back" not in reason + + +def test_shield_reports_a_rollback_it_could_not_make(project, tmp_path, monkeypatch): + """If the rollback ALSO fails the operator has to be told, because the repo then + really does keep a permanent format change that shields nothing — the coherent + case, since a read-only `.git` fails both writes. Reported rather than raised: + this function is contracted never to propagate. + + Both faults are named in the one reason, so the second is not lost behind the + first. + + Ablation: drop the `undone.returncode != 0` branch and this fails — the reason + mentions only the activation, and the format change goes unreported.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + + def fail_worktree_writes_and_the_unset(worktree, *args): + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + if _is_unset(args): + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: could not lock\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_worktree_writes_and_the_unset) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None + assert "read-only .git" in reason # the activation fault + assert "could NOT be rolled back" in reason and "could not lock" in reason + assert "permanent format change" in reason + + +def test_shield_reports_a_rollback_whose_own_git_faulted(project, tmp_path, monkeypatch): + """The rollback's OWN git can raise, not just return non-zero — it is another + chokepoint call. `_shield_undo_extension` catches that and reports it, because it + is called from a function contracted never to propagate, and because a raise + escaping it would lose the activation fault that prompted the rollback. + + The coherent case rather than a contrived one: a dead git or a read-only `.git` + fails the unset for the same reason it failed the activation. + + Ablation: drop the `except GitError` in `_shield_undo_extension` and this fails — + the fault escapes into the caller's tail and the reason names neither the + activation fault nor the retained format change.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + + def fail_activation_and_raise_on_unset(worktree, *args): + if _is_unset(args): + raise verify.GitSpawnError("git could not spawn") + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_activation_and_raise_on_unset) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None + assert "read-only .git" in reason # the activation fault survived the rollback + assert "could NOT be rolled back" in reason and "could not spawn" in reason + assert "permanent format change" in reason + + +@pytest.mark.parametrize("fault", [verify.GitError, verify.GitSpawnError]) +def test_shield_rolls_back_an_enable_whose_git_faulted(project, tmp_path, monkeypatch, fault): + """The ENABLE's own raise, which is round 10 (#384, Codex P2). `git_bytes` fails + two ways and this call handled only the rc, so a fault left the permanent + repo-format change in place with no shield ever activated: the raise went to the + caller's tail, which returns a reason and cannot roll anything back. + + THE FAKE PERFORMS THE WRITE AND THEN RAISES, and that is the whole difference + between this test and a vacuous one. `git config` can be killed after it has + renamed the new config into place — the write landed, we never learned it — so + without the pass-through the flag would never have been set, "the flag is absent + afterwards" would hold trivially, and the assertion below would prove nothing. + That is the exact failure mode round 7 recorded, where a fault-injection test + asserting only the reason string sat on a live defect for two rounds. + + The `--unset-all` is deliberately let through to the real git, so the rollback + asserted here is the real one rather than the fake's. + + Ordering matters as much as the outcome: round 9 found that hoisting a rollback + out of the `with` passed all 53 shield tests, so the rollback is pinned INSIDE + the lock here too. + + Parametrized over both classes because they enter differently — a timeout as + `GitError`, a spawn failure as the `GitSpawnError` subclass. The message says + neither "timed out" (a lie for a spawn failure) nor "did not answer" (taken by + the excludes-read test, which could then not tell which call faulted). + + Ablation: delete the new `except GitError` around the enable and both cases fail — + the fault reaches the tail, no rollback runs, and the flag survives.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + events = [] + real_lock, real_git, real_undo = ( + install_mod.file_lock, + install_mod.git_bytes, + install_mod._shield_undo_extension, + ) + + @contextlib.contextmanager + def spy_lock(path, **kwargs): + events.append("lock-enter") + with real_lock(path, **kwargs): + yield + events.append("lock-exit") + + def enable_then_die(worktree, *args): + if ( + args[:1] == ("config",) + and "extensions.worktreeConfig" in args + and "--get" not in args + and not _is_unset(args) + ): + events.append("enable") + real_git(worktree, *args) # the rename into place DID land... + # non-vacuity, checked mid-flight: without this the harm below is trivial + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + raise fault("git config never reported back") # ...and then git was killed + return real_git(worktree, *args) + + def spy_undo(*a, **kw): + events.append("rollback") + return real_undo(*a, **kw) + + monkeypatch.setattr(install_mod, "file_lock", spy_lock) + monkeypatch.setattr(install_mod, "git_bytes", enable_then_die) + monkeypatch.setattr(install_mod, "_shield_undo_extension", spy_undo) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "never reported back" in reason + # THE HARM, and it is only assertable because the write above really happened: + # the permanent repo-format change is gone again + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + # names its own cause: a still-present flag would ALSO hold if the unset had failed + assert "could NOT be rolled back" not in reason + assert events == ["lock-enter", "enable", "rollback", "lock-exit"] + + +def test_shield_rollback_treats_an_absent_flag_as_completed(project, tmp_path): + """`--unset-all` of a key that is not there exits 5, and that is a COMPLETED + rollback, not a failed one. Round 10 made this reachable for the first time: the + enable's raise routes into the rollback without knowing whether anything was + written, so "nothing was" is now an ordinary outcome rather than an impossible + one, and reporting it as a failure would tell the operator their repository keeps + a format change it does not have. + + `--unset-all` rather than `--unset`, and the spelling is load-bearing because + git-config(1) gives rc 5 TWO meanings — "unset an option which does not exist" and + "unset/set an option for which multiple lines match". Measured at 2.20.4 and + 2.55.0, identical at both: `--unset` against a DOUBLED key exits 5 and removes + nothing, while `--unset-all` exits 0 and removes both lines. So under `--unset`, + "treat 5 as success" would have reported a clean rollback for a key that survived; + under `--unset-all` rc 5 can only mean "no line matched". + + Driven directly rather than through the shield: the caller reaches this state only + via an injected fault, and the claim under test is about the helper's own contract. + + Ablation: drop `5` from the success tuple and this fails — the helper returns the + "could NOT be rolled back" clause for a repository that carries nothing.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git_dir = Path(git(wt, "rev-parse", "--absolute-git-dir")).resolve() + common_dir = Path(git(wt, "rev-parse", "--git-common-dir")).resolve() + # the preconditions the clause depends on: no flag, and no sibling to decline for + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert not (common_dir / "config.worktree").exists() + + clause = _shield_undo_extension(wt, git_dir, common_dir) + + assert clause == "" + + +def test_shield_rollback_removes_every_line_of_a_doubled_flag(project, tmp_path): + """The other half of the `--unset-all` decision, and the one that makes the + spelling load-bearing rather than cosmetic. git-config(1) gives rc 5 two meanings, + and a key written on TWO lines is the second: measured at 2.20.4 and 2.55.0, + `--unset` against it exits 5 and removes NOTHING, while `--unset-all` exits 0 and + removes both. + + So under `--unset` the "treat 5 as success" this fix needs would report a clean + rollback for a flag that is still enabled — the repository keeps a permanent + format change while the operator is told it does not. `--unset-all` collapses rc 5 + to the single meaning "no line matched", which is what lets the sibling test above + treat it as success safely. The guarantee is then structural instead of resting on + an argument about which doubled values can reach here. + + Written by hand rather than through `git config`, which de-duplicates: two literal + lines is the state git itself warns about ("has multiple values"). No path appears + in the fixture, so the `as_posix()` rule for hand-written config does not apply. + + Ablation: change `--unset-all` back to `--unset` and this fails — the clause still + reads as a clean rollback while both lines survive in the config.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git_dir = Path(git(wt, "rev-parse", "--absolute-git-dir")).resolve() + common_dir = Path(git(wt, "rev-parse", "--git-common-dir")).resolve() + shared = repo / ".git" / "config" + shared.write_text( + shared.read_text(encoding="utf-8") + + "[extensions]\n\tworktreeConfig = true\n\tworktreeConfig = true\n", + encoding="utf-8", + ) + assert shared.read_text(encoding="utf-8").count("worktreeConfig") == 2 # non-vacuity + + clause = _shield_undo_extension(wt, git_dir, common_dir) + + assert clause == "" + # ...and the flag really is gone, which is what "" claimed + assert "worktreeConfig" not in shared.read_text(encoding="utf-8") + + +def test_shield_hedges_a_rollback_it_could_not_make_after_an_uncertain_enable( + project, tmp_path, monkeypatch +): + """When the ENABLE raised and the rollback then also failed, the reason may not + assert that this shield set the flag — because nothing may have been written at + all. That is the double fault round 10 introduced a path to: a spawn failure kills + the enable, so we never learn whether the config was replaced, and if the unset + fails too the operator is handed a clause about a format change that may not exist. + + The old wording said "extensions.worktreeConfig **was enabled for this shield** and + could NOT be rolled back", which is a claim this frame cannot support. Both clauses + now hedge it. No precision is lost that was really there: `needs_enable` is + probe-derived and already cannot tell "we enabled it" from "we and a concurrent run + both thought we did". + + The disclosure itself must survive the hedging — the operator still has to be told + the repository may keep a permanent format change — so this asserts the retained + clause AND the absence of the overclaim. + + Ablation: restore the unhedged wording in `_shield_undo_extension` and this fails — + the reason states as fact that this shield enabled the flag.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + + def die_on_enable_and_fail_the_unset(worktree, *args): + if _is_unset(args) and "extensions.worktreeConfig" in args: + return subprocess.CompletedProcess( + args=["git", *args], + returncode=1, + stdout=b"", + stderr=b"fatal: config is read-only\n", + ) + if args[:1] == ("config",) and "extensions.worktreeConfig" in args and "--get" not in args: + # no pass-through: the spawn never happened, so nothing was written + raise verify.GitSpawnError("git binary vanished") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", die_on_enable_and_fail_the_unset) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "git binary vanished" in reason + # the disclosure survives... + assert "could NOT be rolled back" in reason and "config is read-only" in reason + assert "permanent format change" in reason + # ...but it is hedged, because this frame cannot know the write ever landed + assert "was enabled for this shield" not in reason + assert "if this shield set the flag" in reason + + +def test_shield_does_not_claim_a_format_change_it_never_made(project, tmp_path, monkeypatch): + """The message-honesty half of the same fix. A spawn failure can kill the enable + before anything is written at all, and the rollback then finds nothing to undo — + so the reason must not tell the operator their repository keeps a permanent format + change. Before round 10 this path rendered as "extensions.worktreeConfig was + enabled for this shield and could NOT be rolled back (git exited 5)": a claim that + was false twice over, about a flag that was never set and a rollback that in fact + completed. + + The unset is let through to the real git precisely so the rollback SUCCEEDS at + finding nothing; the fault is confined to the enable. Both clauses of + `_shield_undo_extension` now hedge whether this shield set the flag, since + `needs_enable` is probe-derived and cannot tell "we enabled it" from "we and a + concurrent run both thought we did". + + Ablation: drop `5` from the success tuple in `_shield_undo_extension` and this + fails — the reason claims a permanent format change the repository does not carry.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + + def never_spawn_the_enable(worktree, *args): + # the ENABLE only — `--unset-all` names the same key, so it has to be excluded + # explicitly or the rollback would fault too and prove something else + if ( + args[:1] == ("config",) + and "extensions.worktreeConfig" in args + and "--get" not in args + and not _is_unset(args) + ): + raise verify.GitSpawnError("git binary vanished") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", never_spawn_the_enable) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "git binary vanished" in reason + # nothing was written, so nothing may be claimed + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + assert "could NOT be rolled back" not in reason + assert "permanent format change" not in reason + + +def test_shield_never_unsets_an_extension_the_repo_already_carried(project, tmp_path, monkeypatch): + """The rollback is gated on having enabled the flag IN THIS CALL, and that gate is + a safety property rather than a micro-optimization: a repo that already carries + `extensions.worktreeConfig` may have `config.worktree` files other worktrees + depend on, and unsetting it would stop git reading them. So an activation failure + against an already-enabled repo must leave the flag alone. + + The `--unset` is injected as a hard failure rather than asserted on the config + text, for the reason the already-enabled sibling test gives: the flag being + present afterwards would also hold if the unset had run and failed. Forbidding + the CALL is the only form that bites. + + Ablation: drop the `if needs_enable:` gate on the rollback and this fails — the + fake raises.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "extensions.worktreeConfig", "true") + real = install_mod.git_bytes + + def fail_worktree_writes_forbid_unset(worktree, *args): + if _is_unset(args) and "extensions.worktreeConfig" in args: + raise AssertionError(f"unset an extension this call did not enable: {args}") + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_worktree_writes_forbid_unset) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "read-only .git" in reason + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + + +def test_shield_never_unsets_an_extension_a_sibling_worktree_depends_on( + project, tmp_path, monkeypatch +): + """`needs_enable` is necessary and NOT sufficient (#384, Codex round 8). It records + what the PROBE saw, and the enable is an idempotent rc-0 no-op against a flag that + is already `true` — so two concurrent provisionings in one repository both read an + absent flag and both believe they own it. Whichever one's activation then fails + unsets a flag the OTHER's live shield depends on: git stops reading its + `config.worktree`, its `core.excludesFile` goes inert, and its shielded tool files + become stageable again mid-run. So the rollback also refuses when any + `config.worktree` that is not ours exists. + + THE FIRST TEST IN THIS SUITE WITH TWO WORKTREES IN ONE REPOSITORY, which is why the + finding survived eight rounds: the flag is repo-wide state, and a single-worktree + fixture cannot express a dependent. + + The interleaving is reproduced rather than mimed. The sibling's shield is run for + REAL — its `config.worktree` and private exclude are git's own work, not files + written by hand — and the flag is then unset to rewind the repository to what this + run's probe saw. That is exactly the state the race produces: A probes, B enables + and activates, A's own enable is a no-op, A's activation fails. + + The `--unset` is injected as a hard failure rather than asserted on the config + text, for the reason the sibling test above gives: the flag being present + afterwards would also hold if the unset had run and failed. Forbidding the CALL is + the only form that bites. + + Ablation: drop the dependents scan in `_shield_undo_extension` and this fails — the + fake raises. With the raise removed too, the last assertion is the harm: the + sibling's own `git status` starts showing the file its shield was hiding.""" + repo = project.project + sibling = tmp_path / "sibling" + wt = tmp_path / "wt" + verify.worktree_add(repo, sibling, "sib", "main") + verify.worktree_add(repo, wt, "feat", "main") + # the concurrent run that got there first, in full + assert _worktree_local_exclude(sibling, ["/probe-sibling"]) is None + sibling_gitdir = Path(git(sibling, "rev-parse", "--absolute-git-dir")).resolve() + assert (sibling_gitdir / "config.worktree").is_file() # non-vacuity: a real dependent + # rewind to what THIS run's probe saw, leaving the sibling's config.worktree behind + git(repo, "config", "--unset", "extensions.worktreeConfig") + real = install_mod.git_bytes + + def fail_worktree_writes_forbid_unset(worktree, *args): + if _is_unset(args) and "extensions.worktreeConfig" in args: + raise AssertionError(f"unset a flag a sibling worktree depends on: {args}") + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_worktree_writes_forbid_unset) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "read-only .git" in reason # our activation did fail + assert "LEFT enabled" in reason and str(sibling_gitdir) in reason + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + # THE HARM, asked of git rather than of the config text: the sibling's shield still + # holds. A rolled-back flag would make this file stageable again mid-run. + (sibling / "probe-sibling").write_text("noise\n", encoding="utf-8") + assert "probe-sibling" not in git(sibling, "status", "--short") + + +def test_shield_rolls_back_despite_its_own_partial_config_worktree(project, tmp_path, monkeypatch): + """The dependents scan excludes OUR OWN gitdir, and that exclusion is load-bearing + rather than tidiness: a `config.worktree` in it is the half-written product of the + very activation whose failure prompted the rollback — a timeout can leave one + behind — so counting it as a dependent would suppress every rollback rounds 6 and 7 + exist to make, and leave the operator's repo carrying a permanent format change for + a shield that never held. + + The fixture has to be hand-written — asking git for it would need the extension + already enabled, which makes `needs_enable` False and removes the rollback this + test is about — and `as_posix()` on the embedded value is load-bearing, not style. + WINDOWS CI CAUGHT THIS AND POSIX CANNOT: `str(WindowsPath)` renders + `C:\\Users\\…`, and a backslash in a git config VALUE is an escape sequence, so + `\\U`/`\\A`/`\\T` make the file unparseable. Once this call's own enable turns the + extension on, every git invocation from this worktree — including the rollback's + `--unset` — then dies with `fatal: bad config line 2 in …/config.worktree`, the + flag survives, and the test fails for a reason the fixture invented (measured; + git accepts forward slashes on Windows and needs no escaping for them). The + PRODUCTION write is unaffected: it passes the path to `git config` as an argument + and git escapes it itself, storing `C:\\\\Users\\\\…` and reading it back intact. + + Ablation: include our own `git_dir` in the scan and this fails — `worktreeConfig` + stays in the shared config and the reason claims a sibling depends on it.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git_dir = Path(git(wt, "rev-parse", "--absolute-git-dir")).resolve() + # what a timed-out activation leaves behind: our own pointer, already written + (git_dir / "config.worktree").write_text( + f"[core]\n\texcludesFile = {(git_dir / 'info' / 'exclude').as_posix()}\n", + encoding="utf-8", + ) + real = install_mod.git_bytes + + def fail_worktree_writes(worktree, *args): + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_worktree_writes) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "read-only .git" in reason + assert "LEFT enabled" not in reason # our own file is not a dependency on anyone + # Separates the two ways the flag could survive, because the flag-text assertion + # below cannot: "the guard declined" (above) and "the unset itself failed" (here). + # Without this the Windows fixture fault above presented as a silent failure of + # the property under test, and cost several rounds of measurement to attribute. + assert "could NOT be rolled back" not in reason + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + + +def test_shield_rollback_scan_fault_is_reported_not_raised(project, tmp_path, monkeypatch): + """`_shield_undo_extension` is contracted never to raise — every caller is already + reporting a degrade, and a raise escaping it would replace the activation fault + AND the retained-flag disclosure with the caller's generic tail reason, losing + exactly what rounds 6 and 7 added. + + The dependents scan added in round 8 broke that promise for two fault shapes its + `except OSError` did not name (#384, CodeRabbit round 9). On the 3.11/3.12 floor + `Path.resolve()` raises `RuntimeError` for a symlink loop rather than `OSError` — + the caller's own tail already carries `RuntimeError` for precisely that reason, so + the gap was internally inconsistent as well as wrong. + + Driven at the helper rather than through `_worktree_local_exclude`, which is the + lowest layer that can catch this regression: the shield resolves its own `git_dir` + (also under `worktrees/`) before the scan runs, so a fault injected end-to-end + would fire on the wrong call and prove something else. + + Ablation: narrow the tuple back to `OSError` and this fails — the RuntimeError + escapes instead of being reported.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "extensions.worktreeConfig", "true") + git_dir = Path(git(wt, "rev-parse", "--absolute-git-dir")).resolve() + common = Path(git(wt, "rev-parse", "--git-common-dir")).resolve() + real_resolve = Path.resolve + + def loop(self, *a, **kw): + if self.parent.name == "worktrees": # the scan's own resolve, not ours + raise RuntimeError(f"Symlink loop while resolving {self}") + return real_resolve(self, *a, **kw) + + monkeypatch.setattr(Path, "resolve", loop) + + clause = _shield_undo_extension(wt, git_dir, common) + + assert "Symlink loop" in clause # reported... + assert "LEFT enabled" in clause # ...and it took the conservative branch + monkeypatch.undo() + # the conservative default really is conservative: the flag is still there, which + # is the cosmetic residue this trades for never silently un-shielding a sibling + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + + +def test_shield_rolls_back_inside_the_lock(project, tmp_path, monkeypatch): + """The ROLLBACK has to happen while the lock is still held, and that placement is + load-bearing rather than incidental: released first, a second run probes in the + gap, sees the flag this run is about to unset, skips its own enable as redundant, + activates against it — and then this run's `--unset` lands. That is the round-8 + race rebuilt out of the fix for it. + + The sibling lock test cannot pin this: its activation SUCCEEDS, so no rollback + ever runs and `activation < lock_exit` is all it can show. Ablation proved the gap + was real — hoisting the rollback out of the `with` passed all 53 shield tests + (#384, CodeRabbit round 9). + + Ablation: move the rollback below the `with` block and this fails — the rollback + is recorded after the lock's exit.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + events = [] + real_lock, real_git, real_undo = ( + install_mod.file_lock, + install_mod.git_bytes, + install_mod._shield_undo_extension, + ) + + @contextlib.contextmanager + def spy_lock(path, **kwargs): + events.append("lock-enter") + with real_lock(path, **kwargs): + yield + events.append("lock-exit") + + def fail_activation(worktree, *args): + if "--worktree" in args: + events.append("activation") + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real_git(worktree, *args) + + def spy_undo(*a, **kw): + events.append("rollback") + return real_undo(*a, **kw) + + monkeypatch.setattr(install_mod, "file_lock", spy_lock) + monkeypatch.setattr(install_mod, "git_bytes", fail_activation) + monkeypatch.setattr(install_mod, "_shield_undo_extension", spy_undo) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "read-only .git" in reason + assert events == ["lock-enter", "activation", "rollback", "lock-exit"] + # and the rollback it performed inside the lock was the real one + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + + +def test_shield_takes_the_repo_scoped_lock(project, tmp_path, monkeypatch): + """The probe→enable→activate→rollback sequence is ONE transaction, serialized per + repository — the ownership guard above covers a flag enabled outside bmad-loop's + discipline, and this covers the ordinary case of two bmad-loop runs. + + Three properties, and each one is a separate way to get this wrong: + + - the lock lives in the COMMON dir, so every worktree of a repo contends on one + file (a per-worktree gitdir would give each run its own lock and exclude + nothing); + - it is a dedicated file rather than the config or the exclude, per `file_lock`'s + own contract — the lock rides an open fd's inode, and an `atomic_replace` would + swap that inode out from under later acquirers; + - it is taken BEFORE the extension probe and held past the ACTIVATION, since the + race is the window between the probe's answer and the activation's outcome. + + That third bullet is the whole span only in company: this test's activation + SUCCEEDS, so no rollback runs in it and `activation < lock-exit` is the strongest + ordering it can witness. The rollback half — released only after the `--unset` — + is a separate property with its own test, `test_shield_rolls_back_inside_the_lock`, + which had to exist because hoisting the rollback out of the `with` passed all 53 + shield tests including this one (#384, CodeRabbit round 9). A success-path ordering + test cannot pin a rollback-path ordering property. + + Ordering is recorded from the calls themselves rather than asserted on the lock's + existence: a lock taken after the probe, or released before the activation, leaves + exactly the window this closes. + + Ablation: drop the `with` and this fails — `file_lock` is never entered.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + events = [] + real_lock, real_git = install_mod.file_lock, install_mod.git_bytes + + @contextlib.contextmanager + def spy_lock(path, **kwargs): + events.append(("lock-enter", path)) + with real_lock(path, **kwargs): + yield + events.append(("lock-exit", path)) + + def spy_git(worktree, *args): + events.append(("git", args)) + return real_git(worktree, *args) + + monkeypatch.setattr(install_mod, "file_lock", spy_lock) + monkeypatch.setattr(install_mod, "git_bytes", spy_git) + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + kinds = [kind for kind, _ in events] + assert kinds.count("lock-enter") == 1 and kinds.count("lock-exit") == 1 + common = Path(git(wt, "rev-parse", "--git-common-dir")).resolve() + assert events[kinds.index("lock-enter")][1] == common / "bmad-loop-shield.lock" + probe = next( + i + for i, (kind, a) in enumerate(events) + if kind == "git" and "extensions.worktreeConfig" in a + ) + activation = next( + i for i, (kind, a) in enumerate(events) if kind == "git" and "--worktree" in a + ) + assert kinds.index("lock-enter") < probe + assert activation < kinds.index("lock-exit") + # the residue this buys, disclosed in the docs: a zero-length file inside `.git`, + # so nothing the operator's own `git add -A` can ever see + lock_file = common / "bmad-loop-shield.lock" + assert lock_file.is_file() and lock_file.stat().st_size == 0 + assert git(repo, "status", "--short") == "" + + +def test_shield_degrades_when_the_lock_cannot_be_taken(project, tmp_path, monkeypatch): + """Taking the lock can fail, and on Windows that is a routine outcome rather than a + contrived one: POSIX `flock` blocks indefinitely, but `msvcrt.locking` gives up + after ~10 s and raises `OSError` — and this holder spans ~7 git spawns, each bounded + by `[limits] git_timeout_s`, so a real contender can outlast it. + + The acquisition's `OSError` is caught at the lock rather than left to the function's + tail purely so the operator is told which step failed; the tail would return a + reason too, just not one naming the lock. What must hold either way is that a + shield that never started leaves NOTHING behind — no permanent repo-format flag, no + activated pointer. + + Ablation: drop the `except OSError` around the acquisition and this fails — the + tail's generic "could not update the worktree-local git exclude" reason says + nothing about a lock.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + + @contextlib.contextmanager + def unavailable(path, **kwargs): + # the shape `msvcrt.locking` raises when the ~10 s blocking retry runs out + raise OSError(11, "Resource deadlock avoided") + yield # pragma: no cover — unreachable; keeps this a generator function + + monkeypatch.setattr(install_mod, "file_lock", unavailable) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None + assert "shield lock" in reason and "Resource deadlock avoided" in reason + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + git_dir = Path(git(wt, "rev-parse", "--absolute-git-dir")) + assert not (git_dir / "config.worktree").exists() # nothing was activated + # ...and the reason is honest about the consequence: the shield is off, so the + # files it would have hidden really are stageable. Reported, never silent. + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + assert "probe-384" in git(wt, "status", "--short") + + +def test_shield_reuses_already_enabled_extension(project, tmp_path, monkeypatch): + """A repo that already carries the extension is used as found — the second + isolated run in a repo must not re-assert a permanent format change, and must + not re-run the refusal gates against a repo whose format it did not change. + + Injected as a hard failure rather than an equality assertion on the shared + config: `git config` rewriting `true` over `true` leaves the file byte-identical, + so a bytes comparison would pass with the early return deleted. Forbidding the + WRITE is the only form of this that bites. + + Ablation: drop the already-true early return in `_shield_enable_worktree_config` + and this fails — the enable call fires and the fake raises.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + git(repo, "config", "extensions.worktreeConfig", "true") + real = install_mod.git_bytes + + def no_reenable(worktree, *args): + # reads of the key must still pass through, or nothing works at all + if args[:1] == ("config",) and "extensions.worktreeConfig" in args and "--get" not in args: + raise AssertionError(f"re-enabled an extension already on: {args}") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", no_reenable) + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert "/probe-384" in _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + + +def test_shield_enables_extension_when_only_the_global_config_claims_it( + project, tmp_path, monkeypatch +): + """`extensions.*` is repo-format state git honors ONLY from the repository's own + config, so the "is it already enabled" question has exactly one legitimate place + to be asked. Asked unscoped, a `worktreeConfig = true` in the operator's + ~/.gitconfig answers it — the shield concludes the repo is ready, skips the + write, and the activation then dies with git's own `--worktree cannot be used + with multiple working trees` — a repo one write away from being shielded, + skipped over instead. + + The global value is planted through GIT_CONFIG_GLOBAL, pinned around the shield + call alone for the reason the XDG test gives at length: an env pin that outlives + the code under test changes what git thinks of files already checked out. + + Ablation: drop `--file ` from the extension probe and this fails — the + global value is believed, the extension never reaches the repo config, and the + activation returns a reason instead of None.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + fake_global = tmp_path / "gitconfig-with-the-extension" + fake_global.write_text("[extensions]\n\tworktreeConfig = true\n", encoding="utf-8") + + with monkeypatch.context() as pinned: + pinned.setenv("GIT_CONFIG_GLOBAL", str(fake_global)) + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is None + # the repo's OWN format was changed, i.e. the global claim was not mistaken + # for one about this repository + assert "worktreeConfig" in (repo / ".git" / "config").read_text(encoding="utf-8") + # ...and the shield it unlocks actually holds, which is what the unscoped probe + # cost: without the write above, `config --worktree` cannot store the pointer. + assert "/probe-384" in _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + + +def test_shield_seeds_users_excludesfile(project, tmp_path): + """A worktree-scoped `core.excludesFile` SHADOWS the operator's own — git reads + the key from the most specific scope that sets it and does not concatenate + across scopes (verified) — so their patterns are copied into the private file + when it is created. Without the copy, activating the shield silently un-ignores, + inside the worktree, everything they ignore globally, and the unit's + `git add -A` commits it: a shield that creates the leak it exists to plug. + + Asserted through git rather than the file's content alone: the content could be + right while the activation pointed somewhere else. + + Ablation: make `_shield_inherited_excludes` return "" and this fails — + `mine.log` comes back untracked in the worktree.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", str(users)) + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "mine.log" in lines and "/probe-384" in lines + (wt / "mine.log").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + + +@pytest.mark.skipif( + sys.platform == "win32", reason="POSIX-only: Windows strips a trailing space from a filename" +) +def test_shield_seeds_an_excludesfile_whose_path_has_edge_whitespace(project, tmp_path): + """Round 5 (#384, Codex P2): the same leak as the encoding bug, through the PATH + rather than the content. A `.strip()` on git's answer destroyed a legal POSIX + path — `git config --type=path --get` returns leading and trailing whitespace + VERBATIM (git writes such a value quoted, so it round-trips; measured at 2.20.4 + and 2.55.0) — so the stripped path read absent via `is_file()`, the seed came + back empty, and the activation then SHADOWED the excludes it had failed to copy. + Silent: `_shield_inherited_excludes` returned empty rather than raising, and the + caller only journals a non-None reason. + + `-z` is the fix: it terminates the value with NUL, so the path survives whatever + whitespace it carries. + + One filename covers BOTH sides of the strip — `" my-global-ignores "` has a + leading and a trailing space — so a half-fix (`rstrip` only, say) still fails. + + Both halves asserted, as in the non-UTF-8 sibling below: the private file's + content (the mechanism) and git's own answer inside the worktree (the harm). + + Ablation: restore `.strip()` in place of the `-z` split in + `_shield_inherited_excludes` and this fails — `mine.log` comes back untracked.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / " my-global-ignores " + users.write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", str(users)) + # git really did keep the whitespace, so the seed below has something to lose. + # Read from the config FILE, not through `git config --get`: conftest's git() + # strips its stdout, which would defeat the very check being made here. + assert f'"{users}"' in (repo / ".git" / "config").read_text(encoding="utf-8") + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "mine.log" in lines and "/probe-384" in lines + (wt / "mine.log").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + + +@pytest.mark.skipif( + sys.platform == "win32", reason="POSIX-only: a 0xff byte cannot be a Windows filename" +) +def test_shield_preserves_a_non_utf8_excludesfile(project, tmp_path): + """THE round-4 regression (#384, Codex P1): the seed is a VERBATIM BYTE COPY, so + an operator's excludes file that is not UTF-8 survives it intact. + + A `read_text(encoding="utf-8")` inside a handler naming `UnicodeError` made one + non-UTF-8 byte anywhere in their file collapse the WHOLE seed to empty — and the + activation that follows then SHADOWED the excludes it had just failed to copy, + because git takes `core.excludesFile` from the most specific scope that sets it + and never concatenates across scopes. So `git add -A` committed a file the + operator had told git to ignore: the exact leak the seed exists to prevent, + caused by the seed. An exclude file holds path patterns and POSIX paths are + arbitrary bytes, so a legacy 8-bit encoding here is ordinary, not exotic. + + Both halves are asserted because either alone would pass against a different + bug: the private file's BYTES (the mechanism — a lossy copy is still a copy) and + git's own answer inside the worktree (the harm). + + Ablation: restore `source.read_text(encoding="utf-8")` in + `_shield_inherited_excludes` and this fails — the seed comes back empty and + `secret-\\377` shows up untracked in the worktree, ready for `git add -A`.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_bytes(b"secret-\xff\nplain.log\n") + git(repo, "config", "core.excludesFile", str(users)) + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert _wt_private_exclude(wt).read_bytes() == b"secret-\xff\nplain.log\n/probe-384\n" + (wt / os.fsdecode(b"secret-\xff")).write_text("noise\n", encoding="utf-8") + (wt / "plain.log").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + + +def test_shield_degrades_when_the_users_excludesfile_cannot_be_read(project, tmp_path, monkeypatch): + """An excludes file that EXISTS but cannot be read must skip the shield, not + activate over patterns it never copied. Absent is silent (the common case, and + there is nothing to shadow); unreadable is a degrade, because the shadow is real. + + The other half of the round-4 fix, and untested in either direction before it: + `OSError` was swallowed beside the decode fault, so an EACCES/EIO on their file + produced the identical silent empty seed — and then the shadow. + + Injected at `Path.read_bytes` rather than `chmod(0o000)`, for a reason the + assertions depend on: git runs as this same user, so a file WE cannot read is + one git cannot read either, and "their ignores still apply" — the property that + makes the shadow a harm — would be untestable. With the fault injected the file + stays readable to git, so the third assertion is git's own answer. (It also + sidesteps a root-owned CI runner ignoring the mode bits, the reason the sibling + write-fault test injects too.) + + The last assertion is the non-vacuity check: without it this passes just as well + if the shield had silently excluded everything. + + Ablation: put `OSError` back in `_shield_inherited_excludes`'s except tuple and + this fails — `reason` comes back None, the activation shadows their file, and + `mine.log` shows up untracked.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", str(users)) + real_read_bytes = Path.read_bytes + + def unreadable(self): + if self == users: + raise PermissionError(13, "Permission denied", str(users)) + return real_read_bytes(self) + + monkeypatch.setattr(Path, "read_bytes", unreadable) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + monkeypatch.undo() # the assertions below do their own file and git I/O + assert reason is not None and "Permission denied" in reason + assert not _wt_private_exclude(wt).exists() # nothing to point a shadowing key at + (wt / "mine.log").write_text("noise\n", encoding="utf-8") + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + status = git(wt, "status", "--short") + assert "mine.log" not in status # their ignore was never shadowed + assert "probe-384" in status # ...and the shield really was skipped + + +def test_shield_reseeds_after_an_interrupted_creation(project, tmp_path, monkeypatch): + """A creation that died between `touch()` and the write must not leave a + placeholder that the NEXT attempt reads as authoritative. + + `atomic_write_bytes` "leaves the original untouched and removes the temp" — and the + original here is the zero-byte file `touch()` just made, so an ENOSPC in between + leaves it behind. That attempt is harmless by itself: the degrade arm returns + above the `config --worktree`, and an unactivated private exclude does nothing. + The harm is deferred to the next one — an empty file taken as authoritative skips + the inherited-excludes seed and THEN activates a shadowing `core.excludesFile`, + reintroducing through the back door the exact leak + `test_shield_seeds_users_excludesfile` exists to plug. + + Two attempts against the helper directly, because the engine cannot produce this + sequence today: every re-mount of a unit path runs `discard_worktree`, whose + `worktree_prune` deletes the whole per-worktree gitdir — placeholder included — + and when that best-effort prune fails, `git worktree add` refuses the stale path + outright and the unit defers before provisioning runs. What this pins is the + helper's OWN contract, so a future caller cannot inherit the defect. Do not + "simplify" the size check away on the grounds that nothing reaches it. + + The zero-byte assertion between the attempts is not decoration: without it this + passes just as well if the first attempt left no file at all, i.e. whenever the + placeholder this test is about never existed. + + Ablation: restore `existed = exclude.is_file()` and this fails — `mine.log` is + missing from the private exclude and comes back untracked in the worktree.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", str(users)) + + def boom(*a, **kw): + raise OSError("no space left on device") + + # patched at install's OWN binding, as everywhere else in this file: the write + # goes through atomic_write_bytes (#375, #384 round 4) on an mkstemp fd via + # os.fdopen, so a Path.write_bytes patch never fires and would pass vacuously. + # The NAME matters as much as the binding — left pointing at atomic_write_text + # this patch became a silent no-op and the test failed on the degrade assertion. + monkeypatch.setattr(install_mod, "atomic_write_bytes", boom) + assert _worktree_local_exclude(wt, ["/probe-384"]) is not None + monkeypatch.undo() # the assertions below do their own file and git I/O + assert _wt_private_exclude(wt).stat().st_size == 0 # the placeholder touch() left + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "mine.log" in lines and "/probe-384" in lines + (wt / "mine.log").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + + +def test_shield_reprovision_does_not_duplicate_patterns(project, tmp_path): + """Provisioning the SAME worktree twice must leave the private exclude + byte-identical. The dedupe is what makes that true, and it compares the + already-present lines against the patterns being added — so both sides have to + be the same type. Left as `str` against a `bytes` set (the shape the round-4 + bytes conversion invites), every pattern reads as absent and is re-appended on + every single re-provision, growing the file without bound. + + Nothing pinned this before: `test_shield_reseeds_after_an_interrupted_creation` + does run the helper twice, but the second run sees a ZERO-BYTE file, i.e. the + create path both times — it never exercises the dedupe at all. + + Through `provision_worktree` rather than the helper, because the real pattern + set is what a re-provision re-offers, and `on_degraded` is collected so a + surprise degrade cannot make the two runs match by both doing nothing. + + Ablation: drop the `os.fsencode` and compare `str` patterns against the bytes + `present` set — this fails, with every tool pattern appearing twice.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + msgs: list[str] = [] + + provision_worktree(wt, [get_profile("claude")], repo, on_degraded=msgs.append) + first = _wt_private_exclude(wt).read_bytes() + provision_worktree(wt, [get_profile("claude")], repo, on_degraded=msgs.append) + + assert msgs == [] + assert _wt_private_exclude(wt).read_bytes() == first + lines = first.splitlines() + assert lines and len(set(lines)) == len(lines) # nor duplicated within one run + + +def test_shield_dedupe_does_not_split_on_non_git_line_breaks(project, tmp_path): + """The dedupe splits the existing content into lines the way GIT does, which is + the second thing the bytes conversion bought (#384 round 4). + + `str.splitlines()` breaks on `\\x0b`, `\\x0c`, `\\x1c`, `\\x1d`, `\\x1e` and + `\\x85` as well as on newlines; git treats none of those as a line boundary, and + every one of them is a legal byte in a POSIX filename. So a legitimate pattern + containing one fragmented into two wrong dedupe keys, the identical pattern then + read as absent, and it was appended a second time. `bytes.splitlines()` splits on + `\\n`, `\\r` and `\\r\\n` only — git's own boundary set (it also strips a trailing + `\\r`, which is why `\\r` costs nothing here). + + Asserted on the bytes rather than through `git status`: the subject is which + dedupe KEY the pattern produces, and a `\\x0c` in a filename is POSIX-only + while this fault is not. + + Ablation: restore `set(existing.splitlines())` over decoded text and this fails — + the seeded pattern fragments into `weird`/`pattern`, the identical pattern reads + as absent, and the file comes back carrying it twice.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_bytes(b"weird\x0cpattern\n") + git(repo, "config", "core.excludesFile", str(users)) + + assert _worktree_local_exclude(wt, ["weird\x0cpattern", "/probe-384"]) is None + + assert _wt_private_exclude(wt).read_bytes() == b"weird\x0cpattern\n/probe-384\n" + + +def test_shield_appends_a_pattern_an_inherited_negation_would_cancel(project, tmp_path): + """A pattern the file already CONTAINS can still be ineffective: gitignore's rule + is LAST MATCH WINS, so a `!` line below it cancels it (#384, Codex round 17). + + The plain set-membership dedupe therefore declined to append the shield's own + pattern, and the provisioned tool files stayed stageable — SILENTLY, because + nothing failed. That is why the assertions here go through git rather than through + the return value: `_worktree_local_exclude` answers None both with the bug and + with the fix, so a `reason` assertion cannot bite at all. Same shape as the + fault-injection test that sat on a live defect for two rounds by asserting only + the reason string. + + Measured, the negation need not repeat the positive's spelling — `!.claude/skills` + cancels `/.claude/skills` too. What does NOT cancel it is a negation leaving the + directory itself excluded (`!*.md` below it, or `!/.claude`, which re-includes only + the parent): git never descends into an excluded directory. The fix is deliberately + conservative across that line, appending a harmless duplicate in those cases rather + than reimplementing git's matcher, so this test pins the defeating shape only. + + Ablation (run): restore `settled = set(existing.splitlines())` and this fails at the + status assertion with `?? .claude/skills/tool.md`.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_bytes(b"/.claude/skills\n!/.claude/skills\n") + git(repo, "config", "core.excludesFile", str(users)) + + assert _worktree_local_exclude(wt, ["/.claude/skills"]) is None + + # non-vacuity: the operator's negation really did reach the private file, so the + # fixture expresses the scenario rather than merely naming it + assert b"!/.claude/skills\n" in _wt_private_exclude(wt).read_bytes() + # THE HARM, through git's own answer. A substring rather than whole-worktree + # cleanliness: that form has already failed on Windows CI in this PR over unrelated + # CRLF churn, and the subject here is this one path. + (wt / ".claude" / "skills").mkdir(parents=True, exist_ok=True) + (wt / ".claude" / "skills" / "tool.md").write_text("generated\n", encoding="utf-8") + assert "tool.md" not in git(wt, "status", "--porcelain", "-uall") + + +def test_shield_seeds_a_relative_excludesfile_resolved_like_git(project, tmp_path, monkeypatch): + """`--type=path` expands `~` and stops there — a RELATIVE `core.excludesFile` + comes back from git verbatim. Git resolves such a value against the worktree's + top level; `Path(value)` resolves it against whatever directory the orchestrator + happens to have been launched from. + + The miss is silent, which is what makes it worth a test: `is_file()` comes back + false, the seed returns "", and the activation below then SHADOWS the operator's + real excludes file — so their patterns stop applying inside the worktree and the + unit's `git add -A` stages what they told git to ignore. + + `monkeypatch.chdir` is load-bearing, not hygiene. Run from the worktree, the + unfixed code resolves the same relative path by accident and this test passes + against the bug it exists to catch. + + Ablation: drop the `is_absolute()` branch and this fails — `mine.log` is missing + from the private exclude and comes back untracked in the worktree.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + (wt / "ignores").mkdir() + (wt / "ignores" / "global").write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", "ignores/global") + # anywhere that is NOT the worktree, and where no `ignores/global` exists + monkeypatch.chdir(tmp_path) + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "mine.log" in lines and "/probe-384" in lines + (wt / "mine.log").write_text("noise\n", encoding="utf-8") + # `ignores/` is the fixture's own scaffolding, not the subject — it is untracked + # either way, so exclude it rather than let it mask the assertion. + assert "mine.log" not in git(wt, "status", "--short") + + +def test_shield_seeds_xdg_default_when_unset(project, tmp_path, monkeypatch): + """With `core.excludesFile` unset git falls back to `$XDG_CONFIG_HOME/git/ignore` + (gitignore(5)) — a real file on plenty of developer boxes and shadowed just as + hard. The probe has to reproduce that fallback because git cannot be asked for + it: `config --get` answers "unset", not "here is the default I would use". + + That is true of the fallback FILE only, and the distinction is load-bearing now + that the sibling limb depends on it: git CAN be asked where its own `$HOME` is, + because `--type=path` interpolates a leading `~/` through the same + `getenv("HOME")`. This limb needs no such probe — `XDG_CONFIG_HOME` is read from + the same environment git reads it from — but see + `test_shield_resolves_the_home_fallback_through_git_not_python` for the one that + does, and why Python's answer is wrong there. + + The environment is pinned three ways, and each one is load-bearing: + XDG_CONFIG_HOME so the file under test is this test's, GIT_CONFIG_GLOBAL and + GIT_CONFIG_NOSYSTEM so a developer box whose own `~/.gitconfig` sets + `core.excludesFile` reaches the fallback branch at all instead of passing + through the branch above and never testing this one. + + That pinning is scoped to the probe rather than the whole test, because + switching the system config off mid-test changes what git thinks of files + already on disk. Git for Windows ships `core.autocrlf = true` in its SYSTEM + config, so `worktree_add` above checks out CRLF; with `GIT_CONFIG_NOSYSTEM` + still set, the status below re-reads those same files with autocrlf off and + every tracked one reads as modified. The env is what the probe needs, not + what the assertions need — so it ends with the probe. + + Ablation: return "" when the key is unset (drop the XDG branch) and this fails.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + xdg = tmp_path / "xdg" + (xdg / "git").mkdir(parents=True) + (xdg / "git" / "ignore").write_text("xdg-ignored.tmp\n", encoding="utf-8") + + with monkeypatch.context() as pinned: + pinned.setenv("XDG_CONFIG_HOME", str(xdg)) + pinned.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / "no-such-gitconfig")) + pinned.setenv("GIT_CONFIG_NOSYSTEM", "1") + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert "xdg-ignored.tmp" in _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + (wt / "xdg-ignored.tmp").write_text("noise\n", encoding="utf-8") + # The exclude is activated through a worktree-scoped `core.excludesFile`, i.e. + # repo-local config — so this holds without the env pinning, which is the point. + assert git(wt, "status", "--short") == "" + + +def test_shield_resolves_the_home_fallback_through_git_not_python(project, tmp_path, monkeypatch): + """With `core.excludesFile` AND `XDG_CONFIG_HOME` unset, git's fallback is + `$HOME/.config/git/ignore` — and WHOSE `$HOME` is a real question. This branch + used to spell it `Path.home()`, which is wrong on Windows. + + Git resolves it with `getenv("HOME")` on every platform + (`path.c::xdg_config_home`, semantics unchanged 2.20.0 → 2.55). Python does not: + `ntpath.expanduser` reads `USERPROFILE` first and never consults `HOME` at all, + while Git for Windows DERIVES `HOME` in-process (`compat/mingw.c`) preferring + `HOMEDRIVE`+`HOMEPATH` over `USERPROFILE`. So the two genuinely disagree whenever + `HOME` is set (Git Bash and MSYS2 set it) or the home share is a network drive. + The miss is SILENT — the wrong path is simply not a file, so the seed comes back + empty and the activation then shadows the operator's real global ignores, which + is #384's own harm reached through the platform split. + + The env makes both halves explicit: `HOME` is the answer git must give, + `USERPROFILE` the wrong one Windows' Python would give. On POSIX those two agree + by construction, so the divergence is SIMULATED by pointing `Path.home()` at the + wrong directory — which is what makes this bite on every platform instead of + waiting for Windows CI, the way this shield's other platform bugs had to. The + wrong home carries a real ignore file of its own so that seeding the wrong one is + distinguishable from seeding nothing. + + Ablation: restore `Path.home() / ".config"` for the no-XDG limb and this fails — + `home-ignored.tmp` is missing from the private exclude, `wrong-home.tmp` is in + it, and `git status` stops hiding the file the operator's real home names.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + home = tmp_path / "githome" + (home / ".config" / "git").mkdir(parents=True) + (home / ".config" / "git" / "ignore").write_text("home-ignored.tmp\n", encoding="utf-8") + wrong = tmp_path / "python-home" + (wrong / ".config" / "git").mkdir(parents=True) + (wrong / ".config" / "git" / "ignore").write_text("wrong-home.tmp\n", encoding="utf-8") + + with monkeypatch.context() as pinned: + pinned.delenv("XDG_CONFIG_HOME", raising=False) + pinned.setenv("HOME", str(home)) + pinned.setenv("USERPROFILE", str(wrong)) + pinned.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / "no-such-gitconfig")) + pinned.setenv("GIT_CONFIG_NOSYSTEM", "1") + pinned.setattr(Path, "home", staticmethod(lambda: wrong)) + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + seeded = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "home-ignored.tmp" in seeded + assert "wrong-home.tmp" not in seeded + # The activation is repo-local config, so this holds with the env restored. + (wt / "home-ignored.tmp").write_text("noise\n", encoding="utf-8") + assert "home-ignored.tmp" not in git(wt, "status", "--porcelain", "-uall") + + +def test_shield_degrades_when_git_will_not_resolve_its_home_directory( + project, tmp_path, monkeypatch +): + """A probe that does not ANSWER is not "there is no fallback" — the same + absent/unknown split this function's docstring is built on, at the newest limb. + + `HOME` unset makes the probe exit 128, and so does a transient fault; git's own + message is version-dependent, so the two cannot be told apart without asserting + on wording this suite has already been burned by. Guessing "no fallback" is the + SILENT direction — seed nothing, then activate over whatever git does read — so + the non-zero rc is funnelled into `GitError` and the caller degrades with a + reason instead. The cost is that a genuinely `HOME`-less environment skips the + shield rather than proceeding, and that direction is reported. + + The flag assertion is the second half: the seed read runs ABOVE the enable, so a + fault here must leave no permanent repo-format change behind. + + Ablation: return an empty seed instead of raising on the probe's non-zero rc and + this fails — `reason` comes back None and `git status` stops showing the probe + file, i.e. the shield reports success while shadowing the operator's excludes.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + real = install_mod.git_bytes + seen: list[tuple[str, ...]] = [] + + def fail_home_probe(worktree, *args): + # A PREFIX predicate, not tuple membership: the key travels both as the `-c` + # assignment and as the bare `--get` argument (#384, round 10's de-fanged + # fakes). Recording it is what proves this faulted the probe and not the + # seed read, whose argv is otherwise byte-identical. + if any(a.startswith("bmadloop.xdghomeprobe") for a in args): + seen.append(args) + return subprocess.CompletedProcess( + args=list(args), + returncode=128, + stdout=b"", + stderr=b"fatal: failed to expand user dir in: '~/.config/git/ignore'\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_home_probe) + + with monkeypatch.context() as pinned: + pinned.delenv("XDG_CONFIG_HOME", raising=False) + pinned.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / "no-such-gitconfig")) + pinned.setenv("GIT_CONFIG_NOSYSTEM", "1") + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert seen, "the fake never saw the home probe — it faulted a different arm" + assert reason is not None + assert "could not resolve its own home directory" in reason + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + assert "probe-384" in git(wt, "status", "--porcelain", "-uall") + + +def test_shield_seeds_a_relative_xdg_config_home_resolved_like_git(project, tmp_path, monkeypatch): + """The relative-path defect of the `core.excludesFile` branch, at the XDG fallback + branch (#384, Codex round 12). This branch exists to REPRODUCE git's fallback, so + it has to reproduce git's resolution too. + + A relative `XDG_CONFIG_HOME` is invalid per the XDG base-directory spec, which says + an implementation "should consider the path invalid and ignore it". Git does not + ignore it. Measured at 2.20.4 and 2.55.0: git honors the value and resolves it + against the worktree's TOP LEVEL — and measured from a SUBDIR to separate + top-level from cwd, `git -C /sub` with `XDG_CONFIG_HOME=rel` read + `/rel/git/ignore` rather than `/sub/rel/git/ignore`. So "git ignores an + invalid value" is the plausible guess this test exists to refute. + + `monkeypatch.chdir` is load-bearing here for the same reason as in the + `core.excludesFile` sibling: run from inside the worktree, the unfixed code + resolves the same relative path BY ACCIDENT and passes against the bug. + + The env pinning is scoped to the probe, not the test — `GIT_CONFIG_NOSYSTEM` + suppresses Git for Windows' `core.autocrlf`, and leaving it set across the status + below makes every tracked file read as modified. + + Ablation: drop the `is_absolute()` branch from the XDG arm and this fails — + `xdg-relative.tmp` is missing from the private exclude and comes back visible to + git in the worktree, which is the harm: the activation shadows the file git really + reads, so patterns the operator set are switched off inside the unit.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + (wt / "relxdg" / "git").mkdir(parents=True) + (wt / "relxdg" / "git" / "ignore").write_text("xdg-relative.tmp\n", encoding="utf-8") + # non-vacuity: the key really is unset, so this run reaches the XDG arm at all + assert "excludesFile" not in (repo / ".git" / "config").read_text(encoding="utf-8") + # anywhere that is NOT the worktree, and where no `relxdg/git/ignore` exists + monkeypatch.chdir(tmp_path) + + with monkeypatch.context() as pinned: + pinned.setenv("XDG_CONFIG_HOME", "relxdg") + pinned.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / "no-such-gitconfig")) + pinned.setenv("GIT_CONFIG_NOSYSTEM", "1") + assert _worktree_local_exclude(wt, ["/probe-r12"]) is None + + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "xdg-relative.tmp" in lines and "/probe-r12" in lines + (wt / "xdg-relative.tmp").write_text("noise\n", encoding="utf-8") + # THE HARM, in git's own words. One filename rather than whole-worktree + # cleanliness: `relxdg/` is this fixture's own untracked scaffolding. + assert "xdg-relative.tmp" not in git(wt, "status", "--short") + + +def test_shield_honors_an_explicitly_empty_excludesfile(project, tmp_path, monkeypatch): + """An EXPLICITLY EMPTY `core.excludesFile` is an ANSWER, not an unset key: it is the + operator saying "no excludes file at all", and git honors that literally — no + patterns, and NO fallback to `$XDG_CONFIG_HOME/git/ignore`. Reading it as unset + (#384, Codex round 8) imported the XDG file's patterns and ACTIVATED them, which is + the mirror image of every other finding in this family: instead of shadowing + patterns it failed to copy it OVER-ignores, so session-created files the operator + deliberately stopped ignoring go silently missing from the unit's `git add -A` and + from the story's commit. Same silent-file-loss class as #384 itself, inverted. + + Measured at git 2.55.0 on a fixture built for exactly this: `-z --type=path --get` + answers rc 0 with a lone NUL, `git check-ignore -v` then exits 1 against a name the + XDG file lists, and `git status` shows it untracked. The mechanism is in git's + source rather than inferred — `dir.c::setup_standard_excludes` guards the XDG + fallback on `if (!excludes_file)`, a NULL POINTER, while an empty value resolves + through `interpolate_path("")` to a non-NULL empty string. Byte-identical guard at + v2.20.0, this shield's floor, and at master. It is undocumented: `core.adoc` says + only "Defaults to $XDG_CONFIG_HOME/git/ignore" — the same standing as the relative- + value behavior the sibling test above pins. + + `GIT_CONFIG_NOSYSTEM` is deliberately NOT pinned, unlike the XDG sibling: a + repo-LOCAL key already outranks a global one, so there is nothing to suppress, and + pinning it would suppress Git-for-Windows' `core.autocrlf` and make unrelated + tracked files read as modified. The assertions name one file each instead of + demanding whole-worktree cleanliness. + + Ablation: restore `and raw` on the rc branch of `_shield_inherited_excludes` (the + pre-round-8 condition) and this fails — `xdg-ignored.tmp` is seeded into the private + exclude and disappears from git's status. Deleting the `if not raw:` arm alone does + NOT reproduce the bug: `os.fsdecode(b"")` makes `Path(".")`, which resolves to the + worktree directory and reads `is_file()` false, so the seed comes back empty + anyway. The defect is the ROUTING of this answer into the XDG branch.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + xdg = tmp_path / "xdg" + (xdg / "git").mkdir(parents=True) + (xdg / "git" / "ignore").write_text("xdg-ignored.tmp\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", "") + # non-vacuous both ways: git really did write an EMPTY value (not a quoted one, and + # not a deleted key), and the XDG file it must NOT reach for really does exist and + # really does carry the pattern the assertions below look for. + assert re.search(r"excludesFile = *\n", (repo / ".git" / "config").read_text(encoding="utf-8")) + assert (xdg / "git" / "ignore").read_text(encoding="utf-8") == "xdg-ignored.tmp\n" + + with monkeypatch.context() as pinned: + pinned.setenv("XDG_CONFIG_HOME", str(xdg)) + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + assert "xdg-ignored.tmp" not in lines # switched-off patterns were not imported + assert "/probe-384" in lines # ...and the shield's own pattern still is + (wt / "xdg-ignored.tmp").write_text("noise\n", encoding="utf-8") + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + status = git(wt, "status", "--short") + # THE HARM, in git's own words: the file stays visible to `git add -A`, exactly as + # it does in the main checkout. The shield did not quietly re-ignore it. + assert "xdg-ignored.tmp" in status + assert "probe-384" not in status + + +def test_shield_config_fault_skips_shield_entirely(project, tmp_path, monkeypatch): + """When the activation fails, the shield stops there. It must NEVER fall back to + the repository-wide exclude: that fallback is #384 itself, trading one reported + degrade for permanent, silent, unreviewable damage to the operator's checkout. + + The fault is name-filtered — only `config --worktree` calls fail — so everything + before it runs for real and this pins the LAST step rather than a short-circuit + somewhere earlier. Returned as a non-zero rc rather than raised, because that is + the shape git actually produces and the shield reads rc, not exceptions. + + Ablation: add any shared-exclude fallback on this path and the last two + assertions fail.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() + real = install_mod.git_bytes + + def fail_worktree_writes(worktree, *args): + if "--worktree" in args: + return subprocess.CompletedProcess( + args=["git", *args], returncode=1, stdout=b"", stderr=b"fatal: read-only .git\n" + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fail_worktree_writes) + + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "read-only .git" in reason + assert shared.read_bytes() == before + # and nothing else the MAIN checkout consults changed either: a file created + # there afterwards is still visible to git + (repo / "written-after-the-run.tmp").write_text("x\n", encoding="utf-8") + assert "written-after-the-run.tmp" in git(repo, "status", "--short") + + +def test_shield_main_checkout_degrades(project): + """Handed a MAIN checkout there is nothing to scope to — its gitdir IS the + common dir — so the only exclude file on offer is the shared one. That is the + #384 write, so the shield refuses and says why rather than falling back. + + Not a hypothetical caller: `provision_worktree` is handed plain directories and + project roots by tests and by non-isolated paths, and the pre-fix helper wrote + the repo-wide exclude for every one of them. + + Ablation: restore the common-dir target and this fails — the reason is None and + `/probe-384` lands in the shared exclude.""" + repo = project.project + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() + + reason = _worktree_local_exclude(repo, ["/probe-384"]) + + assert reason is not None and "not a linked worktree" in reason + assert shared.read_bytes() == before + + +@pytest.mark.skipif(sys.platform == "win32", reason="newline is a legal POSIX path byte") +def test_shield_survives_a_newline_in_the_repository_path(tmp_path): + """A repository directory carrying a NEWLINE must still get its shield. + + `git rev-parse` separates its answers with a newline, so asking ONE call for both + `--absolute-git-dir` and `--git-common-dir` read a legal path byte as a record + delimiter: measured on git 2.55, the two answers below split into FOUR entries and + the helper returned a degrade instead. Degrading is not a safe default here — + `provision_worktree` has already copied the tool files by the time the shield runs, + so the unit's `git add -A` commits them into the story's merge. + + The degrade was also a FALSE one, which is why the assertion that matters is git's + own answer rather than the private file's content: git honors every step of this at + such a path. `config --worktree core.excludesFile` round-trips the newline (escaped + as `\\n` in `config.worktree`) and the pattern then applies. + + A fresh repo rather than the `project` fixture, whose path has no newline. The + worktree itself may sit at a plain path — the private gitdir inherits the newline + through the repo, which is where `--absolute-git-dir` picks it up. + + Ablation: ask for both dirs in one `rev-parse` again and this fails on the first + assertion, with "could not read this worktree's git dirs".""" + repo = tmp_path / "re\npo" + repo.mkdir() + git(repo, "init", "-q", "-b", "main") + git(repo, "config", "user.email", "test@test") + git(repo, "config", "user.name", "test") + (repo / "seed.txt").write_text("x\n", encoding="utf-8") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "seed") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + + assert _worktree_local_exclude(wt, ["/probe-384"]) is None + + assert "/probe-384" in _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" + assert shared.read_bytes() == before # ------------------------------------------------- local exclude, best-effort (issue #359) @@ -1471,10 +4065,14 @@ def test_worktree_local_exclude_degrades_on_symlink_loop_resolve(project, monkey 3.11/3.12 CI legs — so the tail's except tuple must name RuntimeError or the fault escapes a function documented as best-effort. - The `project` MAIN CHECKOUT (not a linked worktree) is the subject on purpose: - `git rev-parse --git-common-dir` answers a plain checkout with a relative - ".git" and a linked worktree with the main repo's ABSOLUTE .git, so the plain - checkout is the only shape that reaches the `.resolve()` branch at all. + Both dirs are resolved before they are compared (a symlinked repo path is + absolute but not canonical, and an unresolved comparison would read a main + checkout as a linked worktree), so the injected fault fires on the common dir + every shape has. The `project` MAIN CHECKOUT is the subject only because it + needs no worktree to mount. + + Not vacuous through the main-checkout refusal that also returns a reason: the + assertion is on the LOOP's message, which only the propagating fault produces. Ablation: drop `RuntimeError` from the tail's except tuple and this fails — the RuntimeError propagates out of the call instead of becoming a reason.""" @@ -1495,94 +4093,123 @@ def unresolvable(self, *a, **kw): assert not exclude.is_file() or "/probe-359" not in exclude.read_text(encoding="utf-8") -def test_worktree_local_exclude_degrades_on_write_fault(project, monkeypatch): +def test_worktree_local_exclude_degrades_on_write_fault(project, tmp_path, monkeypatch): """A write fault on the exclude file itself (e.g. a read-only .git) degrades to a reason instead of propagating — this pins the guard reaching the LAST statement of the tail, not just the early resolve/mkdir steps. Injected rather than chmod: a root-owned CI runner ignores a read-only bit. + A real LINKED worktree is the subject here and in the rest of this block: since + #384 the tail's write only happens for one (a main checkout is refused before + it), so a main-checkout subject would degrade for the wrong reason and pass + without ever reaching the statement under test. + Ablation: drop the tail's `try` (or `OSError` from its tuple) and this fails — the OSError propagates out of `_worktree_local_exclude`.""" + wt = tmp_path / "wt" + verify.worktree_add(project.project, wt, "feat", "main") def boom(*a, **kw): raise OSError("read-only .git") # patched at install's OWN binding: the exclude write goes through - # atomic_write_text (#375), which writes via os.fdopen on an mkstemp fd, so a - # Path.write_text/Path.open patch never fires and would pass vacuously. - monkeypatch.setattr(install_mod, "atomic_write_text", boom) + # atomic_write_bytes (#375, #384 round 4), which writes via os.fdopen on an + # mkstemp fd, so a Path.write_bytes/Path.open patch never fires and would pass + # vacuously — and so would a patch left on the atomic_write_TEXT this replaced. + monkeypatch.setattr(install_mod, "atomic_write_bytes", boom) - reason = _worktree_local_exclude(project.project, ["/probe-359"]) + reason = _worktree_local_exclude(wt, ["/probe-359"]) assert reason is not None and "read-only .git" in reason -def test_worktree_local_exclude_degrades_on_undecodable_exclude(project): - """A REAL fault, no injection needed: an exclude file that is not UTF-8 makes - `read_text` raise UnicodeDecodeError — a ValueError subclass, so neither the - OSError nor the RuntimeError arm covers it. The bytes must survive untouched; - rewriting someone's legacy-encoded exclude would be worse than skipping it. +def test_worktree_local_exclude_appends_to_a_non_utf8_private_exclude(project, tmp_path): + """REPURPOSED, and the reversal is the fix (#384 review round 4). This case used + to assert that a private exclude which is not UTF-8 DEGRADES the shield away, + because `read_text` raised UnicodeDecodeError on it. The payload is bytes + end-to-end now, so those bytes survive untouched *and* the shield still applies + — strictly better than the old policy, which preserved the file by giving up on + shielding the worktree at all. - Ablation: drop `UnicodeError` from the tail's except tuple and this fails — - it propagates (it is not an OSError). Narrowing that member to - `UnicodeDecodeError` does NOT fail this test; that ablation belongs to the - encode sibling below, which is how the two halves stay independently - pinned.""" - exclude = project.project / ".git" / "info" / "exclude" + The legacy bytes are asserted byte-identical in place, as before: rewriting + someone's legacy-encoded exclude would still be worse than skipping it. + + Ablation: restore `exclude.read_text(encoding="utf-8")` for the re-read and this + fails — UnicodeDecodeError comes back as a degrade reason and `/probe-359` never + reaches the file.""" + wt = tmp_path / "wt" + verify.worktree_add(project.project, wt, "feat", "main") + exclude = _wt_private_exclude(wt) exclude.parent.mkdir(parents=True, exist_ok=True) exclude.write_bytes(b"\xff\xfe legacy-encoded\n") - reason = _worktree_local_exclude(project.project, ["/probe-359"]) - - assert reason is not None - assert exclude.read_bytes() == b"\xff\xfe legacy-encoded\n" - + assert _worktree_local_exclude(wt, ["/probe-359"]) is None -def test_worktree_local_exclude_degrades_on_unencodable_pattern(project): - """The codec fault has a WRITE direction too, and it is not the same exception: - `read_text` raises UnicodeDecodeError, `write_text` raises UnicodeEncodeError. - Neither is an OSError and they share no subclass but `UnicodeError`, so naming - only the decode half leaves the tail escaping on the encode half. + assert exclude.read_bytes() == b"\xff\xfe legacy-encoded\n/probe-359\n" + (wt / "probe-359").write_text("noise\n", encoding="utf-8") + assert git(wt, "status", "--short") == "" - Reachable, not theoretical: `provision_worktree` builds a pattern per - `seed_globs` match via `rel.as_posix()`, so a repo file whose name is not valid - UTF-8 arrives surrogate-escaped and cannot be written back as UTF-8. - REAL fault, no injection. The surrogate is written literally rather than via - `os.fsdecode(b"...\xff...")`: that helper decodes with surrogatepass on - Windows, which rejects a lone invalid byte outright and would raise in the - test's own setup. "\\udcff" is exactly what POSIX surrogateescape yields, and - strict UTF-8 refuses to encode it on every platform. +@pytest.mark.skipif( + sys.platform == "win32", + reason="fsencode uses utf-8/surrogatepass on Windows, which encodes any surrogate", +) +def test_worktree_local_exclude_degrades_on_unencodable_pattern(project, tmp_path): + """RE-POINTED (#384 review round 4) at the one shape that still raises. The + encode fault used to be reachable from a real filename: `provision_worktree` + builds a `seed_globs` pattern via `rel.as_posix()`, a non-UTF-8 name arrived + surrogate-escaped, and `write_text` could not encode it back. The payload is + bytes now and `os.fsencode` is surrogateescape's inverse, so THAT name + round-trips to its exact original bytes and no longer degrades anything — + `test_shield_preserves_a_non_utf8_excludesfile` covers the improvement. + + What remains is a pattern carrying a surrogate surrogateescape never produces: + a hand-authored `"\\ud800"` in a config string (`worktree_seed`, a plugin's + `seed_globs`) rather than a decoded filename. `os.fsencode` rejects it on POSIX, + because surrogateescape only round-trips \\udc80-\\udcff. Windows is skipped + rather than adapted: its filesystem codec is utf-8/surrogatepass, which encodes + every surrogate, so there is no encode fault to pin there at all. + + `"\\udcff"` — what this test used to use — is deliberately NOT the subject any + more: it is the case that now succeeds. - Ablation: narrow the tail's tuple back to `UnicodeDecodeError` and this fails - — UnicodeEncodeError propagates.""" - pattern = "/vendor/weird-\udcff-name" + Ablation: drop `UnicodeError` from the tail's except tuple and this fails — + UnicodeEncodeError propagates out of a function contracted never to.""" + pattern = "/vendor/weird-\ud800-name" + wt = tmp_path / "wt" + verify.worktree_add(project.project, wt, "feat", "main") - reason = _worktree_local_exclude(project.project, [pattern]) + reason = _worktree_local_exclude(wt, [pattern]) assert reason is not None and "surrogates not allowed" in reason -def test_worktree_local_exclude_short_write_leaves_exclude_intact(project, monkeypatch): +def test_worktree_local_exclude_short_write_leaves_exclude_intact(project, tmp_path, monkeypatch): """#375: a fault PARTWAY THROUGH the write must leave the operator's exclude - byte-identical, not truncated. `write_text` opens "w" (truncate-then-write) + byte-identical, not truncated. `write_bytes` opens "wb" (truncate-then-write) and this is a read-modify-REWRITE carrying their content in `prefix`, so a direct write left the file cut mid-content while the reason still said "could not update". The surviving tail is a valid git pattern and a prefix of the intended one — cut one character in and the last line is `/`, which - excludes the whole worktree. Blast radius is the MAIN repo: a linked - worktree's --git-common-dir points at the main .git. + excludes the whole worktree. - The fault is injected at `Path.open`, NOT at `Path.write_text`: patching - write_text means the file is never opened, so the truncation cannot happen + Blast radius is smaller since #384 (the file is this worktree's own, not the + main repo's shared exclude) but the fault is not: the content carried through + `prefix` is the operator's global excludes, copied in when the private file was + created, and `/` still swallows the whole worktree. + + The fault is injected at `Path.open`, NOT at `Path.write_bytes`: patching + write_bytes means the file is never opened, so the truncation cannot happen and the test would pass against the very bug it exists to catch. Going - through the real `open(mode="w")` is the whole point — that is what + through the real `open(mode="wb")` is the whole point — that is what truncates. - Ablation: swap `atomic_write_text` back to a direct `exclude.write_text` and + Ablation: swap `atomic_write_bytes` back to a direct `exclude.write_bytes` and this fails — the file comes back truncated.""" - exclude = project.project / ".git" / "info" / "exclude" + wt = tmp_path / "wt" + verify.worktree_add(project.project, wt, "feat", "main") + exclude = _wt_private_exclude(wt) exclude.parent.mkdir(parents=True, exist_ok=True) exclude.write_text("# OPERATOR'S OWN\n/secret-local\n", encoding="utf-8") before = exclude.read_bytes() @@ -1620,7 +4247,7 @@ def opener(file, *a, **kw): monkeypatch.setattr(io, "open", opener) - reason = _worktree_local_exclude(project.project, ["/probe-359"]) + reason = _worktree_local_exclude(wt, ["/probe-359"]) monkeypatch.undo() assert reason is not None and "No space left" in reason @@ -1652,24 +4279,75 @@ def test_worktree_local_exclude_undecodable_git_output_degrades(tmp_path, monkey bin_dir = tmp_path / "stubbin" bin_dir.mkdir() stub = bin_dir / "git" - # Emits a path carrying byte 0xff, exactly what a repo whose directory name - # is not valid UTF-8 makes `git rev-parse --git-common-dir` print. The path is - # rooted in tmp_path, NOT a literal /tmp: the helper mkdir(parents=True)s the - # common dir it is handed, so a hardcoded path would create real directories - # in the shared system /tmp on every run, outside pytest's cleanup. - common = os.fsencode(str(tmp_path)) + b"/repo-\377-name/.git" + # Emits paths carrying byte 0xff, exactly what a repo whose directory name is + # not valid UTF-8 makes `git rev-parse` print. Rooted in tmp_path, NOT a + # literal /tmp: the helper mkdir(parents=True)s the gitdir it is handed, so a + # hardcoded path would create real directories in the shared system /tmp on + # every run, outside pytest's cleanup. + root = os.fsencode(str(tmp_path)) + b"/repo-\377-name/.git" + gitdir = root + b"/worktrees/wt" # distinct from the common dir: a LINKED worktree + # SINGLE-QUOTED into the script. Unquoted, a temp root carrying whitespace # (TMPDIR, --basetemp, a username with a space) word-splits into a second # printf argument, and `printf '%s\n'` repeats its format per argument — so # the helper is handed a path with an embedded NEWLINE and mkdir(parents=True)s # it as a sibling of the basetemp pytest reaps. Measured with a spaced # --basetemp: it leaked such a directory and the test still passed. - stub.write_bytes(b"#!/bin/sh\nprintf '%s\\n' '" + common.replace(b"'", b"'\\''") + b"'\n") + def sq(raw): + return b"'" + raw.replace(b"'", b"'\\''") + b"'" + + # Dispatches on the subcommand, because the shield asks git several questions + # now and a stub that answered them all identically would not get past the + # first. `shift 2` drops the `-C ` every call carries. The version + # clears the 2.20 gate and the extension reads as already enabled, so the stub is + # never asked to change repo state. + # + # `rev-parse` dispatches one level further, on the FLAG, because the shield asks + # for the two dirs in two calls — a newline is a legal byte in a POSIX path, so it + # cannot serve as a record separator between them. Answering both here regardless + # of the flag is what real git does not do, and it would hand the helper one + # two-line "path" for each dir, making them equal and reading as a main checkout. + # + # `core.excludesFile` is asked TWICE with byte-identical argv — once to seed the + # private file from whatever applies now, and once after the activation to verify + # the written key is the one git resolves. Real git tells them apart by STATE, not + # by the arguments, so the stub has to as well: unset before the activation (git's + # own rc 1, which sends the seed down its XDG branch) and the activated path after. + # A stub that answered one way for both would either strand the seed or fail the + # verification, and in each case for a reason having nothing to do with #374. + # No external commands — PATH is replaced below, so `cat` would not resolve; + # `read`, `printf` and `[` are shell builtins. Written without a trailing NUL on + # purpose: the reader splits on the first one and takes what precedes it, so + # emitting none leaves the whole payload, and `\000` through `printf` is not + # portable across the shells that may be /bin/sh here. + activated = os.fsencode(str(tmp_path / "activated")) + stub.write_bytes( + b'#!/bin/sh\nshift 2\ncase "$1" in\n' + b"version) printf 'git version 2.55.0\\n' ;;\n" + b'rev-parse) case "$2" in\n' + b" --absolute-git-dir) printf '%s\\n' " + sq(gitdir) + b" ;;\n" + b" *) printf '%s\\n' " + sq(root) + b" ;;\n" + b" esac ;;\n" + b'config) case "$*" in\n' + b" *--worktree*) printf '%s' \"$4\" > " + sq(activated) + b" ;;\n" + b" *extensions.worktreeConfig*) printf 'true\\n' ;;\n" + b" *core.excludesFile*)\n" + b" if [ -f " + sq(activated) + b" ]; then\n" + b" IFS= read -r seen < " + sq(activated) + b'; printf %s "$seen"\n' + b" else exit 1 ; fi ;;\n" + b" *) exit 1 ;;\n" + b" esac ;;\n" + b"*) exit 1 ;;\nesac\n" + ) stub.chmod(0o755) # PATH is REPLACED, not prepended: the stub has to be the only resolvable # `git`, or the box's real git answers first with a decodable path and this # passes without ever exercising the decode. monkeypatch.setenv("PATH", str(bin_dir)) + # the excludes-file seed falls back to the XDG default when the key is unset + # (which the stub reports); pointed at an empty dir so the runner's own + # ~/.config/git/ignore cannot leak into the assertion below. + monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path / "xdg")) # must not raise: the contract is best-effort in both arms reason = _worktree_local_exclude(tmp_path / "wt", ["/probe-374"]) @@ -1687,26 +4365,31 @@ def test_worktree_local_exclude_undecodable_git_output_degrades(tmp_path, monkey # that glob is non-recursive and tmp_path sits three levels down, so it # could not see this test's own output, while it DID fail for anyone whose # box still carried litter from the hardcoded-path version of this test. - landed = Path(os.fsdecode(common)) / "info" / "exclude" + landed = Path(os.fsdecode(gitdir)) / "info" / "exclude" assert landed.is_file() assert "/probe-374" in landed.read_text(encoding="utf-8") + # and not in the common dir's exclude, which is where #384 put it + assert not (Path(os.fsdecode(root)) / "info" / "exclude").exists() @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits; Windows has no umask") -def test_worktree_local_exclude_created_exclude_stays_readable(project): +def test_worktree_local_exclude_created_exclude_stays_readable(project, tmp_path): """An exclude this helper CREATES keeps the mode the `write_text` it replaced - would have produced, not `mkstemp`'s private 0600. `atomic_write_text` carries - a mode over only when the target already EXISTS (its docstring is explicit), - so a fresh one lands 0600 — and git treats an exclude it cannot read as one - that is not there. Measured: git warns `unable to access`, exits 0, and - `git add -A` stages the very files the exclude was written to shield, with no - degrade reason at all, because the write SUCCEEDED. That is the harm the - helper's own docstring calls out, arrived at through a successful write. - - Reachable where the common dir is read under another UID — - `core.sharedRepository`, a shared checkout — and the create path is live - whenever `.git/info/exclude` is absent, which is why the `mkdir(parents=True)` - above exists. + would have produced, not `mkstemp`'s private 0600 — behavior preservation + across #375's move to `atomic_write_bytes`, which carries a mode over only when + the target already EXISTS (its own docstring is explicit that a fresh one gets + 0600). Without the `touch()` the refactor would have narrowed every private + exclude's mode with nothing recording the change, and the create path is live + for EVERY worktree since #384, not just a repo whose `.git/info/exclude` is + missing: the private exclude never exists until the shield writes it. + + The mode itself only matters where another OS user reads the gitdir, and that + configuration is refused above the lock now (`_shield_shared_repository`), so + this pins a mode rather than a supported cross-user path. It is still worth + pinning for the SHAPE of the failure a wrong mode produces: measured, git treats + an exclude it cannot read as one that is not there — it warns `unable to + access`, exits 0, and `git add -A` stages the very files the exclude was written + to shield, with no degrade reason at all, because the write SUCCEEDED. The umask is pinned rather than inherited: at the box's own 0077 the ablated code produces 0600 too, and the ablation would not bite. @@ -1714,16 +4397,18 @@ def test_worktree_local_exclude_created_exclude_stays_readable(project): Ablation: drop the `exclude.touch()` and this fails, reporting 0o600.""" old_umask = os.umask(0o022) try: - exclude = project.project / ".git" / "info" / "exclude" + wt = tmp_path / "wt" + verify.worktree_add(project.project, wt, "feat", "main") + exclude = _wt_private_exclude(wt) exclude.parent.mkdir(parents=True, exist_ok=True) - exclude.unlink(missing_ok=True) + assert not exclude.exists() # what the replaced write_text would have produced, measured not assumed probe = exclude.with_name("umask-probe") probe.write_text("x", encoding="utf-8") expected = stat.S_IMODE(probe.stat().st_mode) probe.unlink() - assert _worktree_local_exclude(project.project, ["/probe-mode"]) is None + assert _worktree_local_exclude(wt, ["/probe-mode"]) is None assert stat.S_IMODE(exclude.stat().st_mode) == expected, oct( stat.S_IMODE(exclude.stat().st_mode) @@ -1733,26 +4418,35 @@ def test_worktree_local_exclude_created_exclude_stays_readable(project): def test_worktree_local_exclude_non_git_dir_returns_none(tmp_path): - """Not-a-repo is an EXPECTED skip, not a degradation: `OSError` in the - subprocess arm means "no git to query" and must stay silent, while the same - type in the tail means "surface it". That is why the two arms cannot merge. + """Not-a-repo is an EXPECTED skip, not a degradation, and must stay silent — + while a fault in the tail means "surface it". That is why the two arms cannot + merge. - INVERSE ablation (a negative assertion needs one): make the subprocess arm + Two corrections to what this comment used to say, both worth keeping straight: + not-a-repo is not a raised fault at all — `rev-parse` answers rc 128 and the + silent arm reads that returncode — and the arm's `except` catches `GitError`, + not `OSError`, since the chokepoint translates a failed spawn before it arrives. + + INVERSE ablation (a negative assertion needs one): make the rev-parse arm return a reason string instead of None and this fails.""" assert _worktree_local_exclude(tmp_path, ["/probe-359"]) is None def test_worktree_local_exclude_success_returns_none(project, tmp_path): """The happy path returns None too — `None` is "nothing to report", not - "nothing happened": the pattern really lands in the common dir's exclude.""" + "nothing happened": the pattern really lands in the worktree's own exclude, + and nowhere the main checkout reads.""" repo = project.project wt = tmp_path / "wt" verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() assert _worktree_local_exclude(wt, ["/probe-359"]) is None - lines = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8").splitlines() + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() assert "/probe-359" in lines + assert shared.read_bytes() == before def test_provision_worktree_exclude_fault_reports_on_degraded(project, tmp_path, monkeypatch): @@ -1771,9 +4465,10 @@ def boom(*a, **kw): raise OSError("read-only .git") # patched at install's OWN binding: the exclude write goes through - # atomic_write_text (#375), which writes via os.fdopen on an mkstemp fd, so a - # Path.write_text/Path.open patch never fires and would pass vacuously. - monkeypatch.setattr(install_mod, "atomic_write_text", boom) + # atomic_write_bytes (#375, #384 round 4), which writes via os.fdopen on an + # mkstemp fd, so a Path.write_bytes/Path.open patch never fires and would pass + # vacuously — and so would a patch left on the atomic_write_TEXT this replaced. + monkeypatch.setattr(install_mod, "atomic_write_bytes", boom) msgs: list[str] = [] provision_worktree(wt, [get_profile("claude")], repo, on_degraded=msgs.append) @@ -1801,6 +4496,374 @@ def test_provision_worktree_non_git_no_degrade_callback(tmp_path): assert (wt / ".claude" / "skills" / "bmad-loop-sweep" / "SKILL.md").is_file() +# ------------------------------------------- the shield runs on the chokepoint (issue #389) +# +# The shield's git used to be a bare `subprocess.run` in this module, so its +# guards caught `subprocess.SubprocessError` and `OSError` — the raw forms of a +# timeout and a failed spawn. Through `verify.git_bytes` neither type ever +# arrives: the chokepoint translates them into `GitError` and its `GitSpawnError` +# subclass first. Every guard was re-derived for that, and these are the tests +# that would have caught leaving one behind. Each injects at the boundary the +# chokepoint actually raises from, which is why the fakes RAISE rather than +# return a non-zero rc — an rc is an answer here, never a fault. + + +@pytest.mark.parametrize("fault", [verify.GitError, verify.GitSpawnError]) +def test_worktree_local_exclude_git_fault_before_answering_is_a_silent_skip( + project, tmp_path, monkeypatch, fault +): + """A git that never answered leaves the FIRST arm's contract untouched: silence. + Both classes are covered because they enter differently — a timeout is raised + as `GitError` itself, a spawn failure as the `GitSpawnError` subclass — and a + guard naming only one of them looks correct until the other happens. + + Ablation: narrow the `rev-parse` guard to `subprocess.SubprocessError` (what it + caught before the chokepoint) and both cases fail — the fault propagates out of + a function whose whole first arm promises it never will.""" + wt = tmp_path / "wt" + verify.worktree_add(project.project, wt, "feat", "main") + + def unanswerable(worktree, *args): + raise fault(f"git {args[0]} did not answer in {worktree}") + + monkeypatch.setattr(install_mod, "git_bytes", unanswerable) + + assert _worktree_local_exclude(wt, ["/probe-389"]) is None + + +def test_worktree_local_exclude_common_dir_probe_rc_degrades(project, tmp_path, monkeypatch): + """The SECOND `rev-parse` is past the silent arm: `--absolute-git-dir` has already + answered, so git has identified the repository and a failure here is a degrade. + + Round 3 split one combined `rev-parse` into two calls (a newline is a legal byte + in a POSIX path), and the second call inherited the FIRST call's silent arm. That + left a shield that was owed, did not happen, and said nothing — after provisioning + had already copied the files it exists to hide. Both the function's own docstring + ("a timeout or spawn failure on the FIRST rev-parse ... that scope is the whole of + it") and `worktree_flow`'s contract ("any fault after that ... is reported to + on_degraded rather than swallowed") already specified the fix; only the code + disagreed. + + Name-filtered onto `--git-common-dir` alone, so `--absolute-git-dir` answers for + real and this pins the boundary BETWEEN the two arms rather than the first arm. + If the production flag ever changes, this filter stops matching, the shield + succeeds, and `reason is not None` fails loudly — the opposite polarity to a + tripwire that goes quiet (round 10). + + The stderr detail is asserted, not just non-None: deleting the new arm lets the + empty stdout fall through to the `could not read this worktree's git dirs` guard, + which ALSO returns a reason. Asserting only `is not None` would pass against the + bug. + + Ablation: replace this arm's `return (...)` with `return None` — the round-8 rule + that a gate whose deletion does not reproduce the bug must be ablated by restoring + the OLD behavior, since deleting it lands on that other guard instead.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + + def rc_on_common_dir(worktree, *args): + if args[:2] == ("rev-parse", "--git-common-dir"): + return subprocess.CompletedProcess( + args=["git", *args], + returncode=128, + stdout=b"", + stderr=b"fatal: common dir vanished mid-probe\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", rc_on_common_dir) + + reason = _worktree_local_exclude(wt, ["/probe-r11"]) + + assert reason is not None + assert "common dir vanished mid-probe" in reason + assert "could not name its common dir" in reason + + +@pytest.mark.parametrize("fault", [verify.GitError, verify.GitSpawnError]) +def test_worktree_local_exclude_common_dir_probe_raise_degrades( + project, tmp_path, monkeypatch, fault +): + """The raise half of the same boundary, which is the shape that is actually + reachable. + + A non-zero rc from `--git-common-dir` after `--absolute-git-dir` answered rc 0 has + no static occupant — measured at 2.20.4 and 2.55.0, a healthy linked worktree + answers both rc 0, and an unknown flag is ECHOED at rc 0 rather than refused, so + "a git too old for the flag" cannot land there either. What reaches this boundary + is a TRANSIENT fault on the second spawn: the two probes are two separate + processes, each bounded by `limits.git_timeout_s`. Same standing round 9 accepted + one function away, and the reason it is worth a guard at all. + + Both classes, because they enter differently — a timeout raises `GitError` + itself, a spawn failure the `GitSpawnError` subclass. Neither is caught locally: + the call sits inside the tail's `try`, so the raise is funnelled into a reason + there rather than by an `except` of its own (round 7 — enumerating one failure + shape per round is what cost this PR three of them). + + Ablation: wrap the `--git-common-dir` call in `try: ... except GitError: return + None`, restoring the pre-round-11 structure — both cases then return None.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + real = install_mod.git_bytes + + def raise_on_common_dir(worktree, *args): + if args[:2] == ("rev-parse", "--git-common-dir"): + raise fault("git rev-parse was killed mid-probe") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", raise_on_common_dir) + + reason = _worktree_local_exclude(wt, ["/probe-r11"]) + + assert reason is not None + assert "killed mid-probe" in reason + + +@pytest.mark.parametrize("fault", [verify.GitError, verify.GitSpawnError]) +def test_worktree_local_exclude_git_fault_after_answering_degrades( + project, tmp_path, monkeypatch, fault +): + """Once `rev-parse` has answered, the same fault means the opposite thing — the + shield was owed and did not happen — so it must come back as a reason the + caller can journal, not as silence. + + Name-filtered onto the activation so everything before it runs for real; that + is also what makes this the SECOND arm rather than the first. + + THE FLAG ASSERTION IS THE POINT OF THE SECOND HALF. This test injected exactly + the raised activation fault for two review rounds while asserting only the + returned reason — so it sat directly on top of a live defect and could not see + it: the enable one line above had already made a permanent repo-format change, + and the raise bypassed the rollback that the non-zero-rc arm had. Codex found it + by reading this test rather than the code. A degrade assertion that stops at the + reason string cannot tell a clean degrade from one that left state behind. + + Name-filtering on `--worktree` is what keeps this pinned to the ACTIVATION. The + enable's own raise is a different degrade with its own rollback (round 10) — see + `test_shield_rolls_back_an_enable_whose_git_faulted` — so this test no longer + covers "every way the flag can be left behind", only this one. + + Parametrized over both classes because they enter differently — a timeout is + raised as `GitError` itself, a spawn failure as the `GitSpawnError` subclass. + + Ablation: drop the `except GitError` around the activation and both cases fail on + the flag assertion (the fault reaches the tail, which cannot roll back). Note the + ablation this docstring USED to name — "drop `GitError` from the tail's except + tuple" — no longer applies here, since the activation's fault is now caught + locally; the tail's guard is pinned by + `test_shield_degrades_when_git_will_not_say_what_the_users_excludes_are`.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + real = install_mod.git_bytes + + def fault_on_activation(worktree, *args): + if "--worktree" in args: + raise fault("git config timed out after 120s") + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", fault_on_activation) + + reason = _worktree_local_exclude(wt, ["/probe-389"]) + + assert reason is not None and "timed out" in reason + # ...and the permanent format change the enable made one line earlier is gone + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + + +@pytest.mark.parametrize("fault", [verify.GitError, verify.GitSpawnError]) +def test_shield_degrades_when_git_will_not_say_what_the_users_excludes_are( + project, tmp_path, monkeypatch, fault +): + """A git fault reading `core.excludesFile` means we did NOT FIND OUT which file + applies — and that must skip the shield, not activate over it. + + THIS TEST REVERSES what it asserted before round 5. It used to pin the swallow, + on the premise that "git unqueryable ⇒ no key to resolve ⇒ nothing to copy" and + that skipping was "strictly worse than losing the seed". Both halves were wrong: + + - The premise is a false inference. It holds for `rc != 0`, which is an ANSWER + (an unset key exits 1, and that arm is still correctly silent). A raised fault + is not an answer, so the code mapped UNKNOWN onto ABSENT. + - The outcome was never "a lost seed". It was a lost seed PLUS a worktree-scoped + key that SHADOWS the file the seed failed to read — the compound harm round 4 + named as the bug. Skipping stages a bounded, journaled set of bmad-loop's own + files; continuing silently un-ignores whatever the operator's personal excludes + cover, which is by definition what their `.gitignore` does not, and merges it + back into their checkout as tracked content. + + Assertion 3 is the harm and is why the operator's excludes are planted FIRST: the + version of this test that pinned the swallow planted none at all, so it could not + observe the shadow in either direction. Assertion 4 is the non-vacuity check — + without it this passes just as well if the shield had excluded everything. + + Parametrized because of RE-INTRODUCTION, not coverage: `GitSpawnError` exists so + callers can tell "git said no" from "the machine is broken", which makes + `except GitSpawnError: return b""` the most plausible future re-narrowing — and a + `GitError`-only test would pass against it. + + Ablation: restore `except GitError: return b""` in `_shield_inherited_excludes` + and both cases fail — on assertion 1 (`reason` comes back None) and again on + assertion 3 (`mine.log` shows up untracked, the shadow). + + Asserts on "did not answer", NOT "timed out": the latter is a lie for + `GitSpawnError`, and it is the substring the activation-fault test above already + asserts, so it could not tell which call faulted.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + users = tmp_path / "my-global-ignores" + users.write_text("mine.log\n", encoding="utf-8") + git(repo, "config", "core.excludesFile", str(users)) + real = install_mod.git_bytes + + def unanswerable_excludes_read(worktree, *args): + if "core.excludesFile" in args and "--get" in args: + raise fault(f"git config did not answer in {worktree}") + return real(worktree, *args) + + # No monkeypatch.undo() below, unlike the read_bytes sibling: this patch is on + # `install_mod.git_bytes`, while conftest's git() is a bare subprocess.run — so + # the assertions' own git is untouched. + monkeypatch.setattr(install_mod, "git_bytes", unanswerable_excludes_read) + + reason = _worktree_local_exclude(wt, ["/probe-389"]) + + assert reason is not None and "did not answer" in reason + assert not _wt_private_exclude(wt).exists() # nothing to point a shadowing key at + (wt / "mine.log").write_text("noise\n", encoding="utf-8") + (wt / "probe-389").write_text("noise\n", encoding="utf-8") + status = git(wt, "status", "--short") + assert "mine.log" not in status # their ignore was never shadowed — THE HARM + assert "probe-389" in status # ...and the shield really was skipped + + +def test_shield_degrades_when_the_excludes_read_answers_with_a_fault_rc( + project, tmp_path, monkeypatch +): + """The sibling above is UNKNOWN arriving as a RAISE; this is UNKNOWN arriving as an + rc. They are one funnel deliberately: `git_bytes` has exactly these two failure + shapes, and this PR's activation arm took three review rounds precisely because + each fix enumerated the shape it had just been shown. + + rc 1 is the ONLY non-zero rc that means "no such key". Every other one is git + saying it could not answer, and the `else:` used to route all of them into the XDG + fallback — seeding ignore patterns the operator never chose for this repository and + then ACTIVATING them over the file it had failed to read (#384, CodeRabbit round 9). + + INJECTED rather than provoked from a config value, and the measurement behind that + is why this docstring is long, because the plausible version is wrong. + `core.excludesFile = ~nosuchuser/ignore` really does make this read exit 128 + (`fatal: failed to expand user dir`) — but git expands that same value while + parsing core config for any command that sets the repository up, so + `rev-parse --absolute-git-dir` in the caller fatals identically and the shield + silently skips ABOVE this branch. Measured at git 2.20.4 and 2.55.0, both ends of + the supported range. Nothing static reaches this arm; its occupants appear between + that rev-parse and this read, and the repo-scoped lock in between — `flock`, which + blocks indefinitely on POSIX — is what makes the window wide enough to hold a + dotfile sync or an NSS/LDAP hiccup. + + Injecting the ANSWER rather than breaking the repository is also what lets the harm + be asserted through git's own status: the fault clears, the activation does not, so + what a shielded-over-a-transient worktree would carry is a durable + `core.excludesFile` shadowing the operator's real excludes with the XDG file's. + + Ablation: replace `elif answer.returncode != 1:` with `elif False:` and this fails — + the fallback seeds `xdg-ignored.tmp`, the activation succeeds, the reason comes back + None, and a file the operator can still see in their own checkout goes invisible + inside the worktree.""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + xdg = tmp_path / "xdg" + (xdg / "git").mkdir(parents=True) + # non-vacuity: the fallback this must NOT take has a real file with a real pattern + # behind it, so the assertions below can tell "skipped" from "found nothing". + (xdg / "git" / "ignore").write_text("xdg-ignored.tmp\n", encoding="utf-8") + real = install_mod.git_bytes + + def unresolvable_excludes_read(worktree, *args): + # name-filtered onto the READ (`--get`), never the activation, which also names + # core.excludesFile — everything else runs for real, so this pins this branch + # rather than a short-circuit somewhere earlier. + if "--get" in args and "core.excludesFile" in args: + return subprocess.CompletedProcess( + args=["git", *args], + returncode=128, + stdout=b"", + stderr=b"fatal: failed to expand user dir in: '~nosuchuser/ignore'\n", + ) + return real(worktree, *args) + + monkeypatch.setattr(install_mod, "git_bytes", unresolvable_excludes_read) + + with monkeypatch.context() as pinned: + pinned.setenv("XDG_CONFIG_HOME", str(xdg)) + reason = _worktree_local_exclude(wt, ["/probe-384"]) + + assert reason is not None and "failed to expand user dir" in reason + assert not _wt_private_exclude(wt).exists() # nothing to point a shadowing key at + # ...and no permanent repo-format change was left behind for a shield that never ran + assert not (Path(git(wt, "rev-parse", "--absolute-git-dir")) / "config.worktree").exists() + assert "worktreeConfig" not in (repo / ".git" / "config").read_text(encoding="utf-8") + (wt / "xdg-ignored.tmp").write_text("noise\n", encoding="utf-8") + (wt / "probe-384").write_text("noise\n", encoding="utf-8") + status = git(wt, "status", "--short") + # THE HARM, in git's own words. git is healthy here — the fault was one answer, not + # a broken repo — so this is exactly what the operator would see once a transient + # cleared: their own file, still visible, un-shadowed by patterns they never chose. + assert "xdg-ignored.tmp" in status + assert "probe-384" in status # ...and the shield really was skipped + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX /bin/sh stub git") +def test_shield_git_calls_carry_the_locale_pin(project, tmp_path, monkeypatch): + """`LC_ALL=C` is one of the two things standing outside the chokepoint used to + cost (the other, the engine-configured timeout, has no observable seam here). + The shield embeds git's stderr verbatim in its degrade reasons, so without the + pin those reasons are whatever language the operator's box speaks — and #236 + exists because a translated git message had already been misread once. + + Recorded from a real child process rather than asserted on the argv: the pin is + an `env=` on the spawn, so only the child can testify that it arrived. + + The ambient `LC_ALL` is deliberately set to something else first — without that + the recorded value could be "C" because the runner's box already said so, and + the ablation below would pass. + + Ablation: spawn with a bare `subprocess.run` (no `env=`) and this fails — the + stub records `en_US.UTF-8`.""" + bin_dir = tmp_path / "stubbin" + bin_dir.mkdir() + seen = tmp_path / "locale-seen" + stub = bin_dir / "git" + # shlex.quote, for the reason the `sq()` helper further up exists: unquoted, a + # temp root carrying whitespace (a spaced `--basetemp`, or TMPDIR) word-splits + # this redirect, so the stub appends to the wrong path and leaks a stray file + # outside the basetemp pytest reaps (#384, CodeRabbit at round 15). + stub.write_text( + f'#!/bin/sh\nprintf "%s\\n" "${{LC_ALL-}}" >> {shlex.quote(str(seen))}\nexit 1\n', + encoding="utf-8", + ) + stub.chmod(0o755) + monkeypatch.setenv("PATH", str(bin_dir)) + monkeypatch.setenv("LC_ALL", "en_US.UTF-8") + + # rc 1 from the stub is the "not a repo" answer, i.e. the silent skip — all + # this test needs is that one call was spawned at all. + assert _worktree_local_exclude(tmp_path / "wt", ["/probe-389"]) is None + + # non-empty is itself an assertion: an unexecutable stub would leave no file, + # and every claim below would hold vacuously. + recorded = seen.read_text(encoding="utf-8").split() + assert recorded and set(recorded) == {"C"} + + # ----------------------------------------------------------------- hookless profiles @@ -1841,11 +4904,15 @@ def test_provision_worktree_hookless_exclude_never_blankets_worktree(project, tm wt = tmp_path / "wt" verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() + provision_worktree(wt, [get_profile("opencode-http")], repo) - lines = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8").splitlines() + lines = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() assert "/.claude/skills" in lines assert "/" not in lines + assert shared.read_bytes() == before # ----------------------------------------------------------------- seed_globs (engine plugin) @@ -1893,13 +4960,16 @@ def test_provision_worktree_seed_globs_shielded_in_local_exclude(project, tmp_pa (skill / "SKILL.md").write_text("tool", encoding="utf-8") wt = tmp_path / "wt" verify.worktree_add(repo, wt, "feat", "main") + shared = repo / ".git" / "info" / "exclude" + before = shared.read_bytes() provision_worktree(wt, [get_profile("claude")], repo, seed_globs=[".claude/skills/*"]) assert (wt / ".claude" / "skills" / "tests-run" / "SKILL.md").is_file() - exclude = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8").splitlines() + exclude = _wt_private_exclude(wt).read_text(encoding="utf-8").splitlines() assert "/.claude/skills/tests-run" in exclude assert git(wt, "status", "--short", "--", ".claude/skills/tests-run") == "" + assert shared.read_bytes() == before # ----------------------------------------------------------------- seed file modes (issue #126) diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 07a0b5eb..9b2a5aca 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -148,14 +148,18 @@ def test_atomic_write_text_replaces_contents(tmp_path): def test_atomic_write_text_preserves_the_target_mode(tmp_path): """`os.replace` swaps in a NEW inode, so a naive tmp-write-and-replace resets the file's permissions to the umask default — silently widening a 0600 ledger - to world-readable, or dropping group-write on a shared artifact dir.""" + to world-readable, or dropping group-write on a shared artifact dir. + + 0o640, NOT 0o600: `mkstemp` creates its temp at 0600 already, so asserting that + value passed with `copymode` deleted — the pin held for the wrong reason. Any + mode the staging temp does not arrive with makes the ablation bite.""" target = tmp_path / "ledger.md" target.write_text("before", encoding="utf-8") - target.chmod(0o600) + target.chmod(0o640) platform_util.atomic_write_text(target, "after") - assert stat.S_IMODE(target.stat().st_mode) == 0o600 + assert stat.S_IMODE(target.stat().st_mode) == 0o640 @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") @@ -268,6 +272,211 @@ def record(src, dst): assert len(set(seen)) == 2 +# ------------------------------------------------------------ atomic_write_bytes +# +# Mirrored from the eight text cases above rather than parametrized over both, and +# deliberately: each property is a separate PIN the git-add shield now depends on +# (install.py's `_worktree_local_exclude` writes through this variant, not the text +# one), and the two helpers share a private tail that a later edit could split +# without either name changing. A parametrized suite would also lose the +# per-property rationale the text docstrings carry, which is where the reasons live. +# What is NOT mirrored is anything about encoding — the last case below is the whole +# difference between the two. + + +def test_atomic_write_bytes_replaces_contents(tmp_path): + target = tmp_path / "exclude" + target.write_bytes(b"before") + + platform_util.atomic_write_bytes(target, b"after") + + assert target.read_bytes() == b"after" + + +def test_atomic_write_bytes_writes_bytes_no_codec_can_decode(tmp_path): + """The reason this variant exists (#384 review round 4): the payload is an + operator's git exclude file, whose patterns are POSIX paths and therefore + arbitrary bytes. `atomic_write_text` would have to encode, and a strict UTF-8 + encode of a legacy-encoded file's content raises.""" + target = tmp_path / "exclude" + target.write_bytes(b"before\n") + + platform_util.atomic_write_bytes(target, b"secret-\xff\n/probe\n") + + assert target.read_bytes() == b"secret-\xff\n/probe\n" + + +def test_atomic_write_bytes_does_not_translate_newlines(tmp_path): + """The one behavioral difference from the text sibling, and it is load-bearing on + Windows. `atomic_write_text` opens in text mode with the translating `newline` + default, so an LF payload lands as CRLF there — correct for a ledger a human + edits, wrong for a file being copied byte-for-byte from somewhere else. Binary + mode does no translation on any platform, which is what makes a verbatim copy + verbatim.""" + target = tmp_path / "exclude" + target.write_bytes(b"before\n") + + platform_util.atomic_write_bytes(target, b"a\nb\n") + + assert target.read_bytes() == b"a\nb\n" # never b"a\r\nb\r\n" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits") +def test_atomic_write_bytes_preserves_the_target_mode(tmp_path): + """Same pin as the text sibling — and the one the shield leans on hardest: an + exclude file git cannot READ is one git silently IGNORES, so a mode reset here + stages the very files the exclude was written to hide. + + 0o640 rather than 0o600 for the reason the text sibling gives: `mkstemp`'s temp + is already 0600, so that value cannot tell a preserved mode from a fresh one.""" + target = tmp_path / "exclude" + target.write_bytes(b"before") + target.chmod(0o640) + + platform_util.atomic_write_bytes(target, b"after") + + assert stat.S_IMODE(target.stat().st_mode) == 0o640 + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_atomic_write_bytes_writes_through_a_symlink(tmp_path): + real = tmp_path / "real-exclude" + real.write_bytes(b"before") + link = tmp_path / "exclude" + link.symlink_to(real) + + platform_util.atomic_write_bytes(link, b"after") + + assert link.is_symlink() + assert real.read_bytes() == b"after" + + +def test_atomic_write_bytes_preserves_extended_attributes(tmp_path): + """Skipped where the platform or filesystem has no user xattrs, exactly as the + text sibling is — the helper is best-effort there by design.""" + target = tmp_path / "exclude" + target.write_bytes(b"before") + setxattr = getattr(os, "setxattr", None) + if setxattr is None: + pytest.skip("no os.setxattr on this platform") + try: + setxattr(target, "user.bmad-loop-test", b"kept") + except OSError as e: + pytest.skip(f"filesystem does not support user xattrs: {e}") + + platform_util.atomic_write_bytes(target, b"after") + + assert target.read_bytes() == b"after" + assert os.getxattr(target, "user.bmad-loop-test") == b"kept" + + +def test_atomic_write_bytes_fsyncs_before_it_publishes(tmp_path, monkeypatch): + """Same ordering pin as the text sibling: a durable rename over data still in the + page cache comes back as a zero-length file, which for an exclude reads as "no + patterns" — a shield that silently excludes nothing.""" + order = [] + real_fsync, real_replace = os.fsync, os.replace + monkeypatch.setattr( + platform_util.os, "fsync", lambda fd: (order.append("fsync"), real_fsync(fd))[1] + ) + monkeypatch.setattr( + platform_util.os, "replace", lambda s, d: (order.append("replace"), real_replace(s, d))[1] + ) + target = tmp_path / "exclude" + target.write_bytes(b"before") + + platform_util.atomic_write_bytes(target, b"after") + + assert order == ["fsync", "replace"] # the sync must precede the publish + assert target.read_bytes() == b"after" + + +def test_atomic_write_bytes_leaves_no_temp_behind(tmp_path): + target = tmp_path / "exclude" + target.write_bytes(b"before") + + platform_util.atomic_write_bytes(target, b"after") + + assert [p.name for p in tmp_path.iterdir()] == ["exclude"] + + +def test_atomic_write_bytes_cleans_up_and_keeps_the_original_on_failure(tmp_path, monkeypatch): + target = tmp_path / "exclude" + target.write_bytes(b"before") + + def boom(src, dst): + raise OSError("disk full") + + monkeypatch.setattr(platform_util.os, "replace", boom) + + with pytest.raises(OSError): + platform_util.atomic_write_bytes(target, b"after") + + assert target.read_bytes() == b"before" + assert [p.name for p in tmp_path.iterdir()] == ["exclude"] # no orphaned temp + + +def test_atomic_write_bytes_temp_name_is_unique_per_call(tmp_path, monkeypatch): + target = tmp_path / "exclude" + target.write_bytes(b"before") + seen: list[str] = [] + real_replace = os.replace + + def record(src, dst): + seen.append(os.path.basename(src)) + real_replace(src, dst) + + monkeypatch.setattr(platform_util.os, "replace", record) + + platform_util.atomic_write_bytes(target, b"a") + platform_util.atomic_write_bytes(target, b"b") + + assert len(set(seen)) == 2 + + +def test_atomic_write_bytes_stages_in_the_targets_own_directory(tmp_path, monkeypatch): + """The temp has to be a SIBLING of the target, and this pins the shared tail for + both variants. `os.replace` cannot cross a filesystem, and the default temp dir + is a different mount on plenty of boxes (always, on a runner with a tmpfs + `/tmp`) — so a temp staged there fails the publish with EXDEV *after* the + content is written, turning an atomic write into a guaranteed one. + + Recorded from `os.replace`'s source argument, because the pre-existing + "leaves no temp behind" assertion cannot see this: it lists the TARGET's + directory, which a temp staged in `/tmp` is trivially absent from. Measured — + with `dir=` dropped, that test passed.""" + target = tmp_path / "sub" / "exclude" + target.parent.mkdir() + target.write_bytes(b"before") + staged: list[str] = [] + real_replace = os.replace + + def record(src, dst): + staged.append(os.path.dirname(str(src))) + real_replace(src, dst) + + monkeypatch.setattr(platform_util.os, "replace", record) + + platform_util.atomic_write_bytes(target, b"after") + + assert staged == [str(target.parent)] + assert target.read_bytes() == b"after" + + +def test_atomic_write_bytes_creates_a_missing_target_at_the_private_mode(tmp_path): + """A target that does not exist yet is created at `mkstemp`'s 0600, not the umask + default — the shared contract's deliberate choice, and the reason + `_worktree_local_exclude` `touch()`es the exclude before calling this: it needs + the file to already exist so a readable mode is what gets carried over.""" + target = tmp_path / "exclude" + + platform_util.atomic_write_bytes(target, b"fresh") + + assert target.read_bytes() == b"fresh" + if sys.platform != "win32": + assert stat.S_IMODE(target.stat().st_mode) == 0o600 + + # --------------------------------------------------------------- retrying_unlink @@ -353,6 +562,34 @@ def test_file_lock_reentry_after_exception(tmp_path): pass +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX file modes; Windows has no umask") +def test_file_lock_is_created_owner_only(tmp_path): + """The lock's mode is stated by the code, not inherited from whoever ran first. + + A repository shared between OS users is refused before the shield ever takes + this lock (`install._shield_shared_repository`, #384), so owner-only is the + whole policy — but leaving `os.open` mode-less would still make the mode a + property of the creator's umask rather than a decision: measured, 022 yields + 0o755 and 077 yields 0o700. + + `os.umask(0o022)` is the point of the fixture, not hygiene. At this box's own + 0o077 the mode-less code produces 0o600 by accident and the ablation does not + bite — the same trap as + `test_install.py::test_worktree_local_exclude_created_exclude_stays_readable`. + + Ablation (run): drop the `0o600` argument from `file_lock`'s `os.open` and this + fails reporting 0o755.""" + lock = tmp_path / "s.lock" + previous = os.umask(0o022) + try: + with platform_util.file_lock(lock): + pass + finally: + os.umask(previous) + + assert stat.S_IMODE(lock.stat().st_mode) == 0o600, oct(stat.S_IMODE(lock.stat().st_mode)) + + # ------------------------------------------------------------------ safe_segment