-
Notifications
You must be signed in to change notification settings - Fork 872
fix: background task completion detection and silent notifications #628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: background task completion detection and silent notifications #628
Conversation
- Fix TS2742 by adding explicit ToolDefinition type annotations - Add stability detection (3 consecutive stable polls after 10s minimum) - Remove early continue when sessionStatus is undefined - Add silent notification system via tool.execute.after hook injection - Change task retention from 200ms to 5 minutes for background_output retrieval - Fix formatTaskResult to sort messages by time descending Fixes hanging background tasks that never complete due to missing sessionStatus.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 8 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Apply upstream PR code-yeongyu#628 by @Gladdonilli - Fix background tasks that hang indefinitely due to missing sessionStatus - Add silent notification system via tool.execute.after hook injection - Add stability detection (3 consecutive stable polls after 10s minimum) - Change task retention from 200ms to 5 minutes for background_output - Fix formatTaskResult to sort messages by time descending - Fix TS2742 by adding explicit ToolDefinition type annotations Co-authored-by: Gladdonilli <tianyi35@illinois.edu>
|
new alternative method found + more issues identified, moving to a new pr |
- Add stability-based completion detection (10s min + 3 stable polls) - Fix message extraction to recognize 'reasoning' parts from thinking models - Switch from promptAsync() to prompt() for proper agent initialization - Remove model parameter from prompt body (use agent's configured model) - Add fire-and-forget prompt pattern for sisyphus_task sync mode - Add silent notification via tool.execute.after hook injection - Fix indentation issues in manager.ts and index.ts Incorporates fixes from: - PR code-yeongyu#592: Stability detection mechanism - PR code-yeongyu#610: Model parameter passing (partially) - PR code-yeongyu#628: Completion detection improvements Known limitation: Thinking models (e.g. claude-*-thinking-*) cause JSON Parse errors in child sessions. Use non-thinking models for background agents until OpenCode core resolves this.
…ion (#638) * fix: background task completion detection and silent notifications - Fix TS2742 by adding explicit ToolDefinition type annotations - Add stability detection (3 consecutive stable polls after 10s minimum) - Remove early continue when sessionStatus is undefined - Add silent notification system via tool.execute.after hook injection - Change task retention from 200ms to 5 minutes for background_output retrieval - Fix formatTaskResult to sort messages by time descending Fixes hanging background tasks that never complete due to missing sessionStatus. * fix: improve background task completion detection and message extraction - Add stability-based completion detection (10s min + 3 stable polls) - Fix message extraction to recognize 'reasoning' parts from thinking models - Switch from promptAsync() to prompt() for proper agent initialization - Remove model parameter from prompt body (use agent's configured model) - Add fire-and-forget prompt pattern for sisyphus_task sync mode - Add silent notification via tool.execute.after hook injection - Fix indentation issues in manager.ts and index.ts Incorporates fixes from: - PR #592: Stability detection mechanism - PR #610: Model parameter passing (partially) - PR #628: Completion detection improvements Known limitation: Thinking models (e.g. claude-*-thinking-*) cause JSON Parse errors in child sessions. Use non-thinking models for background agents until OpenCode core resolves this. * fix: add tool_result handling and pendingByParent tracking for resume/external tasks Addresses code review feedback from PR #638: P1: Add tool_result type to validateSessionHasOutput() to prevent false negatives for tool-only background tasks that would otherwise timeout after 30 minutes despite having valid results. P2: Add pendingByParent tracking to resume() and registerExternalTask() to prevent premature 'ALL COMPLETE' notifications when mixing launched and resumed tasks. * fix: address code review feedback - log messages, model passthrough, sorting, race condition - Fix misleading log messages: 'promptAsync' -> 'prompt (fire-and-forget)' - Restore model passthrough in launch() for Sisyphus category configs - Fix call-omo-agent sorting: use time.created number instead of String(time) - Fix race condition: check promptError inside polling loop, not just after 100ms
Apply upstream PR code-yeongyu#628 by @Gladdonilli - Fix background tasks that hang indefinitely due to missing sessionStatus - Add silent notification system via tool.execute.after hook injection - Add stability detection (3 consecutive stable polls after 10s minimum) - Change task retention from 200ms to 5 minutes for background_output - Fix formatTaskResult to sort messages by time descending - Fix TS2742 by adding explicit ToolDefinition type annotations Co-authored-by: Gladdonilli <tianyi35@illinois.edu>
Summary
sessionStatusin polling loopToolDefinitiontype annotationsChanges
Completion Detection (PR #592 re-implementation)
continuewhensessionStatusis undefined - allows fall-through to message-based detectionlastMsgCountandstablePollsfields toBackgroundTaskinterfaceMIN_STABILITY_TIME_MSconstant (10 seconds)Silent Notification System (new feature)
PendingNotificationinterface andpendingNotificationsmap toBackgroundManagerhasPendingNotifications()andconsumePendingNotifications()methodssession.prompt()with storing notifications for later injectiontool.execute.afterhook handler that injects pending notifications into tool outputsrc/index.tsOther Fixes
background_outputretrievalformatTaskResultto sort messages by time descending (matches sync pattern)String()wrapper forlocaleCompareto handle non-string time valuesToolDefinitiontype annotations toskillandslashcommandexportsTesting
bun run typecheckpassesbun run buildsucceedsFiles Modified
src/features/background-agent/types.tslastMsgCount,stablePollsfieldssrc/features/background-agent/manager.tssrc/features/background-agent/index.tsPendingNotificationtypesrc/hooks/background-notification/index.tstool.execute.afterhandlersrc/index.tssrc/tools/background-task/tools.tsformatTaskResultsortingsrc/tools/skill/tools.ts: ToolDefinitiontype annotationsrc/tools/slashcommand/tools.ts: ToolDefinitiontype annotationSummary by cubic
Fixes background tasks that could hang and adds silent, inline notifications injected into tool outputs. Improves completion detection, keeps results available longer, and resolves TypeScript typing errors.
Bug Fixes
New Features
Written for commit 9e81260. Summary will update on new commits.