Skip to content

fix(review): re-check live mergeable state before executing a conflict close - #3881

Merged
loopover-orb[bot] merged 2 commits into
mainfrom
fix-mergeable-state-recheck-before-close
Jul 7, 2026
Merged

fix(review): re-check live mergeable state before executing a conflict close#3881
loopover-orb[bot] merged 2 commits into
mainfrom
fix-mergeable-state-recheck-before-close

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • Fixes Review latency can create a merge-conflict close window an author didn't cause #3863.
  • A conflict-justified heuristic close (closeRequiresMergeableState: true) was executed against whatever mergeable state was current when the review pass planned the action, not what's current when it actually executes. If the base branch changes in the window between planning and actuation — and Gittensory's own review latency is exactly what creates that window — a PR that has since become mergeable again still gets closed for a conflict that no longer exists.
  • The staged/deferred-approval accept-flow (decidePendingAgentAction in src/services/agent-approval-queue.ts) already re-checks live mergeable state before honoring a conflict-justified close. The immediate, no-approval-required execution path in src/services/agent-action-executor.ts did not — its own doc comment explicitly listed "conflict" among the close reasons exempt from any live re-verification (the fix(agent-actions): actuation freshness guard never re-verifies CI/mergeable state before merge or close #2128 CI-recheck note). That exemption was the gap Review latency can create a merge-conflict close window an author didn't cause #3863 describes.
  • Extended the existing live-recheck block in agent-action-executor.ts (previously CI-only) to also fetch live mergeable_state via fetchLivePullRequestMergeState for this one case (action.closeKind === "heuristic" && action.closeRequiresMergeableState === true), running concurrently with the existing CI recheck via Promise.all. If the live state now reads "clean", the close is denied (audited as denied, not executed) instead of proceeding on stale planning-time data.
  • Second commit: while auditing this file for the mergeable-state fix, found that executeAgentMaintenanceActions/executeIssueMaintenanceActions recorded every mutation failure to audit_events only, with zero Sentry visibility — unlike the equivalent "maintainer must see this" convention already used for AI-review-pass failures (captureReviewFailure). Added captureError at each already-terminal point (a merge held for a human; any non-merge action failure, which has no retry loop) so a real GitHub-mutation failure is no longer invisible without a manual audit_events query.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage — verified via lcov: all new lines/branches from both commits show non-zero hit counts; the file's only uncovered lines are pre-existing, in handleMergeFailure's retry-exhaustion arm and an unrelated .catch(() => undefined), neither touched by this PR.
  • npm run test:ci (full local gate, green — 538 test files / 10,733 tests passed, 2 files / 7 tests skipped)
  • npm audit --audit-level=moderate (0 vulnerabilities)
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — added: denied-when-live-state-cleared, proceeds-when-still-dirty, fails-open-on-ambiguous "unknown" state, skips-entirely-when-closeRequiresMergeableState: false, a persist/replay round-trip regression test, and Sentry-capture assertions on both the terminal-merge-hold and non-merge-failure paths (plus a negative-path assertion that a retryable, non-terminal merge failure stays silent). Also fixed 3 pre-existing agent-approval-queue.test.ts tests whose mocks needed a second chained value now that both the approval-queue's own recheck and this new executor-level recheck call the same mocked function.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — no API/OpenAPI/MCP surface touched.
  • UI changes use live API data or real empty/error/loading states. — N/A, no UI changes.
  • Visible UI changes include a UI Evidence section. — N/A, no UI changes.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. — N/A, no changelog-worthy public surface change.

…t close

Fixes #3863.

A conflict-justified heuristic close (`closeRequiresMergeableState: true`)
was executed against whatever mergeable state was current when the review
pass planned the action, not what's current when it actually executes. If
the base branch changes between planning and actuation (Gittensory's own
review latency creates exactly this window), a PR that has since become
mergeable again gets closed anyway for a conflict that no longer exists.

The approval-queue's staged accept-flow (agent-approval-queue.ts) already
re-checks live mergeable state before honoring a conflict-justified close,
but the immediate, no-approval-required execution path in
agent-action-executor.ts did not — its own doc comment explicitly listed
"conflict" as exempt from live re-verification. Extend the existing live-CI
recheck block to also fetch live mergeable state for this one case, and
deny (not merely skip) the action if the conflict has since cleared.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 7, 2026
@loopover-orb

loopover-orb Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Tip

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

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-07 02:47:36 UTC

3 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR extends the existing actuation-time live-recheck block in agent-action-executor.ts to also cover conflict-justified heuristic closes, closing the stale-planning-state window described in #3863 by fetching live mergeable_state concurrently with the existing CI recheck via Promise.all, and denies the close only on a confirmed 'clean' read (fail-safe on ambiguous states), mirroring the existing approval-queue accept-time recheck. The refactor correctly splits ciStaleReason/mergeableStaleReason so the CI-only and mergeable-only paths don't interfere, and the test suite covers denial, dirty-still-proceeds, ambiguous-fail-open, non-conflict-skip, and the persist/replay round trip. A second, less-related change adds captureError/Sentry visibility to previously audit-log-only mutation failures, which is reasonable defensive hardening but is only loosely tied to the stated 'Fixes #3863' scope.

Nits — 6 non-blocking
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3863
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 51 registered-repo PR(s), 43 merged, 348 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 348 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 is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 348 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.56%. Comparing base (56421ce) to head (0d9fd9a).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3881   +/-   ##
=======================================
  Coverage   93.56%   93.56%           
=======================================
  Files         339      339           
  Lines       33445    33455   +10     
  Branches    12242    12249    +7     
=======================================
+ Hits        31292    31302   +10     
  Misses       1528     1528           
  Partials      625      625           
Files with missing lines Coverage Δ
src/services/agent-action-executor.ts 96.98% <100.00%> (+0.11%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The agent-action executor (executeAgentMaintenanceActions,
executeIssueMaintenanceActions) recorded every mutation failure to
audit_events only — real GitHub-mutation failures (merge/close/approve/
label/etc.) had zero Sentry visibility, unlike the equivalent "a real
failure the maintainer must see" convention already used for exhausted
AI-review-pass failures (captureReviewFailure in queue/processors.ts).

Capture at each already-terminal point, not on every attempt: a merge
held for a human (handleMergeFailure's terminal branch, after retries
are exhausted or an immediately-terminal classification), and any
non-merge action class, which has no retry loop so a single failure is
already this pass's terminal outcome. A retryable, non-terminal merge
failure stays silent, matching how a review pass that succeeds on
fallback never fires captureReviewFailure either.

Found while auditing the #3863 mergeable-state-recheck code path for
instrumentation gaps.

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 669b492 into main Jul 7, 2026
10 checks passed
@loopover-orb
loopover-orb Bot deleted the fix-mergeable-state-recheck-before-close branch July 7, 2026 02:47
JSONbored added a commit that referenced this pull request Jul 7, 2026
…xecutes

A duplicate-justified heuristic close (linkedDuplicateCount > 0) had no live
recheck at all, unlike merge-conflict closes (fixed by #3881/#3863) and
CI-failed closes, which both already re-verify right before the mutation.
The duplicate-sibling data driving the close is reconciled ONCE at planning
time (reconcileLiveDuplicateSiblings), before the often-slow AI-review/gate
pass runs, and is never re-checked afterward -- if the blocking sibling PR
closes or merges independently during that window, this PR still gets closed
for a duplicate reason that no longer holds. The close was also immune to
the close-precision circuit breaker, since hasConcreteCloseEvidence treated
a duplicate link as concrete, non-judgment evidence with no staleness risk.

Add closeRequiresDuplicateStillOpen + duplicateWinnerPrNumber to the planned
close action, mirroring closeRequiresMergeableState's own discipline, and add
a live recheck in both the immediate execution path
(agent-action-executor.ts) and the staged-approval accept path
(agent-approval-queue.ts): when a specific winning sibling was named, re-fetch
its live state right before acting and deny the close if that PR is no longer
open. When no specific winner was named (duplicate-winner election disabled,
or an ambiguous election), there's no cheap single-PR signal to check, so the
recheck is a no-op for that case -- same scoping precedent as the mergeable-
state recheck for a non-conflict close.

Left linkedDuplicateCount in hasConcreteCloseEvidence's concrete-evidence set
rather than removing it: the breaker exists to catch a systematically WRONG
heuristic judgment call, not to guard against an otherwise-correct
deterministic fact going stale between planning and actuation -- that's
exactly what the new live recheck now handles, the same relationship a base
conflict already has with its own live recheck.

Also fixes a second, related bug in maybePublishPrPublicSurface: the
duplicate-cluster slop penalty (duplicateClusterMembership, weight 15) was
computed from a raw, un-reconciled listPullRequests read, completely separate
from the properly-reconciled sibling set the gate's own close decision uses
-- despite a comment directly above it claiming they were the same source.
If a lower-numbered sibling is closed on GitHub but its cached DB row hasn't
caught up (a missed/delayed webhook), this path wrongly denied the current
PR winner status and applied the duplicate slop penalty, which under
slopGateMode: block becomes a real gate blocker independent of
duplicatePrGateMode. Thread the same reconciled otherOpenPullRequests into
this function (extending buildAuthorizedPrActionAdvisory to reconcile and
return it too, for the panel-retrigger call site) instead of re-deriving a
second, separately-stale answer, and correct the comment to describe what's
actually happening.
JSONbored added a commit that referenced this pull request Jul 7, 2026
…xecutes (#3930)

A duplicate-justified heuristic close (linkedDuplicateCount > 0) had no live
recheck at all, unlike merge-conflict closes (fixed by #3881/#3863) and
CI-failed closes, which both already re-verify right before the mutation.
The duplicate-sibling data driving the close is reconciled ONCE at planning
time (reconcileLiveDuplicateSiblings), before the often-slow AI-review/gate
pass runs, and is never re-checked afterward -- if the blocking sibling PR
closes or merges independently during that window, this PR still gets closed
for a duplicate reason that no longer holds. The close was also immune to
the close-precision circuit breaker, since hasConcreteCloseEvidence treated
a duplicate link as concrete, non-judgment evidence with no staleness risk.

Add closeRequiresDuplicateStillOpen + duplicateWinnerPrNumber to the planned
close action, mirroring closeRequiresMergeableState's own discipline, and add
a live recheck in both the immediate execution path
(agent-action-executor.ts) and the staged-approval accept path
(agent-approval-queue.ts): when a specific winning sibling was named, re-fetch
its live state right before acting and deny the close if that PR is no longer
open. When no specific winner was named (duplicate-winner election disabled,
or an ambiguous election), there's no cheap single-PR signal to check, so the
recheck is a no-op for that case -- same scoping precedent as the mergeable-
state recheck for a non-conflict close.

Left linkedDuplicateCount in hasConcreteCloseEvidence's concrete-evidence set
rather than removing it: the breaker exists to catch a systematically WRONG
heuristic judgment call, not to guard against an otherwise-correct
deterministic fact going stale between planning and actuation -- that's
exactly what the new live recheck now handles, the same relationship a base
conflict already has with its own live recheck.

Also fixes a second, related bug in maybePublishPrPublicSurface: the
duplicate-cluster slop penalty (duplicateClusterMembership, weight 15) was
computed from a raw, un-reconciled listPullRequests read, completely separate
from the properly-reconciled sibling set the gate's own close decision uses
-- despite a comment directly above it claiming they were the same source.
If a lower-numbered sibling is closed on GitHub but its cached DB row hasn't
caught up (a missed/delayed webhook), this path wrongly denied the current
PR winner status and applied the duplicate slop penalty, which under
slopGateMode: block becomes a real gate blocker independent of
duplicatePrGateMode. Thread the same reconciled otherOpenPullRequests into
this function (extending buildAuthorizedPrActionAdvisory to reconcile and
return it too, for the panel-retrigger call site) instead of re-deriving a
second, separately-stale answer, and correct the comment to describe what's
actually happening.
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review latency can create a merge-conflict close window an author didn't cause

1 participant