ci: INT-726 let scoped PRs satisfy their required checks by piekstra · Pull Request #475 · open-cli-collective/atlassian-cli

piekstra · 2026-08-14T11:07:27Z

[INT-726]

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 in this repo is therefore unmergeable without an admin bypass. Not an edge case: #464 (cfl-only) and #474 (cfl-only) both hit it, and 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 exactly like the twenty benign ones before it.

Change

The six required jobs run on every PR; the paths-filter condition moves to their steps via a job-level RELEVANT env. A job with nothing to do now reports success instead of skipped — an honest result, since the check did run and found no work — and the required checks are satisfied without touching repository settings.

build-test-shared and lint-shared keep their job-level if; neither is required, so skipping is fine for them. pr-title is unchanged.

Alternative considered

A rollup gate job with the required list reduced to it also works, but it changes branch protection out of band — settings drift that isn't visible in the repo. The workflow-only fix is self-contained and reviewable.

Validation — and its limit

.github/workflows/ci.yml appears in all three paths filters, so changing it marks every tool relevant. This PR therefore makes all jobs run and cannot demonstrate its own fix. Green here proves the workflow is valid and non-regressive, not that the skip path now reports success.

What I did verify:

YAML parses; the six required jobs have no job-level if and every step carries the gate; the two non-required shared jobs are untouched.
needs is available to job-level env, and env is available to step-level if, so the condition resolves in both positions.

The real proof is the next tool-scoped PR: its off-tool checks should read success with all steps skipped rather than skipped. Happy to confirm that with a throwaway cfl-only PR after this merges rather than waiting to find out.

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]

monit-reviewer

Automated PR Review

Reviewed commit: 8f6ea40eab21
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
automation:ci-release	0
policies:conventions	0
structure:repo-health	1
harness-engineering:repo-health	0

structure:repo-health (1 finding)

Major - `.github/workflows/ci.yml:40`

The new gate (env.RELEVANT, then if: env.RELEVANT == 'true' on every step) is copy-pasted six times — once per required job, ~5-6 duplicated if lines per job — with nothing enforcing that a future step added to any of these jobs also carries the guard. Since actions/checkout is itself gated, a step added without the if on an irrelevant PR would run against an unchecked-out workspace, likely failing the job outright and reintroducing exactly the unmergeable-PR problem this PR fixes (a required check now fails instead of skipping). The only safeguard today is a repeated code comment, which is prose, not an enforced invariant, and the diff's own validation section concedes this PR can't exercise the skip path (every filter matches ci.yml itself). Consider consolidating the per-tool duplication so the guard can't be forgotten — e.g. a reusable workflow/composite action parameterized by tool name (cfl/jtk) that applies if: env.RELEVANT == 'true' at the job or template level once, or a small CI lint step that greps ci.yml for the six required job blocks and fails if any step lacks the RELEVANT guard. Either turns "every step must carry the gate" from a comment convention into something CI itself checks.

Reviewer Coverage

automation:ci-release — complete (broad); skipped: none; constraints: ci.yml is in all three paths-filter blocks, so this PR always runs every job and cannot itself demonstrate the skip->success path; that gap is inherent to the diff, not fixable within it.
policies:conventions — complete (broad); skipped: none; constraints: No local convenience copies of cli-common/docs or open-cli-collective/.github were present in the workbench, so this review relies on the diff and repo-local context only.
structure:repo-health — complete (broad); skipped: none; constraints: PR modifies .github/workflows/ci.yml only; changes are workflow YAML, so live gating behavior (job-level env -> step-level if) could not be executed, only statically read.
harness-engineering:repo-health — complete (broad); skipped: none; constraints: The PR itself cannot exercise the skip-with-success path it fixes, since ci.yml is in all three paths-filters; this is disclosed in the PR body with a committed follow-up verification PR, so it is not raised as a finding here.

Inspected files (1)

.github/workflows/ci.yml

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 4m 02s | ~$2.65 (est.) | claude-sonnet-5 | cr 0.10.286

Field	Value
Model	claude-sonnet-5
Reviewers	automation:ci-release, policies:conventions, structure:repo-health, harness-engineering:repo-health
Engine	claude_cli · claude-sonnet-5
Reviewed by	cr · monit-reviewer
Duration	4m 02s wall · 6m 00s compute
Cost	~$2.65 (est.)
Tokens	56 in / 16.7k out

Per-workstream usage

orchestrator-selection — claude-sonnet-5
- In: 6
- Out: 3.1k
- Cache read: 100.8k
- Cache create: 79.8k
- Cost: ~$0.38 (est.)
- Duration: 39s
automation:ci-release — claude-sonnet-5
- In: 20
- Out: 5.2k
- Cache read: 653.8k
- Cache create: 90.7k
- Cost: ~$0.61 (est.)
- Duration: 2m 41s
policies:conventions — claude-sonnet-5
- In: 10
- Out: 1.6k
- Cache read: 272.0k
- Cache create: 94.7k
- Cost: ~$0.46 (est.)
- Duration: 44s
structure:repo-health — claude-sonnet-5
- In: 8
- Out: 2.9k
- Cache read: 182.3k
- Cache create: 80.4k
- Cost: ~$0.40 (est.)
- Duration: 53s
harness-engineering:repo-health — claude-sonnet-5
- In: 8
- Out: 3.4k
- Cache read: 185.7k
- Cache create: 83.7k
- Cost: ~$0.42 (est.)
- Duration: 49s
orchestrator-rollup — claude-sonnet-5
- In: 4
- Out: 469
- Cache read: 31.8k
- Cache create: 96.4k
- Cost: ~$0.38 (est.)
- Duration: 12s

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]

piekstra · 2026-08-14T11:13:26Z

Taken — and the hazard you named is worse than duplication, which is why I've replaced the approach rather than patching it.

Because actions/checkout was itself gated, a step added later without the guard wouldn't just run needlessly: on an irrelevant PR it would execute against an unchecked-out workspace and fail for a reason with nothing to do with the change. A guard you must remember, whose failure mode is a confusing error in unrelated CI, is the wrong shape.

Wrapper pattern instead. The work stays in <job>-run, which keeps its job-level condition and may skip freely — no per-step guards at all. The protected check name lives on a wrapper that always runs and reports the -run job's outcome, treating skipped as a pass because there was nothing to verify:

build-test-cfl:
  needs: build-test-cfl-run
  if: always()
  steps:
    - name: Report build-test-cfl-run outcome
      env:
        RESULT: ${{ needs.build-test-cfl-run.result }}
      run: |
        case "$RESULT" in
          success) ... ;;
          skipped) echo "no changes for this tool; nothing to verify" ;;
          *) exit 1 ;;
        esac

The guard exists once per job rather than once per step, and adding a step to a -run job now needs no thought at all. failure and cancelled both fail the wrapper, so nothing is weakened.

Branch protection is still untouched — the six required names are unchanged, just produced by the wrappers.

Verified: actionlint clean, YAML parses, no -run job carries a per-step if, and the two non-required shared jobs are as they were.

The validation limit from the description still stands: ci.yml is in every paths filter, so this PR marks all tools relevant and can't exercise the skip path itself. The next tool-scoped PR is the proof.

monit-reviewer

Automated PR Review

Reviewed commit: 848b752c94d6
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
automation:ci-release	0
policies:conventions	0
structure:repo-health	1
harness-engineering:repo-health	1

structure:repo-health (1 finding)

Minor - `.github/workflows/ci.yml:210`

This revision correctly resolves the prior concern by moving the gate out of per-step ifs and into six new wrapper jobs (build-test-cfl, build-test-jtk, lint-cfl, lint-jtk, identity-check-cfl, identity-check-jtk) that report the outcome of their -run counterpart. Good structural fix. The remaining risk is that the wrapper's success/skipped/fail case statement is now copy-pasted six times verbatim, and this duplication scales linearly with each future tool the repo onboards (a third tool adds a 7th near-identical -run + wrapper pair). Since it's uniform boilerplate today drift risk is low, but there's nothing stopping the six copies from diverging over time (e.g. one wrapper's case list getting a new branch the others don't). Consider factoring the reporting step into a small reusable/composite action (e.g. .github/actions/report-required-check) parameterized by the -run job's needs.<job>.result, so the pass/skip/fail policy for 'skipped counts as satisfied' is defined once and every current and future required-check wrapper calls it.

harness-engineering:repo-health (1 finding)

Major - `.github/workflows/ci.yml:230`

Each required wrapper job (build-test-cfl, build-test-jtk, lint-cfl, lint-jtk, identity-check-cfl, identity-check-jtk) treats needs.-run.result == 'skipped' as an unconditional pass. But a -run job also comes back skipped if detect-changes itself fails: its if: needs.detect-changes.outputs.cfl == 'true' || ... sees empty outputs on failure and evaluates false, so the job is skipped for a reason unrelated to 'no relevant changes'. In that scenario all six required checks report success with zero verification performed -- the exact failure mode this PR exists to prevent for benign skips, now reproduced for a genuinely broken pipeline. Fix: have each wrapper also gate on needs.detect-changes.result == 'success' (or add detect-changes to needs: and branch on its result) before accepting a skip as a pass.

Reviewer Coverage

automation:ci-release — complete (constrained); skipped: none; constraints: none
policies:conventions — complete (constrained); skipped: none; constraints: No local convenience copies of cli-common/docs or open-cli-collective/.github were present in the workbench, so this review relies on the diff and repo-local context only.
structure:repo-health — complete (constrained); skipped: none; constraints: none
harness-engineering:repo-health — complete (constrained); skipped: none; constraints: Could not fetch the external canonical harness-engineering guidance repo; reviewed against repo-local CLAUDE.md pointers and the PR's own stated invariant instead.

Inspected files (1)

.github/workflows/ci.yml

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 3m 11s | ~$2.74 (est.) | claude-sonnet-5 | cr 0.10.286

Field	Value
Model	claude-sonnet-5
Reviewers	automation:ci-release, policies:conventions, structure:repo-health, harness-engineering:repo-health
Engine	claude_cli · claude-sonnet-5
Reviewed by	cr · monit-reviewer
Duration	3m 11s wall · 5m 21s compute
Cost	~$2.74 (est.)
Tokens	44 in / 18.1k out

Per-workstream usage

automation:ci-release — claude-sonnet-5
- In: 12
- Out: 6.0k
- Cache read: 420.3k
- Cache create: 128.5k
- Cost: ~$0.70 (est.)
- Duration: 1m 26s
policies:conventions — claude-sonnet-5
- In: 8
- Out: 1.8k
- Cache read: 236.7k
- Cache create: 133.5k
- Cost: ~$0.60 (est.)
- Duration: 32s
structure:repo-health — claude-sonnet-5
- In: 8
- Out: 2.7k
- Cache read: 205.7k
- Cache create: 103.0k
- Cost: ~$0.49 (est.)
- Duration: 45s
harness-engineering:repo-health — claude-sonnet-5
- In: 12
- Out: 6.9k
- Cache read: 336.7k
- Cache create: 91.2k
- Cost: ~$0.55 (est.)
- Duration: 2m 14s
orchestrator-rollup — claude-sonnet-5
- In: 4
- Out: 761
- Cache read: 31.8k
- Cache create: 104.6k
- Cost: ~$0.41 (est.)
- Duration: 22s

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]

piekstra · 2026-08-14T11:19:10Z

Both taken — and the first was a hole this PR introduced, one level up from the one it set out to close.

detect-changes failing would have passed all six required checks with nothing verified. Exactly as you describe: empty outputs make every -run condition false, all six skip, and a wrapper treating skipped as a pass reports green across the board. I'd replaced "skipped check blocks a good PR" with "skipped check waves through a broken one", which is strictly worse.

Wrappers now take detect-changes in needs and fail unless it succeeded, so a skip only passes when it genuinely means no work.

Verified the decision table directly rather than reasoning about it — extracted the gate script and ran every combination:

GATE      RESULT     exit
success   success    0     ran and passed
success   skipped    0     no relevant changes; nothing to verify
success   failure    1
success   cancelled  1
failure   skipped    1     <- the hole, now closed
skipped   skipped    1
cancelled skipped    1

Duplication — fair, and it would have grown with every tool onboarded. The logic moved into a local composite action at .github/actions/required-check, so each wrapper is four lines and the case statement exists once. That also means the fix above was a one-place change rather than six.

actionlint clean on the workflow.

Validation limit still stands: ci.yml is in all three paths filters, so this PR marks every tool relevant and can't exercise the skip path. The next tool-scoped PR is the proof — happy to confirm with a throwaway cfl-only PR after merge rather than assume.

monit-reviewer

Automated PR Review

Reviewed commit: 3a3b6dc06c11
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
automation:ci-release	0
policies:conventions	0
structure:repo-health	0
harness-engineering:repo-health	0
structure:harness-engineering	0

Reviewer Coverage

automation:ci-release — complete (broad); skipped: none; constraints: none
policies:conventions — complete (broad); skipped: none; constraints: Sibling repos cli-common and .github (canonical shared-standards sources) are not checked out locally, so this review relies on the repo-local STANDARDS.md/docs/development.md convenience copies, which contain no CI/required-check-specific guidance to check against.
structure:repo-health — complete (broad); skipped: none; constraints: ci.yml appears in all three paths-filter rules, so this PR cannot itself exercise the skip path it changes; the fix relies on structural review of the composite action rather than an observed passing skip.
harness-engineering:repo-health — complete (broad); skipped: none; constraints: The PR's own validation section notes ci.yml matches all three paths-filters, so this review (and CI on this PR) cannot exercise the skip path the fix targets; correctness of the skip/pass mapping is judged by reading the composite action logic, not by an observed skipped-check run.
structure:harness-engineering — complete (broad); skipped: none; constraints: none

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 24s | ~$3.39 (est.) | claude-sonnet-5 | cr 0.10.286

Field	Value
Model	claude-sonnet-5
Reviewers	automation:ci-release, policies:conventions, structure:repo-health, harness-engineering:repo-health, structure:harness-engineering
Engine	claude_cli · claude-sonnet-5
Reviewed by	cr · monit-reviewer
Duration	3m 24s wall · 6m 42s compute
Cost	~$3.39 (est.)
Tokens	86 in / 22.9k out

Per-workstream usage

orchestrator-selection — claude-sonnet-5
- In: 6
- Out: 2.9k
- Cache read: 103.3k
- Cache create: 82.1k
- Cost: ~$0.38 (est.)
- Duration: 35s
automation:ci-release — claude-sonnet-5
- In: 14
- Out: 5.3k
- Cache read: 418.8k
- Cache create: 91.1k
- Cost: ~$0.55 (est.)
- Duration: 1m 29s
policies:conventions — claude-sonnet-5
- In: 22
- Out: 3.2k
- Cache read: 711.5k
- Cache create: 86.0k
- Cost: ~$0.58 (est.)
- Duration: 1m 04s
structure:repo-health — claude-sonnet-5
- In: 16
- Out: 4.1k
- Cache read: 481.8k
- Cache create: 87.1k
- Cost: ~$0.53 (est.)
- Duration: 1m 11s
harness-engineering:repo-health — claude-sonnet-5
- In: 10
- Out: 2.8k
- Cache read: 268.0k
- Cache create: 85.9k
- Cost: ~$0.44 (est.)
- Duration: 51s
structure:harness-engineering — claude-sonnet-5
- In: 12
- Out: 4.2k
- Cache read: 339.9k
- Cache create: 85.8k
- Cost: ~$0.49 (est.)
- Duration: 1m 14s
orchestrator-rollup — claude-sonnet-5
- In: 6
- Out: 448
- Cache read: 109.0k
- Cache create: 101.2k
- Cost: ~$0.42 (est.)
- Duration: 16s

piekstra · 2026-08-14T11:30:28Z

Superseded by #476 — same three commits, cherry-picked and signed.

main requires signed commits, and mine here were unsigned. That was a second blocker independent of the skipped-check problem this PR fixes, and the admin bypass on #474 had hidden it: I never saw it there because the bypass skipped every gate at once, not just the one I was reasoning about.

Review feedback from this PR is carried over and already applied in #476.

## [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 `-run` job's behalf via a local composite action: ```yaml build-test-cfl: needs: [detect-changes, build-test-cfl-run] if: always() steps: - uses: actions/checkout@v4 - uses: ./.github/actions/required-check with: job: build-test-cfl-run result: ${{ needs.build-test-cfl-run.result }} gate-result: ${{ needs.detect-changes.result }} ``` `detect-changes` is in `needs` deliberately. Without it, a `detect-changes` **failure** empties the outputs, every `-run` condition 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: ``` GATE RESULT exit success success 0 ran and passed success skipped 0 no relevant changes; nothing to verify success failure 1 success cancelled 1 failure skipped 1 <- detect-changes failure cannot wave checks through skipped skipped 1 cancelled skipped 1 ``` `actionlint` clean. On #475 all six required checks reported **SUCCESS**, confirming the wrapper mechanism works end to end. **Limit:** `ci.yml` is 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_request` failing in `build-test-shared` — *"transport connection broken: CloseIdleConnections called"*. Not from this change (the diff touches only `.github/`), passes 5/5 locally under `-race`, and `shared/client` last changed in #348. It passed on re-run. Worth noting that `build-test-shared` only runs when `shared/**`, `go.work`, `Makefile` or `ci.yml` changes, so this flake is normally invisible on tool-scoped PRs. ### Why this replaces #475 `main` also 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.

monit-reviewer reviewed Aug 14, 2026

View reviewed changes

Comment thread .github/workflows/ci.yml

monit-reviewer requested changes Aug 14, 2026

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

Comment thread .github/workflows/ci.yml

monit-reviewer approved these changes Aug 14, 2026

View reviewed changes

piekstra marked this pull request as ready for review August 14, 2026 11:23

piekstra mentioned this pull request Aug 14, 2026

ci: INT-726 let scoped PRs satisfy their required checks #476

Merged

piekstra closed this Aug 14, 2026

Navigation Menu

Uh oh!

ci: INT-726 let scoped PRs satisfy their required checks - #475

ci: INT-726 let scoped PRs satisfy their required checks#475
piekstra wants to merge 3 commits into
mainfrom
piekstra/INT-726-ci-scoped-pr-gates

piekstra commented Aug 14, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

piekstra commented Aug 14, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Uh oh!

piekstra commented Aug 14, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

piekstra commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

piekstra commented Aug 14, 2026

[INT-726]

Change

Alternative considered

Validation — and its limit

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Major - .github/workflows/ci.yml:40

Reviewer Coverage

Uh oh!

Uh oh!

piekstra commented Aug 14, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Minor - .github/workflows/ci.yml:210

Major - .github/workflows/ci.yml:230

Reviewer Coverage

Uh oh!

Uh oh!

Uh oh!

piekstra commented Aug 14, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Reviewer Coverage

Uh oh!

piekstra commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Major - `.github/workflows/ci.yml:40`

Minor - `.github/workflows/ci.yml:210`

Major - `.github/workflows/ci.yml:230`