fix(webhook): stale payloads can no longer revert close-critical columns; handle transfers; wake on legacy CI events - #9116
Merged
Conversation
…handle transfers; wake on legacy CI events #9057 -- the out-of-order guard protected only state/headSha/mergedAt/ githubUpdatedAt/labels. Every OTHER column was written unconditionally from a possibly-stale payload, including the ones the linked-issue gate reads -- a hard blocker and a close reason. Real sequence: a contributor pushes (`synchronize`, T1), then edits the body to add "Fixes #123" (`edited`, T2). GitHub does not guarantee delivery order, and once heads diverge the two jobs have different coalesce keys, so `synchronize` can be dequeued LAST. The guard correctly preserved state/headSha/labels -- then reverted body and linkedIssuesJson to the pre-edit snapshot, and since `synchronize` is itself in PR_PUBLIC_SURFACE_ACTIONS the same pass re-ran the gate against the reverted body: a wrong autonomous close for "No linked issue detected" on a PR that has one. That close reason appears in the live ledger. body/linkedIssuesJson/linkedIssueClaimedAt now reuse the existing sparse-body preservation path (the incoming value is not current truth in either case), and title/baseRef/headRef/authorAssociation are protected on the same condition. Applied to the onConflictDoUpdate SET clause as well as the INSERT values -- the UPDATE is the path that actually runs for an existing row -- and to the returned record, which callers act on in-process. #9056 -- `repository.transferred` was never handled despite already being in WEBHOOK_METRIC_ACTIONS, so the case was anticipated and never implemented. Since `repositories` is keyed by full_name with no github_id, a transfer just INSERTed a fresh row and orphaned every table renameRepositoryIdentity migrates -- including repository_settings, so autonomy and gate config silently reverted to defaults while the repo kept operating, and staged approvals in agent_pending_actions were lost. Now derives the old name from changes.owner.from.{organization,user}.login plus the current repo name and runs the same identity migration as a rename. #9059(b) -- `status`/`workflow_run` invalidated the CI cache but never woke a re-review, which only moved the stall: the PR then waited on the ~2-minute sweep, which REST-budget backpressure can skip. That matters specifically because `codecov/patch` arrives as a commit STATUS and is the gate's hardest required check, so a repo whose last green signal is a status got no webhook wake at all and the gate looked hung. Now re-reviews behind the same ciReReviewCoalesced guard the check_run/check_suite path uses, so a status storm cannot amplify. Closes #9056 Closes #9057 Tests: 2 stale-payload regressions (a stale delivery cannot revert body, linked issues, title, baseRef or headRef; a newer one still updates all of them), a status-wakes-re-review test, and the two existing invalidation tests updated to pre-claim the coalesce window so the invalidation stays observable alongside the new wake. Full suite green: 22,175 passing.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9116 +/- ##
==========================================
- Coverage 93.89% 92.67% -1.23%
==========================================
Files 813 813
Lines 80798 80809 +11
Branches 24529 24538 +9
==========================================
- Hits 75864 74887 -977
- Misses 3564 4844 +1280
+ Partials 1370 1078 -292
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-27 00:32:09 UTC
Review summary Blockers
Nits — 6 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
…ned record, and close the coverage gaps Review on #9116 was correct on both counts. 1. The returned record overrode title/headSha/headRef/baseRef/mergedAt/labels but omitted authorAssociation, so an in-process caller could still read the stale value even though the DB row was correctly protected -- contradicting the very invariant the previous commit stated. Added, and asserted on the return value (not just the stored row) in the regression test. 2. codecov/patch was low because the transferred-event derivation and the authorAssociation stale-path had no tests. Both covered now. The transfer-predecessor derivation is extracted into an exported pure helper, resolveRepositoryIdentityPredecessor, because several of its shapes are unreachable through the live pipeline -- the repository upsert running alongside this handler requires a full_name and fails first -- so they could not be tested end to end. That also let the dead `?? payload.repository.owner/name` fallbacks go: full_name is authoritative and already required. Full suite green: 22,182 passing. 100% line+branch coverage on all added lines.
JSONbored
added a commit
that referenced
this pull request
Jul 27, 2026
… health, and stop label/issue churn from going stale (#9169) #9055 — a contributor can move a green PR onto a new base after CI passed against the old one, with the head SHA unchanged. Nothing else notices: GitHub does not re-run pull_request workflows or emit a new head for a base-only edit, so the stored diff/patches, the AI-review cache (its fingerprint deliberately excludes baseSha), and the CI aggregate all keep describing the ABANDONED base -- permanently, since the sweep only re-syncs on head/label drift. classifyPullRequestFreshness gains an explicit `base_changed` reason, checked right after head/state and before the draft check, so a live base mismatch is caught at the last possible moment -- immediately before a merge/approve mutation -- using the SAME live fetch that already proves the head, no extra GitHub call. Threaded through AgentActionExecutionContext.expectedBaseRef from the stored PR row, exactly like expectedHeadSha already is. The webhook side: `pull_request.edited` with `changes.base` is GitHub's own signal that this specific mutation happened. It now forces a fresh file sync (bypassing the head-keyed `filesUpToDate` check that would otherwise skip the refetch since the head hasn't moved) and invalidates the PR-state, CI-state, and review caches -- treated as review-invalidating exactly like a new commit. #9058 — two related event-subscription gaps. `check_run`/`check_suite` were not required events, so an installation missing them reported "healthy" while silently degrading from event-driven to sweep-only for CI settlement -- despite that path being documented in-code as THE auto-merge / close-on-red trigger. `pull_request_review` had the same gap for the human-approval signal the merge gate reads. All three are now required, and the self-host setup wizard's manifest is derived from the same list instead of hand-maintained -- it had already drifted, omitting `issue_comment` and `repository`, so a wizard-created App started with every `@loopover …` command dead and no rename handling. #9059 — two bounded coverage gaps from the same audit. (a) Label/assignment churn on a linked issue never reached upsertIssueFromGitHub: maybeReReviewOnLinkedIssueChange returns true unconditionally once repo+installation+issue are present, and the caller short-circuits on that before handleIssueWebhookEvent -- the only other place the row gets upserted for an `issues` event. So `issues.labels_json` and assignee state only advanced on opened/edited/closed/reopened, or the (up to 6-hourly) backfill -- while label churn is the single most common issue mutation, read by issue-side advisories, slop triage, enrichment, and the MCP/API issue surfaces. The upsert now runs first, before the wake fan-out. (b) `pull_request.labeled`/`unlabeled` did a row re-sync but were never in PR_PUBLIC_SURFACE_ACTIONS, so a maintainer adding or removing the manual-review hold label was only picked up by the sweep -- unbounded lag under REST-budget backpressure. Both actions are disposition INPUTS (the hold reads straight off the PR's labels), so they now join edited/synchronize. Part (c) of #9059 (status/workflow_run never triggering a re-review) was already closed by #9116 -- verified via ciReReviewCoalesced/ reReviewStoredPullRequest already wired into maybeInvalidateCiCacheOnLegacyCiEvent. Targeted tests only (no full local gate run this pass, per instruction): the files this change touches plus the new coverage tests all pass locally (test/unit/{issue-label-churn-upsert,pr-base-retarget-webhook, pr-labeled-public-surface,pr-freshness,agent-action-executor, pending-closure-watchdog,pr-outcome-reconciler,queue-2}.test.ts -- 369 tests). CI runs the full gate on push. Closes #9055 Closes #9058 Closes #9059
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.
Summary
#9057 — a stale webhook reverts the body and causes a wrong close. The out-of-order guard protected only
state/headSha/mergedAt/githubUpdatedAt/labels. Every other column was written unconditionally from a possibly-stale payload — including the ones the linked-issue gate reads, which is a hard blocker and a close reason.The sequence: a contributor pushes (
synchronize, T1), then edits the body to addFixes #123(edited, T2). GitHub doesn't guarantee delivery order, and once heads diverge the two jobs have different coalesce keys — sosynchronizecan be dequeued last. The guard correctly preserved state/headSha/labels, then revertedbodyandlinkedIssuesJsonto the pre-edit snapshot. Sincesynchronizeis itself inPR_PUBLIC_SURFACE_ACTIONS, the same pass re-ran the gate against the reverted body → wrong autonomous close for "No linked issue detected" on a PR that has one. That close reason appears in the live ledger.Body/linked-issue state now reuses the existing sparse-body preservation path — in both cases the incoming value simply isn't current truth — and
title/baseRef/headRef/authorAssociationare protected on the same condition. Applied to theonConflictDoUpdateSET clause as well as the INSERT values (the UPDATE is the path that actually runs for an existing row — my first pass only fixed the INSERT and a test caught it), and to the returned record, which callers act on in-process.#9056 —
repository.transferredwas never handled. It was already listed inWEBHOOK_METRIC_ACTIONS, so the case was anticipated and simply never implemented. Becauserepositoriesis keyed byfull_namewith nogithub_id, a transfer just INSERTed a fresh row and orphaned every tablerenameRepositoryIdentitymigrates — includingrepository_settings, so autonomy and gate config silently reverted to defaults while the repo kept operating under different policy, and staged maintainer approvals inagent_pending_actionswere lost. Now derives the old name fromchanges.owner.from.{organization,user}.login+ the current repo name and runs the same identity migration as a rename.#9059(b) — legacy CI events invalidated the cache but never woke a re-review. That only moved the stall: the PR then waited on the ~2-minute sweep, which REST-budget backpressure can skip. It matters specifically because
codecov/patcharrives as a commit status and is the gate's hardest required check — a repo whose last green signal is a status got no webhook wake at all, so the gate looked hung. Now re-reviews behind the sameciReReviewCoalescedguard thecheck_run/check_suitepath uses, so a status storm can't amplify.Closes #9056
Closes #9057
Scope notes
upsertIssueFromGitHub) and (c) (pull_request.labelednot inPR_PUBLIC_SURFACE_ACTIONS) are likewise left open; this PR takes only the (b) half, which is the one causing the "gate hung" symptom.Test plan
baseReforheadRef; a newer one still updates all of themcheck_runtest already used for the same reason)test:cibuilds firsttsc --noEmit; migrations/schema-drift/docs/engine-parity gates green