fix(review): re-check live review-thread state before executing a thread-only close - #3931
Conversation
…ead-only close A contributor PR that is auto-closed solely because of an unresolved GitHub review thread had no live re-check of that fact before the close actually executed, at either of the two actuation checkpoints in this codebase. This is the same staleness pattern #3863 already fixed for base-conflict closes (re-checking mergeable_state live right before acting, since a slow review pass can let the underlying fact change between planning and actuation) -- but #3863's fix only covers a conflict-justified close (closeRequiresMergeableState: true). A review-thread-only close is planned with closeRequiresMergeableState: false, the same bucket the code explicitly exempts from a CI recheck. But a review thread being resolved (a contributor clicking "Resolve conversation" on GitHub) is exactly the kind of live-changeable fact that needs a recheck too. Tag a review-thread-justified close with a new closeRequiresThreadResolved marker at planning time (mirroring closeRequiresMergeableState's own discipline), then extend both actuation checkpoints to re-check live review-thread state before honoring the close: the approval-queue's accept-time recheck (agent-approval-queue.ts) and the executor's step-8 pre-mutation live-recheck (agent-action-executor.ts). Both now call the existing fetchLiveReviewThreadBlockers helper (already used by the live webhook planning pass) and deny/supersede the close if the threads have since all resolved. Fixes #3919.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-07 07:14:24 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3931 +/- ##
=======================================
Coverage 93.58% 93.58%
=======================================
Files 358 358
Lines 34342 34358 +16
Branches 12570 12585 +15
=======================================
+ Hits 32138 32154 +16
Misses 1580 1580
Partials 624 624
🚀 New features to boost your workflow:
|
Summary
mergeable_statelive right before acting, since a slow review pass can let the underlying fact change between planning and actuation) -- but Review latency can create a merge-conflict close window an author didn't cause #3863's fix only covers a conflict-justified close (closeRequiresMergeableState: true). A review-thread-only close is planned withcloseRequiresMergeableState: false, the same bucket the code explicitly exempts from a CI recheck. A contributor resolving the thread on GitHub between planning and actuation is exactly the kind of live-changeable fact that needs its own recheck, mirroring Review latency can create a merge-conflict close window an author didn't cause #3863's reasoning.closeRequiresThreadResolvedmarker at planning time, then extend both actuation checkpoints (the approval-queue's accept-time recheck, and the executor's step-8 pre-mutation live-recheck) to call the already-existingfetchLiveReviewThreadBlockershelper and deny/supersede the close if the threads have since all resolved.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateSafety
UI Evidencesection below with JPG/JPEG or PNG screenshots -- N/A, this is a backend-only logic fix with no UI surface.Notes
src/settings/agent-actions.ts,src/services/agent-approval-queue.ts,src/services/agent-action-executor.ts,src/types.ts) plus matching unit tests; no schema, OpenAPI, wrangler binding, or migration changes are needed.