Skip to content

fix(github): evaluate CI-status coalescing for the merge/close actuation path — found unsafe, documented - #2600

Merged
JSONbored merged 3 commits into
mainfrom
fix/selfhost-ci-status-refetch-coalesce
Jul 2, 2026
Merged

fix(github): evaluate CI-status coalescing for the merge/close actuation path — found unsafe, documented#2600
JSONbored merged 3 commits into
mainfrom
fix/selfhost-ci-status-refetch-coalesce

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #2539.

An earlier roadmap item (#1941) collapsed most repeated check/status reads onto a shared per-request cache used by the readiness check, the maintenance planner, and the post-publish boundary. This issue identified two remaining call sites that bypass it: the staged-merge approval-queue acceptance path (decidePendingAgentAction) and the auto-maintain action executor's own pre-mutation re-check (executeAgentMaintenanceActions).

This PR's history reflects a real correction, not just an implementation. My first commit implemented a coalescing shortcut (prefetchedLiveCi) between these two call sites, on the assumption that they run "moments apart, in the same synchronous accept pass." The gate's own AI review correctly flagged this as unsafe. I traced the actual call chain and confirmed the review's finding — the assumption was wrong, and I reverted the shortcut in a follow-up commit. Both commits are visible in this PR's history for anyone who wants the full story.

What the gate found, and what I verified

Between decidePendingAgentAction's accept-time CI read and the eventual mergePullRequest mutation, there is real, non-trivial async work:

  1. Promise.all([isHoldOnly(env, ...), isCloseHoldOnly(env, ...)]) — two DB reads.
  2. When a merge is staged and the author is close-eligible: createInstallationToken(...) + resolveLinkedIssueHardRule(...) — a live GitHub API call when hard rules are configured for the repo.
  3. Inside executeAgentMaintenanceActions itself, before reaching the (previously coalesced) CI check: the freshness guard's fetchPullRequestFreshness(...) — an unconditional live GitHub call checking the PR's current head SHA.

That is exactly the window the executor's pre-mutation CI re-check (#2128) exists to catch a CI flip in — its own doc comment says so explicitly: "Re-read live CI right before the mutation so a check that flipped in this narrow window is never acted on from stale information." Reusing the earlier accept-time read defeated the guard it was reusing, on the merge/close actuation boundary — exactly the class of bug #4220 was about.

What changed

Why this is still a legitimate resolution of #2539

The issue's own requirement was explicit: "Preserve the requirement that these are act-boundary reads... this is a coalescing fix, not a staleness-tolerance fix." Having verified that the ONE remaining orphaned pairing (decidePendingAgentActionexecuteAgentMaintenanceActions) cannot be coalesced without becoming a staleness-tolerance change, the correct outcome — consistent with how #2541 (review-enrichment cost-class parallelization) and #2543 (rate-limit-observation write batching) were also resolved in this same roadmap — is to document why, not force an unsafe implementation to satisfy the issue's original framing.

Tests

Validation

  • npm run typecheck
  • npx vitest run test/unit/agent-action-executor.test.ts test/unit/agent-approval-queue.test.ts (118 tests pass)
  • npm run test:ci (the full local gate, exit 0)
  • git diff --check
  • Diff-level coverage check: 100% of changed lines/branches covered per coverage/lcov.info

Scope

  • Change is narrow and limited to the stated problem
  • No secrets, wallets, hotkeys, trust scores, or reward values added anywhere
  • No edits to site/, CNAME, **/lovable/**, or CHANGELOG.md

Safety

…hared live-facts cache

Closes #2539.

The staged-merge approval-queue acceptance path (decidePendingAgentAction's
own #2126 re-check) and the auto-maintain action executor's pre-mutation
re-check (#2128) both call fetchLiveCiAggregate independently, moments apart
in the SAME synchronous accept pass, using the identical unfiltered
fetchLiveCiAggregate(..., requiredContexts: undefined, ...) shape -- a
genuine, safe-to-coalesce duplicate fetch for the exact same question about
the exact same commit.

Thread the accept-time read forward as an optional prefetchedLiveCi on
AgentActionExecutionContext, keyed by headSha: the executor's own re-check
reuses it only when the headSha matches exactly, otherwise it fetches fresh
-- identical to today's behavior for every other caller.

Scope note: processors.ts's readiness/planner/post-publish call sites already
share a DIFFERENT, required-context-FILTERED live-CI cache (#1941). That
cache is deliberately NOT threaded into this fix -- reusing a filtered
aggregate for this unfiltered fold-all re-check would silently loosen the
merge-safety gate for repos with required-status-checks configured. The two
call sites this PR wires together are the only pair that share both timing
(same synchronous pass) and semantics (unfiltered).
@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-02 21:28:18 UTC

1 file · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The diff adds a focused regression test proving the staged merge acceptance path still performs two separate live CI reads: the accept-time guard and the executor's pre-mutation guard. The added test matches the PR description and exercises the real accept flow through `decidePendingAgentAction`, then asserts the merge uses the staged head SHA and that `fetchLiveCiAggregate` was called twice. No production behavior changes are introduced, so the visible change is safe as coverage for the reverted coalescing idea.

Nits — 5 non-blocking
  • test/unit/agent-approval-queue.test.ts:538: nit: the test name is so long that it makes reporter output hard to scan; keep the scenario name short and move the detailed rationale into a comment above the test.
  • test/unit/agent-approval-queue.test.ts:551: nit: the call-count assertion protects against coalescing, but it does not document which two call sites produced the calls, so future failures may require extra tracing.
  • test/unit/agent-approval-queue.test.ts:538: Rename the test to something like `keeps accept-time and pre-mutation CI reads separate for staged merges` and put the detailed fix(agent-actions): approval-queue accept replays a staged merge with no live CI/mergeable/review re-check #2126/fix(agent-actions): actuation freshness guard never re-verifies CI/mergeable state before merge or close #2128 rationale in a nearby block comment.
  • test/unit/agent-approval-queue.test.ts:551: If the mock records arguments, assert both calls target `owner/repo` PR `7` at head `h7` so the regression test proves the duplicate reads are for the same staged mutation path.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2539
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:XS; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 64 registered-repo PR(s), 55 merged, 526 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 64 PR(s), 526 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 64 PR(s), 526 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.05%. Comparing base (6f46056) to head (722c7e2).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2600   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files         234      234           
  Lines       26280    26280           
  Branches     9531     9531           
=======================================
  Hits        25244    25244           
  Misses        425      425           
  Partials      611      611           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JSONbored added 2 commits July 2, 2026 13:20
…irmed unsafe

The gate's own AI review correctly flagged this: the "moments apart, same
synchronous pass" assumption behind prefetchedLiveCi was wrong. Traced the
actual call chain between decidePendingAgentAction's accept-time CI read and
the executor's pre-mutation re-check and found real, non-trivial async work
in between -- isHoldOnly/isCloseHoldOnly (DB reads), createInstallationToken
+ resolveLinkedIssueHardRule (a GitHub API call when hard rules are
configured), and, inside the executor itself, fetchPullRequestFreshness (an
unconditional live GitHub call) -- all running before the (previously
coalesced) CI check. That is exactly the window #2128's pre-mutation
re-check exists to catch a CI flip in; reusing the earlier read defeated the
guard it was reusing.

Reverts prefetchedLiveCi entirely: the field on AgentActionExecutionContext,
the capture/thread-through in decidePendingAgentAction, and the reuse branch
in executeAgentMaintenanceActions's pre-mutation check, which now always
fetches fresh again, exactly as before #2539. Replaces the coalescing tests
with one regression test asserting the correct (always-two-fetches) behavior,
so this specific unsafe shortcut can't be silently reintroduced.

#2539's other orphaned call site (the duplicate-sibling reconciliation /
gate-override reads coalesced via cachedFetchLivePullRequestMergeState /
cachedFetchLivePullRequestState, already shipped in #2537) remains correctly
coalesced -- those are genuinely non-authoritative reads, not the merge/close
actuation boundary this revert is about.
@dosubot dosubot Bot added size:XS and removed size:M labels Jul 2, 2026
@JSONbored JSONbored changed the title fix(github): close the remaining CI-status re-fetch gap outside the shared live-facts cache fix(github): evaluate CI-status coalescing for the merge/close actuation path — found unsafe, documented Jul 2, 2026
@JSONbored
JSONbored merged commit b1b335a into main Jul 2, 2026
12 checks passed
@JSONbored
JSONbored deleted the fix/selfhost-ci-status-refetch-coalesce branch July 2, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

fix(github): close the remaining CI-status re-fetch gap outside the shared live-facts cache

1 participant