feat(scripts): gate the de-slopped surfaces against em-dash regression - #3344
Conversation
|
Claude finished @kyle-sexton's task in 4m 45s —— View job Code review complete
This lane covers correctness/maintainability only ( FindingsPosted one inline finding on Coverage-assertion gap: Other things checked, no issues found
Not independently verified: this environment blocked execution of |
|
Claude finished @kyle-sexton's task in 2m 7s —— View job Security review
No security issues found in this PR. Scope reviewed: the new CI gate ( What I checked for and didn't find:
Nothing here touches authn/authz, secrets, or code execution paths beyond a read-only lint gate over tracked markdown, and the design is consistent with the same-shaped |
![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: a1b64959cf
ℹ️ 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: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
The de-slop campaign purges prose surface by surface, and each landed shard is paid for by hand: a mechanical em-dash split changes meaning often enough that every shard so far has needed a rationale-withheld reviewer to catch clauses the automated passes waved through. Nothing then stopped the next contributor from reintroducing one, because no lane enforced the policy. A one-time purge with no gate is a purge that silently rots. Add scripts/check-purged-em-dashes.sh, which enforces zero em dashes on the paths declared in scripts/em-dash-purged-paths.txt, and wire it into the ci hygiene job so its outcome reaches the ci-status aggregate. Enforcement is scoped to an allowlist rather than the corpus. 29,649 em-dash prose lines across 1,074 tracked markdown files remain, so a repo-wide check could only have been merged switched off. Listing what is already clean inverts that: adoption breaks nothing, because every listed path passes the moment it is added, and enforcement grows with the campaign instead of waiting for it. This is the same allowlist-not-blocklist argument, and the same fail-safe direction, that scripts/docs-only-paths.txt already makes. The tracked .claude/ai-slop.json is untouched. Re-enabling rule-em-dash corpus-wide there is a separate decision the campaign has gated on the purge finishing, so the gate instead copies that config into a throwaway layer for its own detector run and removes only rule-em-dash from disabled_rules. Every exclusion the tracked file carries, including the vendor, catalog and eval-fixture paths that hold em dashes as data, keeps applying unchanged. The detector is driven rather than reimplemented: a bare grep would fire inside code fences, inline code spans and ignore-marked regions, which legitimately carry the character. Because that detector exits 0 on every audit path, the gate believes a run only when the rule-em-dash summary is present, reports disabled=0, and accounts for exactly the files handed to it; any other shape is exit 2 rather than a false green. A stale allowlist entry matching no tracked file fails for the same reason. Refs #2891 Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…right The gate shipped mode 100644 while the workflow invokes it as a bare path, `scripts/check-purged-em-dashes.sh`, exactly as it invokes every sibling scanner. Every one of those siblings is tracked 100755. The hygiene job would therefore have failed on contact with a permission error, and the repository's own unconditional exec-bit scanner would have reported the same defect a second time. Both the gate and its test script are now tracked executable. Add plugins/playwright to the allowlist. Its README and its skills were purged in #3320 and contain no em dash anywhere, so the entry meets the stricter standard the allowlist header declares for a seed entry: the whole list stays auditable by expanding it with --list and grepping the named files for the character. The plugin was clean when the list was seeded and was simply not carried over. Restructure the spaced double hyphen in the workflow comment into a sentence break. The gate's own failure message tells a contributor not to reach for a hyphen when an em dash comes out, and a comment introducing that gate should not model the substitution it warns against. Refs #2891 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gate A fresh-context reviewer audited the gate with the authoring rationale withheld and returned two defects that every automated pass had waved through. Both are fixed here, each with a regression case that fails against the previous code. --list ran the whole detector. The mode changed only what the expansion loop printed; control then fell through to the detector invocation and the check verdict, so a listing cost the same minutes as a full check and returned the check's exit code. That contradicted the allowlist header, which sends a reader to --list precisely to audit the declaration WITHOUT running the detector. It now stops after expansion, still failing on a stale entry, and the real allowlist lists in under a minute instead of nine. The coverage assertion double-counted a declined file. The gate proved the detector had accounted for exactly the files handed to it by adding the two totals in the detector's summary, but those totals overlap: a file the detector opens and then declines on an in-file marker is counted under BOTH "files scanned" and "files declined", while a file excluded by a config glob is never opened and appears only under the latter. An allowlisted path carrying an ai-slop-ignore-file marker therefore failed the run at exit 2, naming a coverage problem that did not exist, for using an exemption the allowlist explicitly promises. Unique files handled is now the scanned count plus the excluded-glob declines alone, read from the decline rows the detector already labels with a cause. Two smaller corrections come with them. A declared path that the tracked detector config excludes is now named in the output instead of being folded into the clean count: such a path sits inside the allowlist and outside enforcement at once, which is the one way this gate can report green over a surface it reads nothing on. And the clean verdict now reports files scanned rather than files declared, so the number means what it says. The workflow comment called an ungated docs-only diff a fail-closed hole when the hazard is the opposite, and the two enumerations of the unconditional whole-repo scanners had not been told about the fourth one. Refs #2891 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…orced Two switches in the tracked config could leave a declared path green without being judged, and the gate reported neither as a failure. `em_dash_allowed_paths` and `rule_allowed_paths["rule-em-dash"]` exempt a file from this one rule. The derived config copied both through, and the exemption is invisible in the run it produces: detect.sh still OPENS such a file, so it lands in the scanned count, the coverage assertion still balances, and the rule summary still reports `disabled=0`. Every signal the gate reads says the surface was checked and clean while no finding on it was ever possible. The derivation now strips both, alongside the `disabled_rules` entry it already stripped, because a path on the allowlist declares the surface purged and a per-rule exemption on that path is the opposite claim. A declared file that genuinely carries the character as data still has the detector's in-file exemptions, and failing those it belongs off the allowlist with the reason. `excluded_paths` is the one exclusion left standing, since its files are never opened and so remain visible as excluded-glob declines. The gate named them and then fell through to the clean path, printing "no em dashes" and exiting 0. A declaration the detector never reads is not coverage, and a green exit is how it gets mistaken for some, so that count is now a nonzero verdict with a message that distinguishes it from a clean run. Four cases cover it, each pointed at the seeded violation, which is the only fixture where suppression is observable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N2RpNNVGx3wMp8oFeFVBSA
a1b6495 to
af46aa6
Compare
|
Claude finished @kyle-sexton's task in 3m 11s —— View job Code review complete
This lane covers correctness/maintainability only ( SummaryThis is a re-review after new commits (
I also independently checked, beyond re-verifying the two fixes:
No new findings. Both fixes are correct, minimal, and each has a fixture that fails against the pre-fix code (verified by reading the fixture/assertion pairs, not by executing them). Not independently verified: this sandboxed environment blocks running shell scripts ( |
|
Claude finished @kyle-sexton's task in 1m 14s —— View job Security review
Scope reviewed: commits landed since the last security-reviewed head ( No security issues found. What changed: What I checked for and didn't find:
Nothing in this incremental diff touches authn/authz, secrets, or code execution paths beyond the same read-only lint gate reviewed previously. No inline comments filed. |
main landed three commits after this branch was cut, and one of them collided: ai-slop released 0.4.1 for the Windows jq carriage-return fix while this branch had already used 0.4.1 for its table-of-contents entry. This conflict is why CI had gone quiet rather than red. GitHub schedules pull_request workflows against the computed merge commit, and with a conflict it cannot compute one, so ci and pr-title were never created for this head while the pull_request_target workflows (do-not-merge, pr-issue-linkage) kept reporting green against the base. Three green checks on a PR that should run a dozen is the tell; a missing run reads exactly like a slow one. Same resolution rule as the earlier collision on this branch: main's entry is released and keeps its number, this branch's entry is unreleased and renumbers above it, and the manifest ends at the highest. So the table-of-contents entry becomes 0.4.2, main's 0.4.1 stays where it is with its body verbatim, and the manifest reads 0.4.2. One thing in main's other commit needed checking rather than assuming. #3344 added scripts/check-purged-em-dashes.sh, a ratchet enforcing zero em dashes on an allowlist of already-purged surfaces, and that allowlist covers plugins/implementation/skills/*/SKILL.md and plugins/overengineering/skills/*/SKILL.md, both of which this branch edits. The table-of-contents pass copies heading text verbatim into its rows, em dashes included, so the two changes could have collided by construction. They do not: the gate passes, 21 declared paths and 30 files scanned. Verified on the merged tree: no conflict markers anywhere, changelog parity, order and bump gates clean, the new em-dash gate clean, plugin contracts across 3023 files, and markdownlint clean at 1359 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTVH5ZRVph75kvxzUAnb6q
1 participant
No linked issue
Summary
The de-slop campaign (#2891) has rewritten prose surface by surface for a week, and each shard is paid for by hand: a mechanical em-dash split changes meaning often enough that every shard so far has needed a rationale-withheld reviewer to catch clauses the automated passes waved through. Nothing then stopped the next contributor from reintroducing one, because no lane enforced the policy. This adds the ratchet, so that what the campaign has already cleaned stays clean.
scripts/check-purged-em-dashes.shenforces zero em dashes on the paths declared inscripts/em-dash-purged-paths.txt, and runs unconditionally in the hygiene job.Enforcement is scoped to an allowlist rather than the corpus, and that is the design rather than a concession. Roughly 29,600 em-dash prose lines across 1,074 tracked markdown files remain, so a repo-wide check could only have been merged switched off. Declaring what is already clean inverts the problem: adoption breaks nothing, because every listed path passes the moment it is added, and enforcement grows with the campaign instead of waiting for it. It is the same positive-list shape, and the same fail-safe direction, as
scripts/docs-only-paths.txt.The allowlist currently covers ten plugins. Sixty-nine have had instruction surfaces rewritten by campaign shards, so the gate defends a fraction of what has been cleaned; the backfill and what blocks it are tracked in #3342.
Fix
scripts/check-purged-em-dashes.shdrivesplugins/ai-slop/skills/audit/scripts/detect.shrather than grepping, so fenced code blocks, inline code spans, and ignore-marked regions keep carrying the character as data. The tracked.claude/ai-slop.jsonis not modified; the gate copies it into a throwaway layer for its own run. That layer strips every switch that can quietrule-em-dashon a declared path: thedisabled_rulesentry,em_dash_allowed_paths, andrule_allowed_paths["rule-em-dash"]. A path on the allowlist declares the surface purged, and a per-rule exemption on that path is the opposite claim; honoured, it would let the gate report the surface checked and clean while no finding on it was ever possible, because such a file is still opened and still counted as scanned.excluded_pathsand every threshold are left standing, including the vendor, catalog and eval-fixture paths that hold em dashes deliberately: those files are never opened at all, so they stay visible as excluded-glob declines that the verdict names and fails on. A declared file that genuinely carries the character as data still has the detector's in-file exemptions, and failing those it belongs off the allowlist with the reason.HOMEis emptied so a contributor's user-global config cannot move the verdict.rule-em-dashsummary is present, reportsdisabled=0, and accounts for exactly the files handed to it. Any other shape is exit 2 rather than a false green, and a stale allowlist entry matching no tracked file fails for the same reason. A declared path the tracked config puts out of enforcement throughexcluded_pathsfails too, at exit 1 with a message distinct from a findings failure: the detector never read it, so declaring it enforces nothing, and a green exit is how that gets mistaken for coverage.scripts/em-dash-purged-paths.txtseeds the declaration. Every entry meets a stricter standard than the gate enforces, containing no em dash anywhere rather than none in prose, so the whole list can be audited by expanding it with--listand grepping the named files..github/workflows/ci.ymlruns the self-test first and ungated, so a broken gate cannot mask a regression, then the gate itself, whose outcome reaches theci-statusaggregate throughaggregate-hygiene-results.sh. The step is deliberately not gated onrun_full: it reads markdown prose, which is exactly what a docs-only diff changes.Verification
An independent fresh-context reviewer audited the change with the authoring rationale withheld and returned two defects, both fixed in
a1b64959cwith regression cases that fail against the previous code:--listran the whole detector rather than stopping at expansion, contradicting the allowlist header's own audit instructions; and the coverage assertion added two overlapping detector totals, so an allowlisted file carrying anai-slop-ignore-filemarker failed the run at exit 2 for using an exemption the allowlist promises.plugins/naming/README.md): exit 1, naming file and line. Reverted: exit 0.scripts/check-purged-em-dashes.test.sh:PASS=21 FAIL=0. The suite drives the real detector rather than a stub, plants a violation and requires exit 1, and separately plants em dashes in a code fence and an ignore-marked line and requires silence.scripts/check-purged-em-dashes.sh: 21 declared paths, 30 files scanned, exit 0. No declared path overlaps the tracked config'sexcluded_paths, so the new failing verdict does not fire on this repository today.scripts/check-purged-em-dashes.sh --list: 55 seconds, down from roughly nine minutes.git ls-tree HEADconfirms both scripts tracked100755; the workflow invokes the gate as a bare path.shellcheck -x,shfmt -d -i 2, andactionlintclean.check-changelog-parity.sh --check-bump origin/main,check-silent-skips.sh,check-discriminating-test-skips.sh,check-orphaned-fixtures.sh, andcheck-docs-only-gate.sh --checkall pass. No plugin version changes, so no changelog entry is due.Related
Refs #2891. This shard does not close it. The campaign's second checkbox, carrying the purge through the remaining corpus, is nowhere near done, and its fourth, tightening
.claude/ai-slop.jsononce the purge completes, is explicitly gated on that. A gate over already-cleaned paths is the ratchet the campaign was missing, not its completion.excluded_pathscases in the suite depend on that fix: before it,excluded_pathsnever applied on a Windows workstation, so a case asserting the exclusion took effect passed on the Linux runner and failed locally. That is why the original suite documented the path as deliberately unexercised, and why it is covered now.