fix(background-agent): fix task completion detection for background sessions

[Gladdonilli]

Summary

Fixes background task completion detection and output retrieval.

Issues Fixed

1. Background sessions not detected (original issue)

• client.session.status() doesn't return background sessions - only regular sessions
• Line 455 had an early continue that skipped ALL completion logic when sessionStatus was undefined

2. Tasks deleted before output could be retrieved (new fix)

• notifyParentSession() deleted tasks after 200ms
• This was too fast - background_output would fail with "Task was deleted"
• Changed to 5-minute retention (safe: pruneStaleTasksAndNotifications only cleans after 30min TTL)

3. Silent notifications (new fix)

• Removed client.session.prompt() that injected "[BACKGROUND TASK COMPLETED]" messages
• These interrupted the parent session flow
• Kept toast notification for subtle UI feedback

4. formatTaskResult mismatch (new fix)

• Async path used different code than sync path in call-omo-agent/tools.ts
• Fixed to match: sort by time descending, no optional chaining, take first result

Changes

manager.ts:

• Remove early continue when session not found - fall through to message-based detection
• Use optional chaining sessionStatus?.type for safe property access
• Add stability detection: complete when message count unchanged for 3 polls (6 seconds)
• Remove client.session.prompt() notification - keep only toast
• Retain completed tasks for 5 minutes instead of 200ms

tools.ts:

• Fix formatTaskResult to match sync pattern from call-omo-agent/tools.ts

Testing

Before fix:

• explore agent: Hung indefinitely (never completed)
• librarian agent: Hung indefinitely
• background_output: Returned "Task was deleted"

After fix:

• explore agent: Completed in ~10 seconds
• librarian agent: Completed in ~6 seconds
• background_output: Returns actual agent output
• No interrupting chat messages

Technical Details

Stability detection: Tracks messages.length across polls. When count stays the same for 3 polls (6 seconds at 2s interval), task is complete.

5-minute retention safety:

• DCP/context pruning operates on LLM context, not JS heap memory
• pruneStaleTasksAndNotifications() only prunes after TASK_TTL_MS (30 minutes)
• Polling loop skips completed tasks (status !== "running")

Why agents take multiple minutes (not a bug):

• ULTRAWORK system prompts instruct thorough multi-step work
• Librarian does web search + GitHub search + Context7 (5-15s each)
• Each tool call = LLM inference (2-5s) + execution
• The "6 second" completion was for trivial prompts; real prompts = 10-30+ tool calls

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[github-actions]

All contributors have signed the CLA. Thank you! ✅
_{Posted by the CLA Assistant Lite bot.}

[greptile-apps]

Greptile Overview

Greptile Summary

Fixed critical bug where background tasks never completed because session.status() doesn't return background sessions. The fix removes the early continue that skipped completion detection and adds message-based stability detection as a fallback.

Key Changes:

• Removed early continue on line 455 when session not found in status response
• Added optional chaining sessionStatus?.type to prevent accessing undefined
• Implemented stability detection: marks task complete when message count unchanged for 3 consecutive polls (6 seconds)

Issues Found:

• Critical: Tasks that complete with 0 messages will never trigger stability detection (line 513 checks currentMsgCount > 0)
• Type Safety: stableCount and lastMsgCount properties added to progress object aren't defined in TaskProgress type

Testing Concern:
Per the project's TDD requirements in AGENTS.md, this bug fix should have accompanying tests. The existing manager.test.ts doesn't cover the pollRunningTasks() method or the new stability detection logic.

Confidence Score: 2/5

• This PR fixes a real bug but introduces a logical flaw that could cause tasks to hang indefinitely
• Score reflects a correct diagnosis of the root cause and appropriate fix approach, but contains a critical edge case bug (line 513: tasks with 0 messages never complete) and violates type safety. Additionally, violates TDD requirements from AGENTS.md by not including tests for the new stability detection logic.
• Pay close attention to the stability detection logic on line 513 - the currentMsgCount > 0 check will cause tasks with no messages to hang forever

Important Files Changed

File Analysis

Filename	Score	Overview
src/features/background-agent/manager.ts	3/5	Fixed background task completion detection by removing early continue and adding stability detection, but stability detection may fail for tasks with no messages

Sequence Diagram

sequenceDiagram
    participant Manager as BackgroundManager
    participant Client as OpencodeClient
    participant Session as Background Session
    
    Note over Manager: pollRunningTasks() called every 2s
    
    Manager->>Client: session.status()
    Client-->>Manager: allStatuses (background sessions NOT included)
    
    alt Session NOT in status response
        Note over Manager: sessionStatus = undefined
        Note over Manager: Fall through (no early continue)
        
        Manager->>Client: session.messages({id: sessionID})
        Client-->>Manager: messages array
        
        Note over Manager: Count messages.length
        
        alt Message count unchanged for 3 polls (6s)
            Manager->>Manager: stableCount >= 3
            Manager->>Manager: Mark task as completed
            Manager->>Manager: notifyParentSession()
        else Message count changed
            Manager->>Manager: Reset stableCount to 0
            Note over Manager: Continue polling
        end
    else Session in status response
        Note over Manager: sessionStatus defined
        
        alt sessionStatus.type === "idle"
            Manager->>Client: session.todo({id: sessionID})
            Client-->>Manager: todos array
            
            alt Has incomplete todos
                Note over Manager: Continue polling
            else No incomplete todos
                Manager->>Manager: Mark task as completed
                Manager->>Manager: notifyParentSession()
            end
        else sessionStatus.type !== "idle"
            Note over Manager: Task still running, continue polling
        end
    end

Loading

[greptile-apps]

Task that completes instantly with 0 messages will never reach stability detection

Suggested change

	if (progress.lastMsgCount === currentMsgCount && currentMsgCount > 0) {
	if (progress.lastMsgCount === currentMsgCount) {

[greptile-apps]

stableCount and lastMsgCount not in TaskProgress type - extend the type in types.ts to include these fields

[cubic-dev-ai]

1 issue found across 1 file

Confidence score: 3/5

• Instant-completion background tasks in src/features/background-agent/manager.ts will hang because stability detection is blocked by the currentMsgCount > 0 guard, so the reported change can’t successfully complete those tasks.
• Given the high confidence and user-facing impact of the hang, this PR carries some regression risk unless the guard is removed or adjusted.
• Pay close attention to src/features/background-agent/manager.ts - stability detection never triggers for zero-message tasks.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/features/background-agent/manager.ts">

<violation number="1" location="src/features/background-agent/manager.ts:513">
P1: Tasks that complete instantly with 0 messages will never reach stability detection because of the `currentMsgCount > 0` condition. This causes such tasks to hang indefinitely. Remove the `currentMsgCount > 0` check to allow stability detection for all cases.</violation>
</file>

---

Since this is your first cubic review, here's how it works:

• cubic automatically reviews your code and comments on bugs and improvements
• Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
• Ask questions if you need clarification on any suggestion

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[Gladdonilli]

I have read the CLA Document and I hereby sign the CLA

[cubic-dev-ai]

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/features/background-agent/manager.ts">

<violation number="1" location="src/features/background-agent/manager.ts:513">
P1: Removing `currentMsgCount > 0` can cause premature task completion. If the background task hasn't generated any messages yet (e.g., network delay, slow agent startup), the stability detection will trigger after 3 polls with 0 messages, incorrectly marking the task as complete before it starts.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/features/background-agent/manager.ts">

<violation number="1" location="src/features/background-agent/manager.ts:349">
P1: Potential double-release of concurrency slot. After releasing concurrency, `task.model` should be cleared to prevent other handlers (`session.deleted`, `pruneStaleTasksAndNotifications`) from releasing again during the 5-minute retention period.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[code-yeongyu]

@sisyphus-dev-ai review this

[sisyphus-dev-ai]

Code Review: Background Task Completion Detection Fix

✅ Successfully Resolved Issues

1. Type Safety (✓): The stableCount and lastMsgCount properties are now properly defined in TaskProgress interface (types.ts:13-14). No more type casting needed.

2. Double-Release Prevention (✓): Line 351 now includes task.model = undefined after releasing concurrency, preventing potential double-release during the 5-minute retention period.

3. Core Bug Fix (✓): The removal of early continue statement and addition of optional chaining (sessionStatus?.type) correctly addresses the root cause where background sessions weren't being detected for completion.

⚠️ Critical Issue: Stability Detection Logic Flaw

The stability detection has a logical flaw that creates an impossible choice:

Current behavior (currentMsgCount > 0 check at line 496):

• Tasks that complete instantly with 0 messages will hang forever
• Example: A task that errors immediately or completes synchronously before the first poll

Without the check (as attempted in commit edd9d9f):

• Tasks will be marked complete prematurely after 6 seconds (3 polls × 2s)
• Example: A slow-starting task (network delay, agent initialization) would be marked done before it even starts

Root Problem: The stability detection cannot distinguish between:

1. A task that hasn't started yet (0 messages, still initializing)
2. A task that completed with 0 messages (error or instant completion)

🔧 Recommended Solutions

Option 1: Minimum Time Threshold (Recommended)
Only enable stability detection after a minimum elapsed time (e.g., 10 seconds):

const elapsedSeconds = (Date.now() - task.startedAt.getTime()) / 1000
const minStabilityTime = 10 // seconds

if (progress.lastMsgCount === currentMsgCount && elapsedSeconds >= minStabilityTime) {
  progress.stableCount = (progress.stableCount ?? 0) + 1
  // ... rest of logic
}

This gives tasks time to start while still catching legitimately stuck tasks.

Option 2: Hybrid Detection
Use different logic based on whether messages exist:

• If messages.length > 0: Use current stability detection
• If messages.length === 0: Wait longer (e.g., 6 polls instead of 3) before marking complete

Option 3: Additional Signals
Check for other completion signals:

• Look at session.idle events (though these don't fire for background sessions)
• Check if session still exists vs was deleted
• Verify if promptAsync promise settled

❌ Missing Tests (TDD Violation)

Per AGENTS.md requirements, this bug fix should follow TDD (RED-GREEN-REFACTOR). However:

• No tests were added for the new stability detection logic
• Existing manager.test.ts doesn't cover pollRunningTasks() method
• The critical edge cases (0-message tasks, slow-starting tasks) are untested

Recommended test cases:

describe("BackgroundManager.pollRunningTasks - stability detection", () => {
  test("should complete task after 3 stable polls with messages")
  test("should not prematurely complete slow-starting tasks")
  test("should handle tasks that complete with 0 messages")
  test("should reset stability counter when message count changes")
})

💡 Additional Observations

UX Change: Silent Completion
The PR removes automatic notification to the parent session chat. Users now only get:

• OS toast notification (if available)
• Manual check via background_output

This is likely intentional to avoid interrupting the main session, but worth noting as a behavior change.

Retention Period Increase
Task retention increased from 200ms to 5 minutes, which is good for allowing background_output retrieval, but ensure the cleanup logic (pruneStaleTasksAndNotifications) doesn't cause memory issues with many tasks.

📊 Overall Assessment

Progress: The PR makes significant progress on fixing the core issue. Type safety and double-release issues are resolved.

Blocker: The stability detection logic needs refinement before merge. The current implementation will cause either hangs (with check) or premature completions (without check).

Recommendation: Implement Option 1 (minimum time threshold) and add comprehensive tests before merging.

---

Files Changed: src/features/background-agent/manager.ts:346-527, types.ts:13-14

[Gladdonilli]

Addressed Code Review Feedback

Fixed: Stability Detection Logic Flaw

The code review correctly identified that the currentMsgCount > 0 check was accidentally re-added in a previous commit.

Root Problem: The original check created an impossible choice:

• With the check: Tasks with 0 messages hang forever
• Without the check: Tasks could be marked complete prematurely (before they even start)

Solution Implemented: Option 1 (Minimum Time Threshold)

const MIN_STABILITY_TIME_MS = 10000 // 10 seconds

if (progress.lastMsgCount === currentMsgCount && elapsedMs >= MIN_STABILITY_TIME_MS) {
  // Stability detection only kicks in after minimum time
}

This ensures:

1. ✅ Tasks with 0 messages can complete (after 10s + 3 stable polls)
2. ✅ Slow-starting tasks won't be prematurely completed
3. ✅ Normal tasks complete as expected via stability detection

Added: Comprehensive Test Suite

Added 6 new tests for pollRunningTasks stability detection:

Test	Description
should complete task after 3 stable polls with messages	Happy path verification
should not prematurely complete slow-starting tasks	Tasks under 10s threshold are protected
should handle tasks that complete with 0 messages	The bug fix verification
should reset stability counter when message count changes	Activity detection works
should require exactly 3 stable polls to complete	Threshold verification
should handle edge case: stability resets then builds up again	Complex scenario

All 20 tests in manager.test.ts pass. ✅

[Gladdonilli]

This is not a compelete solution, do not use until either someone else figures out a elegant solution or I figure something out myself

[Gladdonilli]

continued in pr#628