perf(ci): short-circuit hygiene's docs-irrelevant linters on docs-only diffs (#628)

[kyle-sexton]

Summary

Docs-only PRs still paid the hygiene job's path-scoped linter cost — actionlint and four check-jsonschema steps — for a diff that provably cannot affect any of them. #617 (via #622) short-circuited plugin-gate and miro-plugin, which cut runner-minutes but not the merge-latency bottleneck. This applies the SAME self-test-first, fail-closed, inner-step-gated discipline to hygiene's genuinely path-scoped subset, so those runs report an honest evaluated-and-not-applicable success instead of running.

ShellCheck and exec-bit are deliberately NOT gated: both scan the whole repo extension- and directory-agnostically (ShellCheck lints every tracked *.sh/*.bash via git ls-files; exec-bit flags every tracked shebang file recorded 100644 via git grep -- .), so a shell or shebang file added under an otherwise docs-only prefix like docs/topics/ is real input they must still catch. They run on every diff alongside the docs-linting checks that read the changed docs directly: markdown, typos, editorconfig, gitleaks, eol-renormalize, comment-hygiene, machine-specific-paths.

Scope of the perf win (honest): the initial revision of this PR also gated ShellCheck (~53s, the dominant cost) and exec-bit. Bot review (P2, thread on ci.yml) surfaced that both are whole-repo scanners, so gating them opened a fail-closed hole — a docs-only PR adding a mode-100644 shebang file (or a *.sh) under docs/topics/ skipped both and the ternary mapped them to success, letting a file the commit procedure requires exec-bit to catch merge unlinted. Ungating them (commit f73be27) closes the hole but removes the dominant ShellCheck skip, so the remaining docs-only win is limited to actionlint + the four check-jsonschema steps. A perf-recovery follow-up (cache/pin the ShellCheck install, which downloads a release tarball every run) is tracked separately and lives in melodic-software/ci-workflows, not this repo.

Fix

Reuses the existing scripts/check-docs-only.sh detector and scripts/docs-only-paths.txt allowlist from #622 unchanged — no new detector.

In the hygiene job (.github/workflows/ci.yml):

• Added the always-run Test the docs-only detector self-test and a PR-only Detect a docs-only diff step (id: scope), mirroring plugin-gate/miro-plugin. The checkout already had fetch-depth: 0, so the base ref resolves.

• Gated exactly the five path-scoped steps with if: steps.scope.outputs.docs_only != 'true': actionlint, marketplace_schema, plugin_schema, dependabot_schema, workflow_schema. shellcheck and exec_bit stay unconditional.

• The job never skips and the aggregator is never weakened. The hygiene job uses continue-on-error: true per step plus a CHECK_RESULTS feed into scripts/aggregate-hygiene-results.sh, which fails closed on any non-success outcome (including skipped). A gated step that skips would therefore red the job — so each gated entry in CHECK_RESULTS maps the intentional docs-only skip to success in the expression layer:

  actionlint=${{ steps.scope.outputs.docs_only == 'true' && 'success' || steps.actionlint.outcome }}

  This is honest, not a swallow: the ternary condition (docs_only == 'true') is the exact inverse of each step's gate, so success is fed ONLY for a step that provably did not run — no real outcome is ever masked. The two whole-repo scanners (shellcheck, exec-bit) feed their raw .outcome. The aggregator script is untouched and still fails closed on any real failure (or a skipped it isn't told to tolerate). This is the same evaluated-and-not-applicable success plugin-gate reports with its log step, expressed here through the feed the hygiene job already uses.

• Added an explicit Report docs-irrelevant checks not applicable log step (gated on docs_only == 'true') so a human reading a green run understands why those checks show skipped.

Also extended the two non-workflow artifacts per the established two-part-proof discipline:

• scripts/docs-only-paths.txt — updated the inertness-proof comment to name only the genuinely path-scoped gated subset (actionlint + the four schema checks, none of which read docs/topics/) and to state explicitly that ShellCheck and exec-bit scan the whole repo and stay unconditional (as do the docs-linters). Allowlist itself unchanged (still docs/topics/ only — no growth).
• scripts/check-docs-only.test.sh — added three cases pinning the remaining check-jsonschema inputs to false (.claude-plugin/marketplace.json, plugins/*/.claude-plugin/plugin.json, .github/dependabot.yml); the actionlint / workflow-schema inputs (.github/workflows/ci.yml) were already pinned false. These fail the moment someone widens the allowlist to cover an input a gated check reads.

Verification

Run in the worktree (Git Bash):

• bash scripts/check-docs-only.test.sh → PASS=19 FAIL=0. Confirms docs/topics/-only → true and every gated check's input (.github/workflows/ci.yml, .claude-plugin/marketplace.json, plugins/*/.claude-plugin/plugin.json, .github/dependabot.yml, lockfiles) → false.
• scripts/aggregate-hygiene-results.sh --self-test → Hygiene result aggregation contract passed. The aggregator is unchanged and still fails closed on a named empty/failure/non-success outcome.
• actionlint .github/workflows/ci.yml → clean. Validates the five added gate conditions and the five ternary expressions in CHECK_RESULTS.
• shellcheck --rcfile .shellcheckrc and shfmt -d on the edited/reused scripts → clean, no diff.

Honest scope of what this PR's own CI proves: this PR touches .github/ and scripts/, so on it docs_only=false and the full hygiene suite runs — a live proof only of the run-path (nothing is wrongly skipped on a code diff). The skip-path is not live-observable here by construction; it is proven by the detector unit test (docs/topics/-only → true) plus the gate/expression logic being the exact inverse of each other. Correctness of the intentional-skip → success mapping rests on that unit test and the expression, not on a fabricated observation.

Versioning/changelog: none. No plugins/<name>/ directory is touched — this is a repo-root CI-only change — so per the precedent #622 followed (matching CI-only #490/#593) there is no plugin version bump or CHANGELOG entry.

Closes #628

Related

• ci: short-circuit hygiene lane's docs-irrelevant full-repo linters (ShellCheck ~53s) on docs-only diffs #628 — this issue.
• ci: path-scope heavy CI jobs so docs-only PRs skip them (merge-latency reduction) #617 (closed) / perf(ci): short-circuit plugin-gate and miro-plugin on docs-only diffs (#617) #622 (merged) — the established precedent this extends: same scripts/check-docs-only.sh detector, same self-test-first / fail-closed / inner-step-gated discipline, applied to hygiene instead of plugin-gate/miro-plugin.
• convention: false-green checks — surfaces report success while the capability is dead or findings invisible (liveness-assertion umbrella) #532 — the false-green / liveness class this must not reintroduce. A job-level skip (or teaching the aggregator to pass on skipped) is exactly that shape and is deliberately avoided: the job always runs, the detector self-tests first, and only inner steps are gated with their skip reported as an honest not-applicable success.

🤖 Generated with a Claude Code implementation subagent (issue #628)

[claude]

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

---

PR Review

• Gather context and understand the request
• Read changed files and diff
• Review gate logic and aggregator feed expressions
• Review test coverage additions
• Review docs-only-paths.txt comment updates
• Post review findings

---

This is a clean, well-reasoned extension of the established plugin-gate/miro-plugin docs-only short-circuit pattern to the hygiene job. The logic is sound and the fail-closed invariant is preserved at every level. A few observations:

Gate logic — correct

The seven gated steps (lines 93–148) all use if: steps.scope.outputs.docs_only != 'true'. Three cases check out:

Trigger	docs_only output	Gate evaluates to	Steps run?
PR, docs-only diff	'true'	false	Skipped ✓
PR, code diff	'false'	true	Run ✓
Push to main (scope skipped)	'' (empty)	true	Run ✓ (fail-closed)

The empty-output case on push to main is the implicit fail-closed path — no special handling needed.

Aggregator ternary — correct and honest

The ternary pattern (lines 206–212):

shellcheck=${{ steps.scope.outputs.docs_only == 'true' && 'success' || steps.shellcheck.outcome }}

The ternary condition (docs_only == 'true') is the exact inverse of the step gate (docs_only != 'true'), so 'success' is injected only when the step provably did not run — it can never mask a real failure. The aggregator script confirms it treats anything other than 'success' as a failure, including 'skipped', so there's no path where a genuinely-skipped step could silently pass.

Push-to-main trace: scope doesn't run → docs_only == '' → ternary is false → real step outcome used → correct.

Scope step failure behavior — correct

The scope step has no continue-on-error. If it fails before writing to GITHUB_OUTPUT, docs_only remains empty. Subsequent gated steps would be skipped by GitHub Actions' implicit success() requirement on steps without explicit status-check functions in their if: condition. The aggregator runs via if: always(), sees 'skipped' for every gated step, and fails closed. This is the right behavior.

Test coverage — complete

The three new cases (lines 100–102) correctly pin the three remaining check-jsonschema inputs that weren't already covered (.claude-plugin/marketplace.json, plugins/p1/.claude-plugin/plugin.json, .github/dependabot.yml). The comment at line 97–99 accurately notes that scripts/, .github/workflows/, and plugins/** were already covered by earlier cases.

The PASS=19 total (was 16 + 3 new) matches what the test file contains.

docs-only-paths.txt comment — accurate

The updated comment (lines 14–28) correctly enumerates the hygiene subset's inertness argument and explicitly calls out that the docs-linting steps (markdown, typos, editorconfig, gitleaks, eol-renormalize, comment-hygiene, machine-specific-paths) remain unconditional. The allowlist itself is unchanged (docs/topics/ only).

Minor observations (no action required)

Self-test triplication: The check-docs-only.test.sh suite now runs in three jobs per CI invocation (hygiene, plugin-gate, miro-plugin). The PR body acknowledges this is intentional — a broken detector in any job can't mask a regression. The 19-case suite is fast enough that this is acceptable, and the pattern is pre-established.

exec_bit categorization: Correctly placed in the docs-irrelevant set — the exec-bit action scans for shebang files missing the executable bit, which requires a scripts/ or plugins/ change to matter.

machine-specific-paths stays unconditional: Correct — checking for accidentally-committed local paths (e.g., /home/username/...) is relevant to doc files too.

Summary

The implementation is logically correct, consistent with the established precedent, and the inertness proof is complete (gate + ternary are inverses, allowlist is positive-only, test suite pins every gated input). No issues found.

[kyle-sexton]

Post-green review classification

No findings. Reviewer verified gate/ternary inverse-correctness, fail-closed behavior on scope-step failure and push-to-main, and complete test coverage for the newly-gated jsonschema inputs.

Applied area: security — this PR modifies .github/workflows/ci.yml, the merge-gate CI infrastructure itself, per rule 6c.

Orchestrator's own diff read (rule 10, cross-plugin-contract blast radius — this affects every plugin's PR checks) confirms the review bot's assessment independently.

CI green (16/16). Flipping to ready for review.

[claude]

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

---

PR Review

• Gather context and understand the request
• Read changed files and diff
• Review gate logic and aggregator feed expressions
• Review test coverage additions
• Review docs-only-paths.txt comment updates
• Post review findings

---

This is a clean extension of the established plugin-gate/miro-plugin docs-only short-circuit pattern to the hygiene job. The logic is sound, the fail-closed invariant is preserved at every level, and the inertness proof is complete.

Gate logic — correct

The seven gated steps (lines 93–148) all use if: steps.scope.outputs.docs_only != 'true'. GitHub Actions implicitly wraps this as success() && docs_only != 'true', so the full truth table is:

Trigger	docs_only	Gate	Steps
PR, docs-only diff	'true'	false	Skipped ✓
PR, code diff	'false'	true	Run ✓
Push to main (scope skipped)	''	true	Run ✓ (fail-closed)
scope itself fails	''	success()=false	Skipped → aggregator sees skipped → fails closed ✓

Aggregator ternary — correct and honest

The ternary pattern (lines 206–212):

shellcheck=${{ steps.scope.outputs.docs_only == 'true' && 'success' || steps.shellcheck.outcome }}

The ternary condition (docs_only == 'true') is the exact inverse of the step gate (docs_only != 'true'), so 'success' is injected only when the step provably did not run — it can never mask a real failure. Tracing all cases:

• Push to main: '' == 'true' → false → real step outcome used ✓
• PR, code diff: 'false' == 'true' → false → real step outcome used ✓
• PR, docs-only: 'true' == 'true' → true → 'success' injected for a step that provably didn't run ✓
• scope fails: '' == 'true' → false → aggregator gets shellcheck=skipped from the real (skipped) step outcome → fails closed ✓

Test coverage — complete

The three new cases (lines 100–102) correctly cover the remaining check-jsonschema inputs not already pinned by earlier cases:

assert_flag "marketplace manifest (marketplace-schema reads it)" false ".claude-plugin/marketplace.json"
assert_flag "plugin manifest (plugin-schema reads it)" false "plugins/p1/.claude-plugin/plugin.json"
assert_flag "dependabot.yml (dependabot-schema reads it)" false ".github/dependabot.yml"

These paths aren't seeded in mk_repo(), but assert_flag uses mkdir -p + creates the file and commits it as a new addition to the tree — a new file outside docs/topics/ is correctly detected as false by the allowlist. The PASS=19 total is correct (16 original + 3 new).

docs-only-paths.txt comment — accurate

The updated inertness-proof comment (lines 19–28) correctly enumerates the hygiene subset (ShellCheck, actionlint, the four check-jsonschema steps, exec-bit) and explicitly states the docs-linting steps remain unconditional. Allowlist is unchanged.

Minor observations (no action required)

Self-test triplication: check-docs-only.test.sh now runs in three jobs (hygiene, plugin-gate, miro-plugin). This is intentional per the pattern — a broken detector in any job can't mask a regression in that job's gated steps. The 19-case suite is fast and this is acceptable overhead given the CI safety guarantee it provides.

exec_bit categorization: Correctly in the docs-irrelevant set — the exec-bit scanner looks for shebang files missing the executable bit, which only matters when scripts/ or plugins/ changes.

machine-specific-paths and comment_hygiene stay unconditional: Both are correctly excluded from gating — they scan changed files directly and doc files are valid inputs for both checks.

No continue-on-error on scope: Correct by design. If scope fails, docs_only stays empty, gated steps are skipped via implicit success(), aggregator sees skipped, and fails closed. The absence of continue-on-error is the mechanism, not a gap.

Summary

Implementation is logically correct at every path (PR docs-only, PR code diff, push to main, detector failure). The gate and ternary are exact inverses, the aggregator is untouched and still fails closed on any real non-success outcome, and the test suite pins all inputs that feed gated checks. No issues found.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1685b5f90c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

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

---

PR Review

• Gather context and understand the request
• Read the fix commit and changed files
• Verify shellcheck/exec-bit ungate correctness
• Verify aggregator feed cleanup
• Verify remaining five gated steps (path-scoped subset)
• Verify test-suite and docs-only-paths.txt updates
• Post review findings

---

The fix commit (f73be27) correctly addresses the Codex P2 / kyle-sexton finding. Here's the full verification.

What the fix does

Removes the if: steps.scope.outputs.docs_only != 'true' gate from shellcheck and exec_bit, and switches their CHECK_RESULTS feeds from the ternary to raw .outcome. The five truly path-scoped steps (actionlint, four check-jsonschema) remain gated unchanged.

Correctness — four-case truth table

shellcheck and exec_bit (now unconditional):

Trigger	Was (broken)	Now (fixed)
PR, docs-only diff	skipped + ternary → success ❌	runs; real outcome fed ✓
PR, code diff	ran normally ✓	ran normally ✓
Push to main	ran normally ✓	ran normally ✓
scope fails	skipped + ternary → success ❌	ran (no gate); real outcome fed ✓

The fail-closed hole is closed. No gate → no ternary override → no masked outcome.

Five remaining gated steps (actionlint, four check-jsonschema) — unchanged and still correct:

These all read fixed, path-scoped inputs (.github/workflows/**, .claude-plugin/marketplace.json, plugins/*/.claude-plugin/plugin.json, .github/dependabot.yml) that a docs/topics/-only diff cannot touch. Their ternary feeds remain:

actionlint=${{ steps.scope.outputs.docs_only == 'true' && 'success' || steps.actionlint.outcome }}

On push to main, scope is skipped → docs_only == '' → ternary is false → real outcome used ✓

Aggregator feed — correct

ci.yml lines 206–221:

shellcheck=${{ steps.shellcheck.outcome }}    # raw — unconditional step
...
exec-bit=${{ steps.exec_bit.outcome }}        # raw — unconditional step

No ternary, no override path. Aggregator sees the real outcome in every scenario.

Prose corrections — accurate

docs-only-paths.txt (lines 19–35): Now correctly names only the five path-scoped steps in the "inertness proof," and explicitly moves ShellCheck and exec-bit to the unconditional list with the accurate reasoning: both scan the whole repo extension- and directory-agnostically.

ci.yml header comment (lines 28–44): Matches the new gate scope.

Report step log message (line 185): Updated to name only actionlint, check-jsonschema x4 as gated, and explicitly states ShellCheck/exec-bit are unconditional.

Test suite — no changes needed (observation only)

The check-docs-only.test.sh file is unchanged in this commit, which is correct. The scripts/ change → false case (line 90) and the plugins/miro/index.ts → false case still serve as valid regression guards; the fact that ShellCheck is now unconditional doesn't invalidate them as detector tests (they test the detector, not the gate).

Minor: the comment at lines 96–98 still reads "hygiene job's docs-irrelevant step set" and mentions "ShellCheck (scripts/, plugins/**)." With ShellCheck now unconditional, this label is stale — ShellCheck no longer belongs to the gated set at all. The cases themselves are correct; only the comment header is inaccurate. Low priority to fix, but worth noting.

Summary

The fix is correct, minimal, and closes the fail-closed hole identified in the prior review without introducing new issues. The five path-scoped gated steps are correctly retained. The inertness proof in docs-only-paths.txt and the ci.yml inline comments now accurately reflect what is and isn't gated. No logic issues found.

One cosmetic follow-up if desired: update the check-docs-only.test.sh comment at line 96 to remove ShellCheck from "docs-irrelevant step set" since it's no longer gated. Not blocking.