diff --git a/.changeset/stage4-pr-policy.md b/.changeset/stage4-pr-policy.md new file mode 100644 index 000000000..3887a4016 --- /dev/null +++ b/.changeset/stage4-pr-policy.md @@ -0,0 +1,6 @@ +--- +"@aoagents/ao-core": patch +"@aoagents/ao-web": patch +--- + +Decouple canonical session state from PR state so workers stay idle while waiting on reviews or merged/closed PR decisions, stop cleanup from auto-killing merged PR sessions, and make the dashboard/rendered labels follow canonical PR truth instead of inferring it from legacy lifecycle aliases. diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 006553729..f56df07bb 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -375,7 +375,7 @@ describe("check (single session)", () => { runtime: plugins.runtime, agent: plugins.agent, }); - vi.mocked(registryWithoutAgent.get).mockImplementation((slot: string, name?: string) => { + vi.mocked(registryWithoutAgent.get).mockImplementation((slot: string, _name?: string) => { if (slot === "runtime") return plugins.runtime; if (slot === "agent") return null; return null; @@ -584,6 +584,38 @@ describe("check (single session)", () => { expect(lm.getStates().get("app-1")).toBe("ci_failed"); }); + it("keeps canonical session state idle while waiting on external review", async () => { + const mockSCM = createMockSCM({ getReviewDecision: vi.fn().mockResolvedValue("pending") }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + const session = makeSession({ status: "pr_open", pr: makePR() }); + vi.mocked(mockSessionManager.get).mockResolvedValue(session); + + writeMetadata(env.sessionsDir, "app-1", { + worktree: "/tmp", + branch: session.branch ?? "main", + status: session.status, + project: "my-app", + pr: session.pr?.url, + runtimeHandle: session.runtimeHandle ? JSON.stringify(session.runtimeHandle) : undefined, + }); + + const lm = createLifecycleManager({ + config, + registry, + sessionManager: mockSessionManager, + }); + + await lm.check("app-1"); + + expect(lm.getStates().get("app-1")).toBe("review_pending"); + expect(session.lifecycle.session.state).toBe("idle"); + expect(session.lifecycle.session.reason).toBe("awaiting_external_review"); + }); + it("skips PR auto-detection when metadata disables it", async () => { const mockSCM = createMockSCM({ detectPR: vi.fn().mockResolvedValue(makePR()) }); const registry = createMockRegistry({ @@ -702,23 +734,77 @@ describe("check (single session)", () => { expect(lm.getStates().get("app-1")).toBe("merged"); }); - it("treats closed PRs as done when the canonical lifecycle marks them complete", async () => { + it("keeps closed PR sessions idle and emits a PR-closed notification", async () => { + const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("closed") }); + const notifier = createMockNotifier(); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + notifier, + }); + + const session = makeSession({ status: "pr_open", pr: makePR() }); + const lm = setupCheck("app-1", { + session, + registry, + configOverride: { + ...config, + notificationRouting: { + ...config.notificationRouting, + info: ["desktop"], + }, + }, + }); + + await lm.check("app-1"); + expect(lm.getStates().get("app-1")).toBe("idle"); + const meta = readMetadataRaw(env.sessionsDir, "app-1"); + expect(meta?.["status"]).toBe("idle"); + expect(meta?.["statePayload"]).toContain('"state":"closed"'); + expect(meta?.["statePayload"]).toContain('"reason":"pr_closed_waiting_decision"'); + expect(notifier.notify).toHaveBeenCalledWith(expect.objectContaining({ type: "pr.closed" })); + }); + + it("routes closed PR transitions through the pr-closed reaction key", async () => { + const notifier = createMockNotifier(); const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("closed") }); const registry = createMockRegistry({ runtime: plugins.runtime, agent: plugins.agent, scm: mockSCM, + notifier, }); + const session = makeSession({ status: "pr_open", pr: makePR() }); const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr: makePR() }), + session, registry, + configOverride: { + ...config, + reactions: { + ...config.reactions, + "pr-closed": { + auto: true, + action: "notify", + priority: "action", + }, + }, + notificationRouting: { + ...config.notificationRouting, + action: ["desktop"], + }, + }, }); await lm.check("app-1"); - expect(lm.getStates().get("app-1")).toBe("done"); - const meta = readMetadataRaw(env.sessionsDir, "app-1"); - expect(meta?.["status"]).toBe("done"); + + expect(notifier.notify).toHaveBeenCalledWith( + expect.objectContaining({ + type: "reaction.triggered", + data: expect.objectContaining({ reactionKey: "pr-closed" }), + }), + ); }); it("detects mergeable when approved + CI green", async () => { @@ -1563,6 +1649,42 @@ describe("reactions", () => { expect(metadata?.["lastMergeConflictDispatched"]).toBeFalsy(); }); + it("clears merge conflict tracking when PR is closed", async () => { + config.reactions = { + "merge-conflicts": { + auto: true, + action: "send-to-agent", + message: "Resolve merge conflicts.", + }, + }; + + const getMergeability = vi.fn(); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("closed"), + getMergeability, + }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + + const lm = setupCheck("app-1", { + session: makeSession({ + status: "pr_open", + pr: makePR(), + metadata: { lastMergeConflictDispatched: "true" }, + }), + registry, + }); + + await lm.check("app-1"); + + const metadata = readMetadataRaw(env.sessionsDir, "app-1"); + expect(metadata?.["lastMergeConflictDispatched"]).toBeFalsy(); + expect(getMergeability).not.toHaveBeenCalled(); + }); + it("notifies humans on significant transitions without reaction config", async () => { const notifier = createMockNotifier(); const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); @@ -2068,6 +2190,49 @@ describe("rate limiting optimizations", () => { await lm.check("app-1"); expect(getPendingMock).toHaveBeenCalledTimes(1); }); + + it("clears review backlog tracking when PR is closed", async () => { + const getPendingMock = vi.fn(); + const getAutomatedMock = vi.fn(); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("closed"), + getPendingComments: getPendingMock, + getAutomatedComments: getAutomatedMock, + }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + + const lm = setupCheck("app-1", { + session: makeSession({ + status: "pr_open", + pr: makePR(), + metadata: { + lastPendingReviewFingerprint: "fingerprint", + lastPendingReviewDispatchHash: "dispatch", + lastPendingReviewDispatchAt: "2025-01-01T00:00:00.000Z", + lastAutomatedReviewFingerprint: "auto-fingerprint", + lastAutomatedReviewDispatchHash: "auto-dispatch", + lastAutomatedReviewDispatchAt: "2025-01-01T00:00:00.000Z", + }, + }), + registry, + }); + + await lm.check("app-1"); + + const metadata = readMetadataRaw(env.sessionsDir, "app-1"); + expect(metadata?.["lastPendingReviewFingerprint"]).toBeFalsy(); + expect(metadata?.["lastPendingReviewDispatchHash"]).toBeFalsy(); + expect(metadata?.["lastPendingReviewDispatchAt"]).toBeFalsy(); + expect(metadata?.["lastAutomatedReviewFingerprint"]).toBeFalsy(); + expect(metadata?.["lastAutomatedReviewDispatchHash"]).toBeFalsy(); + expect(metadata?.["lastAutomatedReviewDispatchAt"]).toBeFalsy(); + expect(getPendingMock).not.toHaveBeenCalled(); + expect(getAutomatedMock).not.toHaveBeenCalled(); + }); }); describe("summary pinning", () => { it("pins first quality summary when pinnedSummary not set", async () => { diff --git a/packages/core/src/__tests__/lifecycle-state.test.ts b/packages/core/src/__tests__/lifecycle-state.test.ts new file mode 100644 index 000000000..f8168784f --- /dev/null +++ b/packages/core/src/__tests__/lifecycle-state.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; +import { createInitialCanonicalLifecycle, deriveLegacyStatus } from "../lifecycle-state.js"; + +function createOpenPRLifecycle() { + const lifecycle = createInitialCanonicalLifecycle("worker", new Date("2025-01-01T00:00:00Z")); + lifecycle.session.startedAt = lifecycle.session.lastTransitionAt; + lifecycle.pr.state = "open"; + lifecycle.pr.reason = "review_pending"; + lifecycle.pr.number = 42; + lifecycle.pr.url = "https://github.com/org/repo/pull/42"; + lifecycle.pr.lastObservedAt = lifecycle.session.lastTransitionAt; + lifecycle.runtime.state = "alive"; + lifecycle.runtime.reason = "process_running"; + return lifecycle; +} + +describe("deriveLegacyStatus", () => { + it("preserves urgent session states ahead of open PR aliases", () => { + const needsInput = createOpenPRLifecycle(); + needsInput.session.state = "needs_input"; + needsInput.session.reason = "awaiting_user_input"; + + const stuck = createOpenPRLifecycle(); + stuck.session.state = "stuck"; + stuck.session.reason = "probe_failure"; + + const terminated = createOpenPRLifecycle(); + terminated.session.state = "terminated"; + terminated.session.reason = "manually_killed"; + + expect(deriveLegacyStatus(needsInput)).toBe("needs_input"); + expect(deriveLegacyStatus(stuck)).toBe("stuck"); + expect(deriveLegacyStatus(terminated)).toBe("terminated"); + }); + + it("preserves prior terminal legacy statuses for terminated sessions", () => { + const terminated = createOpenPRLifecycle(); + terminated.session.state = "terminated"; + terminated.session.reason = "manually_killed"; + + expect(deriveLegacyStatus(terminated, "killed")).toBe("killed"); + expect(deriveLegacyStatus(terminated, "cleanup")).toBe("cleanup"); + expect(deriveLegacyStatus(terminated, "errored")).toBe("errored"); + }); + + it("keeps PR-oriented aliases for idle workers with open PRs", () => { + const reviewPending = createOpenPRLifecycle(); + reviewPending.session.state = "idle"; + reviewPending.session.reason = "awaiting_external_review"; + + const mergeReady = createOpenPRLifecycle(); + mergeReady.session.state = "idle"; + mergeReady.session.reason = "awaiting_external_review"; + mergeReady.pr.reason = "merge_ready"; + + expect(deriveLegacyStatus(reviewPending)).toBe("review_pending"); + expect(deriveLegacyStatus(mergeReady)).toBe("mergeable"); + }); +}); diff --git a/packages/core/src/__tests__/plugin-integration.test.ts b/packages/core/src/__tests__/plugin-integration.test.ts index 6b64aef1c..663575a4f 100644 --- a/packages/core/src/__tests__/plugin-integration.test.ts +++ b/packages/core/src/__tests__/plugin-integration.test.ts @@ -392,7 +392,7 @@ describe("plugin integration", () => { // ------------------------------------------------------------------------- describe("SessionManager + SCM", () => { - it("cleanup() calls scm-github getPRState() and kills merged PR sessions", async () => { + it("cleanup() calls scm-github getPRState() but keeps merged PR sessions alive", async () => { const registry = createTestRegistry(); const sm = createSessionManager({ config, registry }); @@ -412,7 +412,7 @@ describe("plugin integration", () => { const result = await sm.cleanup("my-app"); - expect(result.killed).toContain("app-1"); + expect(result.skipped).toContain("app-1"); // Verify gh CLI was called for PR state check expect(ghMock).toHaveBeenCalledWith( "gh", diff --git a/packages/core/src/__tests__/session-manager/lifecycle.test.ts b/packages/core/src/__tests__/session-manager/lifecycle.test.ts index 7a24277db..6a656cf5f 100644 --- a/packages/core/src/__tests__/session-manager/lifecycle.test.ts +++ b/packages/core/src/__tests__/session-manager/lifecycle.test.ts @@ -229,11 +229,11 @@ describe("kill", () => { }); describe("cleanup", () => { - it("kills sessions with merged PRs", async () => { + it("kills sessions with closed PRs", async () => { const mockSCM: SCM = { name: "mock-scm", detectPR: vi.fn(), - getPRState: vi.fn().mockResolvedValue("merged"), + getPRState: vi.fn().mockResolvedValue("closed"), mergePR: vi.fn(), closePR: vi.fn(), getCIChecks: vi.fn(), @@ -272,7 +272,7 @@ describe("cleanup", () => { expect(result.skipped).toHaveLength(0); }); - it("deletes mapped OpenCode session during cleanup", async () => { + it("deletes mapped OpenCode session during cleanup for closed PRs", async () => { const deleteLogPath = join(tmpDir, "opencode-delete.log"); const mockBin = installMockOpencode(tmpDir, "[]", deleteLogPath); process.env.PATH = `${mockBin}:${originalPath ?? ""}`; @@ -280,7 +280,7 @@ describe("cleanup", () => { const mockSCM: SCM = { name: "mock-scm", detectPR: vi.fn(), - getPRState: vi.fn().mockResolvedValue("merged"), + getPRState: vi.fn().mockResolvedValue("closed"), mergePR: vi.fn(), closePR: vi.fn(), getCIChecks: vi.fn(), @@ -322,14 +322,14 @@ describe("cleanup", () => { expect(deleteLog).toContain("session delete ses_cleanup"); }); - it("treats missing mapped OpenCode session as already cleaned", async () => { + it("treats missing mapped OpenCode session as already cleaned for closed PRs", async () => { const mockBin = installMockOpencodeWithNotFoundDelete(tmpDir, "[]"); process.env.PATH = `${mockBin}:${originalPath ?? ""}`; const mockSCM: SCM = { name: "mock-scm", detectPR: vi.fn(), - getPRState: vi.fn().mockResolvedValue("merged"), + getPRState: vi.fn().mockResolvedValue("closed"), mergePR: vi.fn(), closePR: vi.fn(), getCIChecks: vi.fn(), @@ -392,7 +392,7 @@ describe("cleanup", () => { expect(result.killed).toContain("app-6"); const deleteLog = readFileSync(deleteLogPath, "utf-8"); expect(deleteLog).toContain("session delete ses_archived"); - }); + }, 15_000); it("does not skip archived cleanup for matching session IDs in other projects", async () => { const deleteLogPath = join(tmpDir, "opencode-delete-archived-cross-project.log"); diff --git a/packages/core/src/__tests__/session-manager/spawn.test.ts b/packages/core/src/__tests__/session-manager/spawn.test.ts index 222039714..968ce9149 100644 --- a/packages/core/src/__tests__/session-manager/spawn.test.ts +++ b/packages/core/src/__tests__/session-manager/spawn.test.ts @@ -576,7 +576,7 @@ describe("spawn", () => { const metadata = readMetadataRaw(sessionsDir, session.id); expect(metadata?.["opencodeSessionId"]).toBeUndefined(); - }); + }, 15_000); it("throws for unknown project", async () => { const sm = createSessionManager({ config, registry: mockRegistry }); @@ -1865,7 +1865,7 @@ describe("spawn", () => { }), }), ); - }); + }, 15_000); it("throws for unknown project", async () => { const sm = createSessionManager({ config, registry: mockRegistry }); diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index c5b3f7eed..862355b3c 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -548,6 +548,13 @@ function validateProjectUniqueness(config: OrchestratorConfig): void { /** Apply default reactions */ function applyDefaultReactions(config: OrchestratorConfig): OrchestratorConfig { const defaults: Record = { + "pr-closed": { + auto: true, + action: "notify", + priority: "action", + message: + "A PR was closed without merging. Decide whether to learn from the closure, resume the work, or terminate the session.", + }, "ci-failed": { auto: true, action: "send-to-agent", diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index 3d567e41b..f37b4375d 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -142,9 +142,24 @@ function statusToEventType(_from: SessionStatus | undefined, to: SessionStatus): } } +function prStateToEventType( + from: Session["lifecycle"]["pr"]["state"], + to: Session["lifecycle"]["pr"]["state"], +): EventType | null { + if (from === to) return null; + switch (to) { + case "closed": + return "pr.closed"; + default: + return null; + } +} + /** Map event type to reaction config key. */ function eventToReactionKey(eventType: EventType): string | null { switch (eventType) { + case "pr.closed": + return "pr-closed"; case "ci.failing": return "ci-failed"; case "review.changes_requested": @@ -683,8 +698,8 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan if (cachedData.state === PR_STATE.CLOSED) { lifecycle.pr.state = "closed"; lifecycle.pr.reason = "closed_unmerged"; - setSessionState("done", "research_complete"); - return commit(SESSION_STATUS.DONE, "pr_closed", 0); + setSessionState("idle", "pr_closed_waiting_decision"); + return commit(SESSION_STATUS.IDLE, "pr_closed", 0); } lifecycle.pr.state = "open"; @@ -701,18 +716,18 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan if (cachedData.reviewDecision === "approved" || cachedData.reviewDecision === "none") { if (cachedData.mergeable) { lifecycle.pr.reason = "merge_ready"; - setSessionState("working", "awaiting_external_review"); + setSessionState("idle", "awaiting_external_review"); return commit(SESSION_STATUS.MERGEABLE, "merge_ready", 0); } if (cachedData.reviewDecision === "approved") { lifecycle.pr.reason = "approved"; - setSessionState("working", "awaiting_external_review"); + setSessionState("idle", "awaiting_external_review"); return commit(SESSION_STATUS.APPROVED, "review_approved", 0); } } if (cachedData.reviewDecision === "pending") { lifecycle.pr.reason = "review_pending"; - setSessionState("working", "awaiting_external_review"); + setSessionState("idle", "awaiting_external_review"); return commit(SESSION_STATUS.REVIEW_PENDING, "review_pending", 0); } @@ -723,7 +738,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } lifecycle.pr.reason = "in_progress"; - setSessionState("working", "pr_created"); + setSessionState("idle", "pr_created"); return commit(SESSION_STATUS.PR_OPEN, "pr_open", 0); } @@ -737,8 +752,8 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan if (prState === PR_STATE.CLOSED) { lifecycle.pr.state = "closed"; lifecycle.pr.reason = "closed_unmerged"; - setSessionState("done", "research_complete"); - return commit(SESSION_STATUS.DONE, "pr_closed", 0); + setSessionState("idle", "pr_closed_waiting_decision"); + return commit(SESSION_STATUS.IDLE, "pr_closed", 0); } lifecycle.pr.state = "open"; @@ -759,18 +774,18 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const mergeReady = await scm.getMergeability(session.pr); if (mergeReady.mergeable) { lifecycle.pr.reason = "merge_ready"; - setSessionState("working", "awaiting_external_review"); + setSessionState("idle", "awaiting_external_review"); return commit(SESSION_STATUS.MERGEABLE, "merge_ready", 0); } if (reviewDecision === "approved") { lifecycle.pr.reason = "approved"; - setSessionState("working", "awaiting_external_review"); + setSessionState("idle", "awaiting_external_review"); return commit(SESSION_STATUS.APPROVED, "review_approved", 0); } } if (reviewDecision === "pending") { lifecycle.pr.reason = "review_pending"; - setSessionState("working", "awaiting_external_review"); + setSessionState("idle", "awaiting_external_review"); return commit(SESSION_STATUS.REVIEW_PENDING, "review_pending", 0); } @@ -781,7 +796,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } lifecycle.pr.reason = "in_progress"; - setSessionState("working", "pr_created"); + setSessionState("idle", "pr_created"); return commit(SESSION_STATUS.PR_OPEN, "pr_open", 0); } catch { // Keep current status on SCM failure. @@ -1018,7 +1033,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const humanReactionKey = "changes-requested"; const automatedReactionKey = "bugbot-comments"; - if (TERMINAL_STATUSES.has(newStatus)) { + if (TERMINAL_STATUSES.has(newStatus) || session.lifecycle.pr.state !== "open") { clearReactionTracker(session.id, humanReactionKey); clearReactionTracker(session.id, automatedReactionKey); lastReviewBacklogCheckAt.delete(session.id); @@ -1350,8 +1365,8 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const conflictReactionKey = "merge-conflicts"; - // Clear tracking when PR is closed/merged - if (newStatus === "merged" || newStatus === "killed") { + // Clear tracking when PR is no longer open. + if (session.lifecycle.pr.state !== "open" || newStatus === "killed") { clearReactionTracker(session.id, conflictReactionKey); updateSessionMetadata(session, { lastMergeConflictDispatched: "", @@ -1463,6 +1478,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const tracked = states.get(session.id); const oldStatus = tracked ?? ((session.metadata?.["status"] as SessionStatus | undefined) || session.status); + const previousPRState = session.lifecycle.pr.state; const assessment = await determineStatus(session); const newStatus = assessment.status; const lifecycleChanged = session.metadata["statePayload"] !== JSON.stringify(session.lifecycle); @@ -1572,6 +1588,37 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } + const prEventType = prStateToEventType(previousPRState, session.lifecycle.pr.state); + if (prEventType) { + let reactionHandledNotify = false; + const reactionKey = eventToReactionKey(prEventType); + + if (reactionKey) { + const reactionConfig = getReactionConfigForSession(session, reactionKey); + if (reactionConfig && reactionConfig.action) { + if (reactionConfig.auto !== false || reactionConfig.action === "notify") { + await executeReaction(session.id, session.projectId, reactionKey, reactionConfig); + reactionHandledNotify = true; + } + } + } + + if (!reactionHandledNotify) { + const prEvent = createEvent(prEventType, { + sessionId: session.id, + projectId: session.projectId, + message: `${session.id}: PR ${previousPRState} → ${session.lifecycle.pr.state}`, + data: { + oldPRState: previousPRState, + newPRState: session.lifecycle.pr.state, + prNumber: session.lifecycle.pr.number, + prUrl: session.lifecycle.pr.url, + }, + }); + await notifyHuman(prEvent, inferPriority(prEventType)); + } + } + // Pin first quality summary for title stability if ( session.agentInfo?.summary && diff --git a/packages/core/src/lifecycle-state.ts b/packages/core/src/lifecycle-state.ts index a347d768b..9b1266bea 100644 --- a/packages/core/src/lifecycle-state.ts +++ b/packages/core/src/lifecycle-state.ts @@ -252,6 +252,16 @@ export function deriveLegacyStatus( lifecycle: CanonicalSessionLifecycle, previousStatus: SessionStatus = "working", ): SessionStatus { + if ( + lifecycle.session.state === "terminated" && + (previousStatus === "cleanup" || + previousStatus === "errored" || + previousStatus === "killed" || + previousStatus === "terminated") + ) { + return previousStatus; + } + switch (lifecycle.session.state) { case "not_started": return "spawning"; @@ -263,17 +273,28 @@ export function deriveLegacyStatus( return "done"; case "terminated": return "terminated"; - case "idle": - return lifecycle.pr.state === "merged" ? "merged" : "idle"; case "detecting": return "detecting"; + default: + break; + } + + if (lifecycle.pr.state === "merged") { + return "merged"; + } + if (lifecycle.pr.state === "open") { + if (lifecycle.pr.reason === "ci_failing") return "ci_failed"; + if (lifecycle.pr.reason === "changes_requested") return "changes_requested"; + if (lifecycle.pr.reason === "review_pending") return "review_pending"; + if (lifecycle.pr.reason === "approved") return "approved"; + if (lifecycle.pr.reason === "merge_ready") return "mergeable"; + return "pr_open"; + } + + switch (lifecycle.session.state) { + case "idle": + return "idle"; case "working": - if (lifecycle.pr.reason === "ci_failing") return "ci_failed"; - if (lifecycle.pr.reason === "changes_requested") return "changes_requested"; - if (lifecycle.pr.reason === "review_pending") return "review_pending"; - if (lifecycle.pr.reason === "approved") return "approved"; - if (lifecycle.pr.reason === "merge_ready") return "mergeable"; - if (lifecycle.pr.state === "open") return "pr_open"; return "working"; } } diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index bbfe5ab6e..a8de6c6c4 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -1872,11 +1872,11 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM const plugins = resolvePlugins(project); let shouldKill = false; - // Check if PR is merged + // Check if PR was closed without merging. if (session.pr && plugins.scm) { try { const prState = await plugins.scm.getPRState(session.pr); - if (prState === PR_STATE.MERGED || prState === PR_STATE.CLOSED) { + if (prState === PR_STATE.CLOSED) { shouldKill = true; } } catch { diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index fd9b23bc1..bab548771 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -41,6 +41,7 @@ export type CanonicalSessionReason = | "agent_acknowledged" | "task_in_progress" | "pr_created" + | "pr_closed_waiting_decision" | "fixing_ci" | "resolving_review_comments" | "awaiting_user_input" diff --git a/packages/web/src/components/SessionCard.tsx b/packages/web/src/components/SessionCard.tsx index 04181e43b..e32740f8f 100644 --- a/packages/web/src/components/SessionCard.tsx +++ b/packages/web/src/components/SessionCard.tsx @@ -53,6 +53,25 @@ function getDoneStatusInfo(session: DashboardSession): { }; } + if (prState === "closed") { + return { + label: "closed", + pillClass: "done-status-pill--exited", + icon: ( + + + + + ), + }; + } + if (status === "killed" || status === "terminated") { return { label: status, diff --git a/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts b/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts index 7ceb794d8..b212213f6 100644 --- a/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts +++ b/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts @@ -60,15 +60,15 @@ describe("getDashboardPageData fast path", () => { hoisted.enrichSessionsMetadataFastMock.mockResolvedValue(undefined); }); - it("runs fast enrichment, uses cache-only PR hydration, and infers merged/closed state for terminal cache misses even without SCM", async () => { + it("runs fast enrichment, uses cache-only PR hydration, and preserves canonical PR state on cache misses even without SCM", async () => { const noPrCore = { id: "session-no-pr", status: "working", pr: null }; - const closedCore = { id: "session-closed", status: "killed", pr: { number: 2 } }; - const mergedCore = { id: "session-merged", status: "merged", pr: { number: 3 } }; + const closedCore = { id: "session-closed", status: "idle", pr: { number: 2 } }; + const mergedCore = { id: "session-merged", status: "idle", pr: { number: 3 } }; const allSessions = [noPrCore, closedCore, mergedCore]; const dashboardNoPr = { id: "session-no-pr", pr: null }; - const dashboardClosed = { id: "session-closed", pr: { state: "open", enriched: false } }; - const dashboardMerged = { id: "session-merged", pr: { state: "open", enriched: false } }; + const dashboardClosed = { id: "session-closed", pr: { state: "closed", enriched: false } }; + const dashboardMerged = { id: "session-merged", pr: { state: "merged", enriched: false } }; hoisted.getServicesMock.mockResolvedValue({ config: { projects: { docs: { id: "docs" } } }, diff --git a/packages/web/src/lib/__tests__/serialize.test.ts b/packages/web/src/lib/__tests__/serialize.test.ts index 749734582..a2dfa7d47 100644 --- a/packages/web/src/lib/__tests__/serialize.test.ts +++ b/packages/web/src/lib/__tests__/serialize.test.ts @@ -149,6 +149,27 @@ describe("sessionToDashboard", () => { }); }); + it("should seed dashboard PR state from canonical lifecycle truth", () => { + const lifecycle = createInitialCanonicalLifecycle("worker", new Date("2025-01-01T00:00:00Z")); + lifecycle.session.state = "idle"; + lifecycle.session.reason = "merged_waiting_decision"; + lifecycle.session.startedAt = lifecycle.session.lastTransitionAt; + lifecycle.pr.state = "merged"; + lifecycle.pr.reason = "merged"; + lifecycle.runtime.state = "alive"; + lifecycle.runtime.reason = "process_running"; + + const coreSession = createCoreSession({ + status: "idle", + lifecycle, + pr: createPRInfo(), + }); + + const dashboard = sessionToDashboard(coreSession); + + expect(dashboard.pr?.state).toBe("merged"); + }); + it("should use agentInfo summary with summaryIsFallback false", () => { const coreSession = createCoreSession({ agentInfo: { diff --git a/packages/web/src/lib/dashboard-page-data.ts b/packages/web/src/lib/dashboard-page-data.ts index efb6e2419..52b6354d8 100644 --- a/packages/web/src/lib/dashboard-page-data.ts +++ b/packages/web/src/lib/dashboard-page-data.ts @@ -1,7 +1,7 @@ import "server-only"; import { cache } from "react"; -import { TERMINAL_STATUSES, type DashboardSession, type DashboardOrchestratorLink } from "@/lib/types"; +import { type DashboardSession, type DashboardOrchestratorLink } from "@/lib/types"; import { getServices, getSCM } from "@/lib/services"; import { sessionToDashboard, @@ -73,8 +73,7 @@ export const getDashboardPageData = cache(async function getDashboardPageData(pr FAST_METADATA_ENRICH_TIMEOUT_MS, ); - // PR cache hits only (in-memory lookup, no SCM API calls) - // TERMINAL_STATUSES includes merged, killed, cleanup, done, terminated, errored + // PR cache hits only (in-memory lookup, no SCM API calls). for (let i = 0; i < coreSessions.length; i++) { const core = coreSessions[i]; if (!core.pr) continue; @@ -83,17 +82,6 @@ export const getDashboardPageData = cache(async function getDashboardPageData(pr if (scm) { await enrichSessionPR(pageData.sessions[i], scm, core.pr, { cacheOnly: true }); } - - // For cache-miss PRs, infer terminal PR state from lifecycle status - // to avoid showing merged/closed PRs as "open" until client refresh. - const sessionPR = pageData.sessions[i].pr; - if (sessionPR && !sessionPR.enriched && TERMINAL_STATUSES.has(core.status)) { - if (core.status === "merged") { - sessionPR.state = "merged"; - } else if (core.status === "killed") { - sessionPR.state = "closed"; - } - } } } catch { pageData.sessions = []; diff --git a/packages/web/src/lib/serialize.ts b/packages/web/src/lib/serialize.ts index e6d569f37..4bbdc49da 100644 --- a/packages/web/src/lib/serialize.ts +++ b/packages/web/src/lib/serialize.ts @@ -75,7 +75,12 @@ export function sessionToDashboard(session: Session): DashboardSession { summaryIsFallback: agentSummary ? (session.agentInfo?.summaryIsFallback ?? false) : false, createdAt: session.createdAt.toISOString(), lastActivityAt: session.lastActivityAt.toISOString(), - pr: session.pr ? basicPRToDashboard(session.pr) : null, + pr: session.pr + ? { + ...basicPRToDashboard(session.pr), + state: normalizeDashboardPRState(session.lifecycle.pr.state), + } + : null, metadata: session.metadata, }; } @@ -137,6 +142,17 @@ function basicPRToDashboard(pr: PRInfo): DashboardPR { }; } +function normalizeDashboardPRState(state: Session["lifecycle"]["pr"]["state"]): DashboardPR["state"] { + switch (state) { + case "merged": + return "merged"; + case "closed": + return "closed"; + default: + return "open"; + } +} + /** * Enrich a DashboardSession's PR with live data from the SCM plugin. * Uses cache to reduce API calls and handles rate limit errors gracefully. diff --git a/packages/web/src/lib/services.ts b/packages/web/src/lib/services.ts index cccae0125..bdd5541a8 100644 --- a/packages/web/src/lib/services.ts +++ b/packages/web/src/lib/services.ts @@ -129,7 +129,9 @@ async function labelIssuesForVerification( ): Promise { const mergedSessions = sessions.filter( (s) => - s.status === "merged" && s.issueId && !processedIssues.has(`${s.projectId}:${s.issueId}`), + s.lifecycle.pr.state === "merged" && + s.issueId && + !processedIssues.has(`${s.projectId}:${s.issueId}`), ); for (const session of mergedSessions) {