feat(validate): warn on legacy shared-exclude pollution (#384) - #386
feat(validate): warn on legacy shared-exclude pollution (#384)#386pbean wants to merge 8 commits into
Conversation
The #384 fix stops the worktree git-add shield from writing the repository-wide `.git/info/exclude`, but nothing removes what older versions already wrote there. Those lines name paths projects legitimately track (`.claude/skills`, `.claude/settings.json`), so in an already-provisioned repo every NEW file under them stays invisible to `git status` and `git add -A` in the operator's own checkout — the tracked files keep diffing normally, which is what made it invisible. `bmad-loop validate` now reports that residue under `git.exclude-legacy-pollution`, naming the file, the exact lines, the symptom and the manual remedy. Warn only, never repair. `.git/info/exclude` is hand-editable and its lines carry no authorship: a project that wants `/.claude/skills` ignored writes the identical line, and after the fact the two are indistinguishable. So the candidate set is rebuilt exactly as `provision_worktree` builds it and only byte-identical lines count — no prefix, substring or containment matching, which would flag an operator's own rule and tell them to delete it. Both negative tests were verified by ablation (widening the match to containment fails them); the absent-exclude test was verified against removing the OSError catch. Detection is silent on every fault — not a repo, git missing or too old, an unreadable exclude — because a repo whose exclude cannot be read is not thereby a repo with a problem to report.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
`scm.worktree_seed` is user-authored and unvalidated, so an empty entry renders as the candidate "/". Profiles cannot produce it — `skill_tree` and `seed_files` entries are validated non-empty, and the writer skips hookless config_paths for exactly this reason — but policy can. A bare "/" is a line an operator could legitimately have written, and this gate's output tells them to delete what it names, so a false match there is the one that does harm. Ablation-verified: dropping the guard fails the test.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d1880fad4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for profile in profiles: | ||
| seed_rels.extend(profile.seed_files) | ||
| seed_rels.extend(pol.scm.worktree_seed) | ||
| pollution = legacy_exclude_pollution(project, profiles, seed_rels) |
There was a problem hiding this comment.
If a repository was polluted while Claude was selected and the policy later switches every role to Codex, profiles and seed_rels contain only the current Codex paths, so legacy lines such as /.claude/skills and /.claude/settings.json never intersect the candidate set. validate then emits no warning even though those lines continue hiding new files; build the legacy candidate set from all known historical writer inputs rather than only the current policy.
Useful? React with 👍 / 👎.
| # empty entry there renders as "/" here. | ||
| candidates.discard("/") | ||
| try: | ||
| answered = _shield_git(project, "rev-parse", "--git-path", "info/exclude") |
There was a problem hiding this comment.
Every validate execution now adds a git subprocess through _shield_git, bypassing the repository's mandated sole spawn point in verify._run_git; this creates another path outside its centralized timeout, locale, and typed-failure behavior. Extend/use the chokepoint for the required bytes/non-raising semantics instead of adding this caller to the exception.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
Found while guarding the read side of the exclude gate. Three sources feed one seed list into `provision_worktree`'s copy loop, and two of them already reject non-project-relative entries with the same predicate — `adapters/profile.py` for a profile's `seed_files`, and `plugins/manifest.py::_check_relative_paths` for a plugin manifest's. `scm.worktree_seed` arrived unchecked. The empty entry is the one that does damage rather than merely no-op. `""` makes that loop resolve `src` to the repo ROOT and `dst` to the worktree; both pass its `is_relative_to` containment checks, so `_copy_traversable` copies the entire project into the worktree — gitignored and untracked files included — and the rel then renders as the exclude pattern "/", which git-excludes the whole worktree so the unit's `git add -A` stages nothing and the story's work is never committed. Measured, not inferred. Absolute and `..` entries were already contained by those same checks and silently skipped; they are rejected here too, for consistency with the sibling sources and because a silently inert seed entry reads as applied configuration when it is not. Ablation-verified: removing the check fails all five new cases while the existing valid-config test keeps passing.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
`assert "info/exclude" in text` encoded a POSIX separator, so on Windows the message rendered `...\.git\info\exclude` and the substring was never present — the test failed on both Windows jobs while passing everywhere else. Assert the real path instead, which is stricter as well as portable: it pins the exact file the warning names rather than a fragment of it.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…-exclude-pollution
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…-exclude-pollution
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…-exclude-pollution
`legacy_exclude_pollution` shared `_shield_git` with the shield, so routing that through the chokepoint moves its faults too: a timeout and a spawn failure now arrive as `GitError` and `GitSpawnError`, neither of which the old `subprocess.SubprocessError`/`OSError` tuple catches. A warning-only preflight detector has no business propagating either.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Follow-through on #384, stacked on #385. Base is
fix/384-worktree-scoped-exclude— retarget tomainonce #385 lands. No closing keyword: #384 stays open until both halves are in.Why there is a second half
#385 stops the worktree git-add shield from ever writing the repository-wide
.git/info/excludeagain. It does not — and deliberately cannot — remove what older versions already wrote there. #385's own CHANGELOG entry promises this warning lands separately; this is it.The residue matters because those patterns name paths projects legitimately track:
In a repo provisioned by any pre-#384 version, those lines are still in the shared exclude, and every new file under those paths stays invisible to
git statusandgit add -Ain the operator's own checkout, indefinitely. The tracked files keep diffing normally — which is exactly what made it invisible in the original report, where 51 files and three whole skills went unstaged across two installer-upgrade commits with nothing in either diff able to explain why.What lands
bmad-loop validategainsgit.exclude-legacy-pollution:A warning, so it reports without changing the verdict —
okstays true and rc stays 0.Warn only, never repair
This was the maintainer call, and the code is built around it.
.git/info/excludeis a hand-editable file whose lines carry no authorship. A project that genuinely wants/.claude/skillsignored writes the byte-identical line, and after the fact there is no way to tell it from one the shield left behind. Auto-removing would silently undo an operator's own rule.The same reasoning drives the matching rule: the candidate set is rebuilt exactly the way
provision_worktreebuilds it (skill tree and hook config per profile, seeded rels,_bmad/custom), and a line counts only if it is byte-identical to a member. No prefix, substring or containment matching —/.claude/skills-mine,/.claude/skills/local, a bare.claude/skills, a leading space, and a!negation are all the operator's lines, and a preflight that flagged them would be telling someone to delete their own gitignore rules.--git-path info/excludenames git's one shared exclude from either vantage: a main checkout answers relative, a linked worktree answers with the main repo's absolute path (verified, git 2.55). The private per-worktree exclude #385 introduced is never looked at — it is supposed to carry these patterns, and it dies with its worktree.Detection is silent on every fault (not a repo, git missing or too old, an unreadable or undecodable exclude): preflight observation degrades, and a repo whose exclude cannot be read is not thereby a repo with a problem to report.
One deviation worth flagging
The helper returns a small
NamedTuple(path,lines) rather than a barelist[str]. The warning'sdetailhas to carry{"exclude": ..., "lines": ...}, and onerev-parseshould yield both facts rather than the call site re-deriving the path with a second git call. It returnsNonewhen there is nothing to report, so a non-Noneresult always has a non-emptylinesand a file to name.Tests
tests/test_install.py— seven unit tests: finds the shield patterns, covers seeded rels and_bmad/custom, exact-match-only, absent exclude, outside a repo, the private worktree exclude is never reported, and a bare/is never flagged.tests/test_cli.py— four gate tests: the warning fires with lines indetailand does not clearok; the text output carries the file, the lines, the symptom and the remedy; an operator-authored line alone produces nothing; an absent exclude produces nothing.Per AGENTS.md's ablation rule, both negative tests were verified by deleting the gate rather than trusting a green:
c in line) failstest_validate_does_not_flag_an_operator_authored_exclude_lineandtest_legacy_exclude_pollution_matches_exactly_never_by_containment.OSErrorfrom the catch failstest_validate_silent_when_the_exclude_file_is_absentwithFileNotFoundErroron the exact path — so it pins the silent-degrade contract and proves the missing-file branch is really exercised.test_legacy_exclude_pollution_silent_outside_a_repois honest about pinning neither branch individually — two can produce itsNone; the contract it holds is that validate outside a repo yields no finding and no exception. Its docstring says so.Second commit: the bare
/guardscm.worktree_seedis user-authored and unvalidated — nothing rejects an empty entry — soworktree_seed = [""]renders as the candidate/. Profiles cannot produce that (skill_treeandseed_filesentries are validated non-empty, and the writer skips hooklessconfig_paths for precisely this reason), but policy can.That is the one false match that does real harm: a bare
/is a line an operator could legitimately have written, and this gate's output tells them to delete what it names.candidates.discard("/")closes it, ablation-verified.Third commit: the writer-side hole this exposed
Checking the
/guard surfaced a worse bug in the writer, measured rather than inferred. Three sources feed one seed list intoprovision_worktree's copy loop, and two already reject non-project-relative entries with the same predicate —adapters/profile.pyfor a profile'sseed_files,plugins/manifest.py::_check_relative_pathsfor a plugin manifest's.scm.worktree_seedwas the only one arriving unchecked.With
worktree_seed = [""], that loop resolvessrcto the repo root anddstto the worktree; both pass itsis_relative_tocontainment checks:So it copies the entire project into the worktree — gitignored and untracked files included — and the rel then renders as the exclude pattern
/, which git-excludes the whole worktree, so the unit'sgit add -Astages nothing and the story's work is never committed.Fixed at the policy boundary with the identical predicate the sibling sources use, which closes it for all three (
provision_worktreehas exactly one caller, and its three seed sources are now each validated at their own boundary — so no loop guard is needed). Absolute and..entries were already contained by those same checks and silently skipped; they are rejected here too, for consistency and because a silently inert seed entry reads as applied configuration when it is not.Ablation-verified: removing the check fails all five new cases while the pre-existing valid-config test keeps passing.
Verification
uv run pytest -q -n auto— 3718 passed, 24 skippeduvx pyright@1.1.411— 0 errors, 0 warningstrunk fmt+trunk check— no issuesReviewer note
Greptile's review on #385 is a credit-limit notice, not a review (
pbean has reached the 50-credit limit for trial accounts). It reports asCOMMENTEDin the checks list, so #385 currently reads greener than it has been examined.