fix: require every named check to pass, not a matching count

[wroszkowski]

The defect

Promotion was authorised by counting successful check results across all
required names and comparing that total to the number of names
(github/manager.go, hydrateCommits + checkRunSet):

for _, cn := range checkNames {
    for _, ctx := range edge.Node.Status.Contexts { /* +1 per SUCCESS */ }
    checkRunSet(&checksPassed, cn, edge)              // +1 per successful run
}
if checksPassed == numChecks { statusSuccess = true }

checksPassed is one flat counter. It never verified that each required name
passed, so:

• a surplus on one name silently covered a deficit on another, and
• the exact-equality test rejected commits where everything passed but one
  name produced two successful runs.

Duplicate results per name are not exotic here — they are routine. A merge queue
builds a commit twice (queue run + push run), and a reusable workflow called by
several callers publishes its check once per caller: on monolith-app's current
develop HEAD, load_context / Load context appears 4×.

Evidence from DocPlanner/dbt-app

Required checks at the time: run_app_tests,run_dbt_tests,build_push / build_push-dbt.

While a merge queue was active, a develop commit produced run_app_tests
twice:

name	runs	contributes
build_push / build_push-dbt	1 × success	1
run_app_tests	2 × success	2
run_dbt_tests	1 × skipped	0

Total 3, numChecks 3 → promoted to master while run_dbt_tests was never
green. Verified on commit 7d7d1c8b.

The merge queue then stopped being used on 2026-08-03. run_app_tests dropped to
one run, so the best any commit could score became 1+1+0 = 2 ≠ 3, and
promotion became structurally impossible: master sat unchanged from 2026-08-03
to 2026-08-07 while the job kept exiting 0 and reporting
THERE IS NO COMMITS PASSING EVALUATION. Verified counts on 0e6dae35,
268fa5a3, 11f61a6a, bd917063, b28faf42.

One defect, both directions, silent in both.

The fix

Per-name coverage instead of a sum. For each required name:

1. at least one result must carry that name (else: never ran),
2. none may still be queued / in progress (else promotion pre-empts the verdict),
3. every concluded result must have concluded SUCCESS.

Several results for one name are fine as long as they are all green.

The acceptance predicate is deliberately unchanged — SUCCESS only, exactly
what the old counter accepted. This is a counting fix, not a widening of what
counts as green, so no repository starts promoting on a verdict it did not
already accept.

Old vs. new on the same fixtures (generated from the committed test fixtures)

scenario                                         | old   | new   | new --accept-skipped-checks
dbt-app 7d7d1c8b: 1 ok + 2 ok + 1 SKIPPED        | true  | false | true
masked FAILURE: 1 ok + 2 ok + 1 FAILURE          | true  | false | false
masked MISSING: 1 ok + 2 ok + name absent        | true  | false | false
all green, one ran twice (4 successes / 3 names) | false | true  | true
all green, one per name                          | true  | true  | true
post-queue freeze: 1 ok + 1 ok + 1 SKIPPED       | false | false | true
monolith hotfix gate: 1x NEUTRAL (not a hotfix)  | false | false | false
monolith hotfix gate: 1x SUCCESS (authorised)    | true  | true  | true
single name reported twice, both SUCCESS         | false | true  | true
single name: 1 SUCCESS + 1 FAILURE               | true  | false | false
in progress                                      | false | false | false

Note the second-to-last row: with a single required name, one success plus one
failure counted 1 == 1 and promoted. That is the masking bug sitting on
the most dangerous gate we have — see the next section.

❓ Decision for maintainers: what should SKIPPED and NEUTRAL mean?

This is a policy choice, not a technical inevitability, and I have deliberately
not encoded my own repo's preference as the default. Please rule on it.

I set out to make SKIPPED and NEUTRAL satisfy a required name — that is
GitHub's own required-status-check semantics, and dbt-app's run_dbt_tests is
legitimately skipped on develop pushes (change detection finds no dbt files,
because the merge-base diff of develop against itself is empty). While
enumerating the blast radius I found that default would introduce a worse bug
than the one it fixes, so I inverted it.

devops-pipelines/.github/workflows/hotfix_aware_skip_check.yml publishes its
hotfix-skip-tests check as success = "this commit is authorised to promote
without tests" and neutral = "not a hotfix":

} else {
  conclusion = 'neutral';
  title = `Not a ${labelName}`;

monolith-app/.github/workflows/hotfix.yaml then gates the irreversible promote
on exactly that one name:

  promote:
    uses: docplanner/devops-pipelines/.github/workflows/promote_commit_to_master.yml@v1
    with:
      expression: ${{ github.event_name == 'workflow_dispatch' && 'true' || 'StatusSuccess == true' }}
      status_check_names: 'hotfix-skip-tests'

Accepting NEUTRAL would flip that gate from fail-closed to fail-open: every
ordinary commit would satisfy StatusSuccess == true on the hotfix path and
promote untested. So, as shipped here:

• NEUTRAL never satisfies, and no flag can make it — a flag would just
  relocate the footgun. Same as today's behaviour.
• SKIPPED does not satisfy by default, also same as today, with
  --accept-skipped-checks as an opt-in for repos that skip a required job on
  purpose.

dbt-app does not need that flag, because it already solved the skip
upstream: as of 7431e918 its required_checks are
dbt-app CI merge readiness,build_push / build_push-dbt, where
dbt-app CI merge readiness is an if: always() aggregator that accepts
success|skipped per tracked job and emits a single literal green/red check.
noa-whisper-app does the same with all_builds_passed. That is the pattern
worth recommending to other teams, and it keeps working under per-name coverage.

Open questions for you:

1. Is --accept-skipped-checks worth keeping at all, or should teams always be
   pushed to the aggregator pattern? I kept it because it is nearly free, but I
   have no consumer for it — including my own repo.
2. Should NEUTRAL stay permanently non-satisfying, or become configurable once
   the hotfix-skip-tests contract lives somewhere more durable than a workflow
   comment?

Blast radius

One invocation point, ~143 consumers, no staged rollout by default.

• Every caller funnels through
  DocPlanner/devops-pipelines/.github/workflows/promote_commit_to_master.yml
  (job promote, docker run ghcr.io/docplanner/github-flow-manager:${{ inputs.ghfm_version }}).
  Nothing in the org invokes the binary directly.
• 143 distinct repositories call that reusable workflow, across 172 caller
  workflow files. software-templates skeletons carry the same pattern, so
  future repos inherit it.
• 122 repositories pass status_check_names on at least one path → affected by
  this change. Most resolve the names from their own .github/cicd.yml
  (context.release.required_checks / hotfix_required_checks);
  noa-notes-app / noa-notes-front-app use context.promote.required_checks,
  and noa-second-opinion-app uses a 4-entry matrix.
• 21 repositories never pass names on any path → specificChecksNames == ""
  → the StatusCheckRollup fallback → completely unaffected. That path is
  untouched and now has a regression test (TestHydrateCommitsFallsBackToRollup).
  They are .github-private, ai-plugin-manager, backuper-app, crm-app,
  deployments-app, deployments-front-app, gmb-app, integrations-app,
  invoicing-app, legal-operations-front-app, manually-generated-sitemaps,
  marketplace-account-app, messenger-front-app,
  noa-notes-integration-docs, package-app-php-client, patient-request-app,
  payments-app, pdf-viewer-front-app, repman, restorer-app,
  watson-mobile.
• Of the declared name lists, 47 are single-name and 56 are multi-name.
• ghfm_version defaults to latest, and .goreleaser.yml moves
  ghcr.io/docplanner/github-flow-manager:latest on every v* tag. Merging
  this PR ships nothing; tagging ships it to all 122 affected repos at once.

Who changes behaviour, and how

Newly blocked — failures that were previously masked. Any repo where the
successes happened to total N while some name was red, missing, or still pending.
Worst exposure is the 47 single-name repos, where one success plus one
failure counted 1 == 1 and promoted: monolith-app's hotfix path,
authorization-app, one-app, one-front-app, booking-front-app,
platform-app, geocoder-app, crm-front-app, kraken-mcp-app,
package-gateway, rabbit-sidecar-app, permissions-portal-front-app,
revenue-features-front-app, widgets-front-app, noa-notes-hub-front-app,
noa-whisper-app, docker-jupyter-hub, terapia-app, websockets-app, and
every other build_push / build_push-*-only repo. This is the point of the
PR, but it will surface previously invisible red checks as new promotion
blocks. Expect support questions.

Newly promoting — commits previously blocked by an inflated count. Same
single-name set: the moment a name resolved twice (routine for
reusable-workflow checks), 2 != 1 froze promotion silently. Those repos start
promoting again.

Named, deliberate side effects:

• noa-second-opinion-app declares
  run_tests (app), build_push (app) / build_push-noa-second-opinion — note the
  space after the comma. Today strings.Split yields a name with a leading
  space that can never match, so numChecks=2 can only ever reach 1 and that
  repo's automated promotion is currently dead. This PR adds TrimSpace, so
  it will start promoting for the first time (correctly, on its real checks).
  Worth telling that team. Happy to drop the trimming if you would rather fix it
  in their config instead.
• vendor-bills-es-app has the literal required_checks: '[]'; chat-app and
  opibot-app set hotfix_required_checks: 'build_push' where the real check run
  is build_push / build_push-<app>. These are blocked today and stay blocked —
  but the --verbose output now says
  NEVER RAN - no commit status, workflow run or check run carries this name
  instead of a bare "no commits passing evaluation", so they become fixable
  config rather than a mystery.
• phone-front-app runs the inverted expression: 'StatusSuccess == false', so
  per-name coverage changes when it promotes rather than whether. Flagged
  because the inversion makes the blast direction non-obvious.

Known limitation, deliberately not fixed here: GraphQL page caps

github/types.go still hardcodes checkSuites(first: 20) and
checkRuns(first: 25). monolith-app's develop HEAD already carries 23
check suites (atlantis, sentry, sonar, semgrep, claude, wip, dp-testings…), so
a required check can fall outside the window depending on ordering.

Truncation already blocks promotion today — a missing run also reduced the count —
so this PR does not make it worse. It does remove the accidental compensation
where a duplicate elsewhere covered a truncated one, and it makes truncation
diagnosable through the per-name summary. I did not bundle a fix on purpose:
#33 raised both caps to 100 and was fully reverted in #35 because it raised
GraphQL query cost ~20× (worst case ~2,526 points/call against the 5,000/hour
bucket). That needs real pagination, in its own PR. post-office-app (10 names)
and secretary-ai-app (7 names) are the other exposed consumers.

Also in this PR

• CI ran no tests at all — pull_requests_tests.yaml only ran pre-commit.
  Added a Unit Tests job (go build / go vet / go test). It is not in
  branch protection; someone with admin should add it as required.

• --verbose now explains why a commit was held back, one line per required
  name. A commit blocked by its checks previously looked identical to one blocked
  by the expression — which is exactly why a four-day release freeze went
  unnoticed while the job reported success:

  Required checks not satisfied for 7d7d1c8b...:
      OK build_push / build_push-dbt: 1 result(s) [SUCCESS]
      OK run_app_tests: 2 result(s) [SUCCESS SUCCESS]
      run_dbt_tests: 1 result(s) [SKIPPED] - 1 did not pass
  

• Names are trimmed and empty entries dropped, so a stray separator (a,b,) no
  longer contributes a name that can never be satisfied. A list with nothing
  usable (e.g. ",") now fails closed rather than promoting vacuously on an
  empty requirement set.

• Evaluation logic moved to github/checks.go; checkRunSet is gone.

API surface

The CLI is unchanged apart from the new opt-in --accept-skipped-checks; the ,
separator default in cmd/root.go is untouched. Two exported Go signatures gain
one bool parameter (github.GetCommits, manager.Manage) and github.Commit
gains a ChecksSummary []string field. All in-repo callers are updated; nothing
in the org imports the module — it is consumed as a container image.

Verification

$ go build ./...   # ok
$ go vet ./...     # ok
$ go test ./...
ok  	github.com/Docplanner/github-flow-manager/github	0.551s
$ gofmt -l .       # clean
$ pre-commit run --all-files
Run go fmt against the code..............................................Passed
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Check for merge conflicts................................................Passed
Mixed line ending........................................................Passed
Add TOC for md files.....................................................Passed
Run golangci against the code............................................Passed

40 test cases covering: the historical false-pass (duplicate success masking a
skip, a failure, and a missing name); the historical false-block (extra
successful run, and a single name reported twice); genuine failures across every
blocking conclusion; pending runs signalled both via status and via a null
conclusion; a missing name; all-green through check runs, commit statuses and
workflow-run names; the monolith-app NEUTRAL hotfix gate in both directions;
name splitting; and the untouched StatusCheckRollup fallback.

golangci-lint locally was v2.12.2 (0 issues). The CI-pinned v1.44.2 will not
run under Go 1.26 (panic: load embedded ruleguard rules, unrelated to this
change), so that version is verified only by this PR's own CI run.

I also dry-ran the rebuilt binary against live DocPlanner/dbt-app
develop → master with --dry-run; both branches are currently at 97cee590,
so it correctly short-circuited with
IS ALREADY IN master branch. EXITING THE PROCESS WITHOUT ANY ACTION.

Suggested rollout

Merging is inert; tagging is the deploy. Because ghfm_version defaults to
latest, I would not let a tag move latest immediately:

1. Merge this PR.
2. Tag a version and have monolith-app and dbt-app pin ghfm_version to it
   explicitly for a few days — monolith-app because it owns the
   hotfix-skip-tests gate, dbt-app because it has the reproducer.
3. Then let latest move, and expect a short tail of "my repo stopped promoting"
   reports that are genuinely red checks the old counter was hiding.

🤖 Generated with Claude Code