fix(bin): stop false-green PR checks and run CI on our fork - #2855
Open
x45dev wants to merge 12 commits into
Open
fix(bin): stop false-green PR checks and run CI on our fork#2855x45dev wants to merge 12 commits into
x45dev wants to merge 12 commits into
Conversation
Broadening the push trigger to every branch would have started a source-tree suite against the orphan branches the gate publishes test evidence to, which carry no source tree and so could only ever fail. branches-ignore keeps a new branch validated by default and excludes only that namespace.
…ver ran Three pull requests were reported green whose repository suites had never started. Each carried exactly one check, a third-party review bot reporting success, which reads as "1 passed, 0 failed, 1 total" to anything that only tallies conclusions. bin/fm-bearings-snapshot.sh classified that as passing for the same reason: a rollup with nothing red and nothing waiting. bin/fm-ci-checks-lib.sh now owns the rule, structurally rather than by bot name: a GitHub Actions check run records the workflow that produced it, and a check run created by a third-party App has no workflow behind it and so no name. A rollup with no repository-owned check classifies as no-repo-ci, decided ahead of failing and pending so missing suites are never reported as a different problem. bin/fm-pr-ci-verify.sh asks the question directly for one pull request, and resolves the case those three were actually in: it reads the head repository's own workflow runs at the same commit, so a fork run that validated the change counts while GitHub still holds the upstream run. The verdict names which repository the evidence came from, so fork-validated is never reported as the upstream pull request having been checked.
The workflow told a contributor to fork and then aimed everything at the pull request here, leaving no statement that the fork is where the suites actually run. That is what let a held upstream run read as a blocker on knowing whether the change works, when it is only a wait on a maintainer decision. CONTRIBUTING.md now states the split and says why an all-green check list is not evidence anything ran. The generated ship brief and the firstmate contract both require confirming a green claim with bin/fm-pr-ci-verify.sh rather than reading a check total.
…ere sorted in Pre-existing and unrelated to the rest of this branch, reported here rather than left silent. The guard builds every list with LC_ALL=C sort but ran comm under the ambient locale, so on a UTF-8 host comm rejected the C-sorted input as unsorted and the guard failed with a message about sort order rather than about the partition it exists to prove. CI runs under a C locale and never saw it. Verified before and after on the same machine: bin/fm-test-run.sh --check-coverage exited 1 with "comm: file 2 is not in sorted order" under LANG=en_US.UTF-8 and 0 under LC_ALL=C; it now exits 0 under both.
…w produced The bearings fixture rollup carried a bare passing entry with no producer on it, which is what a review bot's check looks like rather than a suite's. Now that the row is classified by whether the repository's own workflows ran, that fixture reads no-repo-ci, which is correct for what it describes and wrong for what the test means. Give it the workflow name a real CI check run carries. Add the bot-only rollup as its own fixture and assert the row reads no-repo-ci and never passing, so the case the classifier exists for is pinned here too.
Self-update reads whatever remote the home calls origin, so a fleet that runs its own suites can update from the fork those runs validated rather than from a repository whose answer it is still waiting on. Nothing in the code has to change for that; the sentence exists because the option is invisible from the outside, and reading origin as a fixed repository is what makes a fork look like a place work goes and never a place work comes from.
The stock-Bash lane pins an exact count so a test cannot quietly stop running. The bot-only rollup case is a new test, so the floor moves with it.
Confidence Score: 3/5The PR is not yet safe to merge because a contributor-controlled fork can still obtain a validated verdict without running the repository’s required suites. The pull-request rollup path now enforces the required roster, but the fork fallback trusts the aggregate success of any head-repository workflow named CI. Because the PR author controls that workflow definition and the verifier does not inspect its jobs, a trivial or reduced workflow can still produce the false-green result raised in the prior review. Files Needing Attention: bin/fm-ci-checks-lib.sh, bin/fm-pr-ci-verify.sh Reviews (3): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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
Make our own fork run our own validation, so upstream maintainer approval stops gating firstmate work.
The problem, in the captain's own framing: he has no relationship with the upstream maintainer (kunchenguid/firstmate, thousands of open pull requests) and no ability to get work integrated. He does NOT want anyone asking upstream for anything. The real question is how we manage firstmate changes in OUR environment while still raising upstream pull requests and hoping, without expecting, that they land. Three completed fixes were stuck because their checks never ran: PRs 2777, 2809 and 2584 each reported "1 passed, 0 failed, 1 total", where the single check was a third-party review bot and all fourteen repository-owned suites never started, because GitHub holds fork-branch workflow runs until a maintainer approves them. That produced three separate FALSE GREEN verdicts where a worker read the bot's pass as CI passing; closing that false-green hole is part of the task, not a footnote.
Three deliverables were required: (1) our fork runs the full suite set on a branch we push to it with nobody's approval, proven by a real run id; (2) the two purposes split explicitly and written down where the next session finds them - our fork is where our changes are VALIDATED and where our homes update FROM, while the upstream pull request becomes a hopeful contribution whose held runs block nothing, updating whatever firstmate material assumed upstream was the validation target; (3) the false-green made impossible to repeat, so whatever reads PR checks must not accept a lone third-party bot as CI and must establish that the repository's own required suites actually ran.
Constraints the captain set: do NOT ask the upstream maintainer for anything and do not open, close or modify any upstream pull request - those three PRs stay exactly as they are. Do not weaken, skip or narrow any suite to make something pass; a suite that genuinely cannot run on the fork is a finding to report, not a suite to remove. Anything changing credentials, repository settings a person must click, or the account's security posture is the captain's decision: report exact steps and stop.
What the work found and did, so the diff is read against it:
Root cause was neither of the two hypotheses offered. The fork's default branch does carry .github/workflows and Actions is enabled on it; ci.yml simply only triggered on push-to-main and pull-request-to-main, so no event our fork ever receives matched a trigger and the fork recorded zero runs. The fix in .github/workflows/ci.yml is therefore a trigger widening: CI now runs on any pushed branch, deliberately excluding the orphan no-mistakes/** evidence branches, which are pipeline artifacts and not source history. This was proven, not assumed: fork run 32617494878 on branch fm/fm-validate-on-our-own-fork at head 0cff881, event=push, attempt 1, conclusion success, no approval required, with all 12 CI suites green.
Deliberate decision (finding 1, reported not fixed): a fork push runs 12 of the 13 repository-owned checks. The "Require no-mistakes" check reads github.event.pull_request.body, so it structurally cannot run on a push event. It is a PR-body contribution policy rather than a source-tree suite, and its substance is enforced by the local no-mistakes pipeline anyway. It was left exactly as it is, per the do-not-narrow constraint. Do not "fix" this by making that workflow run on push or by removing it.
Deliberate decision (finding 2, escalated to the captain, no code change): actually making the fork the source homes update FROM needs x45dev/firstmate main brought current and each home's origin repointed at the fork. Both are outside this worktree - one is a default-branch push, the other edits other checkouts - so neither was performed here. The choice itself is documented in docs/architecture.md rather than implemented.
Reported and deliberately not fixed (finding 3): tests/fm-bearings-snapshot.test.sh 'bad home outcomes' pins FM_SNAPSHOT_SECONDMATE_TIMEOUT=1 and fails on this machine at the base commit in a clean independent clone, and tests/fm-test-run.test.sh needs ruby which this machine lacks. Both are pre-existing and environmental, and both are green on every fork CI run. They are not this change's regressions.
The false-green closure is structural rather than bot-name-based, which is the central design decision in the diff. bin/fm-ci-checks-lib.sh is introduced as the single owner of the question "did this commit's own repository workflows actually run", classifying a check by whether a workflow in the repository produced it rather than by matching known bot names, so a new third-party bot cannot slip through a denylist. bin/fm-pr-ci-verify.sh is the guard built on it: it refuses PRs 2777, 2809 and 2584 today while naming the reason, and it accepts a commit our own fork validated even while the upstream run is still held - which is the point, since a held upstream run is a contribution question and never a validation one. bin/fm-bearings-snapshot.sh was changed to classify through that same owner instead of carrying its own duplicate logic, and its coverage-guard list comparison was fixed to compare under the collation the lists were sorted in. AGENTS.md and the generated ship brief in bin/fm-brief.sh now require that verification before any green claim. The split between validation source and contribution target is documented in CONTRIBUTING.md and docs/architecture.md, with docs/scripts.md indexing the two new scripts. 23 new tests in tests/fm-ci-checks.test.sh plus a bot-only regression case added to tests/fm-bearings-snapshot.test.sh pin the behavior, and the Bearings coverage floor was raised accordingly.
Nothing upstream was contacted or modified, and no suite was weakened, skipped or narrowed.
What Changed
.github/workflows/ci.ymlto trigger on any pushed branch (excluding the orphanno-mistakes/**evidence branches), so the fork's own 12 CI suites actually run on a push with no maintainer approval required.bin/fm-ci-checks-lib.shas the single classifier for whether a commit's own repository workflows produced a given check, andbin/fm-pr-ci-verify.shas the guard built on it: it refuses a PR whose only passing check is a third-party bot or that carries red/pending suites, and accepts a commit the fork has already validated even while the upstream run sits held.bin/fm-bearings-snapshot.shto classify through that shared library instead of duplicating logic, and fixed its coverage-guard list comparison to use consistent collation; documented the fork-as-validation-source vs. upstream-as-contribution-target split inCONTRIBUTING.mdanddocs/architecture.md(indexed indocs/scripts.md), required the new verification before any green claim inAGENTS.mdandbin/fm-brief.sh, and addedtests/fm-ci-checks.test.shplus a bot-only-checks regression case intests/fm-bearings-snapshot.test.sh.Risk Assessment
✅ Low: The change is a well-bounded, thoroughly tested trigger widening plus a structural (not name-based) classifier and guard for CI check provenance; all new logic (fm-ci-checks-lib.sh, fm-pr-ci-verify.sh) is covered by 23 focused unit/integration tests, the bearings snapshot and coverage-guard fixes are minimal and consistent, and the two deliberately-deferred items are honestly reported per the stated constraints rather than silently resolved or hidden.
Testing
All targeted tests pass: the 23 new fm-ci-checks.test.sh tests pin the false-green classifier and PR guard exactly against the reported failure mode (lone third-party bot check reading as green), the 43 fm-bearings-snapshot.test.sh tests match the CI-raised floor and include the new bot-only regression case, and the referenced fork CI run (32617494878) independently verifies as a real, unapproved, all-green push-triggered run — the core end-to-end evidence for deliverable 1. No regressions found; nothing to report.
Evidence: fm-ci-checks.test.sh full pass (23/23)
Evidence: gh-axi run view 32617494878 (deliverable-1 evidence)
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 — 23/23 pass, covering fm_ci_checks_state/fm_ci_runs_state classification and fm-pr-ci-verify.sh's bot-only-check refusal, red/pending refusal, and fork-validated acceptance pathsbash tests/fm-bearings-snapshot.test.sh — 43/43 pass (count viagrep -c '^ok - '), including the new bot-only-checks regression row and the previously-flagged-as-environment-flaky 'bad home outcomes' case, which passed in this runbash bin/fm-test-run.sh --check-coverage — exits 0 under this machine's en_US.UTF-8 locale, confirming the LC_ALL=C comm collation fix (commit b311842)gh-axi run view 32617494878 --repo x45dev/firstmate — confirms the cited fork CI run is real: branch fm/fm-validate-on-our-own-fork, event push, conclusion success, 12/12 jobs green, no approval gate✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.