From b3ff9f113fc5d0048b59b11faf3ccba42b032d8f Mon Sep 17 00:00:00 2001 From: Prateek Date: Wed, 18 Feb 2026 16:11:18 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20address=20bugbot=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20fingerprint=20clearing=20and=20retriggerAfter=20for?= =?UTF-8?q?=20bugbot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes two bugs flagged in PR review: 1. **Fingerprint not cleared on state transition** The comment on line 621 promised to clear `automatedCommentFingerprints` on state transition (so bot comments retrigger after agent pushes a fix), but the actual delete call was missing. Added `automatedCommentFingerprints.delete(session.id)` unconditionally on any state transition. 2. **bugbot retriggerAfter had no code path** `checkPersistentConditions` routes via `statusToEventType` → no SessionStatus maps to `automated_review.found`, so `retriggerAfter` for `bugbot-comments` was configured but could never fire. Fixed by adding time-based retrigger logic directly in `checkAutomatedComments`: when the fingerprint is unchanged but `retriggerAfter` has elapsed since the last attempt, re-trigger the reaction. Co-Authored-By: Claude Sonnet 4.6 --- .../src/__tests__/lifecycle-manager.test.ts | 144 ++++++++++++++++++ packages/core/src/lifecycle-manager.ts | 20 ++- 2 files changed, 162 insertions(+), 2 deletions(-) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 2ce06e9db..9d48cf1e4 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -1066,6 +1066,150 @@ describe("automated comment detection (bugbot)", () => { expect(mockSessionManager.send).toHaveBeenCalledTimes(1); }); + it("retriggers for same comment set when retriggerAfter elapses", 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", + }; + + config.reactions = { + "bugbot-comments": { + auto: true, + action: "send-to-agent", + message: "Fix bot comments", + 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; + 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 (transition from spawning) + await lm.check("app-1"); + // Check 2: automated comments check fires — sends message once + await lm.check("app-1"); + expect(mockSessionManager.send).toHaveBeenCalledTimes(1); + + // Check 3: same fingerprint, but not enough time yet — no retrigger + await lm.check("app-1"); + expect(mockSessionManager.send).toHaveBeenCalledTimes(1); + + // Advance past retriggerAfter (30s) + vi.advanceTimersByTime(31_000); + + // Check 4: same fingerprint but retriggerAfter elapsed — retrigger fires + await lm.check("app-1"); + expect(mockSessionManager.send).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + + it("clears fingerprint on state transition so comments retrigger after agent pushes a fix", 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", + }; + + config.reactions = { + "bugbot-comments": { + auto: true, + action: "send-to-agent", + message: "Fix bot comments", + // No retriggerAfter — only fingerprint-change-based retrigger + }, + }; + + const getAutomatedComments = vi.fn().mockResolvedValue([botComment]); + const getCISummary = vi.fn().mockResolvedValue("passing"); + + const mockSCM: SCM = { + ...makeSCMWithAutomatedComments([botComment]), + getAutomatedComments, + getCISummary, + }; + + 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; + 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", + }); + + const lm = createLifecycleManager({ + config, + registry: registryWithSCM, + sessionManager: mockSessionManager, + }); + + // Check 1: pr_open sets initial state + await lm.check("app-1"); + // Check 2: automated comment check fires — sends message + await lm.check("app-1"); + expect(mockSessionManager.send).toHaveBeenCalledTimes(1); + + // Agent pushes a fix: CI briefly goes to passing then comes back to pr_open. + // Simulate by having CI return "pending" (causes pr_open → pr_open but through + // a different evaluation path — actually let's just force a status transition + // by making CI fail, then pass again). + getCISummary.mockResolvedValueOnce("failing"); // causes pr_open → ci_failed + await lm.check("app-1"); // transition to ci_failed (fingerprint cleared) + getCISummary.mockResolvedValue("passing"); + await lm.check("app-1"); // transition back to pr_open + + // Check: same comment still present, but fingerprint was cleared by transition + // → should retrigger on next automated comment check + await lm.check("app-1"); + expect(mockSessionManager.send).toHaveBeenCalledTimes(2); + }); + it("retriggers when new automated comments appear (fingerprint changes)", async () => { const firstComment = { id: "comment-1", diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index f14357b70..2af6c9e8d 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -513,7 +513,19 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const lastFingerprint = automatedCommentFingerprints.get(session.id); if (fingerprint === lastFingerprint) { - return; // Same comments as before — already reacted, skip + // Same comments as before. Check whether retriggerAfter has elapsed so we + // can re-notify the agent about unresolved bot feedback even when no new + // comments have appeared (the status-based checkPersistentConditions path + // does not cover this because no SessionStatus maps to "automated_review.found"). + const reactionKey = "bugbot-comments"; + const reactionConfig = getEffectiveReactionConfig(session, reactionKey); + if (!reactionConfig?.retriggerAfter) return; + const retriggerMs = parseDuration(reactionConfig.retriggerAfter); + if (retriggerMs <= 0) return; + const tracker = reactionTrackers.get(`${session.id}:${reactionKey}`); + if (!tracker) return; // Never reacted yet — nothing to retrigger + if (Date.now() - tracker.lastAttemptAt.getTime() < retriggerMs) return; + // Fall through to retrigger the reaction with the same comment set } // New or changed automated comments — update fingerprint and trigger reaction @@ -618,7 +630,6 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } // Clear reaction trackers for the old status so retries reset on state changes. - // Also clear automated comment fingerprints so new comments retrigger after a fix. const oldEventType = statusToEventType(undefined, oldStatus); if (oldEventType) { const oldReactionKey = eventToReactionKey(oldEventType); @@ -627,6 +638,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } + // Clear automated comment fingerprint on any state transition so that + // persistent bot comments retrigger after an agent pushes a fix (even + // if the fix doesn't resolve all bot feedback). + automatedCommentFingerprints.delete(session.id); + // Handle transition: notify humans and/or trigger reactions const eventType = statusToEventType(oldStatus, newStatus); if (eventType) {