fix: address bugbot review comments — fingerprint clearing and retriggerAfter for bugbot
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 <noreply@anthropic.com>
This commit is contained in:
parent
461be2b47e
commit
b3ff9f113f
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue