fix(ai-slop, claude-config): make emit-findings esc() idempotent and resolve repo-root spellings (0.3.6, 0.39.4) - #3242
Conversation
|
Claude finished @kyle-sexton's task in 1m 4s —— View job Security review (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 826ec21174
ℹ️ 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".
|
Last security-reviewed head: |
826ec21 to
250be7b
Compare
…resolve repo-root spellings (0.3.6, 0.39.3) Both sibling emit-findings.sh copies still carried the two defects #3202 fixed only in docs-hygiene. A naive gsub double-escapes a pipe the source already escaped, which GFM reads as a live delimiter. esc() now parks already-escaped pipes on a sentinel first. git rev-parse --show-toplevel can spell the same directory differently than the caller pwd (Git Bash). ai-slop failed open (Location stayed absolute, nothing reported it). claude-config failed closed (in-repo hits were declined as outside-repo-root). Root resolution now prefers the caller pwd minus git --show-prefix, with git's two spellings as fallbacks. Shared code was considered and declined: plugins are portable and there is no existing cross-plugin emit-findings library. Closes #3203 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Walk the full backslash run before each pipe so a\\|b stays one GFM cell, and drop Windows user-path literals from comments/changelog. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…39.4 entry Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
ffa22b4 to
89ed48c
Compare
kyle-sexton
changed the title
|
Claude finished @cursor[bot]'s task in 5m 39s —— View job Code review (
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ALREADY="$TEST_TMPDIR/already-esc.txt" | ||
| cat >"$ALREADY" <<'EOF' | ||
| Finding: rule=ai-slop/audit/rule-em-dash file=doc.md line=1 fired=a\|b excerpt=text | ||
| Summary rule=ai-slop/audit/rule-em-dash findings=1 declined=0 disabled=0 | ||
| EOF | ||
| ALREADY_OUT="$TEST_TMPDIR/findings/already-esc.md" | ||
| bash "$EMIT" --from "$ALREADY" --out "$ALREADY_OUT" --branch test-branch >/dev/null 2>&1 | ||
| already_row="$(LC_ALL=C grep -m1 '^| 1 ' "$ALREADY_OUT")" | ||
| assert_not_contains "emit: an already-escaped pipe is not double-escaped" "$already_row" '\\\|' | ||
| assert_contains "emit: and survives as a single-escaped literal" "$already_row" 'a\|b' | ||
| already_delims="$(printf '%s' "$already_row" | sed 's/\\|//g' | awk -F'|' '{print NF - 1}')" |
There was a problem hiding this comment.
Test-coverage gap in the new esc() idempotency case.
This case only exercises bs=1 (source already carries a single, correctly-escaped \|). The parity branch that actually decides correctness — an even, non-zero backslash run before a pipe (e.g. source excerpt containing a\\|b — two backslashes then a pipe) — has no test in either copy of this suite.
I hand-traced esc() for bs=2: the walk collects bs=2, sees the trailing |, and since 2 % 2 == 0 bumps it to 3, emitting \\\|. Rendered in GFM that's \\ → literal \, then \| → literal | (and the delimiter stays escaped), which matches the pre-table rendering of the two-backslash-then-pipe source. So the logic is correct as merged — but that's exactly the branch Codex flagged as suspect on an earlier revision, and it's the one branch nothing pins down with a regression test today.
Worth adding a sibling case here (and in emit-findings.test.sh) with a source excerpt containing an even, non-zero backslash run before | (e.g. fired=a\\|b), asserting it comes out as a\\\|b and still parses as the expected column count. Cheap insurance against a future edit to esc() silently breaking the one non-obvious parity branch.
| PIPEF2="$CRLFREPO/already-pipe.md" | ||
| # shellcheck disable=SC2016 # backticks and \| are fixture content, not a subshell | ||
| printf 'CRITICAL: run `a \| b` before pushing.\n' >"$PIPEF2" | ||
| # Feed a synthetic scan row whose excerpt (the source line) already contains \|. | ||
| printf '%s\n' "$PIPEF2:1:I28-a" >"$TEST_TMPDIR/already-pipe.txt" | ||
| (cd "$CRLFREPO" && bash "$EMIT" --from "$TEST_TMPDIR/already-pipe.txt" \ | ||
| --out "$TEST_TMPDIR/already-pipe-out.md" --branch testbranch >/dev/null 2>&1) | ||
| ALREADY_ROW=$(LC_ALL=C grep -m1 '^| [0-9]' "$TEST_TMPDIR/already-pipe-out.md") | ||
| assert_not_contains "an already-escaped pipe is not double-escaped" "$ALREADY_ROW" '\\\|' | ||
| assert_contains "and survives as a single-escaped literal" "$ALREADY_ROW" '\|' | ||
| ALREADY_COLS=$(printf '%s\n' "$ALREADY_ROW" | sed 's/\\|//g' | awk -F'|' '{print NF}') |
There was a problem hiding this comment.
Same gap as the ai-slop copy (see comment on detect.test.sh): this case only covers bs=1 (source already has a single \|). The bs % 2 == 0 branch for a non-zero even backslash run before a pipe — the exact shape Codex's review questioned — isn't exercised here either. I traced it by hand and it's correct as merged (a\\|b → a\\\|b, which GFM renders back to literal \|), but a regression test for that branch is cheap and would settle the question for good instead of relying on a manual trace.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…3270) 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 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
1 participant
Summary
#3202 fixed two latent defects in
docs-hygiene:audit-noiseemit-findings.shonly. Each producer carries its own copy, so the same defects were still live inai-slopandclaude-config:audit-instructions.Defect 1 —
esc()is not idempotent. A naivegsub(/\|/, "\\|")turnsa \| bintoa \\| b. GFM reads that as a literal backslash followed by a live delimiter, so the row splits and the fix action misreads it. This repo writes literal\|in its own tables.Defect 2 — repo-root spelling mismatch.
git rev-parse --show-toplevelcan answerC:/Users/…while the caller is at/tmp/…(Git Bash). Consequences differed per producer and were confirmed individually:ai-slopfailed open — Location stayed absolute and nothing reported it, because an absolute path is still a well-formed cell.claude-configfailed closed — a path it could not prove was under the root was declined asoutside-repo-root, so an in-repo finding was silently missed.Fix
esc()now parks an already-escaped pipe on a sentinel first, matching #3202. Root resolution prefers the callerpwdminus git--show-prefix, with git's two spellings as fallbacks, matching the #3180 form that #3202 did not actually land.Shared code was considered and declined: plugins are portable and there is no existing cross-plugin emit-findings library. The three copies now agree on the same two helpers instead.
ai-slop 0.3.6 (0.3.5 is #3179), claude-config 0.39.3.
docs-hygiene residual (not fixed here): #3202 shipped the idempotent
esc()but not the three-spelling root. That producer still resolves root viagit rev-parse --show-toplevelonly.Verification
plugins/ai-slop/skills/audit/scripts/detect.test.sh— 98/98 (idempotent esc + symlink spelling mismatch)plugins/claude-config/skills/audit-instructions/scripts/emit-findings.test.sh— 81/81 (same two cases; fail-closed fence still declines a true out-of-repo path)scripts/check-changelog-parity.sh --check-bump origin/main— passRelated
Closes #3203
Refs #3180, #3202