From 8005585facbe0fa4d4b4f9194c6b57e225f2cd02 Mon Sep 17 00:00:00 2001 From: Prateek Date: Wed, 18 Feb 2026 18:12:29 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20address=20bugbot=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20escalated=20guard=20for=20bugbot=20retrigger=20+=20?= =?UTF-8?q?response=20body=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs identified in re-review: 1. Missing escalated guard in checkAutomatedComments retrigger path The same-fingerprint + timer-elapsed retrigger path in checkAutomated- Comments was missing the tracker.escalated check that was already added to checkPersistentConditions. Without it, every retrigger cycle after escalateAfter elapses would re-escalate to human notification spam. 2. Response body consumed twice in merge error handler (Dashboard.tsx) res.json() and res.text() both consume the response body stream. If res.json() throws (e.g., proxy 502 returns HTML), the catch block's res.text() also fails, so users always saw "Unknown error". Fixed by reading res.text() first, then parsing with JSON.parse. Co-Authored-By: Claude Sonnet 4.6 --- .../src/__tests__/lifecycle-manager.test.ts | 77 +++++++++++++++++++ packages/core/src/lifecycle-manager.ts | 3 + packages/web/src/components/Dashboard.tsx | 9 ++- 3 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 9af97b24c..93ed585e3 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -1282,6 +1282,83 @@ describe("automated comment detection (bugbot)", () => { await lm.check("app-1"); expect(mockSessionManager.send).toHaveBeenCalledTimes(2); }); + + it("does not re-escalate bugbot-comments when tracker.escalated is true", async () => { + const botComment = { + id: "comment-1", + botName: "reviewdog", + body: "Error: unused import", + severity: "error" as const, + createdAt: new Date(), + url: "https://github.com/org/repo/pull/42#comment-1", + }; + + const mockNotifier: Notifier = { + name: "mock-notifier", + notify: vi.fn().mockResolvedValue(undefined), + }; + + config.reactions = { + "bugbot-comments": { + auto: true, + action: "send-to-agent", + message: "Fix bot comments", + // retries: 0 → escalate on first attempt + retries: 0, + retriggerAfter: "30s", + }, + }; + + const mockSCM = makeSCMWithAutomatedComments([botComment]); + const registryWithSCM: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return mockAgent; + if (slot === "scm") return mockSCM; + if (slot === "notifier") return mockNotifier; + return null; + }), + }; + + const session = makeSession({ status: "pr_open", pr: makePR() }); + vi.mocked(mockSessionManager.get).mockResolvedValue(session); + + writeMetadata(sessionsDir, "app-1", { + worktree: "/tmp", + branch: "main", + status: "pr_open", + project: "my-app", + }); + + vi.useFakeTimers(); + try { + const lm = createLifecycleManager({ + config, + registry: registryWithSCM, + sessionManager: mockSessionManager, + }); + + // Check 1: sets pr_open state + await lm.check("app-1"); + // Check 2: automated comments appear → retries:0 → escalates (1 notification) + await lm.check("app-1"); + expect(mockNotifier.notify).toHaveBeenCalledTimes(1); + + // Advance past retriggerAfter — without the guard this would re-escalate + vi.advanceTimersByTime(31_000); + + // Check 3: same fingerprint, timer elapsed, but tracker.escalated → no spam + await lm.check("app-1"); + expect(mockNotifier.notify).toHaveBeenCalledTimes(1); // still 1 + + vi.advanceTimersByTime(31_000); + await lm.check("app-1"); + expect(mockNotifier.notify).toHaveBeenCalledTimes(1); // still 1 + } finally { + vi.useRealTimers(); + } + }); }); describe("persistent retrigger (retriggerAfter)", () => { diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index 08197541d..621167d9f 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -534,6 +534,9 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan if (retriggerMs <= 0) return; const tracker = reactionTrackers.get(`${session.id}:${reactionKey}`); if (!tracker) return; // Never reacted yet — nothing to retrigger + // Once escalated, stop retrying — human has been notified (mirrors the guard + // in checkPersistentConditions to prevent infinite re-escalation spam). + if (tracker.escalated) return; if (Date.now() - tracker.lastAttemptAt.getTime() < retriggerMs) return; // Fall through to retrigger the reaction with the same comment set } diff --git a/packages/web/src/components/Dashboard.tsx b/packages/web/src/components/Dashboard.tsx index 96ca8ebfb..2ca44c263 100644 --- a/packages/web/src/components/Dashboard.tsx +++ b/packages/web/src/components/Dashboard.tsx @@ -74,8 +74,11 @@ export function Dashboard({ sessions, stats, orchestratorId, projectName }: Dash const res = await fetch(`/api/prs/${prNumber}/merge`, { method: "POST" }); if (!res.ok) { let errorMessage = `Failed to merge PR #${prNumber}`; + // Read body as text first — res.json() and res.text() both consume the stream, + // so calling res.text() in the catch after res.json() throws would always fail. + const text = await res.text().catch(() => ""); try { - const body = await res.json(); + const body = JSON.parse(text) as Record; if (body.error) { errorMessage += `:\n\n${body.error}`; } @@ -83,10 +86,10 @@ export function Dashboard({ sessions, stats, orchestratorId, projectName }: Dash errorMessage += `\n${body.detail}`; } if (body.blockers && Array.isArray(body.blockers) && body.blockers.length > 0) { - errorMessage += `\n\nBlockers:\n${body.blockers.map((b: string) => `• ${b}`).join("\n")}`; + errorMessage += `\n\nBlockers:\n${(body.blockers as string[]).map((b) => `• ${b}`).join("\n")}`; } } catch { - errorMessage += `:\n\n${await res.text().catch(() => "Unknown error")}`; + if (text) errorMessage += `:\n\n${text}`; } alert(errorMessage); }