feat(docs-hygiene): negation-without-positive detector on audit-noise, wired to the findings relay (0.21.0) - #3194
Conversation
…, wired to the findings relay (0.21.0) Adds a ninth noise shape to `audit-noise` — a prohibition with no positive alternative stated in the same sentence — and makes the skill a conforming detector-findings producer, reusing the wiring #3120 established in `claude-config:audit-instructions`. The read-only hard rule is amended rather than quietly violated. It read "No Edit, no Write, no mutating Bash ops", which as written forbade the producer contract this adds. It now states the distinction in its own text: read-only binds every audited target unconditionally, while the findings artifact is a new file in the gitignored memory tier, written only under --persist-findings, and is a proposal for a human-gated relay rather than an applied edit. All three withholding boundaries are evidence-gated, so an unresolved candidate is emitted: a paired positive, a hard guardrail whose constraint a positive form cannot carry, and a worked example each require their evidence present on the sentence. The carve-out lives in the shared scanner, so the human report and the relay file give one candidate one disposition. Closes #3123
…egration-o22y7w # Conflicts: # plugins/docs-hygiene/.claude-plugin/plugin.json # plugins/docs-hygiene/CHANGELOG.md # plugins/docs-hygiene/skills/audit-noise/scripts/detect.test.sh
|
Claude finished @kyle-sexton's task in 7m 40s —— View job Code review:
|
|
Claude finished @kyle-sexton's task in 2m 50s —— View job Security ReviewTask list
Scope checked: all 14 changed files (shell scripts, SKILL.md, contract docs, changelogs, eval fixtures). No Finding (IMPORTANT)
This PR adds
Since this skill reads arbitrary tracked markdown as data, an injected instruction in an audited file could steer the agent toward a mutating git call the skill's own stated contract says is impossible — right now that's enforced only in prose, not in the tool grant. Suggested a narrowed grant ( Noted in the inline comment: Everything else reviewed, no issues found
|
![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: bcb7734139
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Last security-reviewed head: |
Seven findings from the automated reviewers on #3194, each reproduced before being fixed: - fence every carve-out marker to a whole word. Bare substrings let `secretary` satisfy the `secret` guardrail and `preferentially` satisfy the `prefer` pairing, silently dropping real findings on the withholding boundaries - match the contraction with an apostrophe class, not `.` (`don.t` matched `donut`) - carry the fired marker from the sentence that triggered, via a new `Finding marker:` field, so a carved-out leading sentence no longer points review at the guardrail. Keeps one implementation of the sentence walk - fail closed on a `..` path segment in the out-of-repo fence - quote `branch:` when YAML would implicitly type it (`true`, `null`, `123`, a date) - narrow `allowed-tools` from `Bash(git:*)` to the two subcommands actually used; the blanket grant authorized mutating git ops the read-only rule added in this same release disclaims Deferred and filed as #3195: the shape is scoped to one physical line, so a soft-wrapped sentence is judged in pieces. False-positive direction only, never a silent withhold; recorded as a known limitation until that lands. Refs #3195
|
Claude finished @kyle-sexton's task in 1m 21s —— View job Security ReviewTask list
Scope checked: commits since the last security-reviewed head ( Previously-flagged IMPORTANT finding: resolvedThe earlier round flagged Verified against actual usage in Other changes in this commit (non-security correctness fixes, reviewed for regressions)
ConclusionNo new security issues introduced by this push. The one prior security-relevant finding (over-broad |
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 2m 27s —— View job Code ReviewTask list
ScopeThis is the CODE-REVIEW lane (correctness/maintainability); security is scoped to the separate Summary: all 7 prior findings are genuinely fixedCommit
Each fix has a corresponding new regression test in Verification note: I could not execute the shell test suites myself in this sandboxed environment ( No new inline findings — nothing here would need a |
… abbreviation Eighth review finding, and the most serious: the left-anchored splitter could not skip a period not followed by whitespace, so `e.g.` made the match fail on the first iteration and the entire line became one "sentence". On `See e.g. the credential rotation policy. Never call the tool directly.` the guardrail word in the first clause then suppressed the real prohibition in the second — silent finding loss, which is the one outcome the carve-outs exist to make impossible, reached through the splitter rather than through a marker. Sentences are now peeled right-to-left with a greedy leading `.*`, splitting at every terminator that IS followed by whitespace. An abbreviation merely over-splits, and over-splitting only narrows the window a suppressing marker can act from. The header comment claimed the opposite behaviour and was wrong. Regression tests cover both directions: the abbreviation case now flags with the later sentence's marker, and a guardrail genuinely in the same sentence still carves out.
Review round addressed —
|
| # | Finding | Note |
|---|---|---|
| 1 | secretary satisfied the secret guardrail; preferentially satisfied the prefer pairing |
Silent finding loss. Every marker is now fenced to a whole word; prefer inflections enumerated so the verb still pairs, vulnerab kept deliberately stemmed but bounded on the left |
| 2 | Sentence splitter collapsed the line on an embedded abbreviation | Silent finding loss, and the most serious of the round. Confirmed exactly as traced: See e.g. the credential rotation policy. Never call the tool directly. split into one sentence, and the guardrail suppressed the prohibition. Now peeled right-to-left with a greedy leading .*, so an abbreviation over-splits instead — the fail-safe direction. The header comment claiming "over-split" was simply wrong and is corrected |
| 3 | don.t matched donut (raised independently by both reviewers) |
Apostrophe class, not . |
| 4 | Fired marker came from the first prohibition on the line | detect.sh now carries a Finding marker: field, so there stays one implementation of the sentence walk rather than a mirror in the writer free to drift |
| 5 | Lexical repo fence admitted a ..-traversing path |
Fails closed on a .. segment. Residual recorded at the site rather than implied: an in-repo symlink pointing outward still resolves past a lexical test |
| 6 | branch: unquoted when YAML implicitly types it |
true / null / no / 123 / dates are quoted; ordinary names stay plain scalars |
| 7 | allowed-tools granted unscoped Bash(git:*) |
Narrowed to the two subcommands actually used — the blanket grant authorized mutating git ops the read-only rule added in this same diff disclaims |
Each carries a regression test. The suites grew 115 → 125 and 23 → 32.
Deferred, with a filed issue
#3195 — negation is scoped to one physical line, so a sentence markdown soft-wraps is still judged in pieces. Distinct from finding 2: that one was a within-line splitter bug and is fixed; this is the cross-line case, whose fix changes detect.sh's iteration model and its interaction with every existing fence (frontmatter, code fences, section exemptions, opt-out markers). The error direction is a false positive, never a silent withhold, so it costs reviewer attention rather than coverage. Recorded as a known limitation in SKILL.md "Hard rules" until #3195 lands.
One note back
the two findings below are backed by careful manual regex-grammar tracing rather than an executed repro
Both traces were correct, including the subtle one — the splitter analysis was right about the mechanism (anchored match failing on the first iteration) and about the consequence. Worth recording, since the reviewer flagged its own confidence as reduced for lack of a repro.
Generated by Claude Code
|
Claude finished @kyle-sexton's task in 53s —— View job Security ReviewTask list
Scope checked: commits since the last security-reviewed head ( Skip gate: no security-relevant surface in this diff
No new security issues introduced by this push. Nothing to report. |
|
Claude finished @kyle-sexton's task in 3m 49s —— View job Code review:
|
| Finding | Verified at |
|---|---|
don.t/shouldn.t unescaped . matching "donut" |
noise-shapes.sh:460-467 — AUDIT_NOISE_APOS="['’]" character class, NEG_DONUT regression test added |
secretary/preferentially substring collisions on withholding boundaries |
noise-shapes.sh:466,474,485 — every alternative now fenced `(^ |
| Fired marker taken from first prohibition on the line, not the triggering sentence | detect.sh:302-308 emits Finding marker:; emit-findings.sh:320-334 prefers it, falling back to the old whole-line scan only for pre-existing detector output |
..-traversing path admitted by the lexical repo fence |
emit-findings.sh:296-307 — fails closed on any .. segment; residual (in-repo symlink escaping outward) is disclosed in the same comment, not silently left |
branch: unquoted when YAML implicitly types it (true/null/123/dates) |
emit-findings.sh:178-194 — yaml_implicit_typed() covers booleans/null, ints, floats, ISO dates, hex/octal/binary |
Unscoped Bash(git:*) contradicting the read-only hard rule |
SKILL.md:6 — narrowed to git branch --show-current / git rev-parse --show-toplevel, matching the two call sites in emit-findings.sh |
Sentence splitter under-splitting on a mid-line abbreviation (e.g. e.g.), letting an earlier guardrail silently suppress a later, unrelated prohibition |
noise-shapes.sh:416-446 |
On the splitter fix specifically (the newest, least-reviewed code): it now peels sentences right-to-left with a greedy ^(.*[.!?])[[:space:]]+(.*)$, collecting tail fragments and reversing them back into original order. I traced this by hand against several cases — the PR's own repro (See e.g. the credential rotation policy. Never call the tool directly. → 3 sentences, correctly isolating the real prohibition), the same-sentence guardrail case (must still carve out), a no-trailing-punctuation final sentence, and a sentence with an embedded abbreviation immediately after a real prohibition (Never do X, i.e. don't do Y.) — the last one over-splits into two fragments, but that's the documented, accepted tradeoff (over-splitting only narrows a suppressing marker's window; it cannot silently withhold, which is the property this whole shape is built to preserve). audit_noise_line_has_negation_without_positive still returns on the first offending sentence per line, so over-splitting can't cause a double-count either. Both new regression cases (NEG_ABBREV, NEG_SAME) in detect.test.sh exercise exactly the two directions that matter.
I did not execute the test suites myself — Bash execution in this environment requires interactive approval that isn't available here, so detect.test.sh (122 checks) and emit-findings.test.sh (32 checks) results are author-reported and traced by hand rather than independently re-run. Everything I could statically verify (regex behavior, control flow, the crosswalk/allowed-tools/YAML-quoting fixes) checks out.
No new findings. Nothing further to flag on e0121e0 or 9fce71f beyond what the two automated reviewers already surfaced and what the author already fixed. The deferred soft-wrapped-sentence limitation (#3195) is a reasonable scope cut — false-positive direction only, documented in SKILL.md, not a regression risk for this PR.
· Branch: claude/work-items-integration-o22y7w
… (0.21.1) (#3202) Closes #3201 Adopts the two scope gates established in #3180: the negation cue must open the line (after list, blockquote and emphasis markers), and the line must close its own sentence. On an 85-file corpus sample this takes the negation shape from 1053 findings to 31, with every other shape's count byte-identical. Refs #3194, #3180, #3123, #3195.
1 participant
Closes #3123
Summary
Adds a ninth noise shape to
docs-hygiene:audit-noise— a prohibition with no positive alternative stated in the same sentence — and makes the skill a conformingdetector-findingsproducer, reusing the wiring #3120 established inclaude-config:audit-instructions. This is the audit-side completion of a doctrine the fleet already adopted on the write side (docs-hygiene:write-for-agents"Prompt the positive").Fix
The detector (D3).
negation, Tier 2 — its treatment includes an edit, so it cannot be Tier 3. Selection is per sentence, case-folded, whole-word, on the backtick-unwrapped line. Reading the unwrapped form rather than the inline-code strip the five older shapes read is load-bearing: a hard-guardrail marker is routinely the code span itself (`--force`,`rm -rf`), so stripping it would erase the very evidence the carve-out needs, turning a guardrail into a false finding rather than a withheld one.The read-only rule is amended, not quietly violated. The rule read "No
Edit, noWrite, no mutatingBashops", which as written forbade the producer contract this PR adds. It now states the distinction in its own text: read-only binds every audited target unconditionally, while the findings artifact is a new file in the gitignored memory tier, written only under--persist-findings, and is a proposal for a human-gated relay rather than an applied edit. The rule widens exactly that far — no audited file becomes writable, and a bare invocation still writes nothing.All three withholding boundaries are evidence-gated, so an unresolved candidate is EMITTED (the crosswalk's admission test 2, checked on every boundary rather than only the one easiest to argue): a paired positive, a hard guardrail whose constraint a positive form cannot carry, and a worked example each require their evidence present on the sentence. Every marker matches as a whole word, and sentences are split so an embedded abbreviation cannot merge two clauses — both are cases where a shortcut would let one clause's carve-out silently swallow another clause's finding. The carve-out lives in the shared scanner, so the human report and the relay file give one candidate one disposition.
Crosswalk rows.
rule-negation-without-positiveat IMPORTANT, argued fromseverity.md's stated-rule limb rather than the degradation limb bothaudit-instructionsrules walk.Auto-applicable: No, matching both sibling rules: contained toLocation, but recovering the positive target is a rewrite judgment. Plus a non-emittingrule-negation-hard-guardrailrow that states which ground it uses (the Boundary's "never reaches a relay", never a tier test).Verification
End-to-end relay proof, run rather than inspected:
emit-findings.shwrote a conformingtype: review-findingsfile (repo-relativeLocation,IMPORTANT/high, rule id leading theFindingcell).review:fanout fix --yeslocated it, admitted it ontype:+ exactbranch:, merged, classified (2 surface-only, per the crosswalk'sAuto-applicable: No), and wrote a digest-keyed consumption record.SUBTRACTEDon name + digest — the file is correctly retired from the next merge set.Fences proven against forged input, so the writer's own fence is under test, not the scanner's: a forged frontmatter row, a body line quoting a trigger phrase from its own
description, a forged non-crosswalk shape, and a..-traversing path are each declined with a counted reason.Gates (local, against the source checkout):
detect.test.shemit-findings.test.shdocs-hygienesuitescheck-skill.shcheck-detector-findings-crosswalk.shcheck-changelog-parity.sh(--check,--check-bump,--check-order)Review round addressed
Eight findings across both automated reviewers, each reproduced by running the detector before being fixed. Two were the failure class this rule set exists to prevent — silent withholding on a carve-out boundary:
secretarysatisfied thesecretguardrail;preferentiallysatisfied thepreferpairingpreferinflections enumerated,vulnerabkept stemmed but boundedSee e.g. the credential rotation policy. Never call the tool directly.split into one sentence, so the guardrail suppressed the prohibition. Now peeled right-to-left with a greedy leading.*, making an abbreviation over-split — the fail-safe direction. The comment claiming "over-split" was wrong and is correcteddon.tmatcheddonut(raised independently by both reviewers).detect.shcarriesFinding marker:, keeping ONE implementation of the sentence walk rather than a mirror free to drift..-traversing pathbranch:unquoted when YAML implicitly types ittrue/null/no/123/ dates quoted; ordinary names stay plainallowed-toolsgranted unscopedBash(git:*)negationis scoped to one physical line, so a soft-wrapped sentence is judged in piecesdetect.sh's iteration model and its interaction with every existing fence. False-positive direction only, never a silent withhold. Recorded as a known limitation inSKILL.md"Hard rules"Each fix carries a regression test; the suites grew 115 → 125 and 23 → 32.
Related
claude-config0.39.0; the issue is still open and is a stale item that can be closed.Caveat — lane provenance
This ran without a
work-itemsseam claim. The seam's coordination verbs are unavailable in the session that produced this PR:ghis 2.45 against a required ≥ 2.94, andGH_TOKENis invalid (gh api403), so no assignee/lease was acquired andclaim-before-dispatch did not run. Flagged rather than left silent — if another lane was working #3123 concurrently, this PR would not have detected the collision.