fix(ci): resolve PR Metadata Preflight base live instead of the stale event-payload SHA - #10784
Conversation
… event-payload SHA github.event.pull_request.base.sha is frozen at whatever PR event last delivered it. A metadata-only edited/labeled/unlabeled event fired days after the last push carries a base SHA that main may have moved far past, so the diff scope silently balloons to include unrelated commits that landed on main in the interim — producing false invariant-citation failures on files the PR never touched. The sibling `changes` job in the same workflow already resolves origin/<base_ref> live against the fetch-depth:0 checkout; point metadata-preflight at the same thing. Failure-Class: new
Evidence: PR #10758, where this exact failure surfaced — a diff computed against a frozen github.event.pull_request.base.sha falsely flagged INV-MEM-1 on files the PR never touched, because main had advanced 37 commits past that captured SHA with no intervening event to refresh it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87a9ef4dd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.assertNotIn("github.event.pull_request.base.sha", metadata_job) | ||
| self.assertIn('--base "origin/${{ github.base_ref }}"', metadata_job) |
There was a problem hiding this comment.
These assertions only inspect the workflow text, so they remain green if checkout/ref-resolution behavior later leaves origin/main stale or the preflight stops diffing the fetched ref correctly. Because this test is registered as the canonical prevention artifact for the new failure class, add a behavioral contract that creates an advancing remote base plus a stale payload SHA and executes the production preflight seam; retain these assertions only as an explicitly labeled static tripwire.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 5d7f534. Added test_stale_event_payload_base_widens_diff_scope_past_the_live_base, which exercises the real changed_files() seam against an actual git repo (a captured-then-stale base SHA vs. a live-resolved ref, through a simulated refs/pull/N/merge), and re-labeled this assertion's comment as the static tripwire it is, pointing at the new test for the real coverage.
…base Codex flagged (P1, PR #10784) that the workflow-content assertions only inspect text and stay green even if changed_files()'s ref-resolution behavior regresses. Add a behavioral test that exercises the real changed_files() seam against an actual git repo: a base SHA captured once (standing in for github.event.pull_request.base.sha) and never refreshed picks up unrelated main commits through a merge-ref diff, while a live-resolved base ref does not. Label the existing text assertion as the static tripwire it is and point it at this test for the real coverage.
…base Codex flagged (P1, PR BasedHardware#10784) that the workflow-content assertions only inspect text and stay green even if changed_files()'s ref-resolution behavior regresses. Add a behavioral test that exercises the real changed_files() seam against an actual git repo: a base SHA captured once (standing in for github.event.pull_request.base.sha) and never refreshed picks up unrelated main commits through a merge-ref diff, while a live-resolved base ref does not. Label the existing text assertion as the static tripwire it is and point it at this test for the real coverage.
1 participant
Summary
PR Metadata Preflight(repo-checks.ymlmetadata-preflightjob) diffed againstgithub.event.pull_request.base.sha— a value frozen at whatever PR event last delivered it. That job only runs on metadata-onlyedited/labeled/unlabeledevents, which fire with no new commit and don't refresh that field. On PR #10758,mainhad advanced 37 commits (including a change tobackend/routers/memories.pyin an unrelated PR) since the last time an event refreshedbase.sha, so the diff scope silently ballooned to include those unrelated commits and the job failed on a falseINV-MEM-1citation requirement for files the PR never touched.The sibling
changesjob in the same workflow already avoids this: it resolvesorigin/${{ github.base_ref }}live against the job's ownfetch-depth: 0checkout instead of trusting a payload field. This PR pointsmetadata-preflightat the same thing.Fix
.github/workflows/repo-checks.yml:metadata-preflight'sscripts/pr-preflight --basenow usesorigin/${{ github.base_ref }}instead ofgithub.event.pull_request.base.sha..github/scripts/test_pr_preflight.py: updated the workflow-content regression test to assert the live-resolved base and guard against reintroducing the stale payload field.No behavior changes for the
changes/hygiene(code) lane — it already used the correct pattern.Product invariants affected
none
Failure class (fixes)
Failure-Class: new
Registered
FC-stale-event-payload-diff-base: a check that diffs a PR against a base ref must resolve that ref live from the checkout, not from a value captured in an event payload that isn't refreshed by every trigger type that can run the check. No existing registered class covers event-payload staleness driving an incorrect diff scope.evidence_prspoints at #10758, where this exact failure surfaced. Guard artifact:test_repo_checks_routes_metadata_events_to_the_narrow_preflightnow asserts the live-resolved base and would fail if the stale field were reintroduced.Verification
python3 .github/scripts/test_pr_preflight.py— 26/26 pass (updated regression test covers this exact case).scripts/pr-preflight --pr-body-file <this file>— passes locally withfiles=2againstorigin/main.main, confirmed the exact upstream commit (fix(backend): bound Firestore list reads to stop 30s GET 504s #10750) that touched the flagged memory files, and confirmedgit diff origin/main...HEAD(fresh, correct base) shows 27 files with no memory files touched — matching whatmetadata-preflightshould have reported.Related
Filed a follow-up issue: the same
github.event.pull_request.base.shapattern also appears inbackend-unit-tests.yml,desktop-swift-ci.yml,backend-hermetic-e2e.yml, anddesktop-backend-image-checks.yml. Those jobs trigger on code-changing events (synchronize/opened) where the payload field is refreshed by the same event that needs it, so the staleness window is much narrower — but not zero under queued/delayed runs. Left out of this PR to keep the fix scoped and verifiable; tracked separately.