fix: address bugbot review comments — escalated guard for bugbot retrigger + response body fix
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 <noreply@anthropic.com>
This commit is contained in:
parent
c4c024a914
commit
8005585fac
|
|
@ -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)", () => {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>;
|
||||
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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue