verify.py interpolates paths into git pathspecs unescaped: _exclude_specs over-excludes, disagrees with _path_under_any, and safe_rollback's preserve checkout over-restores

[pbean]

verify.py builds git pathspecs by interpolating configured paths, so a path carrying *, ? or […] matches a wider set than the one it names. Every affected call is on the over-match side; none of them under-matches.

Sibling of #401, which is the same class one layer over: there the interpolation is into gitignore syntax, here it is into git pathspec syntax. Neither escapes.

Status

Items 1 and 2 shipped in #406 (hotfix/0.9.1-build-auto, commits f0b34b3 + e8979b6), via a _literal_specs helper mirroring _exclude_specs. Items 3, 4 and 5 remain open and are what this issue now tracks.

Two corrections to this issue's own text, both established by measurement while fixing 1 and 2 — kept rather than deleted, because each was load-bearing in the decision to defer:

• The deferral rationale below was wrong. It read "neither a regression it introduced nor reachable on any configuration it ships." Both halves fail for those two sites. git log -S "def path_tracked" returns e370528 alone and git merge-base --is-ancestor e370528 release/0.9.x is false — path_tracked and its only exposed call site are entirely new on that branch. And commit_paths became unattended in dc4f934: until then its only caller was decisions.py:155, behind the operator-invoked bmad-loop decisions; _carry_harvested_deferrals put it inside _defer, on the run path, with no human present.
• Item 2's "already declares safe" claim named the wrong lines. verify.py:325-327 declares the GitError degrade safe, not the over-match. The real gap was :310-316, whose enumeration of considered false-positive sources (core.quotePath, tracked-but-deleted, --error-unmatch, check-ignore) omitted pathspec globbing entirely. The fix carries a new paragraph there.

Still live on main

main is not already fixed, and the forward-port is not a clean cherry-pick:

• path_tracked does not exist on main at all — item 2 is unique to the hotfix branch.
• main's commit_paths still has all three bare operands, plus a fourth unescaped ls-files -z -- *missing pathspec that release/0.9.x does not carry. Its failure direction looks different — the membership test is r in tracked, and an over-match returns the neighbour's name — so it likely drops the rel rather than over-including. Not measured; measure before porting.
• main hand-rolls r.replace("\\", "/") where .as_posix() fixes the same Windows-separator problem at the source. The port should replace it, not keep both.

Version floor (was a blocking unknown)

Measured, not read off the docs, as this issue required. :(literal) behaves identically at git 2.20.4 (alpine:3.9 — the floor install._WORKTREE_CONFIG_GIT declares) and at 2.55.0, non-root: metachar operand → empty; ordinary path → lists; directory prefix _bmad/render → lists beneath; tracked-but-deleted → still lists; add of an ignored path → rc 1. This mattered more than it looked: below the floor the prefix would read as a literal filename, match nothing and answer False — which authorizes the unlink, inverting the failure direction from "skip a delete" to "delete a tracked file".

---

Measured (git 2.55.0)

Fixture: sibling pairs docs[a]/docsa, a*b/axb, q?r/qxr, each holding f.md.

1. commit_paths — FIXED on release/0.9.x, still live on main. verify.py:1838-1866 documents itself as "commit exactly paths (and nothing else) … safe to call out of band", and its add/status/commit all took the rels as bare pathspecs. It was not:

add -- docs[a]/f.md   staged=['docs[a]/f.md', 'docsa/f.md']
add -- a*b/f.md       staged=['a*b/f.md',     'axb/f.md']
add -- q?r/f.md       staged=['q?r/f.md',     'qxr/f.md']

An unrelated user file is staged and committed under a story's name. Note the operand is a superset — it matches its own literal text and the glob's reach — because match_pathspec_item compares literally before falling through to fnmatch. That is also why the under-match direction is impossible.

A second harm this issue did not originally record: with the ledger gitignored (the shape _carry_harvested_deferrals is built to hit), git add refuses an explicitly-named ignored path but skips a globbed one. So the plain form could exit rc 0 having staged nothing, status found no change, and the carry reported success having committed no ledger and journalled no harvest-carry-uncommitted — a silent loss where the explicit form has a record. rc 0 requires a glob-reachable neighbour to exist: with none it is still rc 1; with a clean neighbour it stages nothing; with a dirty one it stages the operator's edit.

2. path_tracked (verify.py:328) over-matched — FIXED on release/0.9.x; does not exist on main. With the metachar path untracked and only the collider tracked:

rel='docs[a]/f.md'  tracked? plain=True  literal=False
rel='a*b/f.md'      tracked? plain=True  literal=False
rel='q?r/f.md'      tracked? plain=True  literal=False

So _ledger_is_gits_to_restore answered True for an untracked ledger and the harvest revert's unlink() was skipped — the ledger revert silently no-opped for such a project, leaving a finding about discarded code open for a later sweep. Found by Codex on #406 (dc4f934); reproduction and ablation table on that thread.

3. _exclude_specs (verify.py:280-282) over-excludes, but only for specific shapes — worth stating precisely, because the general form of this claim is false:

prefix='docs[a]'  victim='docsa/f.md'   victim-excluded=False   <- sibling DIR: NOT swallowed
prefix='docs[a]'  victim='docsa'        victim-excluded=True    <- sibling FILE at root: swallowed
prefix='q?r'      victim='qxr/f.md'     victim-excluded=False
prefix='q?r'      victim='qxr'          victim-excluded=True
prefix='a*b'      victim='axb/f.md'     victim-excluded=False
prefix='doc*'     victim='docsa/f.md'   victim-excluded=True    <- a `*` that can swallow `/`

The pathspec has to match the whole path, so a same-length […]/? collision only reaches a sibling file, never a sibling directory's contents; a * reaches everything because it crosses /. Where it does fire, has_changes_since (:208, the diff --quiet at :237) and attempt_dirty (:247, the same call at :270) under-report the diff — a false "no changes", the same failure #406 fixed at _harvest_gate_exclude by a different route.

4. _exclude_specs (git glob) and _path_under_any (:285-287, a Python startswith) disagree on exactly those rows — the two halves of one has_changes_since call answer differently:

prefix='docs[a]'  path='docsa'      git-excludes=True   python-under=False
prefix='q?r'      path='qxr'        git-excludes=True   python-under=False
prefix='doc*'     path='docsa/f.md' git-excludes=True   python-under=False

The tracked half is excluded while the untracked half is still counted, so the two branches of the same function disagree about what "under the artifact dir" means.

5. safe_rollback's checkout -- <preserve dir> (verify.py:648) — same shape-dependence:

checkout -- 'docs[a]'   target=restored  victim='dirty'    (unaffected)
checkout -- 'doc*'      target=restored  victim=restored   (an unrelated file reverted)

Reachability

implementation_artifacts is read verbatim out of _bmad/bmm/config.yaml and only {project-root}-substituted and resolved (bmadconfig.py:131-146); nothing sanitizes it. * and ? are illegal in Windows path names, so the realistic carrier is [/] — which narrows the live exposure to the victim-is-a-file rows of 3/4. No fixture in the repo has ever carried one, and no report exists; the remaining items are latent-correctness, not a live bug.

Fix shape (remaining items)

:(exclude,literal) in _exclude_specs (item 3) and :(literal) on safe_rollback's preserve dirs (item 5). The floor is settled — :(literal) is measured safe at 2.20.4, see above — so that blocker no longer applies.

Item 4 is a behaviour decision, not an escaping fix: it is about what "under the artifact dir" means. Fixing only the git half does not merely paper over the disagreement — it makes both halves agree on the literal reading, which is the one _path_under_any already implements, so it is the right direction rather than a coin flip.

[coderabbitai]

🔗 Related PRs

#86 - feat(resolve): intent-gap patch-restore re-drive + inline-review-layer preflight (re-cut) [merged]
#182 - fix(worktree): degrade post-merge teardown failures instead of crashing the run (#139) [merged]
#284 - feat: auto-resolve deferred-work entries a story declares (closes #234) [open]
#355 - feat(cli): bmad-loop confirm completes awaiting-operator stories [merged]
#385 - fix(install): scope the worktree git-add shield per-worktree (#384) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[pbean]

Items 1 and 2 shipped in #406 (f0b34b3 + e8979b6); body narrowed to items 3/4/5 above, with the deferral rationale and item 2's line citation corrected in place.

Two things carried forward for whoever takes the rest:

• The floor is settled. :(literal) measured identical at git 2.20.4 and 2.55.0 (13 rows, non-root, Docker). This issue's "check the version floor … measure it, don't read it off the docs" precondition is discharged for items 3 and 5. It mattered: below the floor the prefix reads as a literal filename and the probe answers False, which inverts the failure from "skip a delete" to "delete a tracked file".
• main still carries item 1, in a different and slightly worse shape — a fourth unescaped ls-files -z -- *missing pathspec that release/0.9.x does not have, whose direction is probably a drop rather than an over-include. Not measured. Do not cherry-pick.

Also worth noting for item 3's eventual fix: GIT_LITERAL_PATHSPECS / --literal-pathspecs is not an option. Ablated against the whole suite, it reddens 9 rows across 5 files — attempt_dirty, worktree_clean, dirty_paths, clean_incoming_collisions and three more — because it disarms :(exclude) too. :(exclude,literal) per operand is the only shape that works.