Skip to content

fix(review): PR mergeable-state + CI aggregate fetched live from GitHub twice per review pass #4498

Description

@JSONbored

Part of #4496. P2 — medium severity, high confidence.

Context

reReviewStoredPullRequest calls maybePublishPrPublicSurface, which force-refreshes liveMergeState (src/queue/processors.ts:10621) and the CI aggregate (processors.ts:10617) AFTER its own gate check-run has already been finalized (processors.ts:10306). Moments later, with zero intervening GitHub mutations, it calls runAgentMaintenancePlanAndExecute, which force-refreshes the exact same two resources again (refreshLiveMergeState at processors.ts:2709, refreshLiveCiAggregate at processors.ts:2717-2727) for the identical repo/PR/headSha.

Both refresh helpers (refreshLiveMergeState, refreshLiveCiAggregate, processors.ts:847-877, 908-924) deliberately bypass ALL caching layers — the request-local LiveGithubFacts map AND the durable pull_request_detail_sync_state cache — and unconditionally overwrite the LiveGithubFacts cache entry rather than checking it first. Sibling cachedLiveCiAggregate/cachedLiveMergeState variants (processors.ts:813-845, 879-901) that DO check the cache first exist in the same file but are not reused for the second read.

Both call sites' own doc comments cite the SAME single mutation (the gate's check-run publish) as their reason to force-refresh — but that mutation already happened once, before the FIRST of the two reads. Nothing GitHub-side changes between maybePublishPrPublicSurface's own refresh and runAgentMaintenancePlanAndExecute's refresh (only a text comment/labels are posted in between, which do not affect mergeable_state or CI status).

This is two extra live GitHub calls (one GET /pulls/{n} for mergeable_state, one check-runs+commit-statuses CI fetch) per PR per review pass — across every webhook trigger AND every sweep tick that runs reReviewStoredPullRequest (call sites at processors.ts:1230, 2260, 4322, 4501, 4598, 11472). This only occurs when unifiedCommentAllowed is true (processors.ts:10596 gate) — the legacy comment path never triggers the first refresh — so impact is scoped to repos with that feature on, not universal.

Requirements

  • Have runAgentMaintenancePlanAndExecute call cachedLiveMergeState/cachedLiveCiAggregate instead of refreshLiveMergeState/refreshLiveCiAggregate for its second read, so it reuses the already-fetched value from maybePublishPrPublicSurface's own refresh (running strictly earlier in the same pass, sharing the same LiveGithubFacts bag threaded through both calls) instead of re-fetching.
  • Confirm this doesn't break the FIRST-ever refresh in a pass where maybePublishPrPublicSurface itself was skipped (e.g. unifiedCommentAllowed false, or the legacy comment path) — runAgentMaintenancePlanAndExecute must still get a live value in that case, not a stale/absent cache miss.
  • This also fixes the stated intent of the processors.ts:10606-10608 comment ("so the public chip and the disposition can never disagree") which currently is undermined by the two independent re-fetches potentially observing different live states if GitHub's own eventual consistency shifts between the two calls.
  • Invariant + regression tests (non-negotiable): a test asserting exactly ONE live GitHub call for mergeable_state and ONE for the CI aggregate occurs across a full reReviewStoredPullRequest pass when unifiedCommentAllowed is true (mock fetch call-count assertion); an invariant test confirming the disposition and the public comment both observe the identical merge-state/CI value in the same pass; a regression test for the case where maybePublishPrPublicSurface is skipped, confirming runAgentMaintenancePlanAndExecute still gets a live (not stale) value.

Deliverables

  • runAgentMaintenancePlanAndExecute's second read reuses the cached variant instead of re-fetching, when a prior refresh in the same pass already populated it
  • The "skipped maybePublishPrPublicSurface" fallback path still gets a live fetch
  • Invariant tests: single-fetch-per-pass, disposition/comment agreement
  • Regression test for the skipped-surface fallback case

Expected outcome

One live GitHub mergeable-state fetch and one live CI-aggregate fetch per review pass instead of two of each, cutting real GitHub REST call volume on every reviewed PR, with the added correctness benefit that the disposition and the public comment can no longer observe two different live snapshots.

References

  • src/queue/processors.ts:903-924 (the refresh helpers + their doc-comment rationale)
  • src/queue/processors.ts:813-901 (the unused cached-variant siblings)
  • src/queue/processors.ts:2707-2727 (the second, redundant call site)
  • src/queue/processors.ts:10606-10621 (the first call site + its stated intent)
  • Call sites of reReviewStoredPullRequest: processors.ts:1230, 2260, 4322, 4501, 4598, 11472

Effort

S

Metadata

Metadata

Assignees

Labels

gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.maintainer-onlyOwner-only work — yields no Gittensor points.orbGittensory Orb related - maintainer self-hosting analytics.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions