sapient/oh-my-openagent
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#2732): enhance notification for failed/crashed subagent tasks

Browse Source 
- 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
This commit is contained in:
YeonGyu-Kim
2026-03-27 15:48:07 +09:00
parent e22e13cd29
commit 3be26cb97f
8 changed files with 178 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
.github/workflows/ci.yml vendored
View File

@@ -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 \

3
.github/workflows/publish.yml vendored
View File

@@ -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 \

40
src/features/background-agent/background-task-notification-template.ts
View File

@@ -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 `<system-reminder>
[ALL BACKGROUND TASKS COMPLETE]
${header}
**Completed:**
${completedTasksText || `- \`${task.id}\`: ${task.description}`}
${body.trim()}
Use \`background_output(task_id="<id>")\` to retrieve each result.
Use \`background_output(task_id="<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.` : ""}
</system-reminder>`
}
const agentInfo = task.category ? `${task.agent} (${task.category})` : task.agent
const isFailure = statusText !== "COMPLETED"
return `<system-reminder>
[BACKGROUND TASK ${statusText}]
@@ -39,7 +61,7 @@ Use \`background_output(task_id="<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.
</system-reminder>`

12
src/features/background-agent/manager.test.ts
View File

@@ -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)

52
src/features/background-agent/manager.ts
View File

@@ -147,7 +147,7 @@ export class BackgroundManager {
private queuesByKey: Map<string, QueueItem[]> = new Map()
private processingKeys: Set<string> = new Set()
private completionTimers: Map<string, ReturnType<typeof setTimeout>> = new Map()
private completedTaskSummaries: Map<string, Array<{id: string, description: string}>> = new Map()
private completedTaskSummaries: Map<string, Array<{id: string, description: string, status: string, error?: string}>> = new Map()
private idleDeferralTimers: Map<string, ReturnType<typeof setTimeout>> = new Map()
private notificationQueueByParent: Map<string, Promise<void>> = new Map()
private rootDescendantCounts: Map<string, number>
@@ -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 = `<system-reminder>
[ALL BACKGROUND TASKS COMPLETE]
${header}
**Completed:**
${completedTasksText || `- \`${task.id}\`: ${task.description}`}
${body.trim()}
Use \`background_output(task_id="<id>")\` to retrieve each result.
Use \`background_output(task_id="<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.` : ""}
</system-reminder>`
} else {
// Individual completion - silent notification
notification = `<system-reminder>
[BACKGROUND TASK ${statusText}]
**ID:** \`${task.id}\`
@@ -1612,7 +1634,7 @@ Use \`background_output(task_id="<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.
</system-reminder>`
@@ -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)) {

85
src/features/background-agent/task-poller.test.ts
View File

@@ -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 () => {

14
src/hooks/legacy-plugin-toast/auto-migrate.test.ts
View File

@@ -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)

15
src/hooks/legacy-plugin-toast/auto-migrate.ts
View File

@@ -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 {