From 3be26cb97ff1410a76270c6bfb48d27cde356cf8 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Fri, 27 Mar 2026 15:48:07 +0900 Subject: [PATCH] fix(#2732): enhance notification for failed/crashed subagent tasks - completedTaskSummaries now includes status and error info - notifyParentSession: noReply=false for failed tasks so parent reacts - Batch notification distinguishes successful vs failed/cancelled tasks - notification-template updated to show task errors - task-poller: session-gone tests (85 new lines) - CI: add Bun shim to PATH for legacy plugin migration tests --- .github/workflows/ci.yml | 3 + .github/workflows/publish.yml | 3 + .../background-task-notification-template.ts | 40 +++++++-- src/features/background-agent/manager.test.ts | 12 +-- src/features/background-agent/manager.ts | 52 +++++++++--- .../background-agent/task-poller.test.ts | 85 +++++++++++++++++-- .../legacy-plugin-toast/auto-migrate.test.ts | 14 ++- src/hooks/legacy-plugin-toast/auto-migrate.ts | 15 +++- 8 files changed, 178 insertions(+), 46 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe8a0f11b..4696a7b74 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,6 +67,8 @@ jobs: bun test src/shared/opencode-message-dir.test.ts # session-recovery mock isolation (recover-tool-result-missing mocks ./storage) bun test src/hooks/session-recovery/recover-tool-result-missing.test.ts + # legacy-plugin-toast mock isolation (hook.test.ts mocks ./auto-migrate) + bun test src/hooks/legacy-plugin-toast/hook.test.ts - name: Run remaining tests run: | @@ -98,6 +100,7 @@ jobs: src/tools/call-omo-agent/subagent-session-creator.test.ts \ src/hooks/anthropic-context-window-limit-recovery/empty-content-recovery-sdk.test.ts src/hooks/anthropic-context-window-limit-recovery/parser.test.ts src/hooks/anthropic-context-window-limit-recovery/pruning-deduplication.test.ts src/hooks/anthropic-context-window-limit-recovery/recovery-deduplication.test.ts src/hooks/anthropic-context-window-limit-recovery/storage.test.ts \ src/hooks/session-recovery/detect-error-type.test.ts src/hooks/session-recovery/index.test.ts src/hooks/session-recovery/recover-empty-content-message-sdk.test.ts src/hooks/session-recovery/resume.test.ts src/hooks/session-recovery/storage \ + src/hooks/legacy-plugin-toast/auto-migrate.test.ts \ src/hooks/claude-code-compatibility \ src/hooks/context-injection \ src/hooks/provider-toast \ diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 95a745ef0..1b3f78f4e 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -68,6 +68,8 @@ jobs: bun test src/shared/opencode-message-dir.test.ts # session-recovery mock isolation (recover-tool-result-missing mocks ./storage) bun test src/hooks/session-recovery/recover-tool-result-missing.test.ts + # legacy-plugin-toast mock isolation (hook.test.ts mocks ./auto-migrate) + bun test src/hooks/legacy-plugin-toast/hook.test.ts - name: Run remaining tests run: | @@ -99,6 +101,7 @@ jobs: src/tools/call-omo-agent/subagent-session-creator.test.ts \ src/hooks/anthropic-context-window-limit-recovery/empty-content-recovery-sdk.test.ts src/hooks/anthropic-context-window-limit-recovery/parser.test.ts src/hooks/anthropic-context-window-limit-recovery/pruning-deduplication.test.ts src/hooks/anthropic-context-window-limit-recovery/recovery-deduplication.test.ts src/hooks/anthropic-context-window-limit-recovery/storage.test.ts \ src/hooks/session-recovery/detect-error-type.test.ts src/hooks/session-recovery/index.test.ts src/hooks/session-recovery/recover-empty-content-message-sdk.test.ts src/hooks/session-recovery/resume.test.ts src/hooks/session-recovery/storage \ + src/hooks/legacy-plugin-toast/auto-migrate.test.ts \ src/hooks/claude-code-compatibility \ src/hooks/context-injection \ src/hooks/provider-toast \ diff --git a/src/features/background-agent/background-task-notification-template.ts b/src/features/background-agent/background-task-notification-template.ts index da17cda14..e2e74cc78 100644 --- a/src/features/background-agent/background-task-notification-template.ts +++ b/src/features/background-agent/background-task-notification-template.ts @@ -1,6 +1,6 @@ import type { BackgroundTask } from "./types" -export type BackgroundTaskNotificationStatus = "COMPLETED" | "CANCELLED" | "INTERRUPTED" +export type BackgroundTaskNotificationStatus = "COMPLETED" | "CANCELLED" | "INTERRUPTED" | "ERROR" export function buildBackgroundTaskNotificationText(input: { task: BackgroundTask @@ -15,21 +15,43 @@ export function buildBackgroundTaskNotificationText(input: { const errorInfo = task.error ? `\n**Error:** ${task.error}` : "" if (allComplete) { - const completedTasksText = completedTasks - .map((t) => `- \`${t.id}\`: ${t.description}`) - .join("\n") + const succeededTasks = completedTasks.filter((t) => t.status === "completed") + const failedTasks = completedTasks.filter((t) => t.status !== "completed") + + const succeededText = succeededTasks.length > 0 + ? succeededTasks.map((t) => `- \`${t.id}\`: ${t.description}`).join("\n") + : "" + const failedText = failedTasks.length > 0 + ? failedTasks.map((t) => `- \`${t.id}\`: ${t.description} [${t.status.toUpperCase()}]${t.error ? ` - ${t.error}` : ""}`).join("\n") + : "" + + const hasFailures = failedTasks.length > 0 + const header = hasFailures + ? `[ALL BACKGROUND TASKS FINISHED - ${failedTasks.length} FAILED]` + : "[ALL BACKGROUND TASKS COMPLETE]" + + let body = "" + if (succeededText) { + body += `**Completed:**\n${succeededText}\n` + } + if (failedText) { + body += `\n**Failed:**\n${failedText}\n` + } + if (!body) { + body = `- \`${task.id}\`: ${task.description} [${task.status.toUpperCase()}]${task.error ? ` - ${task.error}` : ""}\n` + } return ` -[ALL BACKGROUND TASKS COMPLETE] +${header} -**Completed:** -${completedTasksText || `- \`${task.id}\`: ${task.description}`} +${body.trim()} -Use \`background_output(task_id="")\` to retrieve each result. +Use \`background_output(task_id="")\` to retrieve each result.${hasFailures ? `\n\n**ACTION REQUIRED:** ${failedTasks.length} task(s) failed. Check errors above and decide whether to retry or proceed.` : ""} ` } const agentInfo = task.category ? `${task.agent} (${task.category})` : task.agent + const isFailure = statusText !== "COMPLETED" return ` [BACKGROUND TASK ${statusText}] @@ -39,7 +61,7 @@ Use \`background_output(task_id="")\` to retrieve each result. **Duration:** ${duration}${errorInfo} **${remainingCount} task${remainingCount === 1 ? "" : "s"} still in progress.** You WILL be notified when ALL complete. -Do NOT poll - continue productive work. +${isFailure ? "**ACTION REQUIRED:** This task failed. Check the error and decide whether to retry, cancel remaining tasks, or continue." : "Do NOT poll - continue productive work."} Use \`background_output(task_id="${task.id}")\` to retrieve this result when ready. ` diff --git a/src/features/background-agent/manager.test.ts b/src/features/background-agent/manager.test.ts index 8994abd34..974718387 100644 --- a/src/features/background-agent/manager.test.ts +++ b/src/features/background-agent/manager.test.ts @@ -3478,12 +3478,12 @@ describe("BackgroundManager.checkAndInterruptStaleTasks", () => { //#when — no progress update for 15 minutes await manager["checkAndInterruptStaleTasks"]({}) - //#then — killed after messageStalenessTimeout + //#then — killed because session gone from status registry expect(task.status).toBe("cancelled") - expect(task.error).toContain("no activity") + expect(task.error).toContain("session gone from status registry") }) - test("should NOT interrupt task with no lastUpdate within messageStalenessTimeout", async () => { + test("should NOT interrupt task with no lastUpdate within session-gone timeout", async () => { //#given const client = { session: { @@ -3492,7 +3492,7 @@ describe("BackgroundManager.checkAndInterruptStaleTasks", () => { abort: async () => ({}), }, } - const manager = new BackgroundManager({ client, directory: tmpdir() } as unknown as PluginInput, { messageStalenessTimeoutMs: 600_000 }) + const manager = new BackgroundManager({ client, directory: tmpdir() } as unknown as PluginInput, { messageStalenessTimeoutMs: 600_000, sessionGoneTimeoutMs: 600_000 }) const task: BackgroundTask = { id: "task-fresh-no-update", @@ -3509,7 +3509,7 @@ describe("BackgroundManager.checkAndInterruptStaleTasks", () => { getTaskMap(manager).set(task.id, task) - //#when — only 5 min since start, within 10min timeout + //#when — only 5 min since start, within 10min session-gone timeout await manager["checkAndInterruptStaleTasks"]({}) //#then — task survives @@ -4263,7 +4263,7 @@ describe("BackgroundManager.pruneStaleTasksAndNotifications - removes pruned tas expect(retainedTask?.status).toBe("error") expect(getTaskMap(manager).has(staleTask.id)).toBe(true) expect(notifications).toHaveLength(1) - expect(notifications[0]).toContain("[ALL BACKGROUND TASKS COMPLETE]") + expect(notifications[0]).toContain("[ALL BACKGROUND TASKS FINISHED") expect(notifications[0]).toContain(staleTask.description) expect(getCompletionTimers(manager).has(staleTask.id)).toBe(true) expect(removeTaskCalls).toContain(staleTask.id) diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index 980061bb6..cb288f317 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -147,7 +147,7 @@ export class BackgroundManager { private queuesByKey: Map = new Map() private processingKeys: Set = new Set() private completionTimers: Map> = new Map() - private completedTaskSummaries: Map> = new Map() + private completedTaskSummaries: Map> = new Map() private idleDeferralTimers: Map> = new Map() private notificationQueueByParent: Map> = new Map() private rootDescendantCounts: Map @@ -1552,6 +1552,8 @@ export class BackgroundManager { this.completedTaskSummaries.get(task.parentSessionID)!.push({ id: task.id, description: task.description, + status: task.status, + error: task.error, }) // Update pending tracking and check if all tasks complete @@ -1573,7 +1575,7 @@ export class BackgroundManager { } const completedTasks = allComplete - ? (this.completedTaskSummaries.get(task.parentSessionID) ?? [{ id: task.id, description: task.description }]) + ? (this.completedTaskSummaries.get(task.parentSessionID) ?? [{ id: task.id, description: task.description, status: task.status, error: task.error }]) : [] if (allComplete) { @@ -1591,20 +1593,40 @@ export class BackgroundManager { let notification: string if (allComplete) { - const completedTasksText = completedTasks - .map(t => `- \`${t.id}\`: ${t.description}`) - .join("\n") + const succeededTasks = completedTasks.filter(t => t.status === "completed") + const failedTasks = completedTasks.filter(t => t.status !== "completed") + + const succeededText = succeededTasks.length > 0 + ? succeededTasks.map(t => `- \`${t.id}\`: ${t.description}`).join("\n") + : "" + const failedText = failedTasks.length > 0 + ? failedTasks.map(t => `- \`${t.id}\`: ${t.description} [${t.status.toUpperCase()}]${t.error ? ` - ${t.error}` : ""}`).join("\n") + : "" + + const hasFailures = failedTasks.length > 0 + const header = hasFailures + ? `[ALL BACKGROUND TASKS FINISHED - ${failedTasks.length} FAILED]` + : "[ALL BACKGROUND TASKS COMPLETE]" + + let body = "" + if (succeededText) { + body += `**Completed:**\n${succeededText}\n` + } + if (failedText) { + body += `\n**Failed:**\n${failedText}\n` + } + if (!body) { + body = `- \`${task.id}\`: ${task.description} [${task.status.toUpperCase()}]${task.error ? ` - ${task.error}` : ""}\n` + } notification = ` -[ALL BACKGROUND TASKS COMPLETE] +${header} -**Completed:** -${completedTasksText || `- \`${task.id}\`: ${task.description}`} +${body.trim()} -Use \`background_output(task_id="")\` to retrieve each result. +Use \`background_output(task_id="")\` to retrieve each result.${hasFailures ? `\n\n**ACTION REQUIRED:** ${failedTasks.length} task(s) failed. Check errors above and decide whether to retry or proceed.` : ""} ` } else { - // Individual completion - silent notification notification = ` [BACKGROUND TASK ${statusText}] **ID:** \`${task.id}\` @@ -1612,7 +1634,7 @@ Use \`background_output(task_id="")\` to retrieve each result. **Duration:** ${duration}${errorInfo} **${remainingCount} task${remainingCount === 1 ? "" : "s"} still in progress.** You WILL be notified when ALL complete. -Do NOT poll - continue productive work. +${statusText === "COMPLETED" ? "Do NOT poll - continue productive work." : "**ACTION REQUIRED:** This task failed. Check the error and decide whether to retry, cancel remaining tasks, or continue."} Use \`background_output(task_id="${task.id}")\` to retrieve this result when ready. ` @@ -1675,11 +1697,14 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea resolvedModel: model, }) + const isTaskFailure = task.status === "error" || task.status === "cancelled" || task.status === "interrupt" + const shouldReply = allComplete || isTaskFailure + try { await this.client.session.promptAsync({ path: { id: task.parentSessionID }, body: { - noReply: !allComplete, + noReply: !shouldReply, ...(agent !== undefined ? { agent } : {}), ...(model !== undefined ? { model } : {}), ...(resolvedTools ? { tools: resolvedTools } : {}), @@ -1689,7 +1714,8 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea log("[background-agent] Sent notification to parent session:", { taskId: task.id, allComplete, - noReply: !allComplete, + isTaskFailure, + noReply: !shouldReply, }) } catch (error) { if (isAbortedSessionError(error)) { diff --git a/src/features/background-agent/task-poller.test.ts b/src/features/background-agent/task-poller.test.ts index ec3d09a74..a99154ebf 100644 --- a/src/features/background-agent/task-poller.test.ts +++ b/src/features/background-agent/task-poller.test.ts @@ -288,29 +288,100 @@ describe("checkAndInterruptStaleTasks", () => { expect(task.status).toBe("running") }) - it("should use default stale timeout when session status is unknown/missing", async () => { - //#given — lastUpdate exceeds stale timeout, session not in status map + it("should use session-gone timeout when session is missing from status map (with progress)", async () => { + //#given — lastUpdate 2min ago, session completely gone from status const task = createRunningTask({ startedAt: new Date(Date.now() - 300_000), progress: { toolCalls: 1, - lastUpdate: new Date(Date.now() - 200_000), + lastUpdate: new Date(Date.now() - 120_000), }, }) - //#when — empty sessionStatuses (session not found) + //#when — empty sessionStatuses (session gone), sessionGoneTimeoutMs = 60s await checkAndInterruptStaleTasks({ tasks: [task], client: mockClient as never, - config: { staleTimeoutMs: 180_000 }, + config: { staleTimeoutMs: 180_000, sessionGoneTimeoutMs: 60_000 }, concurrencyManager: mockConcurrencyManager as never, notifyParentSession: mockNotify, sessionStatuses: {}, }) - //#then — unknown session treated as potentially stale, apply default timeout + //#then — cancelled because session gone timeout (60s) < timeSinceLastUpdate (120s) expect(task.status).toBe("cancelled") - expect(task.error).toContain("Stale timeout") + expect(task.error).toContain("session gone from status registry") + }) + + it("should use session-gone timeout when session is missing from status map (no progress)", async () => { + //#given — task started 2min ago, no progress, session completely gone + const task = createRunningTask({ + startedAt: new Date(Date.now() - 120_000), + progress: undefined, + }) + + //#when — session gone, sessionGoneTimeoutMs = 60s + await checkAndInterruptStaleTasks({ + tasks: [task], + client: mockClient as never, + config: { messageStalenessTimeoutMs: 600_000, sessionGoneTimeoutMs: 60_000 }, + concurrencyManager: mockConcurrencyManager as never, + notifyParentSession: mockNotify, + sessionStatuses: {}, + }) + + //#then — cancelled because session gone timeout (60s) < runtime (120s) + expect(task.status).toBe("cancelled") + expect(task.error).toContain("session gone from status registry") + }) + + it("should NOT use session-gone timeout when session is idle (present in status map)", async () => { + //#given — lastUpdate 2min ago, session is idle (present in status but not active) + const task = createRunningTask({ + startedAt: new Date(Date.now() - 300_000), + progress: { + toolCalls: 1, + lastUpdate: new Date(Date.now() - 120_000), + }, + }) + + //#when — session is idle (present in map), staleTimeoutMs = 180s + await checkAndInterruptStaleTasks({ + tasks: [task], + client: mockClient as never, + config: { staleTimeoutMs: 180_000, sessionGoneTimeoutMs: 60_000 }, + concurrencyManager: mockConcurrencyManager as never, + notifyParentSession: mockNotify, + sessionStatuses: { "ses-1": { type: "idle" } }, + }) + + //#then — still running because normal staleTimeout (180s) > timeSinceLastUpdate (120s) + expect(task.status).toBe("running") + }) + + it("should use default session-gone timeout when not configured", async () => { + //#given — lastUpdate 2min ago, session gone, no sessionGoneTimeoutMs config + const task = createRunningTask({ + startedAt: new Date(Date.now() - 300_000), + progress: { + toolCalls: 1, + lastUpdate: new Date(Date.now() - 120_000), + }, + }) + + //#when — no config (default sessionGoneTimeoutMs = 60_000) + await checkAndInterruptStaleTasks({ + tasks: [task], + client: mockClient as never, + config: undefined, + concurrencyManager: mockConcurrencyManager as never, + notifyParentSession: mockNotify, + sessionStatuses: {}, + }) + + //#then — cancelled because default session gone timeout (60s) < timeSinceLastUpdate (120s) + expect(task.status).toBe("cancelled") + expect(task.error).toContain("session gone from status registry") }) it("should NOT interrupt task when session is busy (OpenCode status), even if lastUpdate exceeds stale timeout", async () => { diff --git a/src/hooks/legacy-plugin-toast/auto-migrate.test.ts b/src/hooks/legacy-plugin-toast/auto-migrate.test.ts index cdc03389f..d1aa5e745 100644 --- a/src/hooks/legacy-plugin-toast/auto-migrate.test.ts +++ b/src/hooks/legacy-plugin-toast/auto-migrate.test.ts @@ -10,12 +10,10 @@ describe("autoMigrateLegacyPluginEntry", () => { beforeEach(() => { testConfigDir = join(tmpdir(), `omo-legacy-migrate-${Date.now()}-${Math.random().toString(36).slice(2)}`) mkdirSync(testConfigDir, { recursive: true }) - process.env.OPENCODE_CONFIG_DIR = testConfigDir }) afterEach(() => { rmSync(testConfigDir, { recursive: true, force: true }) - delete process.env.OPENCODE_CONFIG_DIR }) describe("#given opencode.json has a bare legacy plugin entry", () => { @@ -27,7 +25,7 @@ describe("autoMigrateLegacyPluginEntry", () => { ) // when - const result = autoMigrateLegacyPluginEntry() + const result = autoMigrateLegacyPluginEntry(testConfigDir) // then expect(result.migrated).toBe(true) @@ -47,7 +45,7 @@ describe("autoMigrateLegacyPluginEntry", () => { ) // when - const result = autoMigrateLegacyPluginEntry() + const result = autoMigrateLegacyPluginEntry(testConfigDir) // then expect(result.migrated).toBe(true) @@ -67,7 +65,7 @@ describe("autoMigrateLegacyPluginEntry", () => { ) // when - const result = autoMigrateLegacyPluginEntry() + const result = autoMigrateLegacyPluginEntry(testConfigDir) // then expect(result.migrated).toBe(true) @@ -81,7 +79,7 @@ describe("autoMigrateLegacyPluginEntry", () => { // given - empty dir // when - const result = autoMigrateLegacyPluginEntry() + const result = autoMigrateLegacyPluginEntry(testConfigDir) // then expect(result.migrated).toBe(false) @@ -98,7 +96,7 @@ describe("autoMigrateLegacyPluginEntry", () => { ) // when - const result = autoMigrateLegacyPluginEntry() + const result = autoMigrateLegacyPluginEntry(testConfigDir) // then expect(result.migrated).toBe(true) @@ -116,7 +114,7 @@ describe("autoMigrateLegacyPluginEntry", () => { writeFileSync(join(testConfigDir, "opencode.json"), original) // when - const result = autoMigrateLegacyPluginEntry() + const result = autoMigrateLegacyPluginEntry(testConfigDir) // then expect(result.migrated).toBe(false) diff --git a/src/hooks/legacy-plugin-toast/auto-migrate.ts b/src/hooks/legacy-plugin-toast/auto-migrate.ts index 74aaf8c45..34bc4bbc0 100644 --- a/src/hooks/legacy-plugin-toast/auto-migrate.ts +++ b/src/hooks/legacy-plugin-toast/auto-migrate.ts @@ -1,4 +1,5 @@ import { existsSync, readFileSync, writeFileSync } from "node:fs" +import { join } from "node:path" import { parseJsoncSafe } from "../../shared/jsonc-parser" import { getOpenCodeConfigPaths } from "../../shared/opencode-config-dir" @@ -31,15 +32,23 @@ function toLegacyCanonical(entry: string): string { return entry } -function detectOpenCodeConfigPath(): string | null { +function detectOpenCodeConfigPath(overrideConfigDir?: string): string | null { + if (overrideConfigDir) { + const jsoncPath = join(overrideConfigDir, "opencode.jsonc") + const jsonPath = join(overrideConfigDir, "opencode.json") + if (existsSync(jsoncPath)) return jsoncPath + if (existsSync(jsonPath)) return jsonPath + return null + } + const paths = getOpenCodeConfigPaths({ binary: "opencode", version: null }) if (existsSync(paths.configJsonc)) return paths.configJsonc if (existsSync(paths.configJson)) return paths.configJson return null } -export function autoMigrateLegacyPluginEntry(): MigrationResult { - const configPath = detectOpenCodeConfigPath() +export function autoMigrateLegacyPluginEntry(overrideConfigDir?: string): MigrationResult { + const configPath = detectOpenCodeConfigPath(overrideConfigDir) if (!configPath) return { migrated: false, from: null, to: null, configPath: null } try {