docs(source-control): document unscheduled checks and the tracker-reference form - #3061
Conversation
…erence form Two findings from a babysit session, both of which cost real time and neither of which was written down anywhere. 1. `stuck-checks.md` covered checks that are present and never settle, detected under UNSTABLE. It did not cover the opposite failure with the same surface complaint: checks that never get created at all. A conflicted PR has no computable merge ref, so `pull_request` workflows are never scheduled -- absent rather than pending or failing, and so 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 then shows a short all-green list with no failures anywhere, reading as "passing" or "not started", while most gates are missing. The remedy is to merge or rebase, not to investigate CI; diagnosing it as a trigger or App-token problem is time spent on an uninvolved mechanism, which has already happened here once. The section deliberately NAMES the two `pull_request_target` lanes instead of 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 defect two recent changes to the skill-count gate were about. 2. A new `docs/conventions/tracker-reference-form` records what a code comment may say about an issue. This needs a written home specifically because the `comment-hygiene` lane CANNOT be run locally: the action is not vendored, so a violation is only discoverable after pushing. Written from the action source at the SHA ci.yml pins, not from memory. That mattered: the belief being 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 it -- the allowlist is an extension set that excludes markdown and YAML, so those instances are out of scope rather than tolerated. The doc states the scoping, so the counterexamples do not later read as evidence the rule is lax. Documentation only; no executable surface. Verified: changelog parity (--check, --check-bump, --check-order), the skill-count gate, portability on the changed skill file, markdownlint, typos, editorconfig, and manifest JSON. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM
|
Claude finished @kyle-sexton's task in 46s —— View job Security review — no relevant surface
Skip gate hit: no security-relevant surface. All four changed files are documentation/metadata only:
No GitHub Actions workflows, scripts, credentials handling, or trust-boundary logic are touched, so there is no injection surface, permission-widening, or supply-chain change to evaluate. Per this lane's skip gate, stopping here with no findings. |
|
Last security-reviewed head: |
|
Claude finished @kyle-sexton's task in 3m 32s —— View job Code review complete
Scope: Documentation-only PR ( No blocking or suggestion-level findings. No inline comments were posted. I independently verified (not just restating the author's claimed verification) the factual claims this PR's argument depends on, against the current tree:
Not independently verifiable from this checkout (network access to the external |
![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: 1452a46055
ℹ️ 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 has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…and name both exclusions Two reviewer findings, both real, and the first is the more serious kind: the guidance was not wrong, it was unreachable. 1. `stuck-checks.md` is loaded only when `checks.stuck` is non-empty (`runbook-cycle.md`), and `SKILL.md` advertised it as the UNSTABLE signal alone. The section added in the previous commit applies precisely when that array is EMPTY, so under the documented load rules the flow would never open the file in the situation the section exists for. Content nobody reaches is worse than content that is merely imprecise, because it looks done. The snapshot already models this case: `compute_branch_freshness` maps DIRTY and CONFLICTING to `branch_freshness.state == "conflicting"`. That is the section's real queue signal, so the load condition now fires on it as well, `SKILL.md`'s description covers both directions, and the section states its own entry condition instead of leaving it implied by the file it sits in. 2. The tracker-reference doc named only the `scripts/**` exclusion. `ci.yml` excludes `audit-comment-residue`'s `scripts/**` AND `evals/**`, and the omission is observable rather than academic: `evals/fixtures/ residue-snippet.py` carries a scanned extension and a rejected form, and passes only because the evals tree is excluded. A doc explaining the policy's scope while understating that scope would have contradicted itself. Both are now named, pointing at ci.yml as authoritative. The cause of (2) is worth recording: the exclusion was read through a `sed` window that ended one line above the second entry, and the truncated view was written up as if complete. The reviewer had the whole block. Documentation only. Re-verified after the fixes: markdownlint, typos, editorconfig, changelog parity (--check-bump, --check-order), and the skill-count gate. Version stays 0.54.13 -- unreleased in this PR -- with its changelog entry extended rather than a second version added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM
|
Claude finished @kyle-sexton's task in 35s —— View job Security review — no relevant surface
Skip gate hit: no security-relevant surface. The last successful security review on this PR covered up to
No GitHub Actions workflows, scripts, credentials handling, or trust-boundary logic are touched by this commit. All changes are Markdown prose/reference-link edits inside documentation and skill-guidance files. No injection surface, permission-widening, or supply-chain change to evaluate. Per this lane's skip gate, stopping here with no findings. |
|
Claude finished @kyle-sexton's task in 2m 39s —— View job Code review complete
Scope: This is a re-review triggered by the new commit No blocking or suggestion-level findings. No inline comments were posted. I independently verified both fixes against the actual source they claim to describe, not just the prose:
Both Not independently re-run in this environment (no network access): |
2 participants
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 underUNSTABLE. It did not cover the opposite failure with the same surface complaint. A conflicted PR has no computable merge ref, sopull_requestworkflows are never created — absent rather than pending or failing, and therefore invisible tochecks.stuck, which can only classify records that exist.What makes it actively misleading is that
pull_request_targetlanes 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_targetlanes rather than counting the split, and says to derive the split from theon: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 thecomment-hygienelane 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.ymlpins, not from memory — and that mattered. The belief carried into this change was thatPR #Nandowner/repo#Nare 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-controlbumped with a matching entrycheck-skill-count-claims.sh --check— every claim matches the treecheck-shell-portability.sh --pathson the changed skill file — cleanmarkdownlint-cli2on both docs — 0 issuestypos,editorconfig-checker, andjqon the manifest — cleanThe one rule this PR documents that it cannot verify locally is
comment-hygieneitself; 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