fix(agent): deny approve/merge when a live manual-review hold exists - #3483
Conversation
…3472) Two independent plan-and-execute passes can evaluate the same PR head (a webhook re-review racing a regate-repair/sweep pass): the loser of the AI-review lock holds the PR for manual review (label + assign), while the winner's own slower AI review later resolves cleanly and stages approve/merge from a plan-time snapshot that predates the hold. Neither the per-PR actuation lock nor the executor's freshness guard checks the PR's live labels, so the hold was silently bypassed. Add a live guard immediately before approve/merge that re-checks the same freshness fetch for the configured manual-review label and denies the action if it's present -- self-scoping to the same head (a new commit already fails freshness first) with no new locking or persistence required. Closes #3482
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-05 09:32:28 UTC
⏸️ Suggested Action - Manual Review 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 #3483 +/- ##
=======================================
Coverage 93.03% 93.03%
=======================================
Files 299 299
Lines 31204 31210 +6
Branches 11386 11388 +2
=======================================
+ Hits 29031 29037 +6
Misses 1517 1517
Partials 656 656
🚀 New features to boost your workflow:
|
Closes #3482
Summary
regate-repair/sweep pass) can each independently evaluate the same head. The loser of the AI-review lock (claimAiReviewLock) gets an instant "AI review already in progress" placeholder, the gate resolves toneutral, and the planner holds the PR for manual review (assignee +manual-reviewlabel). The winner keeps running its own, slower, real AI review; when it later resolves cleanly, that pass's own planner independently computesreviewGood && !heldForManualReviewfrom a plan-time snapshot that predates the loser's label — and stages approve + merge. The per-PR actuation lock (claimPrActuationLock, fix(queue): concurrent webhook and sweep jobs for the same PR are not mutually excluded #2129) only serializes each pass's own critical section; it never makes one pass aware of the other's disposition, and the executor's existing freshness guard only re-checks head SHA/open state, never live labels. Net effect: the bot posted "manual review recommended" + assigned + labeledmanual-review, then merged the same PR seconds later.executeAgentMaintenanceActions(src/services/agent-action-executor.ts), immediately beforeapprove/merge, that re-checks the SAME live PR fetch already performed for the freshness guard (fetchPullRequestFreshness, extended to carryliveLabels) for the configuredmanualReviewLabel. If present, the action is denied with a clear audit event instead of executing. This is self-scoping to the same head for free — a new commit already fails the freshness guard first — and self-clears the moment a maintainer removes the label, with no new locking, persistence, or planner input required.manualReviewLabelinto the two call sites that can plan/execute an approve or merge: the live webhook/sweep path (src/queue/processors.ts) and the approval-queue accept-replay path (src/services/agent-approval-queue.ts), so a staged approve/merge sitting in the queue is covered too.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 typecheck(clean)npm run test:coverage(full, unsharded) — 461 test files passed, 9196 tests passed, 7 pre-existing skips, 0 failuresnpx vitest run test/unit/agent-action-executor.test.ts test/unit/pr-freshness.test.ts test/unit/agent-approval-queue.test.ts test/unit/mcp-automation-state.test.ts test/unit/routes-agent-approval.test.ts test/unit/queue.test.tsnpm run test:workers/npm run build:mcp/npm run test:mcp-pack/npm run ui:openapi:check/npm run ui:build— not run individually; no worker/MCP/OpenAPI/UI surface touched.manualReviewLabelname honored case-insensitively;manualReviewLabel: nulldisables the guard; guard is scoped to approve/merge only (label/close actions unaffected).Safety
UI Evidencesection below. — N/A, no visible UI change.Notes
Filed as a same-day follow-up to today's stack-health pass (#3431, #3438, #3440, #3441, #3443, #3456) after a live incident on the self-hosted deployment (PR #3472 was held for manual review and then auto-merged by the same bot pass).