fix(bin): refuse a merge when a PR's CI event may have been dropped - #12
Merged
Merged
Conversation
GitHub's PR Checks summary reads identically for "no CI configured" and "the pull_request event never reached Actions for this push," so an empty statusCheckRollup was trusted as green. Gate GitHub merges on a positively confirmed pull_request-triggered run at the current head once the repo is known to have PR CI, filtering strictly on the pull_request event so a manual workflow_dispatch diagnostic run can never masquerade as arrived checks.
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
1 participant
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
Firstmate nearly merged PR 34 in rub-a-dub-dub/personal-agent-skills on the ground that it had "no CI checks configured" -- that PR was actually red, with a stale generated file, one merge away from landing; the string looked exactly like a green board. Make firstmate unable to treat an absent check as a passing one, per the following detection rule: (1) Repo-level, computed once rather than per-PR: does any workflow declare a pull_request (or pull_request_target) trigger? If none does, the repo genuinely has no PR CI, absence is expected, and nothing further applies. (2) Otherwise, count runs at the PR's current head SHA filtered to event == "pull_request": one or more runs -> read the conclusion as today, the ordinary case; zero runs and the head commit is younger than a five-minute grace window -> "not arrived yet", not actionable, re-check; zero runs and the head commit is older than that window -> suspected dropped event, never green, do not merge. The critical trap: a workflow_dispatch diagnostic run leaves a check-run on the SHA even though it is not a pull_request-triggered run and does not attach to the PR -- filter strictly on event type, never on whether any run exists at all, or a manual diagnostic dispatch will be misread as proof the checks arrived. This rule may only ever make the merge gate stricter: a repo with no PR CI must keep merging exactly as it does today, and a green PR must not become unmergeable because of a latency edge -- both of those must be tested explicitly.
What Changed
bin/fm-pr-merge.shgains a two-step dropped-CI-event gate that runs independently of the check rollup:github_repo_has_pr_ci_workflowasks once per merge attempt whether any workflow file declares apull_requesttrigger (via thegithub_workflow_declares_pull_requesttext heuristic, which scans only theon:block at its immediate child indentation, handles the"on":/'on':spellings and zero-indented sequences, and deliberately does not countpull_request_target), and only a confirmed trigger arms the second step.github_check_dropped_ci_eventthen counts Actions runs at the live head through the API's own?event=pull_requestfilter rather than by whether any run exists at the SHA, so aworkflow_dispatchdiagnostic run no longer reads as proof the checks arrived. A count of zero refuses unconditionally; the head commit's own committer date against the new fixedFM_PR_MERGE_CI_GRACE_SECS=300only selects between the "not arrived yet, re-check" and "suspected dropped event" wording, and an unreadable date lands on the latter. Reads that fail before an absence is confirmed — an unreadable workflow listing or run count — leave the gate disarmed and print a note on stderr.tests/fm-pr-merge.test.shadds 12 cases covering both directions: repos with no workflows directory, push-only,pull_request_target-only, commented-out, and nested-workflow_dispatch-input triggers keep merging unchanged, as do a green PR with a run present and an unreadable workflow listing; single-quotedon:and zero-indented sequence triggers arm the gate; and in-grace, past-grace, and unreadable-commit-date zero-run heads each refuse with their own message.docs/architecture.mddocuments the rule and thebranches:/paths:-filtered exemption it knowingly costs.Risk Assessment
✅ Low: The change is purely additive, matches the design the user explicitly authorized across five decision rounds, and I verified its core heuristic and every prior fix claim empirically rather than by inspection alone; the only known unwaivable-refusal classes are recorded, accepted user decisions tracked under a separate exemption ticket, and the one residual gap I found fails in the conservative direction.
Testing
Ran the change's 12 new cases in tests/fm-pr-merge.test.sh (all pass) and, because passing unit tests alone do not show the near-miss closing, drove bin/fm-pr-merge.sh directly against a mocked GitHub in the exact PR-34 state and captured the CLI transcripts: the base-commit script reports "every required check green" and issues gh pr merge, while this branch refuses with exit 1, issues no merge, and still refuses under --allow-red - with the gh call log showing the run count is read through ?event=pull_request so the workflow_dispatch diagnostic run at the same SHA never counts. Five stricter-only scenarios were run against both script versions and behave identically before and after (no PR CI, a green PR with its pull_request run present, a pull_request_target-only repo, an unreadable workflow listing), with only the 3-minute-old head newly refused and worded as re-check-shortly rather than a dropped event. Two setup problems were handled: a missing tasks-axi (installed into a temp prefix on PATH, then removed) and a host-load flake in shared pre-merge code that also hits the base script, which blocked a complete run of the 130-case file locally and is reported as a finding.
Evidence: PR-34 near miss: base commit merges it, this branch refuses it (CLI transcript)
Source: PR-34 near miss: base commit merges it, this branch refuses it (CLI transcript)
BEFORE - base commit 126eaec bin/fm-pr-merge.sh $ bin/fm-pr-merge.sh task-x1 https://github.com/example/repo/pull/34 verified: https://github.com/example/repo/pull/34 is open and mergeable, with every required check green at head 5a5a...5a5a verified: https://github.com/example/repo/pull/34 is merged (state=MERGED, merged=true, isInMergeQueue=false) exit status: 0 --- VERDICT: MERGED an unverified head - the near miss AFTER - this branch, same mocked GitHub state $ bin/fm-pr-merge.sh task-x1 https://github.com/example/repo/pull/34 error: refusing to merge https://github.com/example/repo/pull/34 - no pull_request-triggered check has reported for head 5a5a...5a5a, and its commit is already past the delivery grace window: wait and retry this merge first, because a run still on its way looks identical here once the commit has aged out of the window, and treat it as a suspected dropped CI event only if a retry still finds none. Neither is ever treated as green exit status: 1 --- gh calls this run made: api repos/example/repo/contents/.github/workflows --jq .[] | select(.type == "file") | .name api repos/example/repo/actions/runs?head_sha=5a5a...5a5a&event=pull_request --jq .total_count api repos/example/repo/commits/5a5a...5a5a --jq .commit.committer.date --- VERDICT: REFUSED - no gh pr merge was ever issued AFTER - retried with --allow-red ci --- VERDICT: REFUSED - --allow-red cannot waive an absent checkEvidence: Stricter-only scenarios, each run against both the base and the branch script
Source: Stricter-only scenarios, each run against both the base and the branch script
A repository with NO pull_request CI keeps merging (empty rollup, hour-old head) [BASE] exit status: 0 | gh pr merge issued: YES [AFTER] exit status: 0 | gh pr merge issued: YES A green pull request whose pull_request-event run exists merges normally [BASE] exit status: 0 | gh pr merge issued: YES [AFTER] exit status: 0 | gh pr merge issued: YES A head committed 3 minutes ago: not arrived yet, re-check - never called a dropped event [BASE] exit status: 0 | gh pr merge issued: YES [AFTER] error: refusing to merge .../pull/203 - no pull_request-triggered check has reported for head 4a4a...4a4a yet, and its commit is younger than the delivery grace window; re-check shortly exit status: 1 | gh pr merge issued: no A pull_request_target-only repository keeps merging [BASE] exit status: 0 | gh pr merge issued: YES [AFTER] exit status: 0 | gh pr merge issued: YES An unreadable workflow listing disarms the gate loudly instead of refusing [AFTER] note: could not read this repository's workflow triggers, so the dropped-CI-event check is disarmed for this merge attempt exit status: 0 | gh pr merge issued: YESEvidence: The 12 new dropped-CI test cases, run case by case
Source: The 12 new dropped-CI test cases, run case by case
ok - fm-pr-merge merges normally when the repository has no .github/workflows directory ok - fm-pr-merge ignores a workflow with no pull_request trigger ok - fm-pr-merge never arms the dropped-event gate on a pull_request_target-only repository ok - fm-pr-merge never reads comment text as a pull_request trigger declaration ok - fm-pr-merge reads only on:'s own children as trigger declarations ok - fm-pr-merge recognises the 'on': trigger-key spelling ok - fm-pr-merge recognises a zero-indented sequence of triggers under on: ok - fm-pr-merge merges normally once a pull_request-event run exists at the head ok - fm-pr-merge treats a fresh zero-run head as not-yet-arrived, not a drop ok - fm-pr-merge never treats a suspected dropped CI event as green, even with --allow-red ok - fm-pr-merge refuses a zero-run head even when the head commit date cannot be read ok - fm-pr-merge does not refuse a merge merely because the workflow listing could not be read 12 cases run, 0 failed.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-pr-merge.sh:648- A zero-indented YAML block sequence underon:silently fails to arm the gate, so the exact PR-34 failure this change exists to close stays reachable for those repositories. Concrete input, verified by running the awk from bin/fm-pr-merge.sh:635-655 directly: a repo whose only workflow isname: CI/on:/- push/- pull_request/jobs: {}. GitHub Actions parses this with a normal YAML parser and runs the workflow on every pull request, butgithub_workflow_declares_pull_requestmatches- pushagainst the block terminator at line 648 (line ~ /^[^[:space:]]/ { in_on = 0; next }), because a block sequence is legally allowed at its parent key's own indentation.in_onis cleared before- pull_requestis ever examined, the function exits 1,github_repo_has_pr_ci_workflowfalls through toFM_PR_GITHUB_PR_CI=noat line 715, and thecaseat line 843 never reaches theyesarm. That repository's pull request whosepull_requestdelivery was dropped then merges on its empty-but-green-looking rollup, with no stderr note, since "no" is the proven-absence verdict and prints nothing. This contradicts the function's own header at line 627, which states that a- pull_requestsequence entry counts as a trigger declaration - only the indented sequence form actually does (I confirmedon:/- push/- pull_requestarms correctly). Remedy is a mechanical repair of the heuristic the change already implements: stop treating a line that begins a sequence entry as the block terminator (e.g.line ~ /^[^[:space:]-]/ { in_on = 0; next }), which leaveschildat indent 0 and lets the existing^[[:space:]]*(-[[:space:]]*)?pull_request[[:space:]]*(:|$)match fire. The same class covers a quoted"pull_request":key, which also never matches; widening the key match is optional, but the sequence form is the common real-world spelling. auto-fix because this corrects the documented behavior of a component the intent requires, not the author's choice of rule.bin/fm-pr-merge.sh:844- Noting an accepted tradeoff, no action needed. Both pre-absence reads fail open: anunreadableworkflow listing (a 403 secondary rate limit, a transient 5xx) at line 844 and anunreadablerun count at line 850 each print a stderr note and let the merge proceed. So during a GitHub API disruption firstmate can still merge a pull request whosepull_requestevent was dropped - the intent's goal ("unable to treat an absent check as a passing one") holds only when the two reads succeed. The change makes this choice deliberately and documents it at lines 668-675 and 738-744, tests it (test_unreadable_workflow_listing_does_not_block_a_green_merge), and the alternative - refusing on an inconclusive read - would be a new refusal healthy pull requests cannot clear, which the intent's "a green PR must not become unmergeable" constraint forbids. Recording it so the residual window is visible, not asking for a change.🔧 Fix: arm dropped-CI gate on zero-indented on: sequence triggers
1 info still open:
bin/fm-pr-merge.sh:653- Residual coverage gap, noted as a tradeoff rather than a request to change. The child-key match^[[:space:]]*(-[[:space:]]*)?pull_request[[:space:]]*(:|$)accepts only the bare event-name spelling, so a workflow writtenon:/"pull_request":(or'pull_request':) reads as no-PR-CI,FM_PR_GITHUB_PR_CI=no, and the gate never arms for that repository — silently, since "no" is the proven-absence verdict and prints no stderr note. I confirmed this by running the extracted awk: the quoted-key input returns 1 while every unquoted spelling returns 0. This is an asymmetry with theon:key itself, where all three ofon,"on"and'on'are accepted at line 640. Two reasons this does not need action here: the failure is in the fail-open direction (it can never turn a healthy PR unmergeable, only leave the PR-34 class reachable for that repo), and quoting an event name has no YAML 1.1 motivation the way quotingondoes, so it is a spelling essentially only tool-generated workflows produce. Round 1 raised this same gap and explicitly called widening it optional; the fix round implemented the mandatory sequence-entry half, which I verified works. Recording it so the boundary of the heuristic stays visible.tests/fm-pr-merge.test.sh- tests/fm-pr-merge.test.sh could not be run to completion on this host: six consecutive attempts each aborted at a different pre-existing case (github-open-unqueued, github-outcome-read-fails, github-refusal-quotes-forge, github-unrecognised-queue-method, and once a half-sourced harness at tests/lib.sh:109). The failures are host-induced, not from this change: under the current load (load average 17-25, ~1250 processes from concurrent agents) subprocesses intermittently fail, which silently breaks the perl Cwd::realpath helper fm_backlog_canonical_existing in bin/fm-backlog-transition-lib.sh:664 and makes the shared pre-merge captain-hold / task-record directory resolution refuse. The same failures reproduce against the base-commit (126eaec) script, and a pre-existing untouched case failed 0/10 while the same runs flaked whenever load spiked. Every case passes in isolation. Remote CI on an unloaded runner still owns the full-file regression.bash tests/fm-pr-merge.test.sh(full file; aborted on host-load flakes at pre-existing cases, see finding)The 12 new cases run individually against the real script:test_no_workflows_directory_merges_unaffected,test_push_only_workflow_does_not_arm_the_dropped_event_gate,test_pull_request_target_only_workflow_does_not_arm_the_dropped_event_gate,test_commented_out_trigger_does_not_arm_the_dropped_event_gate,test_nested_input_named_pull_request_does_not_arm_the_dropped_event_gate,test_single_quoted_on_key_arms_the_dropped_event_gate,test_zero_indented_sequence_trigger_arms_the_dropped_event_gate,test_pr_ci_configured_with_a_run_present_merges_normally,test_dropped_ci_event_within_grace_window_is_not_actionable,test_dropped_ci_event_past_grace_window_refuses_as_suspected_drop,test_unreadable_head_commit_date_still_refuses_a_zero_run_head,test_unreadable_workflow_listing_does_not_block_a_green_merge- 12/12 passManual before/after regression:bin/fm-pr-merge.sh task-x1 https://github.com/example/repo/pull/34run against bothgit show 126eaec:bin/fm-pr-merge.shand the branch script on identical mocked GitHub state (empty statusCheckRollup,on: pull_request:workflow, unfiltered run count 1 from a workflow_dispatch diagnostic,?event=pull_requestcount 0, head commit 3600s old)Manual retry of the same refused head with--allow-red cito confirm the refusal is unwaivableManual stricter-only scenarios run against both script versions: no-PR-CI repo (workflows 404), green PR with a pull_request-event run present, head committed 180s ago, pull_request_target-only repo, and an unreadable workflow listingFlake triage:test_verified_merge_records_pr_and_headandtest_github_open_unqueued_outcome_refuses(untouched pre-existing cases) run 10-12x interleaved with the new cases to compare failure rates under host loadSetup fix: installedtasks-axi0.2.5 into a temp npm prefix and prepended it to PATH, since bin/fm-captain-hold.sh refuses without a compatible build; removed afterwardsdocs/architecture.md:318- Out-of-scope consolidation worth a follow-up, not a defect in this change. The dropped-CI gate's rationale now exists as two near-parallel full copies: the ~40-line header block at bin/fm-pr-merge.sh:16-56 and the 13-line narrative at docs/architecture.md:318-330. Both are currently accurate and mutually consistent — I verified each claim against the final code — but they restate the same five facts point-for-point (rollup is deliberately not read, pull_request_target does not arm because its run carries the base SHA, the ?event= filter defeats the workflow_dispatch trap, the grace window is measured from the head commit's own date and only picks wording, and the branches/paths deadlock is an accepted cost), so any future change to the gate must be landed in both places or one silently drifts. This duplication follows the existing convention in this section — docs/architecture.md:317 similarly restates github_checks_not_green's rule alongside the script — and docs/scripts.md declares the script header authoritative for behavior and contracts, so a fix means deciding the boundary between the two surfaces for this whole section rather than editing one paragraph. Proposing a follow-up that reduces docs/architecture.md to the mechanism boundary plus a pointer to the header, applied consistently across the PR-merge section. Deliberately not done here: it would be a documentation-architecture migration well beyond what this change made stale.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.