From 17e6ee1c1bc97946467de0674cf04102d4f0c76e Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Fri, 10 Jul 2026 20:21:43 -0600 Subject: [PATCH] Add notification-runtime and mcp-review-handlers tests, fix split-pane flake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 37 unit tests covering notification-runtime.ts (web/Slack routing, ack, auto-archive) and mcp-review-handlers.ts (recheck context, completeReview prompt routing, resolution, feedback operations). Increase split-pane E2E timeout from 5s to 10s after reload — parallel test SSE events can delay localStorage atom hydration. Co-Authored-By: Claude Opus 4.6 --- apps/server/test/mcp-review-handlers.test.ts | 624 ++++++++++++++++++ apps/server/test/notification-runtime.test.ts | 353 ++++++++++ e2e/split-pane.spec.ts | 11 +- 3 files changed, 983 insertions(+), 5 deletions(-) create mode 100644 apps/server/test/mcp-review-handlers.test.ts create mode 100644 apps/server/test/notification-runtime.test.ts diff --git a/apps/server/test/mcp-review-handlers.test.ts b/apps/server/test/mcp-review-handlers.test.ts new file mode 100644 index 00000000..14686b65 --- /dev/null +++ b/apps/server/test/mcp-review-handlers.test.ts @@ -0,0 +1,624 @@ +import { describe, expect, it, vi } from "vitest"; + +import { createReviewHandlers } from "../src/server/mcp-review-handlers.js"; + +vi.mock("../src/shared/git/worktree.js", () => ({ + resolveHeadSha: vi.fn().mockResolvedValue("abc1234"), +})); + +vi.mock("../src/shared/git/git-context.js", () => ({ + resolveRepoRoot: vi.fn().mockResolvedValue("/repo"), + resolveWorktreeRoot: vi.fn().mockResolvedValue("/repo/worktree"), +})); + +vi.mock("../src/shared/git/base-ref.js", () => ({ + refreshRemoteBaseRef: vi.fn().mockResolvedValue(undefined), + resolveBaseRef: vi.fn().mockResolvedValue("origin/main"), +})); + +vi.mock("../src/shared/github/pr.js", () => ({ + getPrStatus: vi.fn().mockResolvedValue({ baseRefName: "main" }), +})); + +vi.mock("../src/shared/lib/run-command.js", () => ({ + runCommand: vi + .fn() + .mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }), +})); + +vi.mock("../src/personas/loader.js", () => ({ + loadPersonas: vi.fn().mockResolvedValue([]), + loadPersonaBySlug: vi.fn().mockResolvedValue(null), + assemblePersonaPrompt: vi.fn().mockReturnValue("assembled-prompt"), +})); + +vi.mock("../src/personas/review-diff.js", () => ({ + buildPersonaReviewDiff: vi.fn().mockResolvedValue({ diff: "diff-content" }), +})); + +vi.mock("../src/reviews/injection-prompts.js", () => ({ + buildParentRound1FeedbackPrompt: vi + .fn() + .mockReturnValue("round1-feedback-prompt"), + buildParentReviewCompletePrompt: vi + .fn() + .mockReturnValue("review-complete-prompt"), + buildPersonaKickoffPrompt: vi.fn().mockReturnValue("kickoff-prompt"), + buildReviewerRecheckCancelledPrompt: vi + .fn() + .mockReturnValue("recheck-cancelled-prompt"), + buildReviewerRecheckReadyPrompt: vi + .fn() + .mockReturnValue("recheck-ready-prompt"), +})); + +vi.mock("../src/agents/reviews.js", () => ({ + resolveReviewFeedbackItem: vi.fn(), + addThreadMessage: vi.fn(), + listFeedbackItemsForAgent: vi.fn().mockResolvedValue([]), +})); + +vi.mock("../src/agent-type-settings.js", () => ({ + CLI_AGENT_TYPES: ["claude", "codex", "opencode"], + getEnabledAgentTypes: vi + .fn() + .mockResolvedValue(["claude", "codex", "opencode"]), + isCliAgentType: vi.fn((t: string) => + ["claude", "codex", "opencode"].includes(t) + ), +})); + +function makeDeps(overrides: Record = {}) { + return { + pool: {}, + agentManager: { + getAgent: vi.fn().mockResolvedValue({ + id: "agt_parent", + name: "parent", + cwd: "/repo", + worktreePath: null, + worktreeBranch: null, + baseBranch: null, + type: "codex", + fullAccess: false, + reviewAgentType: null, + pins: [], + }), + listMedia: vi.fn().mockResolvedValue([]), + getPersonaReview: vi.fn().mockResolvedValue(null), + getReviewResolutions: vi.fn().mockResolvedValue([]), + listResolvedFeedbackForRound: vi.fn().mockResolvedValue([]), + completePersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "complete", + roundNumber: 1, + lastReviewedCommit: "abc1234", + }), + countFeedbackForAgent: vi.fn().mockResolvedValue(0), + submitReviewResolution: vi.fn().mockResolvedValue({ + review: { id: "rev_1", status: "awaiting_recheck" }, + resolution: { id: "res_1" }, + }), + cancelReviewRecheck: vi.fn().mockResolvedValue({ + review: { id: "rev_1", parentAgentId: "agt_parent" }, + transitioned: true, + }), + updatePersonaReviewStatus: vi.fn().mockResolvedValue({ + parentAgentId: "agt_parent", + }), + createAgent: vi.fn().mockResolvedValue({ + id: "agt_child", + name: "security-parent", + }), + createPersonaReview: vi.fn().mockResolvedValue(undefined), + ...((overrides.agentManager as Record) ?? {}), + }, + publishUiEvent: vi.fn(), + withStreamFlag: vi.fn( + (agent: Record) => + ({ ...agent, hasStream: false }) as never + ), + sendAgentPrompt: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; +} + +describe("createReviewHandlers", () => { + describe("getRecheckContext", () => { + it("returns null when no review exists", async () => { + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getRecheckContext("agt_child"); + expect(result).toBeNull(); + }); + + it("returns availability=ready with compareRange when awaiting_recheck", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getPersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "awaiting_recheck", + roundNumber: 1, + lastReviewedCommit: "aaa1111", + }), + getReviewResolutions: vi.fn().mockResolvedValue([ + { + roundNumber: 1, + summary: "Fixed issues", + resolutionCommit: "bbb2222", + submittedAt: "2026-07-10T00:00:00Z", + }, + ]), + listResolvedFeedbackForRound: vi + .fn() + .mockResolvedValue([{ id: 1, resolution: "fixed" }]), + }, + }); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getRecheckContext("agt_child"); + + expect(result).toMatchObject({ + availability: "ready", + reviewStatus: "awaiting_recheck", + persona: "security", + compareRange: "aaa1111...bbb2222", + gitDiffCommand: "git diff aaa1111...bbb2222", + resolutionSummary: "Fixed issues", + resolutions: [{ id: 1, resolution: "fixed" }], + }); + }); + + it("returns availability=cancelled when review is cancelled", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getPersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "cancelled", + roundNumber: 1, + lastReviewedCommit: "aaa1111", + }), + getReviewResolutions: vi.fn().mockResolvedValue([]), + }, + }); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getRecheckContext("agt_child"); + + expect(result!.availability).toBe("cancelled"); + }); + + it("returns availability=complete when round >= 2 and status is complete", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getPersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "complete", + roundNumber: 2, + lastReviewedCommit: "aaa1111", + }), + getReviewResolutions: vi.fn().mockResolvedValue([]), + }, + }); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getRecheckContext("agt_child"); + + expect(result!.availability).toBe("complete"); + }); + + it("returns availability=waiting_for_resolution for round 1 in-progress review", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getPersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "in_progress", + roundNumber: 1, + lastReviewedCommit: "aaa1111", + }), + getReviewResolutions: vi.fn().mockResolvedValue([]), + }, + }); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getRecheckContext("agt_child"); + + expect(result!.availability).toBe("waiting_for_resolution"); + }); + + it("returns null compareRange when commits are not valid SHAs", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getPersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "awaiting_recheck", + roundNumber: 1, + lastReviewedCommit: null, + }), + getReviewResolutions: vi.fn().mockResolvedValue([ + { + roundNumber: 1, + summary: "Fixed", + resolutionCommit: "not-a-sha!", + submittedAt: "2026-07-10T00:00:00Z", + }, + ]), + listResolvedFeedbackForRound: vi.fn().mockResolvedValue([]), + }, + }); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getRecheckContext("agt_child"); + + expect(result!.compareRange).toBeNull(); + expect(result!.gitDiffCommand).toBeNull(); + }); + }); + + describe("completeReview", () => { + it("sends round1 feedback prompt for mid-round-trip with non-clean result", async () => { + const { buildParentRound1FeedbackPrompt } = + await import("../src/reviews/injection-prompts.js"); + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + completePersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "complete", + roundNumber: 1, + lastReviewedCommit: "abc1234", + }), + countFeedbackForAgent: vi.fn().mockResolvedValue(3), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.completeReview("agt_child", { + verdict: "request_changes", + summary: "Found issues", + }); + + expect(buildParentRound1FeedbackPrompt).toHaveBeenCalledWith({ + persona: "security", + personaAgentId: "agt_child", + verdict: "request_changes", + feedbackCount: 3, + }); + expect(deps.sendAgentPrompt).toHaveBeenCalledWith( + "agt_parent", + "round1-feedback-prompt" + ); + }); + + it("sends review-complete prompt for round >= 2", async () => { + const { buildParentReviewCompletePrompt } = + await import("../src/reviews/injection-prompts.js"); + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + completePersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "complete", + roundNumber: 2, + lastReviewedCommit: "abc1234", + }), + countFeedbackForAgent: vi.fn().mockResolvedValue(1), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.completeReview("agt_child", { + verdict: "approve", + summary: "Looks good now", + }); + + expect(buildParentReviewCompletePrompt).toHaveBeenCalledWith({ + persona: "security", + personaAgentId: "agt_child", + verdict: "approve", + summary: "Looks good now", + feedbackCount: 1, + roundNumber: 2, + }); + expect(deps.sendAgentPrompt).toHaveBeenCalledWith( + "agt_parent", + "review-complete-prompt" + ); + }); + + it("sends review-complete prompt for clean approval in round 1", async () => { + const { buildParentReviewCompletePrompt } = + await import("../src/reviews/injection-prompts.js"); + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + completePersonaReview: vi.fn().mockResolvedValue({ + id: "rev_1", + parentAgentId: "agt_parent", + persona: "security", + status: "complete", + roundNumber: 1, + lastReviewedCommit: "abc1234", + }), + countFeedbackForAgent: vi.fn().mockResolvedValue(0), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.completeReview("agt_child", { + verdict: "approve", + summary: "All clear", + }); + + // Clean approval skips the round1 feedback prompt and uses review-complete + expect(buildParentReviewCompletePrompt).toHaveBeenCalled(); + expect(deps.sendAgentPrompt).toHaveBeenCalledWith( + "agt_parent", + "review-complete-prompt" + ); + }); + }); + + describe("submitResolution", () => { + it("sends recheck-ready prompt when review status is awaiting_recheck", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + submitReviewResolution: vi.fn().mockResolvedValue({ + review: { id: "rev_1", status: "awaiting_recheck" }, + resolution: { id: "res_1" }, + }), + getAgent: vi.fn().mockResolvedValue({ + id: "agt_parent", + name: "parent", + cwd: "/repo", + }), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.submitResolution("agt_parent", { + personaAgentId: "agt_child", + summary: "Fixed everything", + }); + + expect(deps.sendAgentPrompt).toHaveBeenCalledWith( + "agt_child", + "recheck-ready-prompt" + ); + }); + + it("does not send prompt when status is not awaiting_recheck", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + submitReviewResolution: vi.fn().mockResolvedValue({ + review: { id: "rev_1", status: "complete" }, + resolution: { id: "res_1" }, + }), + getAgent: vi.fn().mockResolvedValue({ + id: "agt_parent", + name: "parent", + cwd: "/repo", + }), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.submitResolution("agt_parent", { + personaAgentId: "agt_child", + summary: "Fixed everything", + }); + + expect(deps.sendAgentPrompt).not.toHaveBeenCalled(); + }); + }); + + describe("cancelRecheck", () => { + it("sends cancelled prompt when transition occurred", async () => { + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + + await handlers.cancelRecheck("agt_parent", { + personaAgentId: "agt_child", + reason: "Wrong approach", + }); + + expect(deps.sendAgentPrompt).toHaveBeenCalledWith( + "agt_child", + "recheck-cancelled-prompt" + ); + }); + + it("does not send prompt when no transition occurred", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + cancelReviewRecheck: vi.fn().mockResolvedValue({ + review: { id: "rev_1", parentAgentId: "agt_parent" }, + transitioned: false, + }), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await handlers.cancelRecheck("agt_parent", { + personaAgentId: "agt_child", + }); + + expect(deps.sendAgentPrompt).not.toHaveBeenCalled(); + }); + }); + + describe("updateReviewStatus", () => { + it("publishes upsert events for both child and parent", async () => { + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + + await handlers.updateReviewStatus("agt_child", { + status: "in_progress", + }); + + expect(deps.publishUiEvent).toHaveBeenCalledWith( + expect.objectContaining({ type: "agent.upsert" }) + ); + }); + }); + + describe("getParentContext", () => { + it("returns pins and media for parent agent", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getAgent: vi.fn().mockResolvedValue({ + id: "agt_parent", + name: "parent", + cwd: "/repo", + pins: [{ label: "URL", value: "http://x", type: "url" }], + }), + listMedia: vi.fn().mockResolvedValue([ + { + fileName: "shot.png", + filePath: "/tmp/shot.png", + description: "Screenshot", + source: "dispatch_share", + sizeBytes: 1024, + createdAt: "2026-07-10T00:00:00Z", + }, + ]), + }, + }); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.getParentContext("agt_parent"); + + expect(result.pins).toHaveLength(1); + expect(result.pins[0]).toEqual({ + label: "URL", + value: "http://x", + type: "url", + }); + expect(result.media).toHaveLength(1); + expect(result.media[0].fileName).toBe("shot.png"); + }); + + it("throws when parent not found", async () => { + const deps = makeDeps({ + agentManager: { + ...makeDeps().agentManager, + getAgent: vi.fn().mockResolvedValue(null), + }, + }); + const handlers = createReviewHandlers(deps as never); + + await expect(handlers.getParentContext("agt_missing")).rejects.toThrow( + "Parent agent not found." + ); + }); + }); + + describe("resolveReviewFeedback", () => { + it("throws when item not found", async () => { + const { resolveReviewFeedbackItem } = + await import("../src/agents/reviews.js"); + vi.mocked(resolveReviewFeedbackItem).mockResolvedValue(null); + + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + + await expect( + handlers.resolveReviewFeedback("agt_child", 99, "fixed") + ).rejects.toThrow("not found or not owned"); + }); + + it("publishes UI events on success", async () => { + const { resolveReviewFeedbackItem } = + await import("../src/agents/reviews.js"); + vi.mocked(resolveReviewFeedbackItem).mockResolvedValue({ + item: { id: 1, status: "resolved", resolution: "fixed" }, + reviewId: 10, + reviewStatus: "complete", + } as never); + + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.resolveReviewFeedback( + "agt_child", + 1, + "fixed" + ); + + expect(result.item.resolution).toBe("fixed"); + expect(deps.publishUiEvent).toHaveBeenCalledWith( + expect.objectContaining({ + type: "review_feedback.updated", + feedbackItemId: 1, + }) + ); + expect(deps.publishUiEvent).toHaveBeenCalledWith( + expect.objectContaining({ + type: "review.updated", + reviewId: 10, + }) + ); + }); + }); + + describe("addReviewThreadMessage", () => { + it("throws when item not found", async () => { + const { addThreadMessage } = await import("../src/agents/reviews.js"); + vi.mocked(addThreadMessage).mockResolvedValue(null); + + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + + await expect( + handlers.addReviewThreadMessage("agt_child", 99, "hello") + ).rejects.toThrow("not found or not owned"); + }); + + it("publishes UI event and returns message on success", async () => { + const { addThreadMessage } = await import("../src/agents/reviews.js"); + vi.mocked(addThreadMessage).mockResolvedValue({ + message: { + id: 5, + feedbackItemId: 1, + content: { body: "hello" }, + }, + reviewId: 10, + } as never); + + const deps = makeDeps(); + const handlers = createReviewHandlers(deps as never); + const result = await handlers.addReviewThreadMessage( + "agt_child", + 1, + "hello" + ); + + expect(result.message.content).toEqual({ body: "hello" }); + expect(result.reviewId).toBe(10); + expect(deps.publishUiEvent).toHaveBeenCalledWith( + expect.objectContaining({ + type: "review_feedback.updated", + feedbackItemId: 1, + }) + ); + }); + }); +}); diff --git a/apps/server/test/notification-runtime.test.ts b/apps/server/test/notification-runtime.test.ts new file mode 100644 index 00000000..893664be --- /dev/null +++ b/apps/server/test/notification-runtime.test.ts @@ -0,0 +1,353 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { createNotificationRuntime } from "../src/server/notification-runtime.js"; + +function makeDeps(overrides: Record = {}) { + let latestEventCb: ((agent: Record) => void) | null = null; + return { + agentManager: { + onLatestEvent: vi.fn((cb: (agent: Record) => void) => { + latestEventCb = cb; + }), + ...((overrides.agentManager as Record) ?? {}), + }, + jobService: { + getLatestRunForAgent: vi.fn().mockResolvedValue(null), + ...((overrides.jobService as Record) ?? {}), + }, + slackNotifier: { + onAgentEvent: vi.fn().mockResolvedValue(undefined), + shouldWebNotify: vi.fn().mockResolvedValue(null), + ...((overrides.slackNotifier as Record) ?? {}), + }, + uiEventBroker: { + publish: vi.fn(), + hasConnectedClient: vi.fn().mockReturnValue(false), + ...((overrides.uiEventBroker as Record) ?? {}), + }, + appLog: { + debug: vi.fn(), + warn: vi.fn(), + }, + webNotifyAckTimeoutMs: 5000, + autoArchiveJobAgent: vi.fn().mockResolvedValue(undefined), + get _latestEventCb() { + return latestEventCb; + }, + }; +} + +describe("createNotificationRuntime", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("registers an onLatestEvent callback", () => { + const deps = makeDeps(); + createNotificationRuntime(deps as never); + expect(deps.agentManager.onLatestEvent).toHaveBeenCalledWith( + expect.any(Function) + ); + }); + + describe("onLatestEvent handler", () => { + it("sends Slack notification for non-job agent when no web client", async () => { + const deps = makeDeps(); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_1", name: "my-agent" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.slackNotifier.shouldWebNotify).toHaveBeenCalledWith(agent); + expect(deps.slackNotifier.onAgentEvent).toHaveBeenCalledWith(agent); + }); + + it("sends Slack notification when shouldWebNotify returns null", async () => { + const deps = makeDeps({ + uiEventBroker: { + publish: vi.fn(), + hasConnectedClient: vi.fn().mockReturnValue(true), + }, + }); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_1", name: "my-agent" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.slackNotifier.onAgentEvent).toHaveBeenCalledWith(agent); + expect(deps.uiEventBroker.publish).not.toHaveBeenCalled(); + }); + + it("publishes web notification when client is connected and shouldWebNotify returns payload", async () => { + const webPayload = { title: "Test", body: "Hello" }; + const deps = makeDeps({ + slackNotifier: { + onAgentEvent: vi.fn(), + shouldWebNotify: vi.fn().mockResolvedValue(webPayload), + }, + uiEventBroker: { + publish: vi.fn(), + hasConnectedClient: vi.fn().mockReturnValue(true), + }, + }); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_1", name: "my-agent" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.uiEventBroker.publish).toHaveBeenCalledWith( + expect.objectContaining({ + type: "notification", + notificationId: expect.any(String), + title: "Test", + body: "Hello", + }) + ); + expect(deps.slackNotifier.onAgentEvent).not.toHaveBeenCalled(); + }); + + it("falls back to Slack after timeout if web notification is not acked", async () => { + const webPayload = { title: "Test", body: "Hello" }; + const deps = makeDeps({ + slackNotifier: { + onAgentEvent: vi.fn(), + shouldWebNotify: vi.fn().mockResolvedValue(webPayload), + }, + uiEventBroker: { + publish: vi.fn(), + hasConnectedClient: vi.fn().mockReturnValue(true), + }, + }); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_1", name: "my-agent" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.slackNotifier.onAgentEvent).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(5000); + + expect(deps.slackNotifier.onAgentEvent).toHaveBeenCalledWith(agent); + expect(deps.appLog.debug).toHaveBeenCalledWith( + expect.objectContaining({ notificationId: expect.any(String) }), + expect.stringContaining("not acked") + ); + }); + + it("skips Slack for job-agent that has a run", async () => { + const deps = makeDeps({ + jobService: { + getLatestRunForAgent: vi + .fn() + .mockResolvedValue({ id: "run_1", status: "completed" }), + }, + }); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_job1", name: "job-backup" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.jobService.getLatestRunForAgent).toHaveBeenCalledWith( + "agt_job1" + ); + expect(deps.slackNotifier.onAgentEvent).not.toHaveBeenCalled(); + }); + + it("sends Slack for job-agent with no run", async () => { + const deps = makeDeps({ + jobService: { + getLatestRunForAgent: vi.fn().mockResolvedValue(null), + }, + }); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_job1", name: "job-backup" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.slackNotifier.onAgentEvent).toHaveBeenCalledWith(agent); + }); + + it("catches and logs errors without rethrowing", async () => { + const deps = makeDeps({ + slackNotifier: { + onAgentEvent: vi.fn(), + shouldWebNotify: vi + .fn() + .mockRejectedValue(new Error("network error")), + }, + }); + createNotificationRuntime(deps as never); + + const agent = { id: "agt_1", name: "my-agent" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + expect(deps.appLog.warn).toHaveBeenCalledWith( + expect.objectContaining({ + err: expect.any(Error), + agentId: "agt_1", + }), + "Agent notification failed" + ); + }); + }); + + describe("ackWebNotification", () => { + it("returns true and clears timer for a pending notification", async () => { + const webPayload = { title: "Test", body: "Hello" }; + const deps = makeDeps({ + slackNotifier: { + onAgentEvent: vi.fn(), + shouldWebNotify: vi.fn().mockResolvedValue(webPayload), + }, + uiEventBroker: { + publish: vi.fn(), + hasConnectedClient: vi.fn().mockReturnValue(true), + }, + }); + const rt = createNotificationRuntime(deps as never); + + const agent = { id: "agt_1", name: "my-agent" }; + deps._latestEventCb!(agent); + await vi.advanceTimersByTimeAsync(0); + + const publishCall = ( + deps.uiEventBroker.publish as ReturnType + ).mock.calls[0][0]; + const notificationId = publishCall.notificationId; + + const result = rt.ackWebNotification(notificationId); + expect(result).toBe(true); + + // After timeout, Slack should NOT be called since we acked + await vi.advanceTimersByTimeAsync(5000); + expect(deps.slackNotifier.onAgentEvent).not.toHaveBeenCalled(); + }); + + it("returns false for unknown notification id", () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + expect(rt.ackWebNotification("unknown-id")).toBe(false); + }); + }); + + describe("publishJobChanged", () => { + it("publishes a job.changed event", () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + rt.publishJobChanged(); + expect(deps.uiEventBroker.publish).toHaveBeenCalledWith({ + type: "job.changed", + }); + }); + }); + + describe("maybeAutoArchiveJobRun", () => { + it("archives when status is terminal, agentId present, and autoArchive defaults true", async () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + const terminalStatuses = new Set(["completed", "failed"]); + + await rt.maybeAutoArchiveJobRun( + { status: "completed", agentId: "agt_1", config: {} }, + terminalStatuses + ); + + expect(deps.autoArchiveJobAgent).toHaveBeenCalledWith("agt_1"); + }); + + it("archives when config is null (autoArchive defaults true)", async () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + const terminalStatuses = new Set(["completed", "failed"]); + + await rt.maybeAutoArchiveJobRun( + { status: "failed", agentId: "agt_2", config: null }, + terminalStatuses + ); + + expect(deps.autoArchiveJobAgent).toHaveBeenCalledWith("agt_2"); + }); + + it("does not archive when autoArchive is false", async () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + const terminalStatuses = new Set(["completed", "failed"]); + + await rt.maybeAutoArchiveJobRun( + { + status: "completed", + agentId: "agt_1", + config: { autoArchive: false }, + }, + terminalStatuses + ); + + expect(deps.autoArchiveJobAgent).not.toHaveBeenCalled(); + }); + + it("does not archive when status is not terminal", async () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + const terminalStatuses = new Set(["completed", "failed"]); + + await rt.maybeAutoArchiveJobRun( + { status: "running", agentId: "agt_1", config: {} }, + terminalStatuses + ); + + expect(deps.autoArchiveJobAgent).not.toHaveBeenCalled(); + }); + + it("does not archive when agentId is null", async () => { + const deps = makeDeps(); + const rt = createNotificationRuntime(deps as never); + const terminalStatuses = new Set(["completed", "failed"]); + + await rt.maybeAutoArchiveJobRun( + { status: "completed", agentId: null, config: {} }, + terminalStatuses + ); + + expect(deps.autoArchiveJobAgent).not.toHaveBeenCalled(); + }); + }); + + describe("clearPendingWebNotifications", () => { + it("clears all pending timers", async () => { + const webPayload = { title: "Test", body: "Hello" }; + const deps = makeDeps({ + slackNotifier: { + onAgentEvent: vi.fn(), + shouldWebNotify: vi.fn().mockResolvedValue(webPayload), + }, + uiEventBroker: { + publish: vi.fn(), + hasConnectedClient: vi.fn().mockReturnValue(true), + }, + }); + const rt = createNotificationRuntime(deps as never); + + // Trigger two notifications + deps._latestEventCb!({ id: "agt_1", name: "agent-1" }); + deps._latestEventCb!({ id: "agt_2", name: "agent-2" }); + await vi.advanceTimersByTimeAsync(0); + + rt.clearPendingWebNotifications(); + + // After timeout, Slack should NOT be called since we cleared everything + await vi.advanceTimersByTimeAsync(5000); + expect(deps.slackNotifier.onAgentEvent).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/e2e/split-pane.spec.ts b/e2e/split-pane.spec.ts index 74472637..0392c0b6 100644 --- a/e2e/split-pane.spec.ts +++ b/e2e/split-pane.spec.ts @@ -56,9 +56,10 @@ test.describe("Split pane", () => { await page.reload({ waitUntil: "domcontentloaded" }); await waitForAppShell(page, agent.name); - // Split mode should be restored from localStorage. + // Split mode should be restored from localStorage. Use a longer timeout + // because parallel test SSE events can delay atom hydration after reload. await expect(page.getByTestId("unsplit-button")).toBeVisible({ - timeout: 5000, + timeout: 10_000, }); await expect(page.getByTestId("terminal-pane")).toBeVisible(); }); @@ -76,7 +77,7 @@ test.describe("Split pane", () => { await waitForAppShell(page, agent.name); const unsplitBtn = page.getByTestId("unsplit-button"); - await expect(unsplitBtn).toBeVisible({ timeout: 5000 }); + await expect(unsplitBtn).toBeVisible({ timeout: 10_000 }); // While split, both tabs are rendered as panes — tab bar is empty. await expect(page.getByTestId("center-tab-terminal")).not.toBeVisible(); @@ -108,7 +109,7 @@ test.describe("Split pane", () => { await page.reload({ waitUntil: "domcontentloaded" }); await waitForAppShell(page, agent.name); await expect(page.getByTestId("unsplit-button")).toBeVisible({ - timeout: 5000, + timeout: 10_000, }); // Terminal and Changes are both placed in split panes (left + right), @@ -139,7 +140,7 @@ test.describe("Split pane", () => { await waitForAppShell(page, agent.name); const unsplitBtn = page.getByTestId("unsplit-button"); - await expect(unsplitBtn).toBeVisible({ timeout: 5000 }); + await expect(unsplitBtn).toBeVisible({ timeout: 10_000 }); await unsplitBtn.click(); await expect(unsplitBtn).not.toBeVisible();