fix(code-tidying): parse porcelain by slicing so spaced paths survive discovery

[kyle-sexton]

Closes #3126

Summary

audit-comment-residue's default-target router dropped any uncommitted path containing a space, then reported the run as files=0 with the note no code targets. An audit that errors prompts a retry; one that confidently reports a clean tree ends the investigation, so this false negative was worse than a crash would have been.

Fix

plugins/code-tidying/skills/audit-comment-residue/scripts/detect.sh parsed git status --porcelain with awk '{print $NF}'. A spaced path was split on its space and git's closing quote was kept, so the reconstructed path named no file and fell out of the target list.

The parse now slices the path out of the porcelain record (${line:3}) instead of splitting on whitespace, takes the right-hand side of a rename, and unwraps git's C-quoting (including the \" and \\ escapes inside it).

Two deliberate choices beyond a straight port of the sibling docs-hygiene/audit-noise block the issue pointed at:

• The rename split is gated on the R/C status letter in either column. Porcelain's short format is XY — X is the index status, Y the worktree status — and a rename can be recorded in either. Ungated entirely, ${path##* -> } would also mangle an ordinary path containing " -> ".
• The residual limitation is recorded at the parse site. Git's octal escapes for control and non-ASCII bytes are still not decoded, so such a path continues to miss. That is a comment and a CHANGELOG sentence rather than an implicit gap.

The skill's own Uncommitted code files pre-computed-context line in SKILL.md carried the identical $NF parse and would have previewed the same false negative to the model, so it is brought to full parity — same column handling, same unescaping. That is Boy Scout scope: the same defect in the same skill, not a widening of the issue.

Review findings, and what they changed

Three commits, each closing a defect the previous one did not. Both review findings were confirmed by local reproduction before being fixed, not taken on assertion.

Commit	What it closes	Found by
6017877e	The reported bug: spaced paths dropped by the $NF split	#3126
92e04c96	Renames recorded in the worktree column (Y) left unsplit	Codex review
04558340	SKILL.md parser missing \"/\\ unescaping, and untested	/review:code-review

92e04c96 was a regression this PR introduced. mv old.py new.py && git add -N new.py emits R old.py -> new.py — R in Y, X blank. The X-only gate left the arrow in place, so the record resolved to nothing and the run reported files=0: the exact failure shape this PR exists to remove. awk '{print $NF}' had returned new.py correctly. Fixtures 9b/9c could not have caught it because git mv always stages into X.

04558340 fixed the cause, not just the symptom. The SKILL.md parser stripped the surrounding quotes but never unescaped what was inside them, so a filename C-quoted for an embedded quote or backslash — rather than a space — was still previewed wrong. It survived because that parser had no automated coverage at all. Verified against real files on disk:

on disk	porcelain	before	after
quote".py	"quote\".py"	quote\".py	quote".py
back\-slash.py	"back\\-slash.py"	back\\-slash.py	back\-slash.py
plain space.py	"plain space.py"	✓	✓

Verification

Reproduced first, per the bug-investigation rule:

=== porcelain ===
R  original.py -> renamed.py
?? "my helper.sh"
=== awk '{print $NF}' parse (pre-fix) ===
renamed.py
helper.sh"          <-- mangled; resolves to nothing

Pre-fix the default-target run found 1 of 2 files; post-fix it finds both. Passing the same spaced file explicitly always found the residue, confirming the finding existed and only discovery missed it.

Regression coverage — sections 9 and 10 of detect.test.sh, 18 new assertions.

Section 9's arms live in separately-rooted fixture repos on purpose: sharing one lets a correctly-parsed file hold files= above zero and mask another arm's disappearance. That is not hypothetical — the first draft shared a repo and only 1 of 5 assertions caught the bug. Arms: spaced path (9a), index rename (9b), spaced rename (9c), worktree-column rename via git add -N (9d).

Section 10 guards the SKILL.md parser by extracting the awk program out of SKILL.md and executing it, rather than restating it in the test — a restatement would keep passing while the real line rotted, which is exactly how the divergence survived. It also asserts every file detect.sh audits appears in the preview, so the two cannot disagree about the tree.

Discrimination was measured, not assumed. Each fix was reverted in place and the suite re-run:

Guard	Assertions failing against the pre-fix code
Section 9a–9c vs. the $NF split	6 of 8
Section 9d vs. the X-only gate	2 of 2
Section 10 vs. the quote-strip-only parser	4 of 7

The assertions that pass either way cover the plain-rename path, which resolved correctly under $NF only by accident of field order.

Gates run locally, all clean:

Gate	Result
detect.test.sh	All 45 checks passed (27 pre-existing + 18 new)
All 5 affected suites (scripts/affected-tests.sh)	0 failing
shellcheck · shfmt -d -i 2	clean · no diff
check-shell-portability.sh	no unexcused GNU-only constructs
editorconfig-checker · typos · markdownlint-cli2	clean
check-changelog-parity.sh --check / --check-order / --check-bump	pass
check-changed-skills.sh	PASS — 0 errors, 1 pre-existing warning
gitleaks	no leaks

Version bumped 0.13.2 → 0.13.3 with the matching CHANGELOG entry.

One incidental note for reviewers: the backslash fixture is named back\-slash.py rather than back\slash.py because shell-portability-lint substring-matches \\s and reads the filename as a GNU-only regex class. Renaming avoids a portability-ok suppression that a future reader would have to re-litigate, and tests the same thing.

Related

• Refs #2872 — the most recent prior change to this suite (fixture git-environment isolation), whose unset GIT_DIR GIT_WORK_TREE GIT_CONFIG guard the new fixtures rely on.
• Refs #3143 — filed from this PR. The sibling docs-hygiene/audit-noise parse has two related defects, found while checking whether the port was faithful. Its rename split is ungated (*" -> "*), so it fails in the opposite direction from this skill's pre-fix parse: it over-splits and mangles an ordinary path literally containing " -> ", where this one under-split. It also unescapes \" but not \\. Deferred rather than fixed here because it is a second plugin, with its own version bump and CHANGELOG.
• Neither parser decodes git's octal escapes for control and non-ASCII bytes. Converging both on git status --porcelain -z would close that class outright; fix(docs-hygiene): audit-noise porcelain parse mangles paths containing " -> " and leaves \\ escaped #3143 carries that suggestion, and it is deliberately not attempted here.

---

Generated by Claude Code

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Run skip-gate check
• Review changed files for security-relevant issues
• Adversarially validate any candidate findings
• Post final report

No security issues found.

This PR changes how plugins/code-tidying/skills/audit-comment-residue/scripts/detect.sh (and the matching one-liner in SKILL.md) parses git status --porcelain output — replacing an awk '{print $NF}' whitespace split with a slice-based parse (${line:3}), a status-letter-gated rename split, and C-quote unwrapping. I reviewed the full diff against the security-review criteria (injection, trust-boundary/authorization gaps, unsafe credential handling, and CI/Actions risk):

• No injection surface. Every derived path (status_path, TARGETS, EXPANDED) is only ever used in quoted contexts — [[ -f "$file" ]], find "$target", <"$file" redirection, printf '%s\n' — never passed to eval, sh -c, or otherwise re-interpreted as shell code. This holds whether or not a crafted filename contains shell metacharacters, backticks, or $( ), so the new parsing doesn't add a code-injection path that didn't already effectively exist (and didn't exist before either).
• Trust boundary unchanged. Targets are still sourced from the local repo's own git status output (the developer's own working tree) or explicit CLI args — no new untrusted/remote input is introduced.
• Quoting/anchoring logic is sound. The ${TARGETS[@]+"${TARGETS[@]}"} idiom, the cr_anchor_path absolute-path check, and the rename-split gate ("${line:0:1}" == [RC], avoiding mangling an ordinary path containing " -> ") are all correctly quoted array/string operations.
• No workflow, permissions, dependency-pin, or secret-handling changes — the diff touches only a read-only audit script, its doc comment/preview line, a test file, the CHANGELOG, and a version bump in plugin.json.
• The one documented residual gap (git's octal escaping of control/non-ASCII bytes in filenames still isn't decoded) is a correctness limitation, not a security issue — it can only cause a file to be skipped, never mis-resolved to an unintended path.

No CRITICAL/IMPORTANT/SUGGESTION findings to report.

[claude]

Claude finished @kyle-sexton's task in 9m 15s —— View job

---

Claude finished the /review:code-review pass

Todo list

• Gather context (PR body, changed files, diff)
• Run /review:code-review and follow its instructions
• Post findings as inline PR comments
• Post final summary

Summary

Reviewed plugins/code-tidying/skills/audit-comment-residue/scripts/detect.sh, its test suite, SKILL.md, CHANGELOG.md, and plugin.json against origin/main.

The core fix — slicing the porcelain path out with ${line:3} instead of splitting on whitespace with awk '{print $NF}' — is correct and well-verified. I confirmed from git's own documentation that plain spaces do trigger C-quoting in git status --porcelain's short-format path field (this is a status-specific rule distinct from core.quotePath's general "space is usual" behavior), so the PR's reproduction and the quote-unwrap logic are both accurate to real git behavior. I also traced the backslash/quote unescape order in detect.sh (scripts/detect.sh:117-122) through several interleaved-escape cases by hand and it reconstructs the original bytes correctly.

Two findings posted as inline comments, both stemming from the same root cause:

1. scripts/detect.sh:111-113 — the rename split is gated on ${line:0:1} (the X/staged column) only. Git's porcelain status table also reports R/C in the Y (worktree) column for a rename detected between the index and working tree that wasn't staged. When that happens, the gate is skipped, the literal old -> new text is left in status_path, and the path silently drops out of the audit — reproducing the exact false-negative class this PR fixes, just via a different trigger. The new detect.test.sh rename fixtures (9b/9c) only use git mv, which always lands R in the X column, so this path isn't exercised.

2. SKILL.md:16 — the awk parity line has the same X-column-only gate, and additionally omits the \"/\\ unescaping that detect.sh does after stripping quotes, so it diverges from "fixed the same way" for filenames C-quoted due to embedded backslash/quote characters rather than whitespace. This line also has no automated regression coverage (only manual verification per the PR description).

Everything else — the --paths-file/explicit-target paths (untouched), the CHANGELOG entry, the version bump, and the directory-expansion/code-file-filter logic downstream — looked consistent and correct.
· branch claude/work-items-integration-9dak62

[github-actions]

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6017877ea5

ℹ️ 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 42s —— View job

---

Security review complete

• Gather context (PR body, changed files, diff)
• Run skip-gate check for /review:security-review
• Review changed files for security-relevant issues
• Adversarially validate any candidate findings
• Post final report

No security issues found.

Skip-gate note: the only commits since the last reviewed head (6017877) are 92e04c9, which extends the existing rename-detection gate in detect.sh/SKILL.md from checking the index-status column (X) alone to checking both the X and Y columns of git status --porcelain's XY short format, closing the worktree-side (intent-to-add) rename case Codex flagged. I reviewed it anyway rather than relying solely on the gate, since it's a small, self-contained diff.

• No new injection surface. The change only widens the character-class test [[ "${line:0:1}" == [RC] || "${line:1:1}" == [RC] ]] that decides whether to strip a rename arrow; status_path is still only ever consumed in quoted contexts ([[ -f "$file" ]], find "$target", <"$file", printf '%s\n'), matching the pattern already cleared in the prior review of this PR.
• Trust boundary unchanged. Same source of input (local git status on the developer's own working tree), no new untrusted data.
• Test-only and doc-only additions (detect.test.sh case 9d, CHANGELOG.md wording) carry no execution-path changes.
• No workflow, permissions, dependency-pin, or secret-handling changes in this commit.

[github-actions]

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

[kyle-sexton]

Both /review:code-review findings addressed. Note that pass ran against 6017877e, before the Codex fix landed, so its finding 1 was already closed by the time it posted.

Finding 1 — X-column-only rename gate. Already fixed in 92e04c96. Same issue Codex raised; both columns are now checked in detect.sh and SKILL.md, with case 9d as the guard (verified failing against the X-only gate).

Finding 2 — SKILL.md diverges from detect.sh. Correct, and the more useful of the two. Fixed in 04558340.

The parser unwrapped the surrounding quotes but never unescaped the \" and \\ inside them, so a filename C-quoted because it contains a quote or backslash — rather than a space — was still previewed wrong. "Fixed the same way" was an overstatement on my part. Verified against real files on disk; both parsers now produce identical bytes:

on disk	porcelain	before	after
quote".py	"quote\".py"	quote\".py	quote".py
back\-slash.py	"back\\-slash.py"	back\\-slash.py	back\-slash.py
plain space.py	"plain space.py"	✓	✓

On the missing coverage — that was the root cause, not a side note: SKILL.md's parser had no automated test at all, which is exactly why it could be half-fixed without anything noticing. Case 10 now extracts the awk program out of SKILL.md with sed and executes it, rather than restating it in the test — a restatement would keep passing while the real line rotted. It also asserts that every file detect.sh audits appears in the preview, so the model is never shown a tree the audit disagrees with. Four of its assertions fail against the previous parser.

Suite is 45/45. One incidental note: the backslash fixture is back\-slash.py rather than back\slash.py because shell-portability-lint substring-matches \\s and read the filename as a GNU-only regex class — renaming beat adding a portability-ok suppression a future reader would have to re-evaluate.

Thanks for both passes. The extract-and-execute test exists because of finding 2.

---

Generated by Claude Code

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Run skip-gate check
• Review changed files for security-relevant issues
• Adversarially validate any candidate findings
• Post final report

No security issues found.

Scope note: the last reviewed head was 92e04c9. The only commit since then is 0455834, which brings the SKILL.md pre-computed-context awk parser to full parity with detect.sh (adds the \"/\\ unescape step matching the earlier fix) and adds regression coverage (detect.test.sh section 10) that extracts and executes the SKILL.md awk program directly against fixtures. I reviewed this delta plus re-confirmed the full diff against origin/main (all 5 changed files) rather than relying solely on the gate:

• No new injection surface. The SKILL.md line is a git status --porcelain | awk '...' | grep ... | head -10 pipeline whose only output is descriptive text folded into the pre-computed-context preview — it is never eval'd or re-interpreted as a command. detect.sh's derived paths (status_path, TARGETS, EXPANDED) remain confined to quoted contexts ([[ -f "$file" ]], find "$target", <"$file", printf '%s\n').
• Trust boundary unchanged. Both parsers still only consume the local repo's own git status output (the developer's own working tree); no new untrusted/remote input.
• Test-only and doc-only additions (detect.test.sh section 10, CHANGELOG.md wording) add no execution-path changes — the new test fixtures (quote".py, back\-slash.py, plain space.py) exercise parsing correctness, not anything with security implications.
• No workflow, permissions, dependency-pin, or secret-handling changes in this diff.
• The residual limitation already flagged in prior reviews (git's octal-escape encoding for control/non-ASCII bytes still isn't decoded) remains a correctness gap only — it can cause a file to be skipped, never mis-resolved to an unintended path.

This is consistent with the prior three security-review passes on this PR (no issues found each time); the two open findings on this PR (worktree-column rename gate — now fixed in 92e04c9; SKILL.md parity — now fixed in 0455834) were code-review/correctness items, not security findings, and are addressed in the code as of this head.
· branch claude/work-items-integration-9dak62

[claude]

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

---

Claude finished the /review:code-review pass

Todo list

• Gather context (PR body, changed files, diff)
• Invoke /review:code-review and follow its instructions
• Post findings as inline PR comments (none — see below)
• Post final summary

Summary

This is a re-review at the current head (0455834), which is two commits past the state the prior code-review pass covered. Both issues flagged in earlier reviews on this PR have been fixed since:

1. Worktree-column rename gate (Codex + earlier Claude review) — scripts/detect.sh:114 now checks "${line:0:1}" == [RC] || "${line:1:1}" == [RC], covering both the X (index) and Y (worktree) status columns. Fixed in 92e04c9, with a dedicated regression fixture (test 9d in detect.test.sh) that reproduces the intent-to-add rename (mv old new && git add -N new) and asserts it no longer resolves to files=0.
2. SKILL.md parser parity (earlier Claude review) — the awk one-liner at SKILL.md:16 now applies the same two-column rename gate (substr($0,1,2) ~ /[RC]/) and the same \"/\\ unescape order as detect.sh. Fixed in 0455834, and — notably — the fix is no longer verified only by manual inspection: section 10 of detect.test.sh now extracts the live awk program straight out of SKILL.md via sed and executes it against fixtures with an embedded quote, an embedded backslash, a plain space, and a worktree-column rename, plus a parity check that every file detect.sh audits also appears in the preview. That closes the "no automated regression test" gap I raised on the prior pass.

I traced the two-pass backslash/quote unescape (gsub(/\\"/,...) then gsub(/\\\\/,...) in the awk version; the mirrored //\\\"/\" / //\\\\/\\ substitutions in bash) by hand through the interleaved case (a filename containing a literal \" byte sequence) in both implementations and confirmed both reconstruct the original bytes correctly — the ordering is load-bearing and it's correct in both places.

I did not find any new issues introduced by the two follow-up commits (92e04c9, 0455834). The rename-arrow split remains correctly gated so it can't mangle an ordinary (non-rename) path that happens to contain " -> ", and the documented residual gap (git's octal-escaped control/non-ASCII bytes still aren't decoded) is accurately scoped and consistent between detect.sh and SKILL.md.

No inline comments posted — no unresolved high-signal findings remain on this head. (Note: I couldn't execute the test suite/shellcheck/gates directly in this environment — the lane's allowed Bash commands don't include running arbitrary scripts — so the "35→37 checks passed" and other gate results in the PR/thread are author-claimed and unverified by me; my read above is a manual trace of the logic, not a test run.)
· branch claude/work-items-integration-9dak62