fix(bin): resolve the CI required-suite roster per repository - #7
Merged
Merged
Conversation
FM_CI_REQUIRED_SUITES was a constant listing firstmate's own twelve CI job
names, so every other repository failed the completeness test by construction:
there is no roster to drift into on another repo, the roster simply does not
describe it. fm-pr-ci-verify.sh refused four consecutive real pull requests for
this reason, each independently hand-verified green, and fm-bearings-snapshot.sh
read every non-firstmate PR row as incomplete for the same reason.
fm_ci_roster now resolves the roster per repository and the classifiers take it
as an argument. jq refuses to compile a program whose variables are unbound, so
a caller that forgets the roster gets no verdict rather than a silent one, and
an empty roster classifies as incomplete rather than passing every green rollup.
Substitution flagged for review: the brief suggested reading job names from the
target repository's .github/workflows/ci.yml. That file is the definition but
not a roster, because a job name is a template GitHub evaluates ("Behavior
portable serial ${{ matrix.shard }}", or a matrix.include leg's
"${{ matrix.name }}"), so parsing it means reimplementing matrix expansion and
the Actions expression language. One of the three reported repositories needs
exactly that. The roster is therefore read by observation, from the job names of
the newest successful CI run on the branch the change targets, which GitHub has
already expanded exactly. No dependency is added: gh and jq only, both already
required. The cost is up to three GitHub API reads per repository per
verification. FM_CI_REQUIRED_SUITES survives as the documented override for a
change that deliberately adds or removes a CI job.
Verified: the derived roster for x45dev/firstmate is byte-identical to the
constant it replaces, and the three reported repositories plus the fourth field
case (home-fintech#76) now verify green, including workspace-template's six
expanded matrix.include legs.
Commit 0dd2fc2 added a landing-target guard to bin/fm-pr-check.sh without updating this pre-existing test, and main has been red since that work merged at 75267ef. The guard resolves whether this machine can merge into the target repository and refuses to arm unless the verdict is mergeable or unchecked, failing closed on unreachable by design. tests/fm-secondmate-safety.test.sh predates that guard. Its FM_HOME parameterization case calls fm-pr-check.sh with the fixture URL https://github.com/example/repo/pull/1 purely to prove that FM_HOME scopes data and state paths. No forge can resolve that repository, so the guard now exits 1 and the assertion fails before reaching the path checks the case exists for. The test needs a forge stub because its intent is FM_HOME path isolation and never landing authority, and the guard is behaving exactly as designed, so the test is what needed changing. The stub is deliberately the smallest thing that removes the dependency: a gh-axi on PATH answering the repository permission read with one bare boolean, the same contract the fake in tests/fm-pr-check-security.test.sh reproduces, via the PATH="$fakebin:$PATH" convention this file already uses for tmux. The guard still runs its own resolve path rather than being bypassed, no escape hatch is added to the library, and the case goes back to asserting path isolation. Verified: the fixture command exits 1 with the refusal "could not confirm example/repo is a landing path" without the stub and exits 0 with it, and the suite is 40/40 with bin/fm-lint.sh clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Generalize bin/fm-pr-ci-verify.sh so it can confirm a pull request is genuinely green on ANY repository, not only on firstmate itself.
The bug: FM_CI_REQUIRED_SUITES in bin/fm-ci-checks-lib.sh was a hardcoded constant listing firstmate's own twelve CI job names verbatim, including jobs such as 'Behavior tests (Herdr)' and 'Stock macOS Bash snapshot compatibility' that exist nowhere else. Any project whose CI defines its own job names therefore failed the required-suite completeness check by construction, however genuinely green it was. This is not roster drift: on another repository the roster simply does not describe it at all. This was confirmed hitting four consecutive real pull requests (workspace-template, go-bip39-validator, home-fintech twice), each independently hand-verified green and merged on that manual evidence because the shared tool could not confirm them. fm-bearings-snapshot.sh read every non-firstmate PR row as incomplete for the same reason.
The fix: fm_ci_roster now resolves the required roster per repository, and the classifiers take it as an argument rather than reading a constant. Firstmate's own repo must keep working exactly as it does today - this is a generalization, not the removal of a special case.
Deliberate decisions a reviewer reading only the diff would not know:
SECOND COMMIT, separately authorized and deliberately included in this same change: main is currently RED and it is not this change's doing. Commit 0dd2fc2 added a landing-target guard to bin/fm-pr-check.sh that resolves whether this machine can merge into the target repository and refuses to arm unless the verdict is mergeable or unchecked, failing closed on 'unreachable' by design. It did not update the pre-existing tests/fm-secondmate-safety.test.sh, whose FM_HOME parameterization case calls fm-pr-check.sh with the fixture URL https://github.com/example/repo/pull/1 purely to prove FM_HOME scopes data and state paths. No forge can resolve that repository, so the guard exits 1 and that test fails. Main was green at c8536c4 immediately before that merge and red at 75267ef immediately after.
This was fixed here, in its own commit, because a red main blocks every commit until it lands - that is the standing rule for a gate failure a change did not cause. The fix is TEST-ONLY and the boundary is explicit and must be held: do NOT weaken, bypass, or add a production escape hatch to the landing guard. Failing closed on a forge it cannot query is that guard working exactly as designed and is the entire reason it exists. The test's intent is FM_HOME path isolation and never landing authority, so the test stubs gh-axi on PATH to answer the repository permission read with one bare boolean - the same contract the existing fake in tests/fm-pr-check-security.test.sh reproduces, using the PATH=fakebin convention that test file already uses for tmux. The guard still runs its own resolve path rather than being bypassed. If any finding pushes toward relaxing the guard instead of the test, that must be escalated rather than applied.
Scope is deliberately limited to these two things: no refactors, no adjacent cleanups, no opportunistic improvements.
What Changed
fm_ci_roster(new, inbin/fm-ci-checks-lib.sh) resolves the required CI suite roster per repository by reading the job names of the newest successful CI run on the target branch, rather than reading them off the hardcodedFM_CI_REQUIRED_SUITESconstant that previously listed firstmate's own twelve job names verbatim.fm_ci_checks_state,fm_ci_run_jobs_state, and the underlying jq classifiers now take the roster as an explicit argument ($fm_ci_roster, bound via--argjson) instead of reading a shell constant; an unbound or empty roster fails closed toincompleterather than passing every green rollup.bin/fm-pr-ci-verify.shresolves the roster from the PR's base repository and base branch before classifying, refuses with a clear error when no roster can be established, prints the roster's provenance alongside the verdict, and documentsFM_CI_REQUIRED_SUITESas the override for a branch that deliberately adds or removes a CI job.bin/fm-bearings-snapshot.shresolves the roster once per repository (via its own boundedfm_ci_ghwrapper) before classifying that repository's PR rows, andCONTRIBUTING.mddocuments the override.tests/fm-secondmate-safety.test.sh's FM_HOME parameterization case now stubs thegh-axirepository-permission read onPATHso the pre-existing landing-target guard infm-pr-check.sh(added in a prior merge, unrelated to this change) doesn't fail the test on an unresolvable fixture repository; the guard itself is untouched.Risk Assessment
✅ Low: The roster-resolution logic (fm_ci_roster), its fail-closed paths (unbound jq vars, empty-roster refusal, unpaginated-jobs refusal), and both call sites (fm-pr-ci-verify.sh resolving against the PR's base repo/branch, fm-bearings-snapshot.sh resolving once per repo) trace correctly against constructed inputs with no reachable false-green path found; all new tests exercise the real functions/scripts and assert observable behavior rather than grepping source; the second commit is verified test-only with the landing guard's production code and fail-closed behavior untouched, matching the explicit boundary in the intent.
Testing
Ran the two directly relevant test suites (fm-ci-checks, fm-secondmate-safety) to completion with no failures, reproduced both original bugs (hardcoded roster refusing a real go-bip39-validator PR, and the FM_HOME test failing on the landing guard) against pre-fix code to confirm they are genuine regressions this change addresses, then demonstrated the fix working live end-to-end against real GitHub repositories and PRs (not just fixtures) — the derived roster for firstmate is byte-identical to the old constant, and a genuinely different repo's PR now verifies correctly. fm-bearings-snapshot.test.sh's full run repeatedly stalled partway through on unrelated e2e/perl-timeout-fallback tests unconnected to this diff; this reproduces identically on the base commit and passes cleanly in isolation or small subsets (including the new roster-integration test
test_include_prs_is_the_only_fetch_path), so it is pre-existing flakiness under this machine's current concurrent load rather than a regression from this change.Evidence: fm-pr-ci-verify.sh on a real firstmate PR (roster resolves, refuses on real failing suite)
Evidence: Before/after on a real non-firstmate PR (x45dev/go-bip39-validator #4)
Evidence: Reproduced the FM_HOME test regression pre-fix, then confirmed fixed
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-ci-checks.test.sh— 52/52 passing, including the newfm_ci_rosterunit tests and fm-pr-ci-verify.sh roster-integration testsbash tests/fm-secondmate-safety.test.sh— full run, all passing, includingtest_fm_home_parameterization(the specific test the second commit fixed)Reproduced the pre-fix regression: swapped in the base-committests/fm-secondmate-safety.test.shagainst the current (unmodified)bin/fm-pr-check.sh— confirmed it fails with 'fm-pr-check failed under FM_HOME' because the landing guard fails closed on the unresolvable fixture repo, exactly as described in the intent; restored the file afterwardLive end-to-end:. bin/fm-ci-checks-lib.sh; fm_ci_roster x45dev/firstmate mainagainst the real GitHub API — derived roster is byte-identical (sorted) to the old hardcoded FM_CI_REQUIRED_SUITES constantLive end-to-end:bin/fm-pr-ci-verify.sh https://github.com/x45dev/firstmate/pull/2— correctly resolves the 12-job roster and refuses on a real failing suiteLive end-to-end:fm_ci_roster x45dev/go-bip39-validator— derived roster is["check"], nothing like firstmate's roster, proving the bug's root causeLive end-to-end:bin/fm-pr-ci-verify.sh https://github.com/x45dev/go-bip39-validator/pull/4(a real merged PR) — target commit accepts it as passing (exit 0); reproduced with the base-commit tool on the same PR to confirm it wrongly refused (exit 1, 'checks do not cover the required suite roster') before the fixLive end-to-end:FM_CI_REQUIRED_SUITESoverride accepted with a valid array (exit 0) and refused closed with a malformed value (exit 1, no silent pass)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.