docs(scripts): name the surfaces the skill-count gate cannot see - #3050
Conversation
The gate's header already states which CLASS of stale count it holds (repository-derivable) and which it does not (document-internal). It said nothing about SURFACES, and then asserted "Nothing else was scoped out" -- which reads as completeness the gate does not have. The omission has a measured cost. The PR that introduced this gate (#3034) carried a hand-written version-and-assertion table in its own body, and that table went stale within the hour when a merge with main renumbered two plugins. Those counts are class A -- plainly derivable from the tree -- and the gate could not see them, because a PR description is not a file in the repo. A reviewer caught it, which is precisely the control this gate exists to replace. So the header now names the surface boundary explicitly and the "nothing else" sentence is qualified to the files the gate actually reads. Prose about this repo that lives on GitHub rather than on disk -- a PR or issue body, a release note, a wiki page -- derives its counts at write time or drifts, and a green gate must not be read as covering it. Comment-only change; no behavior, no new surface scanned. Verified: shellcheck at the repo rcfile, shfmt, the 22 assertions, --check at 24 claims with 2 exempted, typos, editorconfig, and no version bump required (scripts/ is not a plugin, confirmed by changelog-parity --check-bump). Tracker references use the bare (#N) form the comment-hygiene lane requires, checked before pushing since that lane cannot be run locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM
|
Claude finished @kyle-sexton's task in 1m 10s —— View job Code review complete ✅
SummaryThis is a comment-only change to the header of The new No correctness, maintainability, or test-coverage issues found. Nothing here rises to the high-signal bar for this lane (no behavior change, no new surface, no logic touched) — this docs clarification does what it says. No inline findings to post. |
|
Claude finished @kyle-sexton's task in 42s —— View job Security review
Skip gate: no security-relevant surface. The entire diff in No security review findings. Branch: |
|
Last security-reviewed head: |
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![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: 62eb9f3126
ℹ️ 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".
…ning The surfaces paragraph listed "a release note" among off-disk prose whose counts drift, which contradicted a decision stated 40 lines above it in the same header: CHANGELOG.md is deliberately not scanned, because a dated entry describes the release it shipped with and rewriting it to match today's tree falsifies history rather than fixing a claim. A GitHub release note is that same kind of record. Its denominator is the tree at release time, so later divergence is valid history, not stale prose. As written the paragraph could prompt a maintainer to "fix" an accurate historical claim -- the opposite of what the changelog carve-out protects. So the list now names only surfaces that purport to describe the CURRENT repo (a PR or issue body, a wiki page), and the dated-record carve-out is stated explicitly as travelling off-disk with the count. Off-disk surfaces get the same current-state test as the scanned ones, not a broader one. Raised by an automated reviewer on the PR; the contradiction is real and the reviewer's reading of the carve-out matches the header's own words, which I re-read to confirm before changing anything. Comment-only, as before. Re-verified: shellcheck at the repo rcfile, shfmt, the assertion suite, --check against the tree, typos, editorconfig. Line lengths match the file's existing style, and tracker references stay in the bare (#N) form the comment-hygiene lane requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM
|
Claude finished @kyle-sexton's task in 1m 44s —— View job Code review —
|
|
Claude finished @kyle-sexton's task in 38s —— View job Security review
Skip gate: no security-relevant surface. Reviewed the full diff on No security review findings. Branch: |
…aky (#3057) No linked issue ## Summary `scripts/check-shell-portability.sh --all` was treated as environmentally unreliable across four separate attempts, each written off as a command timeout or a container restart. That diagnosis was wrong, and it was the self-perpetuating kind: "it got unlucky" invites retrying the same run, which fails the same way and yields no new information. Nothing in the repo recorded the real reason. ## Fix Adds a `COST of --all` block to the script's header. The cause is structural, not environmental. Per-file cost grows superlinearly with file length, so a handful of the longest files dominate the whole sweep while the great majority cost almost nothing. It cannot fit a 600s command timeout on a tree this size, so it must be detached rather than retried. Two traps are recorded alongside it, because neither is discoverable from a failing run: - `pgrep -f 'check-shell-portability'` matches the **waiting** shell's own command line, so a waiter built on it can never see its condition clear. Two waiters were lost to this before it was spotted. Waiting on the pid is correct. - A per-file sweep keyed only on the timeout status silently passes over any file exiting 1, so its silence reads as a clean audit when nothing was actually checked. The block also states why CI runs the changed-file mode and never `--all` — true of `ci.yml`'s `shell-portability-lint` already, but written down nowhere. **On the numbers:** they are stated as a dated observation rather than a standing claim, and no file list or file count is hardcoded. The exact figures move with the corpus; the shape does not. That is deliberately the same surfaces-and-derivability discipline the sibling skill-count gate's header gained in #3050 — hardcoding "seven files over N lines" here would have created precisely the drift that gate exists to catch. Comment-only. No behavior, no token-list edit, no new construct class. ## Verification - `shellcheck` at the repo's own `.shellcheckrc` — clean (re-run after rebase) - `shfmt -d` — clean (re-run after rebase) - `scripts/check-shell-portability.test.sh`, which CI runs ahead of the gate — `PASS=333 FAIL=0` - `typos`, `editorconfig-checker` — clean - `scripts/check-changelog-parity.sh --check` — no version bump required; `scripts/` is not a versioned plugin and nothing under `plugins/` is touched - Tracker references use the bare `(#N)` form the `comment-hygiene` lane requires, checked before pushing because that lane is an external composite action this repo does not vendor and cannot be run locally - Rebased onto current `main` rather than left on the base it was written against, so `stale-base-overlap-gate` sees a current merge base The claim this documents was itself measured end to end: `--all` completed at exit 0 with `No unexcused GNU-only constructs in 1329 shell file(s)`, which is the verification that had been outstanding since #3034. ## Related Refs #3050 — the sibling gate's surfaces boundary, whose derivability discipline this block deliberately follows. Refs #3034 — the PR whose outstanding `--all` verification this work completed. --- _Generated by [Claude Code](https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
…erence form (#3061) No linked issue ## Summary Two findings from a babysit session, both of which cost real time and neither of which was written down anywhere. They are unrelated in mechanism but share a cause: an operator hits them only in CI, with no local signal. ## Fix **1. `stuck-checks.md` — checks that never schedule.** That file covered checks which are *present and never settle*, detected under `UNSTABLE`. It did not cover the opposite failure with the same surface complaint. A conflicted PR has no computable merge ref, so `pull_request` workflows are never created — **absent** rather than pending or failing, and therefore invisible to `checks.stuck`, which can only classify records that exist. What makes it actively misleading is that `pull_request_target` lanes run against the base and still pass. The PR shows a short all-green list with no failures anywhere — reading as "passing" or "not started yet" — while most gates are simply missing. The remedy is merge-or-rebase, not a CI investigation; diagnosing it as a trigger or App-token problem spends time on an uninvolved mechanism, which has already happened here once. The section **names** the two `pull_request_target` lanes rather than counting the split, and says to derive the split from the `on:` blocks. A count in prose goes stale the next time a lane is added — the exact defect the two recent skill-count-gate changes addressed. **2. New `docs/conventions/tracker-reference-form`.** What a code comment may say about an issue. It needs a written home precisely because the `comment-hygiene` lane **cannot be run locally**: the action is not vendored, so a violation is discoverable only after pushing. Written from the action source at the SHA `ci.yml` pins, not from memory — and that mattered. The belief carried into this change was that `PR #N` and `owner/repo#N` are simply rejected, but the tree contains *passing* instances of both. Reading the action resolved the contradiction: the allowlist is an extension set that excludes Markdown and YAML, so those instances are out of scope rather than tolerated. The doc states that scoping explicitly, so the counterexamples don't later read as evidence the rule is lax. Documentation only; no executable surface. ## Verification - `check-changelog-parity.sh --check`, `--check-bump origin/main`, `--check-order` — all green; `source-control` bumped with a matching entry - `check-skill-count-claims.sh --check` — every claim matches the tree - `check-shell-portability.sh --paths` on the changed skill file — clean - `markdownlint-cli2` on both docs — 0 issues - `typos`, `editorconfig-checker`, and `jq` on the manifest — clean The one rule this PR documents that it cannot verify locally is `comment-hygiene` itself; the added comment text was grepped for each rejected shape before pushing, and both changed docs are Markdown, which the policy does not scan. ## Related Refs #3050 and #3057 — the two changes whose sessions produced these findings. This PR closes out the last of them, so nothing from that work remains unrecorded. --- _Generated by [Claude Code](https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
No linked issue
Summary
The skill-count gate's header states which class of stale count it holds
(repository-derivable) and which it does not (document-internal), then asserts
"Nothing else was scoped out" — which reads as completeness the gate does not
have. It never named the surface boundary: the gate reads files in the repo,
so a class-A count written anywhere else is equally derivable and equally
unguarded, because nothing here can see it.
Fix
Adds a
SURFACES, not just classesparagraph to the header and qualifies the"nothing else" sentence to the files the gate actually reads.
The omission has a measured cost, and it is this gate's own introduction that
paid it: PR #3034 carried a hand-written version-and-assertion table in its
body, and that table went stale within the hour when a merge with
mainrenumbered two plugins. Those counts were class A — plainly derivable from the
tree — and invisible to the gate, because a PR description is not a file in the
repo. A reviewer caught it, which is precisely the control this gate exists to
replace.
So the header now says: prose describing the CURRENT repo that lives on GitHub
rather than on disk — a PR or issue body, a wiki page — derives its counts at
write time or drifts, and a green gate must not be read as covering it.
A second commit narrows that list. The first draft included "a release note",
which contradicted a decision stated 40 lines above it in the same header:
CHANGELOG.mdis deliberately not scanned, because a dated entry describes therelease it shipped with and rewriting it to match today's tree falsifies history
rather than fixing a claim. A GitHub release note is that same kind of record.
The header now states that carve-out explicitly as travelling off-disk with the
count, rather than leaving it implied — off-disk surfaces get the same
current-state test as the scanned ones, not a broader one. Raised by an
automated reviewer; the contradiction was real.
Comment-only change throughout. No behavior, no new surface scanned, no
token-list edit.
Verification
shellcheckat the repo's own.shellcheckrc(not a laxer severity) — cleanshfmt -d— cleanscripts/check-skill-count-claims.test.sh— all assertions passedscripts/check-skill-count-claims.sh --check— every claim matches the treetypos,editorconfig-checker— cleanscripts/check-changelog-parity.sh --checkand--check-bump origin/main—no version bump required;
scripts/is not a versioned plugin, and this PRtouches nothing under
plugins/(#N)form thecomment-hygienelane requires, checked before pushing because that lane isan external composite action this repo does not vendor and so cannot be run
locally
All of the above were re-run after the second commit, not just the first.
This body deliberately carries no hand-written counts of assertions, claims, or
versions — the exact failure mode the change documents. The numbers are in the
command output above, where they are regenerated rather than transcribed.
This body was itself corrected once. After the second commit it still
described the superseded wording, which is the same off-disk drift the change
documents, occurring in this PR's own description. Fixed here rather than left
as a demonstration.
Related
Refs #3034 — the PR that introduced this gate, and whose own body supplied the
stale-count evidence cited above.