feat(hub): add ServerChan task notifications#515
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Direct
data.contenttask-notification payloads are still dropped, so some completed background tasks will never notify. Evidencehub/src/notifications/eventParsing.ts:76, related existing handlinghub/src/sync/backgroundTasks.ts:85. - [Major] Visible web clients no longer get task-completion toasts because
sendTaskNotificationskips the existing SSE fallback used by the other push notifications. Evidencehub/src/push/pushNotificationChannel.ts:95.
Summary
Review mode: initial
Found 2 issues on the current head.
Testing
Not run (security/automation)
HAPI Bot
| return null | ||
| } | ||
|
|
||
| const wrappedMessage = isObject(data.message) ? data.message : null |
There was a problem hiding this comment.
[MAJOR] This only reads data.message.content, but the hub already treats { type: 'user', content: '<task-notification>...' } as a valid completion shape in hub/src/sync/backgroundTasks.ts. Those events will update background-task state without sending any notification. Please accept both payload shapes here and cover the direct data.content variant in the tests.
Suggested fix:
const content = typeof data.content === 'string'
? data.content
: isObject(data.message) && typeof data.message.content === 'string'
? data.message.content
: null
if (content == null) {
return null
}| await this.pushService.sendToNamespace(session.namespace, payload) | ||
| } | ||
|
|
||
| async sendTaskNotification(session: Session, notification: TaskNotification): Promise<void> { |
There was a problem hiding this comment.
[MAJOR] sendTaskNotification goes straight to Web Push and skips the visible-tab SSE toast path that sendPermissionRequest and sendReady already use. When the web app is open, task completions will either become intrusive push notifications or disappear entirely for users who rely on the in-app toast path. Reuse the same visibilityTracker / sseManager.sendToast(...) fallback before calling pushService.
Suggested fix:
const url = payload.data?.url ?? this.buildSessionPath(session.id)
if (this.visibilityTracker.hasVisibleConnection(session.namespace)) {
const delivered = await this.sseManager.sendToast(session.namespace, {
type: 'toast',
data: {
title: payload.title,
body: payload.body,
sessionId: session.id,
url
}
})
if (delivered > 0) {
return
}
}
There was a problem hiding this comment.
Findings
- [Major] Crash exits are now reported as
completedin four wrappers.markCrash(...)setssessionEndReason = 'error', but the newfinallyblocks overwrite that tocompletedwhenever there is no local launch failure. That makes failed Codex/Cursor/Gemini/OpenCode sessions emitsession-end.reason="completed", which triggers false completion notifications and hides real failures from downstream handling. Evidencecli/src/codex/runCodex.ts:220,cli/src/codex/runCodex.ts:228,cli/src/cursor/runCursor.ts:130,cli/src/gemini/runGemini.ts:182,cli/src/opencode/runOpencode.ts:136, related state settercli/src/agent/runnerLifecycle.ts:100.
Suggested fix:let crashed = false try { await loop(...) } catch (error) { crashed = true lifecycle.markCrash(error) logger.debug('[codex] Loop error:', error) } finally { const localFailure = sessionWrapperRef.current?.localLaunchFailure if (localFailure?.exitReason === 'exit') { lifecycle.setExitCode(1) lifecycle.setArchiveReason(`Local launch failed: ${formatFailureReason(localFailure.message)}`) lifecycle.setSessionEndReason('error') } else if (!crashed) { lifecycle.setSessionEndReason('completed') } await lifecycle.cleanupAndExit() }
- [Major] Direct
data.contenttask-notification payloads are still dropped.extractBackgroundTaskDeltaalready treats{ type: 'user', content: '<task-notification>...' }as a completion, butextractTaskNotificationonly readsdata.message.content, so some completed tasks will update counters without notifying any channel. Evidencehub/src/notifications/eventParsing.ts:76, related existing handlinghub/src/sync/backgroundTasks.ts:77; missing coverage inhub/src/notifications/eventParsing.test.ts:111.
Suggested fix:const content = typeof data.content === 'string' ? data.content : isObject(data.message) && typeof data.message.content === 'string' ? data.message.content : null if (content == null) { return null }
- [Major] Visible web clients still bypass the in-app toast path for task notifications.
sendTaskNotificationnow always goes straight to Web Push, unlikesendReadyandsendPermissionRequest. When the PWA tab is open, task results either become browser push notifications or disappear for users who rely on the existing SSE toast channel. Evidencehub/src/push/pushNotificationChannel.ts:95.
Suggested fix:const url = payload.data?.url ?? this.buildSessionPath(session.id) if (this.visibilityTracker.hasVisibleConnection(session.namespace)) { const delivered = await this.sseManager.sendToast(session.namespace, { type: 'toast', data: { title: payload.title, body: payload.body, sessionId: session.id, url } }) if (delivered > 0) { return } } await this.pushService.sendToNamespace(session.namespace, payload)
Summary
Review mode: follow-up after new commits
Found 3 issues on the current head.
Testing
Not run (automation)
HAPI Bot
| lifecycle.setArchiveReason(`Local launch failed: ${formatFailureReason(localFailure.message)}`); | ||
| lifecycle.setSessionEndReason('error'); | ||
| } else { | ||
| lifecycle.setSessionEndReason('completed'); |
There was a problem hiding this comment.
[MAJOR] markCrash(...) already sets sessionEndReason = 'error' in RunnerLifecycle, but this new else branch overwrites it to 'completed' for any loop exception that is not a local launch failure. The same pattern is repeated in cli/src/cursor/runCursor.ts, cli/src/gemini/runGemini.ts, and cli/src/opencode/runOpencode.ts, so crashed sessions will emit session-end.reason="completed" and trigger false completion notifications.
Suggested fix:
let crashed = false
try {
await loop(...)
} catch (error) {
crashed = true
lifecycle.markCrash(error)
logger.debug('[codex] Loop error:', error)
} finally {
const localFailure = sessionWrapperRef.current?.localLaunchFailure
if (localFailure?.exitReason === 'exit') {
lifecycle.setExitCode(1)
lifecycle.setArchiveReason(`Local launch failed: ${formatFailureReason(localFailure.message)}`)
lifecycle.setSessionEndReason('error')
} else if (!crashed) {
lifecycle.setSessionEndReason('completed')
}
await lifecycle.cleanupAndExit()
}| return null | ||
| } | ||
|
|
||
| const wrappedMessage = isObject(data.message) ? data.message : null |
There was a problem hiding this comment.
[MAJOR] This still only reads data.message.content, but the hub already accepts { type: 'user', content: '<task-notification>...' } as a completion shape in hub/src/sync/backgroundTasks.ts. Those events will update background-task counts without sending any notification. Please accept both payload shapes here and cover the direct data.content variant in the tests.
Suggested fix:
const content = typeof data.content === 'string'
? data.content
: isObject(data.message) && typeof data.message.content === 'string'
? data.message.content
: null
if (content == null) {
return null
}| await this.pushService.sendToNamespace(session.namespace, payload) | ||
| } | ||
|
|
||
| async sendTaskNotification(session: Session, notification: TaskNotification): Promise<void> { |
There was a problem hiding this comment.
[MAJOR] sendTaskNotification still goes straight to Web Push and skips the visible-tab SSE toast path that sendPermissionRequest and sendReady already use. When the web app is open, task completions will either become intrusive push notifications or disappear for users who rely on the in-app toast path. Reuse the same visibilityTracker / sseManager.sendToast(...) fallback before calling pushService.
Suggested fix:
const url = payload.data?.url ?? this.buildSessionPath(session.id)
if (this.visibilityTracker.hasVisibleConnection(session.namespace)) {
const delivered = await this.sseManager.sendToast(session.namespace, {
type: 'toast',
data: {
title: payload.title,
body: payload.body,
sessionId: session.id,
url
}
})
if (delivered > 0) {
return
}
}
await this.pushService.sendToNamespace(session.namespace, payload)
| clearInterval(keepAliveInterval); | ||
| await permissionAdapter.cancelAll('Session ended'); | ||
| session.sendSessionDeath(); | ||
| session.sendSessionDeath(shouldExit ? 'terminated' : 'completed'); |
There was a problem hiding this comment.
[MAJOR] The ACP runner still reports unhandled failures as completed. This new reason is derived only from shouldExit, so any exception that escapes the loop before shutdown still reaches this finally block with shouldExit === false and emits session-end.reason="completed". That now drives false session-ended completion notifications downstream for Gemini/ACP sessions.
Suggested fix:
let endReason: SessionEndReason = 'terminated'
try {
while (!shouldExit) {
// existing loop body
}
if (!shouldExit) {
endReason = 'completed'
}
} catch (error) {
endReason = 'error'
throw error
} finally {
clearInterval(keepAliveInterval)
await permissionAdapter.cancelAll('Session ended')
session.sendSessionDeath(shouldExit ? 'terminated' : endReason)
await session.flush()
session.close()
await backend.disconnect()
happyServer.stop()
}| const payload: PushPayload = { | ||
| title: isFailure ? 'Task failed' : 'Task completed', | ||
| body: `${agentName} · ${name} · ${notification.summary}`, | ||
| tag: `task-${session.id}`, |
There was a problem hiding this comment.
[MAJOR] All push task notifications for the same session now share tag: \task-${session.id}`. In the service worker, that tag is passed straight to showNotification(...)`, so later task updates replace earlier ones instead of stacking. A failed task can therefore be overwritten by the next completion in the same session, which loses the earlier alert entirely for background/mobile users.
Suggested fix:
const payload: PushPayload = {
title: isFailure ? 'Task failed' : 'Task completed',
body: `${agentName} · ${name} · ${notification.summary}`,
data: {
type: 'task-notification',
sessionId: session.id,
url: this.buildSessionPath(session.id)
}
}If you still want deduplication, use a per-task unique tag instead of a per-session tag.
There was a problem hiding this comment.
Findings
No issues found.
Summary
Review mode: follow-up after new commits
No issues found on the current head. Prior bot findings appear addressed. Residual risk: static review only; I did not execute the PR test commands in this automation pass.
Testing
Not run (automation; static review only)
HAPI Bot
|
@tiann 我已经搞定了，麻烦合并一下。 |
|
Thank you! |
2 participants
Summary
Testing