feat(claude-config): body-scoped coercive-emphasis detector wired to the findings relay by kyle-sexton · Pull Request #3176 · melodic-software/claude-code-plugins

kyle-sexton · 2026-08-23T05:01:55Z

Summary

Carries D2 (coercive emphasis) end-to-end to review:fanout's apply relay, plus the findings-producer wiring the claude-config plugin did not have. claude-config:audit-instructions becomes the plugin's first conforming detector-findings producer.

Scope note worth reading first: most of the detection already existed. criteria.md I28 is the coercive-emphasis check, and instruction-scan.sh already marked its two families. What was missing was a route to a remediation surface — and a fence making that route safe. The fence turned out to be the substantive part: the scanner read whole files including YAML frontmatter, so IMPORTANT: inside a description: was a live candidate, directly against #3118's hard constraint.

Fix

Body-scope fence, applied twice. instruction-scan.sh --body-only skips YAML frontmatter, so no row can point at a description, when_to_use, or a trigger phrase quoted in one. It is opt-in, not default: the human-facing audit legitimately reports on frontmatter content, and what must never happen is such a row reaching an apply relay. emit-findings.sh then recomputes the fence rather than trusting its input — a fence living only in the caller is one caller away from being bypassed — and additionally declines any body row quoting a trigger phrase present in the file's own description. An unclosed leading --- fences the whole file (fail-safe); a mid-document --- opens nothing. Every decline is counted in ## Surfaces, never silent.

All three delimiter checks match ^---[[:space:]]*$ — deliberately identical to skill_frontmatter::extract, which is what check-skill.sh actually parses frontmatter with. Exact "---" equality would be stricter than that parser, and the mismatch runs the dangerous way (see Review rounds below).

The fence exists because plugins/skill-quality/scripts/check-skill.sh:414 hard-FAILs a dropped 'trigger phrase' versus the base ref: a remediation editing one is an auto-invocation regression, not a debatable suggestion.

Out-of-repo fence. Phase A inventories user-level surfaces under ${CLAUDE_CONFIG_DIR:-~/.claude}, but Location is contractually repo-relative and the fix action fences each remediation to it. Rows outside the repo root are declined as reason=outside-repo-root and stay in the human report.

Two crosswalk rules, rule-coercive-emphasis and rule-blanket-tool-default, both IMPORTANT. Argued from severity.md's tests, first match winning: CRITICAL fails every limb because instruction prose computes nothing — what emphasis changes is how likely a trigger is to fire, a distribution over behavior rather than a determined result. IMPORTANT's degradation-with-a-named-trigger limb then matches, the trigger being the first session in which the over-emphasized instruction fires on input its actual condition does not cover. Official guidance supplies the mechanism rather than a preference, so SUGGESTION's catch-all is never reached: the alternatives do not both work, one overtriggers.

Emitted set is a strict subset of what the scanner marks. Only the two I28 families have crosswalk rows; the other eight are counted as declined with reason=no-severity-crosswalk-row. "No crosswalk row, no relay" made visible rather than enforced by silence.

The read-only contract is unchanged. Persisting is opt-in behind --persist-findings; a bare invocation reports and stops. The file is a proposal artifact for a human-gated relay, never an applied edit.

One contract refinement the end-to-end run forced. #3120 asked for a test proving the directive survives "byte-for-byte". Run against a real leading wrapper, that fails: dropping CRITICAL: You MUST promotes the next word to sentence-initial position, so …MUST resolve the item id → Resolve the item id and exactly one byte changes. It is forced by the edit, not chosen — and the official source's own worked example makes the same change (use → Use), so a strict byte-equality contract would reject upstream's own demonstration. The contract now asserts verbatim survival apart from that forced capitalization; any other wording change means the remediation overreached.

One out-of-theme change, called out deliberately. SKILL.md had 4 lines of headroom under the 500-line hard cap, so documenting a new capability required moving something out. The Phase D state-key mechanics went to context/report-keying.md as a progressive-disclosure spoke. That block also restated rules the plugin-data-report-keying convention already owns, so the spoke points at the owner instead. Every removed claim was verified recovered.

Review rounds

Nine automated review passes ran across three reviewers — Codex, the security-review lane (six passes), and the code-review lane (three). Six findings total, all reproduced empirically before fixing rather than accepted on assertion, and all fixed:

Body-scope fence bypass on non-canonical --- delimiters (P1). Exact "---" equality is stricter than skill_frontmatter::extract's ^---[[:space:]]*$. A CRLF or trailing-whitespace delimiter is real frontmatter to the gate but was invisible to the fence, so the block read as body. Measured: a CRLF fixture emitted a finding pointing at its own description line. Because descr() shares the detection, the quoted-trigger fence was disabled in the same case, and because the writer recomputed the same predicate, both layers failed together rather than independently — the one property the defense-in-depth design was meant to buy. All three checks now use the authoritative pattern.
Out-of-repo Location reached the relay (P1). Declined and counted, per above.
Silent carve-out declines (P2). persist-findings.md told the model to drop carve-out rows before the writer saw them while the same file requires every decline counted. New --declined-carveout N flag records the count.
YAML-indicator branch names misparsed (P2). @foo/!foo/#foo are valid git branches; emitted plain, #foo reads as a comment. Quoting is conditional — an ordinary branch stays a byte-identical plain scalar, so the common path and parity with the sibling ai-slop producer are preserved. (That producer has the same unquoted interpolation; flagged as a fleet-wide edge case, left out of scope here.)
source-line-unreadable decline had zero test coverage — the only decline path without an assertion. Covered by case 8b, verified load-bearing by mutation (deleting the branch fails exactly its three assertions).
Case-ordering nit — case 8b sat out of reading order; moved to sit with case 8.

The final pass of each lane skip-gated, having nothing new to review. The last substantive security verdict: "no outstanding security issues."

fixture-git-isolation-gate separately caught that the new tests build a git init fixture without clearing the ambient git environment — a real hazard, since an inherited absolute GIT_DIR outranks git -C and would write the fixture identity into the caller's .git/config. Cleared at the top of the suite, matching plugin-suite precedent; not added to the baseline file, which records pre-existing debt only.

Verification

Run, not inspected.

End-to-end proof (acceptance criterion 3) against a sample surface carrying coercive emphasis in both frontmatter and body:

Detector runs body-scoped → 2 candidates (frontmatter correctly excluded)
emit-findings.sh writes to the rung-5 findings home for this branch
Every clause of fix-pass-mode.md "Step 1" verified against the written file: .md extension, type: review-findings, branch: exact match, parseable table, no subtracting fix-pass-record — all PASS
Remediations applied at each Location: frontmatter digest unchanged, description and when_to_use byte-identical, quoted trigger phrases intact, directive verbatim modulo the one forced capital, instruction not deleted, protected-content line untouched
Re-scan returns no candidates — no stale finding survives its own remediation

Gates (final):

Check	Result
`emit-findings.test.sh`	All 75 checks passed
`instruction-scan.test.sh`	All 92 checks passed
`conflict-scan.test.sh`	All 48 checks passed
`skill-quality:check` (source checkout)	PASS — 0 errors; 483/500 lines; all 13 base-ref trigger phrases preserved
`check-fixture-git-isolation.sh`	OK (87 isolated, 0 baselined)
shellcheck / shfmt / markdownlint / typos / editorconfig	clean

Every suite was additionally verified under CI's detached-HEAD condition, which is what surfaced the --branch resolution ordering that made one assertion measure the wrong thing.

The negative test feeds the writer deliberately unfenced scan output and asserts no emitted row points at a description or when_to_use. Protected content gets one no-flag line per category the container spec names. Fence regressions cover CRLF, trailing space, trailing tab, and unclosed frontmatter in both layers. The remaining check-skill warning (criteria.md:1020) reproduces on the base ref.

Pre-PR diff gate: frontmatter blast radius is nil — only argument-hint changed, description byte-identical. The cross-plugin contract change is purely additive; no existing crosswalk row or rule modified.

spec: agent-doc register detectors — three cut classes routed through the findings relay #3118 names docs/specs/agent-doc-register-detectors-brief.md as its authoring surface; that file does not exist in the repo.
I28's V1 selection is deliberately narrower than its Detect prose — whole bolded sentences and general all-caps imperative runs are judged by the model lane but not mechanically selected, both being too common in ordinary technical prose to select without swamping the relay. Recorded in criteria.md rather than left as a silent gap, following the precedent where ai-slop deferred its third negative-parallelism pattern.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

…ings-relay wiring Carries the D2 (coercive emphasis) detector end-to-end to the review:fanout apply relay, plus the producer wiring the claude-config plugin did not have. The detection half already existed: criteria.md I28 is the coercive-emphasis check, and instruction-scan.sh already marked its two families. What was missing was a route to a remediation surface, and a fence making that route safe. instruction-scan.sh --body-only skips YAML frontmatter, so no row can point at a description, when_to_use, or a trigger phrase quoted in one. Opt-in rather than default: the human-facing audit legitimately reports on frontmatter content, and what must never happen is such a row reaching an APPLY relay. An unclosed leading `---` fences the whole file (fail-safe); a mid-document `---` opens nothing. emit-findings.sh composes a conforming `type: review-findings` file. It recomputes the body-scope fence rather than trusting its input, and additionally declines any body row quoting a trigger phrase that appears in the file's own description. Both fences exist because check-skill.sh check 3 hard-FAILs a dropped trigger phrase versus the base ref, so a remediation that edits one is an auto-invocation regression. Declines are counted and reported in `## Surfaces`, never silent. Only the two I28 families are emitted; every other family instruction-scan.sh marks has no severity-crosswalk row, and the contract admits no row whose tier cannot be looked up from one. Those stay in the human report, counted as declined. Remediation is a downgrade, never a deletion: both Action cells require the directive to survive and only its volume to change. Tests: 84 scanner checks (14 new, covering the fence and its fail-safe cases) and 38 producer checks, including the negative test that feeds the writer deliberately-unfenced input and asserts no emitted row points at a description or when_to_use. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

…ng for I28 Completes the producer: the code landed in the previous commit, this is the contract half that makes it admissible. docs/conventions/detector-findings/README.md gains two crosswalk rows — rule-coercive-emphasis and rule-blanket-tool-default — both IMPORTANT. The tier is argued from severity.md's tests, first match winning: CRITICAL fails every limb because instruction prose computes nothing (what emphasis changes is how likely a trigger is to fire, a distribution over behavior rather than a determined result), and IMPORTANT's degradation-with-a-named-trigger limb then matches, the trigger being the first session in which the over-emphasized instruction fires on input its actual condition does not cover. Official guidance supplies the mechanism rather than a preference, so SUGGESTION's catch-all is never reached: the alternatives do not both work, one overtriggers. The adopter row records what is novel here — the first producer whose emitted set is a strict subset of what its scanner marks, and the first with a structural fence on which findings may reach the relay at all. context/persist-findings.md is this plugin's read of the producer contract. context/report-keying.md takes the Phase D state-key mechanics out of SKILL.md as a progressive-disclosure spoke; SKILL.md had 4 lines of headroom under the 500-line hard cap and documenting a new capability needed more than that. The moved text also restated rules the plugin-data-report-keying convention already owns, so the spoke points at that owner rather than re-stating it again. criteria.md records on I28: the two rule ids, that the remediation is a downgrade and never a deletion, that relay-bound findings are body-scoped and why, and that V1 selection is deliberately narrower than the Detect prose — whole bolded sentences and general all-caps runs are judged by the model lane but not mechanically selected, both being too common in ordinary technical prose to select without swamping the relay. Written down rather than left as a silent gap. Verification: skill-quality:check PASSes against the source checkout (0 errors; 483/500 lines, all 13 base-ref trigger phrases preserved, 3/3 script suites green). shellcheck, shfmt, markdownlint, typos, editorconfig all clean. The one remaining check-skill warning (criteria.md:1020) reproduces on the base ref. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

…rced-caps The end-to-end proof caught what inspection would not have. #3120 asked for a test proving the directive survives the remediation "byte-for-byte". Run against a real leading wrapper, that assertion fails: dropping "CRITICAL: You MUST " promotes the next word to sentence-initial position, so "...MUST resolve the item id" becomes "Resolve the item id" and exactly one byte changes. The change is forced by the edit rather than chosen, and the official source's own worked example makes it too — "CRITICAL: You MUST use this tool when..." becomes "Use this tool when...", lowercase `use` to capital `Use`. A contract demanding strict byte equality would therefore reject upstream's own demonstration of the remediation. So the contract now asserts verbatim survival APART FROM sentence-initial capitalization forced by dropping a leading wrapper; any other change to the directive's wording still means the remediation overreached. Updated in the Action cell emit-findings.sh writes, in criteria.md's I28 row, in persist-findings.md, and in the crosswalk row. New test case 9b applies each Action at its Location and asserts the result: the wrapper is gone, the frontmatter is untouched, the directive matches case-insensitively, only the first byte differs, and the remediated line is no longer a candidate. End-to-end proof (acceptance criterion 3), run not inspected: 1. detector runs body-scoped over a sample surface -> 2 candidates 2. emit-findings.sh writes to the rung-5 findings home for this branch 3. every clause of fix-pass-mode.md "Step 1" verified against the written file: .md extension, `type: review-findings`, `branch:` exact match, parseable table, no subtracting fix-pass-record 4. remediations applied; frontmatter digest unchanged, description and when_to_use byte-identical, quoted trigger phrases intact, directive verbatim modulo the one forced capital, instruction not deleted 5. re-scan returns no candidates: no stale finding survives its remediation Verification: 43 + 84 + 48 script checks green, skill-quality:check PASS (0 errors), shellcheck / shfmt / markdownlint / typos clean. Closes #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

claude · 2026-08-23T05:02:25Z

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

Security review complete

Gather context (PR diff, changed files, repo CLAUDE.md)
Apply skip gate — PR is open and touches a security-relevant surface (the body-scope fence gating an apply relay)
Review changed files for security-relevant surface
Adversarial validation of candidate finding (independent verifier subagent, verdict: CONFIRMED, high confidence)
Post final findings

Finding: body-scope fence bypass via non-canonical `---` delimiters (IMPORTANT)

Posted as inline comments on:

Summary: this PR's whole point is a fence that guarantees a coercive-emphasis finding can never point into a file's YAML frontmatter before reaching the review:fanout apply relay — because a remediation editing description/when_to_use would trip check-skill.sh:414's hard-FAIL ("auto-invocation regression"). Both fence implementations (instruction-scan.sh's frontmatter_end() and emit-findings.sh's fm_end()/descr()) detect the delimiter via exact string equality against "---". This repo's own authoritative frontmatter extractor, skill-frontmatter.sh:16-17 — used by the very check-skill.sh gate this fence is protecting against, and documented as matching what "a real YAML reader ... the harness loads frontmatter with" does — instead matches ^---[[:space:]]*$, tolerating trailing whitespace and a trailing \r (CRLF).

A file whose --- delimiters carry trailing whitespace or CRLF line endings is therefore real frontmatter to the harness/check-skill.sh, but invisible as frontmatter to both new fences — so the entire file, including its actual description:/when_to_use: line, gets treated as unfenced body. Because emit-findings.sh's secondary "recomputed" fence and its descr()-based quoted-trigger-phrase fence share the identical detection logic, both fail together on this input rather than one catching what the other misses — undercutting the PR's own "a fence living only in the caller is one caller away from being bypassed" defense-in-depth rationale.

This isn't neutralized by the repo's .gitattributes LF normalization: audit-instructions' SKILL.md scans user-level surfaces under ${CLAUDE_CONFIG_DIR:-~/.claude} and untracked project .claude/ files, both outside git's normalization, and trailing-whitespace (as opposed to CRLF) is untouched by eol=lf regardless of tracking status. Neither instruction-scan.test.sh nor emit-findings.test.sh exercises this — the fixtures added by this PR use plain LF with exact ---.

Suggested fix: align all three delimiter checks with skill_frontmatter::extract()'s whitespace/CR-tolerant pattern, and add a CRLF/trailing-whitespace frontmatter fixture asserting the fence still holds.

No other security-relevant issues found — variable quoting, path handling, and the non-overwrite/mkdir logic in emit-findings.sh look sound, and the rest of the diff (crosswalk docs, CHANGELOG, plugin.json version bump) is non-executable prose.

Branch: feat/3120-coercive-emphasis-detector

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 153c808f4c

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

CI runs the suite on a detached-HEAD checkout of the PR merge ref, where `git branch --show-current` returns empty. emit-findings.sh resolves --branch before it checks whether its input is scanner output, so case 3 hit the exit-2 branch guard and never reached the exit-3 condition it was asserting. The script's behavior is correct and unchanged: `branch:` is load-bearing for the consumer — fix-pass-mode.md "Step 1" admits a candidate only when its branch: equals the current branch exactly — so with no branch to write there is nothing correct to emit, and refusing beats writing a file the relay can never match. The defect was the test asserting one condition while tripping over another. Case 3 now passes --branch explicitly so it isolates the scanner-row check. New case 3b asserts the branch-resolution path on purpose: with GIT_DIR pointed at a nonexistent path (a hermetic stand-in for detached HEAD) it exits 2 and writes no file, and an explicit --branch still succeeds and lands in the frontmatter. That path was real and untested until CI exercised it by accident. Reproduced locally by detaching HEAD before the fix (exit 2, not 3), and verified green after. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

github-actions · 2026-08-23T05:10:13Z

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

claude · 2026-08-23T05:10:54Z

Claude finished @kyle-sexton's task in 4m 27s —— View job

Security Review — `/review:security-review`

Gather context (PR diff, existing review comments)
Invoke /review:security-review and hunt for logic/trust-boundary security issues
Post findings

Reviewed the diff (gh pr diff 3176, HEAD da1e80c) against the security-review skill's criteria: logic flaws, trust-boundary gaps, injection surfaces, unsafe handling of tokens/credentials, and dangerous Actions patterns (none of the changed files are workflows, so the Actions-pattern hunt doesn't apply here). No workflow files, no secrets handling, no network calls in this diff — the security-relevant surface is the two new shell scripts (instruction-scan.sh, emit-findings.sh) that compose an automated-relay artifact, specifically the body-scope fence this PR's own description calls "the substantive part."

I independently traced (not just accepted) the two P1 items chatgpt-codex-connector already flagged inline, confirmed both by reading the actual awk/bash line-ending and path-relativization semantics, and posted my own inline comments with the concrete trace and a proposed fix:

1. CRLF line endings silently defeat the body-scope fence (emit-findings.sh#L158-L171, same root cause in instruction-scan.sh#L193-L206) — CONFIRMED. getline/head -n1/command substitution all leave a trailing \r on a CRLF-formatted line; comparing that against the literal "---" fails, so fm_end()/frontmatter_end() both report "no frontmatter" on a CRLF file. Both the --body-only scan-side fence and this script's own "recomputed, not trusted" writer-side fence collapse identically and for the same reason, so a CRLF-formatted instruction file has no surviving layer — a coercive-emphasis hit inside its description:/when_to_use: reaches the apply relay, which is precisely the outcome the PR states must never happen (check-skill.sh:414's hard-FAIL on a dropped trigger phrase).
2. Non-repository surfaces can enter the relay with an absolute Location (emit-findings.sh#L245-L246) — CONFIRMED. The relativization only fires for paths under repo_root; the skill's own Phase A inventory explicitly scans user-level surfaces (${CLAUDE_CONFIG_DIR:-~/.claude}), and nothing upstream restricts what gets handed to --from. The detector-findings contract states Location must be repo-relative specifically because "an absolute path is not portable to the checkout that applies the fix" (docs/conventions/detector-findings/README.md#L116-L119) — this is a real, reachable contract violation, not a hypothetical one.

Both are logic/trust-boundary bugs in the exact mechanism (the body-scope fence, and the relay's repo-scoping invariant) that this PR's own description identifies as the safety-critical part of the change — not pre-existing issues, not linter-catchable, and not generic advice. Full detail and a concrete fix direction are in the two inline comments above.

Two Codex comments I did not re-flag, since they read as reliability/correctness rather than security: the branch-name YAML-quoting edge case (silently-skipped match, no exploitable path) and the declined-count bookkeeping in persist-findings.md (a documentation/process gap, not a vulnerability).

No other security-relevant issues found in this diff — no injection surfaces, no secret/token handling, no permission or Actions-workflow changes.

Branch: feat/3120-coercive-emphasis-detector

@foo

… branch names Four findings from the Codex review on #3176, all verified against the code before acting and all reproduced empirically first. CRLF DEFEATED THE BODY-SCOPE FENCE (the serious one). head/awk leave a terminal CR, so a CRLF frontmatter delimiter reads as "---\r" and matched neither comparison in either layer. The block was then treated as body content and description / when_to_use rows became emittable — the fence inverted on exactly the Windows-authored files it most needs to hold for. Measured before the fix: a CRLF fixture emitted a finding pointing at its own description line, which is the auto-invocation regression the fence exists to prevent. Both the scanner and the writer now strip a terminal CR before comparing, and the excerpt is stripped too so no stray CR lands in a table cell. OUT-OF-REPO LOCATIONS REACHED THE RELAY. Phase A inventories user-level surfaces under CLAUDE_CONFIG_DIR, but Location is contractually repo-relative and the fix action fences each remediation to it. A hit in a user CLAUDE.md entered the findings file with an absolute path, where the fix pass could try to mutate a file outside the working tree. Such rows are now declined and counted as reason=outside-repo-root; they stay in the human report. CARVE-OUT DECLINES WERE SILENT. persist-findings.md told the model to drop carve-out candidates before the writer saw them, while the same document requires every decline to be counted — so ## Surfaces would have reported fewer candidates examined than were actually looked at. New --declined-carveout <n> records the count as its own line; the doc now says to pass it. YAML-INDICATOR BRANCH NAMES MISPARSED. git accepts @foo, !foo, #foo; emitted as plain scalars, #foo reads as a comment and the others as indicators, so the consumer — which admits a candidate only on an exact branch match — silently dropped every finding for such a branch. Quoting is deliberately CONDITIONAL: an ordinary branch name keeps a byte-identical plain scalar, so this cannot perturb the common path or diverge from the sibling ai-slop producer on it. Tests: 69 producer checks (was 47) and 89 scanner checks (was 84). The CRLF cases use a throwaway git repo so the new out-of-repo fence does not mask what they measure, and the pipe-escaping case moved into it for the same reason. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

…r parser Follow-up to the CRLF fix, from the automated review on #3176. Stripping a terminal CR closed the CRLF case but left the general one open. All three delimiter checks — instruction-scan.sh frontmatter_end, and emit-findings.sh fm_end and descr — matched "---" by exact equality. This repo's own authoritative extractor, skill_frontmatter::extract, matches ^---[[:space:]]*$, and that is what check-skill.sh parses frontmatter with — the hard-FAIL gate this entire fence exists to satisfy. Exact equality is STRICTER than that parser, and the mismatch runs the dangerous way: a delimiter carrying trailing whitespace or a CR is real frontmatter to the gate but invisible to the fence, so the block reads as body content and description / when_to_use rows become emittable. Because descr shares the detection, the quoted-trigger-phrase fence is disabled in the same case, and because emit-findings.sh recomputed the same exact-match logic, both layers failed on identical input rather than independently — which is the one property the defense-in-depth design was supposed to buy. All three now use the authoritative pattern, so the three readers agree instead of the fence being only as strong as the loosest one. [[:space:]] subsumes the CR case, so this replaces the strip on the delimiters; the excerpt keeps its own strip so no stray CR reaches a table cell. Tests: 72 producer checks (was 69), 92 scanner checks (was 89) — CRLF, trailing space, trailing tab, and unclosed-frontmatter variants in both layers. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

claude · 2026-08-23T05:16:21Z

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

Security review in progress

Gather context (PR diff, changed files, repo CLAUDE.md, prior review comments)
Apply skip gate — PR is open; touches a security-relevant surface (the body-scope fence gating an apply relay); prior review round no longer applies since two new fix commits landed
Review changed files for security-relevant surface
Adversarial validation of the two fix commits (independent verifier subagent running)
Post final findings

Currently reviewing against the actual current branch tip bab7be7 ("fix(claude-config): align the fence with the authoritative frontmatter parser"), which is one commit ahead of the HEAD SHA (794851a) supplied in my task header. Two fix commits landed since the prior security-review round on this PR (794851a, bab7be7) that appear to directly address all findings from that round: the CRLF/whitespace frontmatter-delimiter fence bypass, out-of-repo Location leakage, unquoted YAML-indicator branch names, and silent carve-out declines. Running an independent adversarial verification pass on those fixes before posting a final verdict.

Branch: feat/3120-coercive-emphasis-detector

claude · 2026-08-23T05:19:53Z

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

Security Review — `/review:security-review`

Gather context (PR diff at HEAD bab7be7, prior review comments/findings)
Invoke /review:security-review and check whether prior findings (CRLF fence bypass, absolute-path Location, unquoted branch YAML) were addressed by the latest commits
Post findings

Result: prior findings resolved, no new security issues

This trigger fired on the same PR after two follow-up commits (794851a, bab7be7) landed in response to the earlier security-review rounds. I re-traced each previously-flagged issue against the current code rather than re-accepting the earlier verdicts:

1. CRLF / trailing-whitespace fence bypass — RESOLVED. All three delimiter checks (instruction-scan.sh's frontmatter_end(), emit-findings.sh's fm_end() and descr()) now match ^---[[:space:]]*$ — the same pattern skill_frontmatter::extract() (plugins/skill-quality/scripts/skill-frontmatter.sh) uses, which is what check-skill.sh's hard-FAIL gate actually parses frontmatter with. [[:space:]] subsumes the CR case, so a CRLF or trailing-whitespace --- is now recognized identically to the authoritative parser. source_line() still strips a trailing \r from the excerpt text (L238) so no stray CR reaches a table cell. Covered by new regression fixtures in both test files (CRLF, trailing space, trailing tab, unclosed-frontmatter variants) — scanner tests grew 89→92, producer tests 69→72 per the bab7be7 commit message.

2. Absolute Location for out-of-repo (~/.claude) surfaces — RESOLVED. emit-findings.sh:292-300 now declines any candidate whose file doesn't fall under repo_root — counted under ## Surfaces as reason=outside-repo-root, matching the pattern used for the other decline reasons, rather than emitting an absolute path into the apply relay. Test: emit-findings.test.sh "an out-of-repo surface emits no findings row" / "the out-of-repo decline is counted, never silent".

3. Unquoted branch names breaking YAML frontmatter — RESOLVED (was P2/Codex). yaml_scalar() now quotes the branch value whenever the plain form would misparse (leading YAML indicator char, : , #, empty, trailing whitespace), leaving ordinary branch names as byte-identical plain scalars. Tested for both quoted and unquoted cases.

4. Silent decline-count loss on model-lane carve-outs (P2/Codex, doc contract) — also addressed: --declined-carveout is now a real flag threaded through to the ## Surfaces output (emit-findings.sh:87-91,145-148,339-344), and persist-findings.md:73-81 documents passing it instead of just dropping rows.

I traced these directly against the current file contents and the fix commit (bab7be7) diff rather than trusting the commit message alone; the logic matches what's claimed. I was not able to execute the shell test suites in this sandboxed environment (bash <script> and other multi-part commands required interactive approval that wasn't available here) — the static trace above substitutes for running them, but if you want the actual pass/fail counts confirmed, that needs --allowedTools covering Bash(bash plugins/claude-config/skills/audit-instructions/scripts/*.sh).

No new security-relevant issues found in the delta since the last review round. The body-scope fence, out-of-repo fence, and YAML-scalar quoting now agree with their respective authoritative references (the frontmatter parser and YAML plain-scalar rules) rather than approximating them.
· Branch: feat/3120-coercive-emphasis-detector

…a fixture fixture-git-isolation-gate caught this on 794851a and bab7be7: the CRLF and out-of-repo cases build a throwaway repository (git init) to give the writer a repo_root of its own, and the suite did not clear the ambient git environment first. The gate is right and the hazard is real. `git -C <dir>` is a readability guard, not isolation: an inherited ABSOLUTE GIT_DIR overrides repository discovery outright and outranks -C, and GIT_CONFIG replaces the file `git config` reads and writes regardless of -C. Under either, the fixture identity lands in the CALLER's .git/config — shared by every worktree of the clone — and the fixture keeps none. Cleared unconditionally at the top of the suite, matching the plugin-suite precedent (ai-slop detect.test.sh and four others) rather than sourcing the repo-level harness. Case 3b's per-command `GIT_DIR=...` prefix is unaffected: it scopes to that one invocation and is set deliberately to simulate a detached HEAD, not inherited. Not added to scripts/fixture-git-isolation-baseline.txt — that file records pre-existing debt only, and this suite is new. Reproduced the failure locally against the gate before fixing, and confirmed after: "fixture git isolation: OK (87 isolated, 0 baselined)". Suites unchanged at 72 / 92 / 48 green. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

claude · 2026-08-23T05:23:18Z

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

Security Review — `/review:security-review`

Gather context (PR diff, changed files, repo CLAUDE.md, prior review comments)
Invoke /review:security-review and re-verify the fixes against current HEAD (4b52a56)
Review changed files for security-relevant surface
Adversarial re-trace of all four previously-flagged issues against the current code
Post final findings

Verdict: no outstanding security issues

This is the fourth review pass on this PR. The prior rounds (chatgpt-codex-connector and earlier claude runs) raised four issues; all four are fixed as of bab7be7/794851a, and the one commit since (4b52a56) only touches test-fixture git isolation — no production code changed. I independently re-traced each fix against the current tree rather than trusting the fix commits' own descriptions:

Body-scope fence bypass via non-canonical --- delimiters (CRLF / trailing whitespace) — fixed. All three delimiter checks now share one pattern, ^---[[:space:]]*$, matching this repo's authoritative extractor:
- skill-frontmatter.sh:16-17 (skill_frontmatter::extract, the authoritative reference)
- instruction-scan.sh#L205-L218 (frontmatter_end, both the opening head -n1 | grep -qE and closing awk checks)
- emit-findings.sh#L199-L231 (is_fence, used by both fm_end() and descr(), so the quoted-trigger-phrase fence shares the fix)
Regression fixtures for CRLF, trailing space, trailing tab, and unclosed frontmatter now exist in both instruction-scan.test.sh and emit-findings.test.sh (e.g. emit-findings.test.sh case 9c, L246-L281).
Non-repository surfaces entering the relay with an absolute Location — fixed. emit-findings.sh#L292-L300 now declines (reason=outside-repo-root) any row whose path doesn't sit under git rev-parse --show-toplevel. The prefix check index(file, repo_root "/") != 1 appends the trailing / before comparing, so a sibling directory sharing repo_root as a string prefix (e.g. …claude-code-pluginsEVIL) can't false-match. persist-findings.md routes these to the human report per the skill's existing routing rule.
Unquoted YAML branch name (@foo, #foo, etc. misparsing as YAML indicators/comments) — fixed via conditional quoting in yaml_scalar() (emit-findings.sh#L168-L183), backslash/quote-escaped when triggered. This was P2/reliability-adjacent rather than a security boundary, but confirmed fixed.
Silent decline-count drop for model-lane criteria carve-outs — fixed via --declined-carveout <n> (emit-findings.sh#L339-L344), with non-numeric input rejected (exit 2). Documentation/process gap, not a vulnerability, but confirmed fixed.

No new security-relevant issues surfaced in the delta since the last reviewed tip (bab7be7 → 4b52a56 is test-harness-only: clearing GIT_DIR/GIT_WORK_TREE/GIT_CONFIG before the fixture-building test cases, per fixture-git-isolation-gate). I also checked the cell-escaping path (esc(), pipe-only) and the --declined-carveout/branch/--from argument handling for injection surfaces — nothing exploitable; these are local file reads and deterministic awk/bash composition with no eval, no unsanitized shell interpolation, and no secret handling.

Note: I was unable to execute the shell test suites directly in this environment (Bash tool calls against the repo's .sh files required interactive approval not available here) — verification here is by static trace against the actual current source rather than by running emit-findings.test.sh/instruction-scan.test.sh. The PR description reports both green (72/92 checks); I did not independently execute them. If you want direct execution as part of this review lane, allowedTools would need to permit running these test scripts non-interactively.

No other security-relevant issues found in this diff — no injection surfaces, no secret/token handling, no permission or Actions-workflow changes.

Branch: feat/3120-coercive-emphasis-detector

claude · 2026-08-23T06:03:05Z

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

Code review — `/review:code-review`

Gather context (PR diff at HEAD 4b52a56, repo CLAUDE.md, prior review rounds)
Apply skip gate — PR is open, not draft, touches real logic (two shell scripts + doc/crosswalk wiring); proceeded
Review changed files for correctness/maintainability (security scoped out — this repo carries a dedicated claude-security-review lane, which has already run three rounds on this PR and confirmed all its findings fixed as of bab7be7/794851a)
Post findings

Summary

Read the full diff (14 files, +1264/-47) and the current state of emit-findings.sh and instruction-scan.sh line by line — not just the delta — since prior security-lane rounds already forced two fix commits (794851a, bab7be7) and I wanted to confirm they hold, not restate them.

Confirmed already fixed (no need to re-flag, scoped to security lane anyway): CRLF/trailing-whitespace frontmatter-delimiter bypass, out-of-repo absolute Location leakage, and unquoted YAML-indicator branch names all now match skill_frontmatter::extract's ^---[[:space:]]*$ pattern and have regression coverage (CRLF, trailing space/tab, unclosed frontmatter, out-of-repo, YAML-indicator branches) in both emit-findings.test.sh and instruction-scan.test.sh.

One minor, non-blocking observation, posted inline on emit-findings.sh:288: the declined_unreadable / reason=source-line-unreadable decline path — hit when a scan row's line number no longer exists in the source file — has zero test coverage, unlike every sibling decline path (no-crosswalk, frontmatter, quoted-trigger, out-of-repo, carve-out), each of which has at least one assertion. The fail-safe direction is already correct (declines rather than emitting a wrong excerpt), so this isn't blocking — just a gap worth a one-line fixture given how rigorously everything else here is covered.

No correctness or maintainability issues beyond that. The crosswalk argumentation (docs/conventions/detector-findings/README.md), the criteria.md additions, persist-findings.md, report-keying.md, and the SKILL.md restructuring (moved to stay under the 500-line cap) are internally consistent, and lib/state-key.sh (referenced by the new report-keying.md) exists as claimed. Version bump (0.38.9 → 0.39.0) and CHANGELOG match the added-feature scope.

Branch: feat/3120-coercive-emphasis-detector

github-actions · 2026-08-23T06:07:55Z

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

Review finding: `declined_unreadable` was the only decline path in emit-findings.sh with zero assertions — every sibling (no-crosswalk, frontmatter, quoted-trigger, out-of-repo, carve-out) had at least one. Confirmed by grep before acting: the reason string appeared only at its two emit sites. It is reachable in the documented pipeline. The scan output and the source file are read at different moments — the model lane edits the --from file to drop carve-out rows — so a source file touched in that window, or a stale scan-output file reused, leaves a row whose line number no longer exists. source_line() returns "" and the row becomes a counted decline rather than a finding with an empty excerpt. That fail-safe direction was already correct; what was missing was the assertion keeping an off-by-one in source_line's counting loop from silently changing it. New case 8b feeds a row pointing past EOF and asserts no findings row, the decline counted, and no row bearing the phantom location. Verified meaningful by mutation: deleting the branch fails exactly these three assertions. The case uses its own output variable. Written against the shared `OUT` it clobbered what cases 10 and 11 read — caught by running the whole suite rather than only the new assertions, and noted inline so the next case does not repeat it. 75 producer checks (was 72); scanner 92 and conflict 48 unchanged. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

claude · 2026-08-23T06:10:32Z

Claude finished @kyle-sexton's task in 38s —— View job

Security Review — `/review:security-review`

Gather context (PR diff, changed files, repo CLAUDE.md, prior review comments)
Invoke /review:security-review skill and follow its instructions for HEAD c14c598
Review changed files for security-relevant surface
Post findings

Skip gate applied — no new security-relevant surface since the last reviewed head

This is the fifth security-review pass on this PR. The last round traced all four prior findings (CRLF/whitespace frontmatter-delimiter fence bypass, out-of-repo Location leakage, unquoted YAML branch-name scalars, silent carve-out declines) as resolved at 4b52a56 and returned "no outstanding security issues."

The only commit since that reviewed head is c14c598 ("test(claude-config): cover the source-line-unreadable decline branch"), and the diff confirms it is test-only:

git diff 4b52a56..c14c598 --stat
 .../audit-instructions/scripts/emit-findings.test.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

It adds a fixture/assertion for the reason=source-line-unreadable decline path (a gap a prior /review:code-review pass flagged as non-blocking test-coverage, not a security finding) — asserting a scan row pointing past EOF is declined and counted rather than emitted with a blank/wrong excerpt. No production script (emit-findings.sh, instruction-scan.sh) or any other file changed.

Per the skip gate ("this head already has a successful security review that still applies"), there is no new security-relevant surface to review. No findings to post.
· Branch: feat/3120-coercive-emphasis-detector

claude · 2026-08-23T06:29:00Z

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

Code review — `/review:code-review`

Gather context (PR diff at HEAD c14c598, prior review rounds, repo CLAUDE.md)
Apply skip gate — PR is open, not draft, touches real logic (test coverage for shell script); security scoped out (dedicated security lane already ran 4 rounds and confirmed all findings fixed as of bab7be7/794851a)
Review changed files for correctness/maintainability
Post findings

Summary

This trigger fired after the one commit since the last code-review round: c14c598 — "test(claude-config): cover the source-line-unreadable decline branch". That's exactly the gap the prior code-review round (on 4b52a56) flagged as a minor, non-blocking observation: the declined_unreadable / reason=source-line-unreadable decline path in emit-findings.sh:288 had zero test coverage.

Verified the fix is correct, not just present. The new "Case 8b" in emit-findings.test.sh:246-264:

Feeds a scan row pointing at line 9999 of frontmatter-emphasis.md (past EOF), which drives source_line() to return "" and hits the exact branch at emit-findings.sh:288.
Asserts all three things that branch's contract requires: zero findings rows emitted, the decline is counted (reason=source-line-unreadable) rather than silent, and no row leaks with an empty/wrong excerpt (assert_not_contains ... "frontmatter-emphasis.md:9999").
I cross-checked the assertion string against the actual printf in emit-findings.sh:338 ("Declined candidates: %s count=%d reason=source-line-unreadable\n") — they match exactly.
Confirmed the new block correctly avoids clobbering shared test state: it uses its own EOFROW/EOF_OUT variables rather than the shared OUT variable that Cases 9/9b set and Cases 10/11 (lines 358, 364) read afterward — the in-file comment's claim about why that matters checks out.

This closes the last open item from the prior review round. No new correctness or maintainability issues in the substantive logic.

One cosmetic, non-blocking nit posted inline: the new block is labeled "Case 8b" but sits physically after "Case 9" and "Case 9b" in the file (all other cases are in strict reading order), which makes the label a slightly unreliable index when scanning top-to-bottom. Posted on emit-findings.test.sh:246.

No security-relevant surface changed in this delta (test-only diff); the security lane's four prior rounds on the production code (instruction-scan.sh, emit-findings.sh) still stand as the authoritative security verdict for this PR.

Note: I was unable to execute the shell test suite directly in this sandboxed environment (non-interactive bash invocations of the .sh files required approval that wasn't available here) — verification above is by static trace against the actual current source (confirmed the assertion strings, branch logic, and variable scoping match byte-for-byte) rather than by running emit-findings.test.sh. The commit message reports 74 (72+2) checks green; I did not independently execute them.
· Branch: feat/3120-coercive-emphasis-detector

…ng order Review nit: case 8b was labelled 8b but placed between 9b and 9c, so the case numbers stopped being a reliable index when scanning the file top to bottom. Moved the block rather than renumbering. The reviewer offered 9g as one option, but that would have been wrong for where the block actually sits — 9g implies it follows 9f, and it preceded 9c. Moving it to Case 8 is what the existing label already claims, renumbers nothing, and puts it beside the case it belongs with: 8 decides which rows are admitted, 8b decides which are declined. Pure block move; no assertion changed. 75 producer checks still pass, and the mutation check still catches exactly the same three (deleting the declined_unreadable branch), so the case is still load-bearing where it now sits. Case order is now strictly ascending end to end. Refs #3120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xn6aUXRQSYx4cGuP5cDxbF

claude · 2026-08-23T06:33:21Z

Claude finished @kyle-sexton's task in 37s —— View job

Security Review — `/review:security-review`

Gather context (PR diff, changed files, repo CLAUDE.md, prior review comments)
Invoke /review:security-review skill and follow its instructions for HEAD bba7e8c
Review changed files for security-relevant surface
Post findings

Skip gate applied — no new security-relevant surface since the last reviewed head

This is the sixth security-review pass on this PR. The last round (on c14c598) traced all four prior findings (CRLF/whitespace frontmatter-delimiter fence bypass, out-of-repo Location leakage, unquoted YAML branch-name scalars, silent carve-out declines) as resolved and returned "no outstanding security issues," with the delta at that point being a test-only addition for the source-line-unreadable decline path.

The only commit since that reviewed head is bba7e8c ("test(claude-config): move case 8b to sit with case 8, restoring reading order"), and the diff confirms it is a pure relocation:

git diff c14c598..bba7e8c --stat
 .../scripts/emit-findings.test.sh | 41 ++++++++++++-----------
 1 file changed, 21 insertions(+), 20 deletions(-)

The "Case 8b" test block (added in c14c598, asserting the reason=source-line-unreadable decline path) is moved to sit immediately after "Case 8" instead of after "Case 9b" — the exact cosmetic ordering nit a prior /review:code-review pass flagged as non-blocking. No assertions, fixtures, or logic changed; EOF_OUT/EOFROW remain scoped to their own block, so no interaction with the OUT variable cases 9/9b set and cases 10/11 read. No production script (emit-findings.sh, instruction-scan.sh) or any other file changed.

Per the skip gate ("this head already has a successful security review that still applies"), there is no new security-relevant surface to review. No findings to post.

Branch: feat/3120-coercive-emphasis-detector

claude · 2026-08-23T06:36:42Z

Claude finished @kyle-sexton's task in 47s —— View job

Code review — `/review:code-review`

Gather context (PR diff at HEAD bba7e8c, prior review rounds, repo CLAUDE.md)
Apply skip gate
Review changed files for correctness/maintainability
Post findings

Skip gate applied — no new review surface since the last code-review round

The last code-review round on this PR (HEAD c14c598) found one non-blocking nit: the new "Case 8b" block in emit-findings.test.sh was physically placed after "Case 9"/"Case 9b" rather than in reading order, unlike every other case in the file.

The only commit since that reviewed head is bba7e8c — "test(claude-config): move case 8b to sit with case 8, restoring reading order". Diffing c14c598..bba7e8c confirms it does exactly that and nothing else:

 .../scripts/emit-findings.test.sh | 41 +++++++++++-----------
 1 file changed, 21 insertions(+), 20 deletions(-)

The Case 8b block (fixture setup, emit() call, and its three assertions) was cut from its old position after Case 9b and pasted in unchanged right after Case 8, plus a one-word comment tweak ("clobbering it here would" → "clobbering it would"). No logic, assertion, or test coverage changed — this is the exact nit from the prior round, resolved.

Per the skip gate ("this head already has a successful review from this lane that still applies," and this delta is a trivial/automated fix with no meaningful new review surface), there's nothing new to review. All four substantive findings from earlier rounds (body-scope fence CRLF/whitespace bypass, out-of-repo Location, unquoted YAML branch scalars, silent carve-out declines) remain fixed as of bab7be7/794851a, and the source-line-unreadable test-coverage gap was closed in c14c598. No findings to post.

Security review is out of scope for this lane — the dedicated claude-security-review lane has already run five rounds on this PR and confirmed no outstanding issues.

Branch: feat/3120-coercive-emphasis-detector

kyle-sexton · 2026-08-23T07:13:29Z

No description provided.

Fifth #2891 de-slop shard: purge em dashes from the claude-config plugin instruction surfaces, the next-worst cluster after session-flow (#3106), planning (#3105), work-items (#3107) and source-control (#3108). Rewrote README.md and all ten SKILL.md files under /ai-slop:audit fix semantics: periods, commas, a colon before a list, or a restructured sentence. Never parentheses, en dashes, or a spaced hyphen, since each of those is the same interruption wearing a different mark. Two review rounds on the PR caught seven places where a paired em dash had become parentheses and two doubled-connective run-ons; all were restructured. Rebased across two releases that landed on main mid-review, in both cases re-applying to the newer text rather than reverting it: - 0.39.0 (#3176) restructured audit-instructions/SKILL.md, moving the Phase D state-key block to context/report-keying.md and adding --persist-findings. The flag, its Phase D paragraph, and both context/ spokes are retained. - 0.39.1 (#3178) normalized setup/SKILL.md and audit-instructions/SKILL.md to canonical fleet SSOT wording with PLUGIN-PHILOSOPHY citations. That wording and those citations are kept verbatim; only their punctuation is de-slopped. context/ files stay out of scope, matching #2891's target set and every prior shard. Frontmatter description and argument-hint values are rewritten too. No quoted auto-invocation trigger phrase contained an em dash, so no trigger changed. Verification (this repo's .claude/ai-slop.json disables rule-em-dash corpus-wide, so the detector runs against an isolated HOME and CLAUDE_PROJECT_DIR to force the rule on): - detect.sh over the 11 shard files: 0 findings, every rule clean - no en dash or spaced hyphen introduced; the four en dashes in the diff are pre-existing numeric ranges (I1-I28, I1-I5, 3-5 lanes) - check-changelog-parity.sh --check and --check-bump origin/main: pass - CHECK_SKILL_SKIP_MARKDOWNLINT=1 check-changed-skills.sh origin/main: 10 skills, 0 errors, every base-ref trigger phrase preserved - markdownlint-cli2 over the 12 changed files: 0 issues - audit-instructions/SKILL.md is 484 lines, under the 500-line cap Pre-existing and not from this diff: three audit-permission-state script suites fail identically on a clean origin/main worktree in this environment. This shard touches no script. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tu5t8rYWv2kDzRcdmLE2ro

claude added 3 commits August 23, 2026 04:45

chatgpt-codex-connector Bot reviewed Aug 23, 2026

View reviewed changes