ci: INT-726 let scoped PRs satisfy their required checks - #476
Conversation
Branch protection requires the six per-tool jobs, but they were gated at the job level on paths-filter output, so a PR touching one tool skipped the other tool's jobs. GitHub does not accept a skipped check as a satisfied required check, so every single-tool PR was unmergeable without an administrator bypass. #464 and #474 both hit it. The cost is not the inconvenience. Branch protection works because merging over an unmet gate is deliberate and visible; when ordinary work cannot merge without a bypass, the bypass stops being remarkable and a real unmet gate looks like the routine ones before it. The six required jobs now run on every PR and gate their steps instead. A job with nothing to do reports success rather than skipping, which is the honest result -- the check ran and found no work -- and the required checks are satisfied without changing repository settings. build-test-shared and lint-shared keep their job-level condition; neither is a required check. [INT-726]
Review round. Gating every step duplicated the condition six times with nothing enforcing that a step added later carried it, and since actions/checkout was itself gated, an unguarded step on an irrelevant PR would have run against an unchecked-out workspace and failed for a reason that had nothing to do with the change. The work now stays in <job>-run, which keeps its job-level condition and may skip freely with no per-step guards at all. The protected check name lives on a wrapper that always runs and reports the outcome of the job that did the work, treating skipped as a pass because there was nothing to verify. The guard exists once per job rather than once per step, so adding a step to a -run job needs no thought. Branch protection is untouched: the required names are unchanged, they are just produced by the wrappers now. [INT-726]
Review round, and the first finding was a hole this change introduced. A -run job is skipped whenever its condition is false, and the condition reads detect-changes outputs. If detect-changes fails, those outputs are empty, every condition evaluates false, and all six jobs skip -- so a wrapper treating skipped as a pass reported every required check green with nothing verified at all. That is the failure this PR exists to prevent, reintroduced one level up. Wrappers now take detect-changes in needs and fail unless it succeeded, so a skip only passes when it genuinely means there was no work. The case statement was also repeated once per wrapper and would grow with each tool the repo adds. It moves into a local composite action, so a wrapper is now four lines and the logic exists once. The decision table is verified directly: success and skipped pass only when change detection succeeded; failure, cancellation, and any non-success gate all fail. [INT-726]
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 578f101d76a4
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| policies:conventions | 0 |
| automation:ci-release | 0 |
| structure:repo-health | 1 |
structure:repo-health (1 finding)
Minor - .github/workflows/ci.yml:38
The gate decision table (7 cases: success/skipped/failure/cancelled combinations of a -run job's result crossed with detect-changes' result) and the actionlint pass the PR cites as verification were both run manually and are not captured anywhere in CI. Nothing in this repo's workflows runs actionlint, and the decision table exists only as prose in the PR body, not as a script/test under version control. This is exactly the class of bug this PR fixes (a workflow-YAML gating mistake that silently breaks required-check enforcement) and it will now recur invisibly: a future edit to required-check/action.yml or to any of the six near-identical wrapper blocks can regress the gate logic (e.g. drop the gate-result check, mis-wire needs, or flip a case) with no CI signal, only manual review to catch it. Add an actionlint step to CI (or a pre-commit/
make linttarget already run in CI) so workflow syntax is checked automatically, and commit the decision-table cases as a small bats/shell test that exercises.github/actions/required-check/action.yml's script directly so a regression fails the build instead of requiring another manual re-verification.
Reviewer Coverage
policies:conventions— complete (broad); skipped: none; constraints: noneautomation:ci-release— complete (broad); skipped: none; constraints: nonestructure:repo-health— complete (broad); skipped: none; constraints: ci.yml is touched by every tools' paths filter, so this PR cannot exercise the skip path it introduces; the next tool-scoped PR is the real end-to-end proof, per the PR's own stated limit.
Inspected files (2)
.github/actions/required-check/action.yml.github/workflows/ci.yml
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 49s | ~$2.40 (est.) | claude-sonnet-5 | cr 0.10.286
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | policies:conventions, automation:ci-release, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 3m 49s wall · 6m 01s compute |
| Cost | ~$2.40 (est.) |
| Tokens | 64 in / 15.7k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 2.3k
- Cache read: 101.3k
- Cache create: 79.4k
- Cost: ~$0.36 (est.)
- Duration: 33s
policies:conventions— claude-sonnet-5- In: 24
- Out: 3.8k
- Cache read: 788.8k
- Cache create: 85.1k
- Cost: ~$0.61 (est.)
- Duration: 1m 11s
automation:ci-release— claude-sonnet-5- In: 14
- Out: 5.0k
- Cache read: 411.0k
- Cache create: 85.5k
- Cost: ~$0.52 (est.)
- Duration: 1m 13s
structure:repo-health— claude-sonnet-5- In: 16
- Out: 4.1k
- Cache read: 488.3k
- Cache create: 86.2k
- Cost: ~$0.53 (est.)
- Duration: 2m 48s
orchestrator-rollup— claude-sonnet-5- In: 4
- Out: 464
- Cache read: 31.8k
- Cache create: 95.5k
- Cost: ~$0.37 (est.)
- Duration: 13s
| @@ -38,7 +38,7 @@ jobs: | |||
| - 'Makefile' | |||
There was a problem hiding this comment.
File-level note: .github/workflows/ci.yml
The gate decision table (7 cases: success/skipped/failure/cancelled combinations of a -run job's result crossed with detect-changes' result) and the actionlint pass the PR cites as verification were both run manually and are not captured anywhere in CI. Nothing in this repo's workflows runs actionlint, and the decision table exists only as prose in the PR body, not as a script/test under version control. This is exactly the class of bug this PR fixes (a workflow-YAML gating mistake that silently breaks required-check enforcement) and it will now recur invisibly: a future edit to required-check/action.yml or to any of the six near-identical wrapper blocks can regress the gate logic (e.g. drop the gate-result check, mis-wire needs, or flip a case) with no CI signal, only manual review to catch it. Add an actionlint step to CI (or a pre-commit/make lint target already run in CI) so workflow syntax is checked automatically, and commit the decision-table cases as a small bats/shell test that exercises .github/actions/required-check/action.yml's script directly so a regression fails the build instead of requiring another manual re-verification.
Reply inline to this comment.
2 participants
[INT-726]
Replaces #475 with signed commits — see "Why this replaces #475" at the bottom. Same diff, already reviewed and approved there.
Branch protection requires
build-test-cfl,build-test-jtk,lint-cfl,lint-jtk,identity-check-cfl,identity-check-jtk. All six were gated at the job level on paths-filter output, so a PR touching one tool skipped the other tool's jobs — and GitHub does not accept a skipped check as a satisfied required check.Every single-tool PR is therefore unmergeable without an admin bypass. #464 and #474 both hit it; a jtk-only change hits it in the opposite direction.
The cost isn't the inconvenience. Branch protection works because merging over an unmet gate is deliberate and visible. When routine work can't merge without a bypass, the bypass stops being remarkable — and a genuine unmet gate looks like the twenty benign ones before it.
Change
Work stays in
<job>-run, which keeps its job-level condition and may skip freely, with no per-step guards. The protected check name lives on a wrapper that always runs and reports on the-runjob's behalf via a local composite action:detect-changesis inneedsdeliberately. Without it, adetect-changesfailure empties the outputs, every-runcondition goes false, all six skip, and the wrappers would pass every required check with nothing verified — trading "skipped check blocks a good PR" for "skipped check waves through a broken one".Branch protection settings are untouched; the required names are unchanged, just produced by the wrappers.
Verification
Decision table exercised directly by running the gate script over every combination:
actionlintclean. On #475 all six required checks reported SUCCESS, confirming the wrapper mechanism works end to end.Limit:
ci.ymlis in all three paths filters, so this PR marks every tool relevant and cannot exercise the skip path itself. The next tool-scoped PR is the real proof.Unrelated flake surfaced
#475 hit
TestClient_Do/DELETE_requestfailing inbuild-test-shared— "transport connection broken: CloseIdleConnections called". Not from this change (the diff touches only.github/), passes 5/5 locally under-race, andshared/clientlast changed in #348. It passed on re-run. Worth noting thatbuild-test-sharedonly runs whenshared/**,go.work,Makefileorci.ymlchanges, so this flake is normally invisible on tool-scoped PRs.Why this replaces #475
mainalso requires signed commits, and my commits on #475 were unsigned — a second, independent blocker that the admin bypass on #474 had masked. These are the same three commits, cherry-picked and signed (verified=true). #475 will be closed.