Skip to content

fix(review): re-verify duplicate-close justification live before it executes#3930

Merged
JSONbored merged 1 commit into
mainfrom
fix/dup-close-live-recheck
Jul 7, 2026
Merged

fix(review): re-verify duplicate-close justification live before it executes#3930
JSONbored merged 1 commit into
mainfrom
fix/dup-close-live-recheck

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Two related bugs in the duplicate-PR close path, both stemming from the same "reconciled once at planning time, never re-verified at actuation" gap that #3881/#3863 already fixed for merge-conflict closes.

Bug 1 — duplicate-justified closes never get a live recheck. A close justified by linkedDuplicateCount > 0 sets neither closeRequiresMergeableState nor a CI dependency, so it triggers none of the existing live-recheck paths in agent-action-executor.ts or agent-approval-queue.ts. The duplicate-sibling data is reconciled once against GitHub (reconcileLiveDuplicateSiblings) before the AI-review/gate pass runs, and never re-checked before the close actually executes minutes later. If the blocking sibling PR closes or merges independently in that window, this PR still gets closed for a duplicate reason that no longer holds. It was also immune to the close-precision circuit breaker, since hasConcreteCloseEvidence classified a duplicate link as concrete evidence.

Fix: added closeRequiresDuplicateStillOpen + duplicateWinnerPrNumber to the planned close action (mirroring closeRequiresMergeableState's own discipline), and a live recheck in both the immediate execution path and the staged-approval accept path — re-fetch the named winning sibling's live state right before acting, deny the close if it's no longer open. When no specific winner was named (duplicate-winner election off, or ambiguous), there's no cheap single-PR signal, so the recheck is a no-op there, same as the mergeable-state recheck's own scoping for a non-conflict close.

I deliberately left linkedDuplicateCount in hasConcreteCloseEvidence's concrete-evidence set rather than pulling it out. The breaker exists to catch a systematically wrong heuristic judgment call (an AI verdict that's often mistaken), not to guard against an otherwise-correct deterministic fact going stale between planning and actuation — that staleness risk is exactly what the new recheck closes. A duplicate-issue link is still a deterministic fact about the linked-issue graph, same as a base conflict, which already stays in that set even after #3863 added its own live recheck.

Bug 2 — the slop gate's duplicate-winner election used stale data. Inside maybePublishPrPublicSurface, the duplicate-winner computation feeding the slop gate's duplicateClusterMembership penalty (weight 15) was built from a raw listPullRequests read — a straight, un-reconciled cache read — while the comment directly above it claimed it used "the SAME open-only sibling source the gate uses." It didn't: the gate's actual close decision uses otherOpenPullRequests, reconciled once against GitHub earlier in the same request. 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.

Fix: threaded the same reconciled otherOpenPullRequests into maybePublishPrPublicSurface from all three call sites (extending buildAuthorizedPrActionAdvisory to reconcile and return it too, for the panel-retrigger path) instead of re-deriving a second, independently-stale answer, and corrected the comment.

Scope

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage locally (full unsharded run) — all 545 test files / 10835 tests pass; both changed non-type source files (agent-actions.ts, agent-approval-queue.ts) are at 100% line/function coverage on the touched code, agent-action-executor.ts's only uncovered lines are pre-existing and unrelated (handleMergeFailure's retry-cap branch), processors.ts's only uncovered lines are pre-existing and unrelated (review-nag cooldown audit write)
  • npm run test:ci — full local gate green, exit 0 (actionlint, migrations/schema-drift checks, cf-typegen check, typecheck, test:coverage, test:workers, build:mcp, test:mcp-pack, build:miner, test:miner-pack, rees:test, ui:openapi checks, docs-drift, command-reference, ui:lint/typecheck/build all passed)
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New/changed behavior has unit tests for new branches, fallback paths, and both sides of the new conditionals: added regression tests to test/unit/agent-action-executor.test.ts, test/unit/agent-approval-queue.test.ts, test/unit/agent-actions.test.ts (planner-level field-setting + round-trip), and an end-to-end processJob regression in test/unit/queue.test.ts for Bug 2 that reproduces the stale-slop-penalty symptom against the pre-fix code and confirms the fix clears it

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 — no auth/session/CORS changes; this is a review-disposition/gate-evaluation change, covered by the negative-path tests described above: ambiguous live reads, no-named-winner, non-duplicate closes all fail open / skip correctly.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface changed; ui:openapi:check passed with no diff.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section below with screenshots. (N/A — backend-only change, no visible UI surface.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (No changelog edit; no public docs describe this internal disposition-planning behavior.)

Notes

Both bugs share the same root cause pattern as #3881 (Fixes #3863): duplicate-sibling data is correctly reconciled against GitHub once, early in a request, but two separate downstream consumers of that data either skip the recheck entirely (Bug 1, the close-execution path) or silently re-derive their own un-reconciled copy instead of reusing the reconciled one (Bug 2, the slop-gate path). Fixing them together made sense since they're the same class of staleness bug on the same feature and touch overlapping code paths (reconcileLiveDuplicateSiblings, otherOpenPullRequests, linkedDuplicateCount/linkedDuplicateWinnerNumber).

@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

Important

🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪

🔍 Gittensory is reviewing…

AI analysis is in progress. This comment will update when the review is complete.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.68%. Comparing base (b8a1186) to head (3b29712).
⚠️ Report is 14 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/services/agent-approval-queue.ts 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3930   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files         373      373           
  Lines       34895    34913   +18     
  Branches    12768    12785   +17     
=======================================
+ Hits        32692    32709   +17     
  Misses       1584     1584           
- Partials      619      620    +1     
Files with missing lines Coverage Δ
src/queue/processors.ts 94.55% <100.00%> (+<0.01%) ⬆️
src/services/agent-action-executor.ts 97.09% <100.00%> (+0.06%) ⬆️
src/settings/agent-actions.ts 97.72% <100.00%> (+0.02%) ⬆️
src/services/agent-approval-queue.ts 99.12% <91.66%> (-0.88%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label 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
JSONbored force-pushed the fix/dup-close-live-recheck branch from 80910b1 to 3b29712 Compare July 7, 2026 07:54
@JSONbored
JSONbored merged commit b95c7bf into main Jul 7, 2026
7 checks passed
@JSONbored
JSONbored deleted the fix/dup-close-live-recheck branch July 7, 2026 07:59
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. manual-review Gittensor contributor context

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