fix(ai-slop): expand directory targets from one path anchor (0.3.8) - #3330
fix(ai-slop): expand directory targets from one path anchor (0.3.8)#3330kyle-sexton wants to merge 2 commits into
Conversation
detect.sh built its directory-expansion prefix from `git rev-parse --show-toplevel` and its filter from `pwd`. A directory has several spellings on Git Bash, where git answers `C:/Users/...` for the checkout a shell reaches as `/tmp/...`, so no prefixed candidate survived the filter, `grep` exited non-zero, and the `|| find` fallback ran in place of the tracked-files listing it was meant to back up. The walk returns untracked and ignored markdown, so a directory target audited files the checkout does not track and said nothing about it. Expansion now runs `git ls-files` with `-C <dir>`, which is already restricted to that directory's subtree and answers in paths relative to it. The caller's own spelling of the directory is the only anchor, so there is no second source to disagree with. The branch is chosen up front from `--is-inside-work-tree` rather than from an empty pipeline, so a filesystem walk is only ever the answer for a directory genuinely outside a checkout; inside one, a listing that fails reports on stderr instead of degrading into a different set of files. Measured on Git Bash against `plugins/ai-slop`: 0 of 13 tracked files survived the old filter, and the walk scanned 14 files including an untracked one. The expansion now scans exactly the 13 tracked files. Adds coverage pinning the invariant that the spelling of the target cannot change the answer, plus a trailing-slash target and a directory inside a checkout holding only untracked markdown. Closes #3266 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XtbWChCVfUWAv1Pi5Qk2hA
…tree Independent review of the directory-expansion fix found two defects in the new code path and one gap in its coverage. `git ls-files` C-quotes any path holding a non-ASCII byte, so a tracked `café.md` came back as a literal `"caf\303\251.md"`, the joined path failed the scan loop's existence test, and the file produced neither a finding nor a declined row. For this plugin the sharpest case is a filename containing an em dash, which the em-dash detector would never open. The listing now sets `core.quotePath=false`. It stays newline-delimited rather than moving to `-z`, because the report format is one line per finding, so a filename holding a newline cannot be represented downstream whichever way the listing is read, and the newline form keeps the listing's exit status observable. The comment introduced with the expansion claimed a walk answers only for a directory genuinely outside a checkout. It does not: every `rev-parse` failure is swallowed, so a git that is absent or refuses to answer also reaches the walk. The claim is now accurate, and the absent case reports on stderr, because tracked-files-only is not achievable without git. Trailing slashes are stripped in full, so a `docs//` target no longer emits a doubled separator that splits one file across two target strings `sort -u` cannot collapse. The added cases asserted the file count alone, which does not pin the invariant they name: an expansion that discards the caller's spelling and rebuilds each path from `git rev-parse --show-toplevel`, the anchor this change exists to remove, still scans exactly one file and passed every one of them. They now assert the emitted path. Verified by mutating the implementation three ways, each of which the strengthened assertions catch and the previous ones did not. Adds subtree-restriction and non-ASCII-filename cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XtbWChCVfUWAv1Pi5Qk2hA
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4765f9e09
ℹ️ 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".
| fi | ||
|
|
||
| while IFS= read -r rel; do | ||
| [[ -n "$rel" ]] && printf '%s/%s\n' "$dir" "$rel" |
There was a problem hiding this comment.
Normalize directory paths before repo-relative glob matching
When a directory target is spelled . or ./docs, this concatenation produces paths such as ./allowed.md. matches_glob only removes an absolute $REPO_ROOT/ prefix, so repository-relative configuration patterns such as allowed.md or docs/** no longer match; consequently excluded_paths and em_dash_allowed_paths are ignored for the common detect.sh . invocation. The previous expansion emitted absolute paths and did not have this regression, so derive a normalized repo-relative path for glob matching while preserving the caller spelling in reported paths.
Useful? React with 👍 / 👎.
|
Closing as a duplicate. #3270 landed the same directory-expansion fix as 0.3.8 while this branch was in review, and it resolved #3266. Its Three defects this branch also carried are not in the merged version and survive on |
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> Closes #3332 ## Summary Three defects in `detect.sh` directory expansion survived the 0.3.8 path-anchor fix: a tracked non-ASCII filename was dropped, a filesystem walk still ran silently when git could not answer, and the new tests asserted only a scan count so the original spelling-rebuild defect could return unnoticed. ## Fix - `git -c core.quotePath=false ls-files` so a filename holding an em dash is actually opened. - Report on stderr when git is missing or `rev-parse --is-inside-work-tree` fails; a silent walk remains only when git reports the directory is genuinely outside a checkout. - Dir-target tests now assert the emitted `file=` path, subtree restriction, a non-ASCII filename, and the git-absent report. - `normalize_dir_target` still keeps a Windows drive-root slash (`C:/`). `ai-slop` 0.3.9. Duplicate stale PR #3330 is still `DIRTY` and would regress drive-root slash preservation; ignore it. ## Verification - `bash plugins/ai-slop/skills/audit/scripts/detect.test.sh`: 149 cases passed - `scripts/check-changed-skills.sh origin/main`: 0 failed - `scripts/check-changelog-parity.sh --check-bump origin/main`: pass - `python3 scripts/sync-plugin-options-docs.py --check`: up to date ## Related Follow-up to the merged directory-anchor work. Do not merge #3330. <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-363de5b1-6044-4e78-99f7-867a293bcfcf?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-363de5b1-6044-4e78-99f7-867a293bcfcf&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> --------- Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Closes #3266
Summary
detect.shexpanded a directory target by building its path prefix fromgit rev-parse --show-topleveland its filter frompwd. Those two answer in different spellings on Git Bash,so every candidate was dropped and the
|| findfallback quietly ran a filesystem walk in placeof the documented tracked-files-only listing. A directory target therefore audited untracked and
ignored markdown while reporting nothing unusual.
Fix
Expansion now runs
git ls-fileswith-C <dir>, which is already restricted to that directory'ssubtree and answers in paths relative to it. The caller's own spelling of the directory becomes
the only anchor in the pipeline, so no second source remains to disagree with it.
The multi-anchor treatment #3242 gave the two
emit-findings.shproducers (REPO_ROOT_ALT/REPO_ROOT_PWD) is deliberately not copied here. Those producers relativize a path a caller handsthem and must therefore recognize whichever spelling arrives. This expansion constructs its own
paths, so the better answer is to never introduce a second spelling rather than to carry three and
compare against each.
Two further defects in that code path came out of independent review and are fixed in the same
change:
git ls-filesC-quotes any path holding a non-ASCII byte, so a trackedcafé.mdarrived as aliteral
"caf\303\251.md", failed the scan loop's existence test, and produced neither afinding nor a declined row. The listing now sets
core.quotePath=false. The sharpest case forthis plugin is a filename containing an em dash, which the em-dash detector would otherwise
never open. The listing stays newline-delimited rather than moving to
-z, because the reportformat is one line per finding, so a filename holding a newline cannot be represented downstream
whichever way the listing is read, and the newline form keeps the listing's exit status
observable.
--is-inside-work-treerather than from an empty pipeline,so a listing that legitimately finds nothing is no longer indistinguishable from one that
failed. A walk still answers whenever git cannot confirm a work tree, which covers a directory
outside any checkout and equally a git that is absent or refuses to answer. The comment
introduced with the first commit claimed otherwise and has been corrected, and the absent case
now reports on stderr, because tracked-files-only is not achievable without git.
Trailing slashes are stripped in full, so a
docs//target no longer emits a doubled separatorthat splits one file across two target strings
sort -ucannot collapse.Verification
The named case in the repository's own suite, before and after:
Suite totals, run on Git Bash where the two spellings genuinely diverge:
The FAIL set is compared by name, not by count. The three surviving failures
(
config: em_dash_allowed_paths exempts the document,config: disabled rule emits no findings,config: disabled rule reported in summary) are identical on both sides and pre-date this change.They are the separate config-cascade defect and are untouched here; one reviewer traced them to
the Windows
jqbuild emitting CRLF, whose trailing carriage return breaks the string-matchedglob and disabled-rule comparisons. No case that passes on
origin/mainfails on this branch.Directory expansion measured against
plugins/ai-slop, with one untracked markdown file placed inthe tree:
The collapsed filter itself, measured on
origin/main: zero of the thirteen tracked markdownfiles survived it, which is what routed every directory target to the walk.
Path agreement verified directly by passing the same fixture directory in both spellings
(
C:/Users/...and/tmp/...); both now yield the tracked file only, and both emit the caller'sspelling.
The new assertions were confirmed to be load-bearing by mutation rather than by inspection. Three
wrong implementations were built and run against them:
git rev-parse --show-toplevelcore.quotePath=falseThe first row is the important one: re-anchoring on the very source this change exists to remove
still scans exactly one file, so a count-only assertion admits the defect. The added cases assert
the emitted path.
Gates run locally:
shellcheckclean on both scripts,markdownlint-cli2clean on the changelog,scripts/check-changelog-parity.sh --checkand--check-bump origin/mainboth pass with themanifest at 0.3.8.
Review was performed by two independent fresh-context agents on different models, each given only
the diff with the author's rationale withheld. Both reproduced the original defect. One returned
PASS, the other returned FAIL on the quoted-filename defect; every finding either agreed on has
been fixed in this PR, and the coverage gap the second raised is what prompted the mutation
testing above. Please note that this change and that review were produced by autonomous agents,
so the usual human authorship assumption does not hold and the diff deserves a human read on its
own terms.
Related
Refs #3242, which resolved the same class of repo-root spelling mismatch in the two
emit-findings.shproducers and whose approach this change deliberately diverges from, for thereason given under Fix. The pre-existing
REPO_ROOTmismatch inmatches_globand in the scanloop's
relcomputation is left untouched and remains open.