fix(docs-hygiene): gate audit-noise's rename split and unescape backslashes (0.18.4)

[claude]

Closes #3143

Summary

audit-noise's git status --porcelain parse had two defects that made markdown files disappear from a no-argument run. Both failed silently — the file dropped out of the audit and the run reported a reassuring files=0 rather than an error, the same false-negative class as #3126 in the sibling code-tidying/audit-comment-residue.

Fix

1. Ungated rename split (scripts/detect.sh). The split fired on any record whose path contained " -> ", not only on a rename, so a file literally named notes -> draft.md was reduced to draft.md — a name matching nothing on disk.

-      if [[ "$local_path" == *" -> "* ]]; then
+      if [[ "${line:0:1}" == [RC] || "${line:1:1}" == [RC] ]]; then

Gating on the status letter is both narrower and complete. Both columns matter: X is the index status and Y the worktree status, and a rename staged only as intent-to-add lands in Y (mv old new && git add -N new emits " R old -> new"), so gating on X alone would leave that record unsplit and unresolvable.

2. \\ not unescaped (scripts/detect.sh). \" → " was handled; \\ → \ was not, so a path such as back\-slash.md stayed escaped and resolved to nothing.

       local_path="${local_path//\\\"/\"}"
+      local_path="${local_path//\\\\/\\}"

Order matters — \" before \\, or the backslash pass re-creates a quote the quote pass has already consumed. Case E covers the interleaved form.

3. Same defect class in SKILL.md. The Uncommitted .md files: preview line shares the class rather than the code — it filters with a grep, not with detect.sh's parse. A C-quoted path ends with the closing quote, not .md, so every spaced, arrowed, backslashed or quoted markdown file was dropped from the preview:

-!`git status --porcelain 2>/dev/null | grep '\.md$' | head -10 ...`
+!`git status --porcelain 2>/dev/null | grep -E '\.md"?$' | head -10 ...`

Note the two parsers previously failed in opposite directions on renames — audit-noise over-split (ungated), audit-comment-residue pre-#3140 under-split (index column only) — so this is a port of the gated-on-both-columns form, not a copy of either.

Verification

Six regression cases added to scripts/detect.test.sh, each built as its own git fixture so a correctly-parsed file cannot keep files= above zero and mask another arm's disappearance.

Case	Covers
A	ordinary path containing " -> " survives intact
B	genuine rename still resolves to the new path (the gate narrows, not removes)
C	intent-to-add rename recorded in the worktree column
D	embedded backslash unescapes
E	interleaved \" + \\ proves the unescape order
F	SKILL.md preview keeps a quoted .md path (extracted and executed, so the surfaces cannot drift)

Each case was verified to fail against the unfixed surface and pass against the fixed one — not asserted by inspection:

$ bash .../detect.test.sh          # detect.sh reverted to pre-fix
7/72 checks failed.
$ bash .../detect.test.sh          # SKILL.md grep reverted to pre-fix
FAIL: SKILL.md preview keeps a quoted .md path
$ bash .../detect.test.sh          # both fixes applied
All 73 checks passed.

Gates run against the source checkout:

shellcheck                                  clean
shfmt -d                                    clean
markdownlint-cli2                           0 issues in 0 files
typos / editorconfig-checker                clean
CHECK-SKILL audit-noise                     PASS — 0 errors, 1 warning
  └ all 6 base-ref trigger phrase(s) preserved

The one check-skill warning (no Gotchas surface) is pre-existing and untouched by this PR.

The suite builds its first git fixtures, so it also picks up the #2872 git-environment isolation guard (unset GIT_DIR GIT_WORK_TREE GIT_CONFIG).

Known residual, recorded at both sites rather than left implicit: neither surface decodes git's octal escapes for control and non-ASCII bytes, so those paths still miss. Converging both skills on git status --porcelain -z would close the whole class outright — NUL-delimited, unquoted paths — at the cost of handling renames as two separate fields instead of old -> new. That is the better long-term fix and is deliberately not taken here.

docs-hygiene 0.18.3 → 0.18.4, with the matching CHANGELOG entry.

Related

• Refs #3126 — the sibling defect in code-tidying/audit-comment-residue, whose fix this ports.
• Refs #3140 — the open PR carrying that fix; the gated-on-both-columns form comes from its post-fix parser. This PR does not depend on it merging.
• Refs #2872 — the fixture git-environment isolation guard the new git fixtures rely on.

🤖 Generated with Claude Code

https://claude.ai/code/session_018ec3EJLqCD2kjdzagyryg4

---

Generated by Claude Code

[claude]

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

---

Security review complete ✅

• Gather context (PR diff, changed files)
• Run skip-gate checks (open state, security-relevant surface, prior review)
• Review diff for security issues (bash quoting/injection, git parsing, GitHub Actions surfaces)
• Post findings or clear result

No security issues found.

This PR touches only the docs-hygiene plugin's audit-noise skill (detect.sh, detect.test.sh, SKILL.md, plugin.json, CHANGELOG.md) — a read-only local audit tool with no network, secrets, auth, or GitHub Actions surface.

Reviewed specifically for injection risk in the changed git status --porcelain parsing (detect.sh, lines 122–145):

• The rename-split gate and backslash-unescape fix only change how local_path is derived from porcelain output; every downstream use remains properly quoted ([[ -f "$file" ]], find "$target" ..., <"$file"), so no new command-injection or path-traversal surface is introduced.
• The SKILL.md preview-line grep change (line 16) only affects a display filter, not an executed path.
• detect.test.sh adds an eval on a grep expression extracted from SKILL.md (lines ~540–548), but the evaluated content is sourced from a file in this same repo/PR (not external or user-controlled input) and only runs in the local test harness, so it isn't an exploitable trust-boundary issue.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

[github-actions]

Last security-reviewed head: d605d8326c06dab3c1df1355cf8c7379f582d710. 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 6m 43s —— View job

---

Claude finished the code review

• Gather context (PR diff, changed files)
• Run /review:code-review per instructions
• Post findings as inline comments / summary
• Final summary

Summary

Reviewed the porcelain-parse fix in plugins/docs-hygiene/skills/audit-noise/scripts/detect.sh, the matching SKILL.md preview fix, the detect.test.sh regression cases, and the version/changelog bump.

The core fixes are correct. I cross-checked the gated rename-split condition ("${line:0:1}" == [RC] || "${line:1:1}" == [RC]) against the sibling implementation this ports from (plugins/code-tidying/skills/audit-comment-residue/scripts/detect.sh:106-116, from #3140) and it's an exact, faithful port — same both-columns gating logic. The \"/\\ unescape addition and the SKILL.md grep -E '\.md"?$' fix both plausibly close the described false-negative (files=0) failure mode.

One finding posted inline (detect.sh:142): the claim that the \"-before-\\ unescape order matters — made in the code comment (L135-137), the PR description, and detect.test.sh case E's stated purpose (L520-535) — appears to not actually hold. Working through ${var//pattern/repl}'s non-overlapping scan semantics on git's two-token C-quote alphabet, I couldn't construct a case (including case E's own fixture) where swapping the two unescape lines changes the result — the shipped order is still correct, but the "order is load-bearing" rationale and the test's claim to "prove" it appear unsubstantiated. Not a functional bug in the shipped code; worth a quick swap-and-rerun to confirm before the comment/test language potentially misleads a future refactor. I couldn't execute a script in this sandbox to verify empirically, so this is offered as a plausible finding, not a confirmed one.

Other things checked, no issues found:

• Version bump (0.19.0 → 0.19.1) and CHANGELOG.md entry are correctly sequenced and don't conflict with the prior 0.19.0 entry already on main. (The PR description's "0.18.3 → 0.18.4" is stale relative to the synced branch — description text only, not a code issue.)
• Regression test fixtures (cases A-F) use isolated per-case git repos as described, and the new unset GIT_DIR GIT_WORK_TREE GIT_CONFIG guard is present.
• The greedy ${local_path##* -> } split correctly resolves to the rightmost " -> " boundary even when a quoted old-path name itself contains the literal substring " -> ".

[claude]

Correctness of the stated rationale (not the code): the "order matters" claim for this two-pass unescape appears to be false.

The comment (L135-137), the PR description, and detect.test.sh case E (L520-535) all assert that \" must be unescaped before \\, or "the backslash pass re-creates a quote the quote pass has already consumed."

Working through ${var//pattern/repl}'s non-overlapping left-to-right semantics on git's C-quote alphabet (only two escape tokens exist: \\→\ and \"→", both 2 chars, distinguished by their second character), a straddle position formed from the tail of one token and the head of the next can never spuriously equal either target pattern — the second character of a straddle is always the fixed head-char (\), which only matches the \\ pattern's second slot, never \"'s. That holds for every token-type combination, so scanning for either pattern always resolves at true token boundaries regardless of which pass runs first.

Concretely, for case E's fixture (both\".md, encoded as \\\"= 4 chars: \,\,\,"):

• quote-first then backslash-first: \\\" → (quote pass) \\" → (backslash pass) \" ✓
• backslash-first then quote-first: \\\" → (backslash pass) \\" → (quote pass) \" ✓

Both orders land on the same correct 2-char result. So case E doesn't actually distinguish the orders — swapping the two ${local_path//...} lines would still pass it. (I derived this by hand; I wasn't able to execute a script in this sandbox to confirm empirically, so treat this as worth a 30-second sanity check by literally swapping L141/L142 and re-running detect.test.sh.)

This doesn't make the shipped code wrong — the order chosen is fine — but the comment and the test's stated purpose assert an invariant that isn't real, which could mislead a future refactor into thinking the order is load-bearing. Note the ported-from sibling (code-tidying/audit-comment-residue/scripts/detect.sh:118-124) makes no such order claim, which lines up with this.

Fix this →

[github-actions]

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

[kyle-sexton]

No description provided.

[kyle-sexton]

Closing as superseded — but the half of this PR that #3171 declined was right, and it is being carried forward rather than dropped.

What happened. #3171 merged as 0.19.1 and landed the detect.sh half: the [RC]-gated rename split and the \\ unescape. Against current main this branch's detect.sh changes are now redundant, and main also already carries the unset GIT_DIR GIT_WORK_TREE GIT_CONFIG isolation line and equivalents of cases A–E.

What was not redundant. #3171 stated that the mirrored-parser half of #3143 "does not apply here", because SKILL.md uses a plain grep '\.md$' pipeline rather than detect.sh's parse. That reasoning does not hold — sharing the defect class is the point, not sharing the code. A C-quoted porcelain record ends with the closing quote, not .md, so that grep matches nothing for my notes.md:

$ git status --porcelain
?? "my notes.md"
?? plain.md

$ git status --porcelain | grep '\.md$'
?? plain.md          # "my notes.md" silently dropped

Reproduced against main today, so the preview defect is live.

Carried forward in #3187, which takes this PR's one-line grep -E '\.md"?$' fix and its case-F approach — extracting the grep out of SKILL.md and executing it rather than restating it, which is the part that keeps the two surfaces from drifting again. Verified as a discriminator: reverting only the SKILL.md line fails exactly that assertion while the ordinary-path assertion still passes.

Closing here rather than rebasing because most of this diff would now conflict with the landed 0.19.1 work for no gain.

---

Generated by Claude Code