fix(ai-slop): expand directory targets from one path anchor (0.3.8)

[kyle-sexton]

Closes #3266

Summary

detect.sh expands a directory target to the markdown files beneath it. The prefix came from git rev-parse --show-toplevel and the filter from pwd. On hosts where those spellings differ (Git Bash: D:/repo vs /d/repo), no prefixed candidate survived the filter, so || find silently replaced the tracked-files listing. The walk includes untracked and ignored markdown.

Fix

Directory 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. The branch is chosen up front from --is-inside-work-tree, 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.

ai-slop 0.3.8.

Verification

• plugins/ai-slop/skills/audit/scripts/detect.test.sh — 132/132
• dir target in git repo: only the tracked file counts — PASS
• scripts/check-changelog-parity.sh --check-bump origin/main — pass
• --check-preserved origin/main — 0.3.7 heading kept
• --check-order — pass

Related

Refs #3242

[claude]

Claude finished @kyle-sexton's task in 1m 7s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review detect.sh changes for security-relevant surface
• Review test file changes
• Post findings / no-issues summary

No security issues found.

This PR is a bug fix to plugins/ai-slop/skills/audit/scripts/detect.sh's directory-target expansion (expand_dir_target, detect.sh#L277-L297). It replaces a git ls-files + sed/grep prefix-matching pipeline (which silently fell back to a raw find walk whenever git's and the shell's spellings of the checkout path disagreed) with git -C "$dir" ls-files '*.md', gated up front by git -C "$dir" rev-parse --is-inside-work-tree.

Notes from a security lens:

• No new command-injection surface: $dir is always passed as a quoted argument to git -C / find, never interpolated into a shell string that gets re-evaluated.
• TARGETS values come from the invoking user's own CLI arguments or --paths-file/repo's own tracked-file listing — this is a local read-only audit CLI, not a network-facing service parsing untrusted/attacker-supplied input.
• The behavioral change is a net security improvement, not a regression: the old fallback silently walked the filesystem and could scan/report on untracked and gitignored markdown (which may hold local secrets/drafts never meant to be surfaced); the fix scopes directory expansion strictly to tracked files inside a checkout, and now reports a failed ls-files on stderr instead of degrading into a different file set.
• Script remains read-only (set -u, always exits 0 on audit paths); no new file writes, no eval/sourcing of external content, no credential or token handling introduced.

Per the skip gate in /review:security-review, this change has no security-relevant surface warranting deeper review, and per the criteria (logic/trust-boundary/auth/injection/Actions-security) I found no exploitable path in this diff.

[github-actions]

Last security-reviewed head: 3a7784264f43f4a527c46c88a6c4f545f702c5b4. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @kyle-sexton's task in 1m 7s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review detect.sh / expand_dir_target changes for security-relevant surface
• Review test file, changelog, and manifest changes
• Post findings / no-issues summary

No security issues found.

Reviewed the full diff at HEAD 26b07b4 against origin/main. Note: the prior review's recorded head (8e3a3bc) is not an ancestor of this head, so the cheap gate's "already reviewed" shortcut doesn't apply cleanly — I re-reviewed the diff in full rather than relying on it.

The change replaces a git ls-files --full-name + sed/grep prefix-matching pipeline with a new expand_dir_target() function (detect.sh#L277-L296) that:

1. Gates on git -C "$dir" rev-parse --is-inside-work-tree up front to choose the tracked-listing vs. filesystem-walk branch, instead of letting an empty pipeline silently fall through to find.
2. Uses git -C "$dir" ls-files '*.md' (scoped to that directory's own subtree, answered relative to it) instead of reconciling two different path spellings (git rev-parse --show-toplevel vs. pwd).

Security-relevant observations:

• No new injection surface. $dir is always passed as a single quoted argument to git -C / find, never interpolated into a re-evaluated shell string. git -C <path> takes the next token as a literal path value, not as an option, so path values starting with - aren't reinterpreted.
• $dir is not attacker-controlled input. TARGETS come from the invoking user's own CLI args or the repo's own tracked-file listing — this is a local, read-only audit CLI, not a service parsing untrusted network input. Same trust model as before.
• Net security improvement, not a regression. The old fallback could silently walk the filesystem and report on untracked and gitignored markdown (which may hold local secrets/drafts never meant to be surfaced) whenever the git/shell path spellings disagreed (e.g., Git Bash D:/repo vs /d/repo). The fix scopes directory expansion strictly to tracked files inside a checkout, and a failed ls-files now reports on stderr instead of silently degrading into a different file set.
• Script remains read-only (set -u, no new file writes, no eval/sourcing of external content, no credential/token handling).
• Test additions (detect.test.sh) only add coverage (path-spelling agreement, trailing slash, untracked-only directory) and introduce no new script logic.

No CRITICAL / IMPORTANT / SUGGESTION findings to report.

[claude]

Claude finished @kyle-sexton's task in 1m 25s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Post findings / no-issues summary

No security issues found.

Skip-gate condition 3 applies: this PR already has a successful security review (prior review) at commit 8e3a3bc, and it still applies to the current HEAD (26b07b4). I diffed 8e3a3bc...26b07b4 scoped to this PR's four changed files and the only change is comment-text edits in detect.sh and detect.test.sh — dropping bare "(issue 3266)" tracker-ref parentheticals per comment-hygiene. No executable logic, test assertions, or Actions/config surface changed since the prior review.

The prior review's conclusion holds: expand_dir_target now scopes directory expansion to git -C "$dir" ls-files '*.md' (gated by --is-inside-work-tree), with no shell re-interpolation of $dir, no new file writes, and a net security improvement over the old fallback (which could silently walk and surface untracked/gitignored markdown).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26b07b4336

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 3m 2s —— View job

---

Code review — /review:code-review

• Gather context (PR diff vs origin/main, changed files)
• Apply skip gate (PR open, not draft, real review surface — proceeding)
• Review detect.sh — expand_dir_target logic
• Review detect.test.sh additions
• Review CHANGELOG.md / plugin.json version bump
• Post findings

No blocking correctness or maintainability issues found.

What changed

expand_dir_target() (new function, detect.sh#L277-L297) replaces the old ls-files --full-name | sed | grep -F ... || find pipeline. Traced through the logic:

• --is-inside-work-tree gates the branch up front, so the filesystem-walk fallback (L282-L285) is reachable only for a directory genuinely outside a checkout — it can no longer be triggered by a spelling mismatch, which was the actual bug (ai-slop audit: directory targets silently fall back to an untracked-inclusive filesystem walk when git and shell path spellings differ #3266).
• git -C "$dir" ls-files '*.md' is run without --full-name, which is correct here: dropping it makes the output relative to -C's directory (i.e. $dir itself) rather than the repo root, matching what the printf '%s/%s\n' "$dir" "$rel" reassembly expects.
• The exit-status capture (listing="$(...)" on its own line, then status=$? as a separate statement) correctly avoids the classic local status=$(...) bug where wrapping the assignment in local clobbers $? before it can be read — worth calling out since it's an easy mistake to introduce and this PR avoids it.
• The ${1%/} trailing-slash strip plus the dir="$1" fallback for the root-only case (L278-L279) is a sound way to avoid a doubled separator in the reassembled path without breaking on / as the target.
• No set -e is active in this script (only set -u), so the plain-assignment command substitution for listing doesn't risk an early exit before status=$? runs.

Test coverage (detect.test.sh#L407-L440) exercises the git-spelling vs. shell-spelling case, a trailing slash, and a git-tracked directory whose markdown is entirely untracked (confirms the walk fallback doesn't fire inside a checkout). That lines up with the fix's stated contract.

Not independently verified

I don't have an approved way to execute shell scripts in this session, so I read the diff and traced the logic rather than running detect.test.sh or check-changelog-parity.sh. The PR body's 132/132 test-suite result and changelog-parity checks are author-claimed, not something I confirmed myself.

[claude]

Claude finished @kyle-sexton's task in 1m 18s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changes since last-reviewed head for security-relevant surface
• Post findings / no-issues summary

No security issues found.

The previously-recorded reviewed head (26b07b4) is an ancestor of the current HEAD (8fedb24), so I scoped this pass to the diff between them rather than the whole PR. That diff is exactly the follow-up commit addressing the Codex review comment on this PR: preserving the trailing slash on Windows drive-root targets (C:/) so normalization doesn't turn it into C: (drive-relative, not drive-root).

Changes in scope (detect.sh#L283-L318):

1. New normalize_dir_target() (detect.sh#L283-L290) strips one trailing /, except when the result would be a bare Windows drive letter (C:) or an empty string (Unix root /) — both cases keep the original spelling.
2. expand_dir_target() now calls this helper, and its path-join step (detect.sh#L309-L316) checks whether dir already ends in / or \ before choosing the join separator, so a preserved drive-root slash doesn't get doubled.
3. Test additions are pure string-contract assertions against the sourced helper — no script logic changes there.

Security-relevant observations:

• No new injection surface. $dir is still only ever passed as a single quoted argument to git -C / find; the glob comparisons ([[ "$dir" == [A-Za-z]: ]], [[ "$dir" == */ || "$dir" == *\\ ]]) are plain [[ ]] pattern matches, not eval'd or re-interpolated into a shell command string.
• No change in trust boundary. $dir is still sourced from the invoking user's own CLI arguments / --paths-file, same as the code already reviewed on this PR — not attacker-supplied network input.
• Behavior-only fix, net-neutral to positive on security posture. Before this commit, a Windows drive-root target like C:/ would normalize to C:, which Windows treats as "cwd on that drive" rather than the drive root — so git -C/find could silently resolve to an unintended directory. That's the same class of silent-wrong-directory risk this PR's underlying fix (0.3.8) was already correcting for POSIX path-spelling mismatches; this commit closes the same gap for the Windows drive-root edge case rather than introducing new risk.
• No new file writes, no eval/sourcing of external/untrusted content, no credential or token handling.

No CRITICAL / IMPORTANT / SUGGESTION findings to report.

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 3m 43s —— View job

---

Code Review

• Gather context (PR diff against merge-base b5e3386, full file reads)
• Review detect.sh — normalize_dir_target / expand_dir_target
• Review detect.test.sh additions
• Review CHANGELOG.md / plugin.json version bump
• Check CI status
• Post findings

No new findings.

What changed

expand_dir_target now anchors directory expansion to git -C "$dir" ls-files '*.md' (gated by --is-inside-work-tree chosen up front, rather than falling through an empty pipeline into find). This replaces the old git rev-parse --show-toplevel + pwd prefix/filter pair that silently diverged on hosts where the two spell the same checkout differently (Git Bash D:/repo vs /d/repo), which previously caused || find to run in its place and pull in untracked/gitignored markdown. See detect.sh#L259-L317.

Codex P2 finding — verified resolved

The earlier automated review flagged that stripping a trailing slash from a Windows drive root (C:/ → C:) changes its meaning to drive-relative on Windows. Commit 8fedb24 addresses this directly: normalize_dir_target (detect.sh#L283-L290) strips one trailing / except when the result matches the single-letter-plus-colon pattern ([A-Za-z]:), in which case the original (unstripped) spelling is returned. I traced this against the cases that matter:

• C:/ → stays C:/ (drive root preserved)
• d:/ → stays d:/ (lowercase handled, pattern is case-insensitive by class)
• C:\ → unchanged (strip only removes /, not \)
• docs/ → docs (ordinary case still normalized)
• / → stays / (Unix root, empty-after-strip guard)
• C: (already drive-relative, no trailing slash) → unchanged, as expected

The concatenation logic in expand_dir_target (detect.sh#L309-L316) correctly avoids a doubled separator when dir still ends in / or \ (the drive-root case), and adds one otherwise.

Test coverage for this is direct: detect.test.sh sources the production normalize_dir_target function via sed extraction (so it can't drift from what expand_dir_target actually calls) and asserts all of the above cases individually (detect.test.sh#L417-L430), plus the git/shell path-spelling-agreement and untracked-only-directory scenarios that motivated the original fix.

Other changes

• CHANGELOG.md and plugin.json (0.3.7 → 0.3.8) are consistent with scripts/check-changelog-parity.sh, per the PR description's verification section.
• No new command-injection or trust-boundary surface: $dir is always a quoted argument to git -C / find, never re-interpolated into an evaluated string. Three separate automated security-review passes on this PR (8e3a3bc, 26b07b4 diff, and the no-op-diff skip at 26b07b4) already confirmed this; nothing in the final commit changes that analysis.

Note on verification

This review's sandbox blocked running detect.test.sh directly and blocked git fetch/gh for a live CI check, so I verified by full manual trace of normalize_dir_target/expand_dir_target against the diff (base b5e3386, which matches the PR's reported +134/-7 across the same 4 files) rather than by executing the suite myself. The PR description's own run (132/132) and this repo's portability-lint / shell-portability-lint CI jobs (both green on this run) cover that ground independently.
· branch

[claude]

Claude finished @kyle-sexton's task in 1m 19s —— View job

---

Security review

• Gather context (PR diff against origin/main, changed files)
• Apply skip gate
• Review changes since last-reviewed head for security-relevant surface
• Post findings / no-issues summary

No security issues found.

Skip-gate condition 2/3 applies. The previously-recorded reviewed head (8fedb24e) is not a direct ancestor of the current HEAD (3a77842) — history was rewritten between passes — so I diffed 8fedb24e..HEAD directly rather than relying on the ancestor shortcut. The only change in that range is in detect.test.sh#L440:

-assert_eq "nested trailing slash is stripped" "$(normalize_dir_target "C:/Users/")" "C:/Users"
+assert_eq "nested trailing slash is stripped" "$(normalize_dir_target "C:/tmp/")" "C:/tmp"

This swaps a hardcoded test literal (C:/Users/ → C:/tmp/) for portability, per the commit message. It's a string-in/string-out assertion against the sourced normalize_dir_target helper — no filesystem access, no script logic touched, nothing that changes detect.sh's behavior or trust surface.

plugins/ai-slop/skills/audit/scripts/detect.sh is byte-identical to what was already reviewed at 8fedb24e — confirmed by diffing the two directly. git diff --stat origin/main..HEAD also matches the PR's reported +134/-7 across the same 4 files, so no other change is in scope.

The prior reviews' conclusions on this PR stand: expand_dir_target/normalize_dir_target scope directory expansion to git -C "$dir" ls-files '*.md' (gated by --is-inside-work-tree), preserve Windows drive-root spellings, never re-interpolate $dir into an evaluated shell string, and only ever operate on the invoking user's own local CLI-supplied paths — not attacker-controlled input. No CRITICAL / IMPORTANT / SUGGESTION findings to report.
· branch

[claude]

Claude finished @kyle-sexton's task in 2m 46s —— View job

---

Code Review

• Gather context (PR diff origin/main...3a77842, full changed-file reads)
• Apply skip gate (PR open, not draft, real review surface — proceeding)
• Review detect.sh — normalize_dir_target / expand_dir_target
• Review detect.test.sh additions, including the new assert_eq helper and drive-root test cases
• Review CHANGELOG.md / plugin.json version bump
• Post findings

No high-signal correctness or maintainability findings.

What changed since the last full pass

The prior five review rounds on this PR (three security, two code-review) already covered expand_dir_target's core rewrite — anchoring directory expansion to git -C "$dir" ls-files '*.md' gated by --is-inside-work-tree, and the follow-up normalize_dir_target helper that preserves a Windows drive-root's trailing slash (C:/ staying C:/ instead of normalizing to the drive-relative C:). Nothing in those functions changed in the current HEAD.

The only new commit is 3a77842, a one-line test-data change in detect.test.sh#L440: the "nested trailing slash is stripped" case swaps its input from C:/Users/ to C:/tmp/, per the commit message, because ai-slop's own machine-specific-paths rule flags C:/Users/ as a Windows user path. Traced the substitution against normalize_dir_target: both strings hit the same code path (stripped result doesn't match the empty or single-letter-plus-colon guard), so C:/tmp is an equivalent assertion — no coverage lost.

Full-diff re-trace (not just the delta)

Walked normalize_dir_target/expand_dir_target end-to-end against the diff independently of the prior rounds' conclusions:

• normalize_dir_target (detect.sh#L283-L290): ${1%/} strips one trailing slash; the guard -z "$dir" || "$dir" == [A-Za-z]: (bash pattern class, exactly one letter + colon) catches both the Unix-root case (stripped to empty) and the Windows-drive-root case, returning the original unstripped $1 in both. Traced all eight test cases in detect.test.sh against this and each holds.
• expand_dir_target (detect.sh#L292-L318): the --is-inside-work-tree gate is evaluated up front (not as an || fallback), so find is reachable only when $dir is genuinely outside a checkout. status=$? is captured on its own statement (not folded into a local assignment), avoiding the classic bug where local x=$(...) clobbers $? before it's read. The join logic checks for an existing trailing / or \ before choosing a separator, so the preserved drive-root slash from normalize_dir_target doesn't double up.
• Test coverage sources the production normalize_dir_target directly via sed extraction (detect.test.sh#L436-L437), so the test can't drift from what expand_dir_target actually calls.
• CHANGELOG.md / plugin.json bump (0.3.7 → 0.3.8) is internally consistent with the described fix.
• Per REVIEW.md's code-review-lane scope (audit-log completeness on high-risk security actions; atomicity on multi-location writes): neither applies — this is a read-only, single-process audit script with no state writes.

Not independently verified

This lane's Bash grant is scoped to gh pr diff/view/comment/review only, so I could not execute detect.test.sh or check-changelog-parity.sh myself in this session. I verified by full manual trace against the diff instead. The PR description's 132/132 → 140/140 test-suite results and changelog-parity checks are author-claimed and unverified by me directly.
· branch