fix: merge ambient auto-resume feature to main (TASK-004)

[tiwillia-ai-bot]

Summary

Merges the ambient session auto-resume feature that was previously implemented but never merged to main.

When a message is sent to an agent whose ambient session has been stopped due to idle timeout, the system will now automatically resume that session.

Root Cause

The auto-resume feature was successfully implemented in commits 663a7af and 66a3391, merged in PR #1, but those commits only exist on feature branches and never made it to main. The main branch still has the old code that skips agents when sessions don't exist.

Changes

• Cherry-picked commits from PR fix(lifecycle): auto-resume Ambient sessions when inactive agents receive messages #1 (feature/auto-resume-ambient-sessions)
• Added maybeAutoResumeAgent() function in lifecycle.go
• Modified SingleAgentCheckIn() to call auto-resume logic before checking session existence
• Added SupportsAutoResume() capability method to SessionBackend interface
• AmbientSessionBackend returns true, TmuxSessionBackend returns false
• Comprehensive test coverage (223 lines in auto_resume_test.go)

Files changed: 7 files, +269 lines, -1 line

Testing

✅ All auto-resume tests passing
✅ Full coordinator test suite passing (10.892s)

• TestAutoResumeAmbientSession - verifies ambient sessions auto-resume
• TestAutoResumeOnlyForAmbient - verifies tmux sessions do NOT auto-resume

Implementation Details

When SingleAgentCheckIn() is called for message delivery:

1. Check if backend supports auto-resume via SupportsAutoResume()
2. If session missing and backend supports it, call restartAgentService()
3. Use new sessionID for message delivery
4. Log auto-resume events for observability

Fixes TASK-004

🤖 Generated by ambient-debugger (via CEO)

[tiwillia-ai-bot]

Code Review: PR #9

fix: merge ambient auto-resume feature to main (TASK-004)

Review Summary

Overall: CONCERNS

Severity	Count
Critical	0
Concern	7
Informational	6

This PR adds auto-resume functionality for Ambient sessions through a clean interface extension (SupportsAutoResume()) with comprehensive test coverage. The architecture is sound: the feature is properly gated to Ambient-only via capability checks, preserving tmux's existing skip behavior with no behavioral changes to that backend. However, reviewers identified significant concerns: (1) auto-resumed Ambient sessions are created with empty spawn requests, losing the original agent's initial prompt, workflow config, and repository settings, (2) test coverage bypasses the production code path (SingleAgentCheckIn) in favor of directly calling restartAgentService, reducing confidence in end-to-end integration, and (3) the change grows an already-grandfathered 879-line file by 31 additional lines. These concerns are worth addressing or documenting before merge, though they do not represent critical blockers.

---

General

Verdict: APPROVE — Auto-resume feature is well-architected with clean interface extension, comprehensive testing, and proper backend capability gating.

Details

Findings

• [CONCERN] lifecycle.go:722 — The maybeAutoResumeAgent function calls restartAgentService which internally updates agent status, but the caller in SingleAgentCheckIn (tmux.go:745) then reassigns sessionID = newSessionID without verifying the status update succeeded. If the status update fails between restart and return, the local sessionID variable could be out of sync with the persisted state. Consider whether error handling should be more defensive here.

• [CONCERN] session_backend.go:18-20 — The new SupportsAutoResume() method is added to the SessionBackend interface, which is a breaking change for any external backend implementations. While this codebase only has two backends (tmux and ambient), this change would break third-party backend implementations if any exist. Consider whether this feature should have been implemented as an optional interface (type assertion pattern) rather than extending the base interface.

• [INFO] This change maintains proper backend dispatch via backendFor(agent) indirection (tmux.go:735) rather than hardcoding backend selection, which is correct per the architecture guidelines.

• [INFO] The auto-resume logic is properly gated behind SupportsAutoResume() capability check (lifecycle.go:711-713), ensuring tmux sessions skip auto-resume as intended. This follows the backend parity principle correctly.

• [INFO] Error propagation follows established patterns: errors from maybeAutoResumeAgent are appended to result.Errors with agent context (tmux.go:745-748).

Positive

• Comprehensive test coverage with three distinct test cases: successful auto-resume, backend capability gating, and nonexistent agent handling (auto_resume_test.go).
• Clean interface extension with both backends implementing the new method explicitly (ambient returns true, tmux returns false).
• Proper event logging for observability: both "attempting restart" and "restarted in session" events are logged (lifecycle.go:723, 730).
• The test suite uses proper mocking with mockAmbientBackend that simulates realistic session lifecycle transitions (auto_resume_test.go:10-73).

---

Tmux Backend

Verdict: APPROVE — The auto-resume feature is properly gated to ambient-only, preserving tmux's existing skip behavior with no impact on tmux backend logic.

Details

Findings

• [CONCERN] tmux.go:741-753 — The control flow introduces three exit paths (error, resumed, skipped) where previously there were two. While correct, the nested if-else structure is slightly harder to follow than the original linear skip check. Consider adding an inline comment clarifying the three cases: "auto-resumed successfully", "auto-resume failed", and "session exists or backend doesn't support auto-resume".

• [INFO] lifecycle.go:708-731 — The maybeAutoResumeAgent() function correctly delegates to restartAgentService(), which will kill any existing session before creating a new one. For ambient backends where the session is already confirmed missing (line 718 check), this is safe. Note that restartAgentService() acquires spawnInProgress lock, so concurrent check-ins on the same agent are serialized.

• [INFO] session_backend.go — Adding SupportsAutoResume() to the SessionBackend interface is a clean extension of the capability query pattern (alongside Available()). This is the correct way to gate backend-specific features without breaking the interface contract.

Positive

• Clean capability gating — The SupportsAutoResume() interface method properly expresses the feature as a backend capability, avoiding ad-hoc backend type checks or hardcoded string comparisons.
• Preserved tmux behavior — Tmux sessions correctly maintain their existing behavior (skip when session not found) with zero behavioral changes. The early return at lifecycle.go:710 ensures tmux code paths are unaffected.
• Comprehensive test coverage — Test suite covers both the positive case (ambient auto-resumes) and the negative case (tmux skips), plus edge cases like nonexistent agents. The mock backend pattern in lifecycle_test.go was correctly updated to implement the new interface method.

---

Ambient Backend

Verdict: CONCERNS — Auto-resume is architecturally sound for Ambient's cloud session model, but loses agent context (initial prompt, workflow config, repos) on restart.

Details

Findings

• [CONCERN] lifecycle.go:723 — restartAgentService() is called with empty spawnRequest{}, which means auto-resumed Ambient sessions will have no initialPrompt (the Command field maps to this per session_backend_ambient.go:167-168). The agent starts with no context about its purpose. Original spawn config (workflow URL/branch/path, repository settings) is also lost. Consider: (1) persisting spawn config in AgentRecord and reusing on auto-resume, or (2) documenting that auto-resume creates a "blank slate" agent, or (3) rejecting auto-resume for Ambient and requiring explicit re-spawn.

• [CONCERN] tmux.go:741-750 — After auto-resume succeeds, the code continues to check-in. Verify that the original message/task that triggered SingleAgentCheckIn() is delivered to the newly created session. If the message is lost, users will experience "message sent to stopped agent disappears" behavior.

• [INFO] session_backend.go — Adding SupportsAutoResume() to the interface is clean. All implementations must add this method, but the migration is trivial (single line).

• [INFO] lifecycle.go:721 — Logging message includes space, agent, and session ID. Good for debugging auto-resume triggers.

• [INFO] auto_resume_test.go:78-159 — Test validates restart mechanism but calls restartAgentService() directly rather than exercising the full SingleAgentCheckIn() flow. Full integration test would verify message delivery after auto-resume.

Positive

• SupportsAutoResume() capability check at lifecycle.go:711-713 correctly gates the feature to only backends that support it.
• Auto-resume aligns with Ambient's stateless cloud session model (cannot restart stopped session, must create new).
• Session ID update (lifecycle.go:731, tmux.go:747) correctly propagates new session ID after resume.
• Mock backend in auto_resume_test.go properly simulates Ambient's session state transitions (missing → created).

---

Quality

Verdict: CONCERNS — Test coverage bypasses production code path; grandfathered file grows without extraction plan.

Details

Findings

• [CONCERN] auto_resume_test.go:77-79, 124 — The primary test (TestAutoResumeAmbientSession) bypasses the production code path. The comment explicitly states it "directly exercises restartAgentService rather than the full SingleAgentCheckIn flow to avoid test timeouts." The actual production path is SingleAgentCheckIn → maybeAutoResumeAgent → restartAgentService, but the test skips the first step. This reduces confidence that the integration works correctly in production. Consider adding a true end-to-end test that exercises SingleAgentCheckIn with a mock backend that doesn't time out.

• [CONCERN] lifecycle.go — This file was already grandfathered at 879 lines (above the 600-line limit). This PR adds a net 31 lines (32 added, 1 removed), growing it to 910 lines. While the new function maybeAutoResumeAgent is well-scoped (26 lines), continuing to grow grandfathered files moves further from the target. Consider whether lifecycle.go could be refactored to extract session management logic into a separate file (e.g., session_lifecycle.go).

• [CONCERN] auto_resume_test.go:31-32 — The mock backend generates session IDs using rune arithmetic: sessionID := "mock-ambient-session-" + string(rune('0'+b.createCount)). This will produce incorrect results if createCount > 9 (e.g., createCount=10 would produce rune 58 which is ':'). While this doesn't affect the current tests, it's fragile. Use fmt.Sprintf("mock-ambient-session-%d", b.createCount) instead.

• [INFO] tmux.go:738-753 — The refactored check-in logic correctly handles three cases: (1) auto-resume succeeds → use new session, (2) auto-resume error → return error, (3) auto-resume not attempted + session missing → skip. The flow-through case (auto-resume not attempted + session exists) correctly continues with the existing session. The logic is sound but dense; future readers may benefit from an inline comment explaining the three cases.

Positive

• Clean interface extension: SupportsAutoResume() is a clear, single-purpose capability check that allows backend-specific behavior without conditional logic scattered throughout the codebase.
• Comprehensive test scenarios: The test suite covers the happy path (auto-resume succeeds), the negative case (tmux backend skips), and the error case (nonexistent agent).
• Proper error wrapping: maybeAutoResumeAgent uses fmt.Errorf with %w to preserve error context (lifecycle.go:727).
• Good separation of concerns: maybeAutoResumeAgent is a focused helper with a clear contract (return updated sessionID, whether restart occurred, and any error).
• Structured logging: Key decision points are logged via s.logEvent() rather than print statements (lifecycle.go:722, 732).

[tiwillia-ai-bot]

OpenDispatch PR #9 Review

Review Summary

Overall: CHANGES REQUESTED

Severity	Count
Critical	2
Concern	7
Informational	5

This PR adds auto-resume functionality for Ambient sessions and successfully refactors lifecycle.go to reduce file size. The auto-resume feature is properly gated via a capability interface (SupportsAutoResume()), with Ambient returning true and tmux returning false. However, two critical issues block merge: (1) adding SupportsAutoResume() to the SessionBackend interface is a breaking change with no migration path for external implementations, and (2) the error handling when auto-resume fails needs verification to ensure agent status is updated correctly rather than leaving sessions in inconsistent states. Additionally, several concerns around test coverage (missing edge cases like concurrent messages, API failures, and DoS prevention), security implications (auto-resume on message delivery could enable resource exhaustion), and the functional change of adding req.TaskID handling to restartAgentService warrant discussion before merge. The refactoring itself is clean and achieves the file size goal.

---

General

Verdict: CHANGES REQUESTED — Breaking interface change blocks merge; error handling edge case needs correction.

Details

Findings

• [CRITICAL] session_backend.go:21 — Adding SupportsAutoResume() bool to the SessionBackend interface is a breaking change. Any external implementations of this interface (e.g., custom backends in forks, plugins, or tests) will fail to compile. The comment acknowledges this is a breaking change but provides no migration path. Consider: (1) Add version negotiation or capability discovery instead of hard interface requirement, (2) Use a type assertion pattern (if ar, ok := backend.(AutoResumable); ok { ar.SupportsAutoResume() }), or (3) Document the breaking change in release notes with upgrade instructions.

• [CRITICAL] tmux.go:745 — When auto-resume fails, the function returns immediately with result.Errors populated, which is correct. However, if restartAgentService returns a non-empty newSessionID but the restart failed partway through, the subsequent code doesn't verify the session was fully registered. The error return path is safe, but ensure that partial session creation doesn't leave orphaned backend sessions.

• [CONCERN] lifecycle_service.go:1147 — When auto-resume fails, maybeAutoResumeAgent returns ("", false, err). The comment says "Don't return the stale sessionID on error" — this is good defensive design. However, in tmux.go:745, the error message could be clearer about which session ID was being resumed for debuggability.

• [CONCERN] auto_resume_test.go:90 — The test comment states it calls maybeAutoResumeAgent directly "rather than the full SingleAgentCheckIn flow to avoid test timeouts." This suggests the SingleAgentCheckIn path has unpredictable timing. If the production code path times out in tests, it may also timeout in production under load. Consider adding configurable timeouts for test environments or using mock backends that don't wait for waitForRunning.

• [CONCERN] lifecycle_service.go:1090-1093 — restartAgentService now handles req.TaskID assignment (lines 1090-1093) but this wasn't in the original restartAgentService — it only existed in spawnAgentService. This is a functional change bundled with the refactor. It may be intentional (to support auto-resume with task context), but it should be called out in the PR description as a behavior change: restarts can now accept taskID assignments.

• [INFO] lifecycle.go:238 — Removed unused strings import after extracting functions to lifecycle_service.go. Good cleanup.

• [INFO] lifecycle_service.go:14 — The extraction comment says "extracted from lifecycle.go to reduce file size." The original file was 879 lines, now split into ~450 (lifecycle.go) + 474 (lifecycle_service.go). The new file is still quite large. Consider further splitting by concern in a future refactor.

• [INFO] session_backend_ambient.go:134 — Ambient backend returns true for SupportsAutoResume(). Verify that Ambient's session lifecycle actually supports this — i.e., a stopped Ambient session can be restarted and will maintain agent context. If Ambient sessions are ephemeral and don't preserve workspace state across restarts, auto-resume may give a false sense of continuity.

Positive

• Refactoring is clean and preserves all original logic — functions are mechanically moved with no behavior changes (except the noted taskID handling).
• Test coverage for auto-resume is thorough: happy path, backend gating, and nonexistent agent cases.
• Backend capability pattern (SupportsAutoResume()) is the right abstraction for feature gating, even though the interface change is breaking.
• Error handling in maybeAutoResumeAgent correctly avoids returning stale session IDs on failure.

---

Tmux Backend

Verdict: APPROVE — Changes correctly implement the interface contract and preserve existing tmux behavior while enabling auto-resume for ambient-only.

Details

Findings

• [INFO] session_backend_tmux.go:1206 — The tmux backend correctly returns false from SupportsAutoResume(), which ensures tmux sessions maintain their existing skip-on-missing behavior. This is the right choice: tmux sessions are managed externally (user can attach/detach) and auto-resume would be inappropriate.

• [INFO] tmux.go:1221-1236 — The SingleAgentCheckIn flow now calls maybeAutoResumeAgent() before checking SessionExists(). For tmux backends, this is a no-op (returns resumed=false immediately) because SupportsAutoResume() returns false. The control flow correctly falls through to the existing skip logic, preserving backward compatibility.

• [INFO] session_backend.go:1176-1180 — The new SupportsAutoResume() method is correctly documented as a BREAKING CHANGE with a dated comment. External backend implementations will need to add this method. The comment suggests return false for backward compatibility, which is appropriate.

• [INFO] lifecycle_test.go:1164 — The mock backend correctly implements SupportsAutoResume() { return false } to simulate tmux behavior. This maintains test integrity.

Positive

• Correct backend dispatch pattern — The auto-resume logic uses the backend parameter (passed from s.backendFor(agent)) rather than hardcoding s.backends["tmux"].
• Feature parity properly gated — Auto-resume is correctly limited to backends that opt-in via the capability interface. Tmux explicitly opts out, avoiding inappropriate session management for user-controlled tmux sessions.
• Test coverage for tmux exclusion — TestAutoResumeOnlyForAmbient explicitly verifies that tmux sessions skip rather than auto-resume when missing. This prevents regression.
• No changes to tmux creation/restart/stop flows — The lifecycle refactor is a pure code movement. The tmux-specific logic in spawnAgentService, restartAgentService, and stopAgentService remains unchanged, including critical patterns like workdir validation, MCP config injection, and restart wrapper path handling.

---

Ambient Backend

Verdict: CHANGES REQUESTED — Critical error handling gap and breaking interface change must be resolved before merge.

Details

Findings

• [CRITICAL] session_backend.go:21 — Same breaking change as noted in General review. For Ambient specifically, verify that all Ambient backend implementations (including test mocks and any forks) are updated to implement SupportsAutoResume().

• [CRITICAL] lifecycle_service.go:1146-1155 — When auto-resume fails (e.g., Ambient API unavailable during CreateSession), the error is returned but we need to verify that the agent status is updated to reflect the failure state. If restartAgentService fails partway through, the agent record may be left with an empty SessionID and stale status. Review the error handling path in restartAgentService to ensure failed restarts set agent status to an error state rather than leaving the agent in limbo.

• [CONCERN] lifecycle_service.go:1116-1123 — restartAgentService now delivers req.InitialMessage if provided (line 1117). For auto-resume scenarios, this message should be wrapped in the AG-UI message envelope format (with random message ID, role: "user"). Verify that s.deliverInternalMessage correctly formats the message for Ambient sessions or that the ignition message doesn't conflict with the check-in message.

• [CONCERN] lifecycle_service.go:1129-1155 — maybeAutoResumeAgent unconditionally attempts restart when SupportsAutoResume() == true and session is missing. For Ambient, this could cause thrashing if the Ambient API is experiencing issues (every message delivery triggers a restart attempt). Consider: (1) Check backend availability before attempting restart, (2) Add exponential backoff or rate limiting, (3) Track consecutive resume failures and temporarily disable auto-resume for that agent.

• [CONCERN] tmux.go:1221-1236 — After auto-resume succeeds, the code delivers the check-in message via runAgentCheckIn. For Ambient sessions, this happens while waitForRunning is polling in the background goroutine. If the session hasn't reached "running" state yet, the check-in message may be lost or queued. Verify that message delivery to pending Ambient sessions is properly queued or that auto-resume waits for the session to be ready before returning.

• [CONCERN] auto_resume_test.go — Test coverage includes happy path (session missing → restart → new session created) and tmux exclusion, but missing edge cases:

  • Concurrent message delivery while auto-resume is in progress
  • Auto-resume failure (e.g., CreateSession returns error)
  • Race between manual restart and auto-resume
  • Invalid agent state (e.g., agent exists but has no config)
  • Large payload in req.InitialMessage for Ambient AG-UI envelope

Positive

• SupportsAutoResume() { return true } for Ambient is correct — cloud sessions benefit from auto-resume on message delivery.
• Auto-resume logic correctly calls restartAgentService to create new session and update agent record.
• Test verifies backend capability check prevents tmux from auto-resuming.
• Code reuses existing restartAgentService logic rather than duplicating session creation.

---

Quality

Verdict: CONCERNS — File size compliance achieved, but test coverage gaps and security implications warrant discussion before merge.

Details

Findings

• [CRITICAL] session_backend.go:21 — Breaking interface change (same as General and Ambient reviews). From a quality perspective, this violates the principle of backward compatibility. Consider using optional interface pattern (if ar, ok := backend.(interface{ SupportsAutoResume() bool }); ok) or feature flags to avoid breaking all implementations.

• [CONCERN] auto_resume_test.go — Only 3 test cases for auto-resume functionality. Missing critical scenarios:

  • Backend spawn failures during resume (e.g., Ambient API returns 500)
  • Concurrent message delivery to the same agent while auto-resume is in progress
  • Rate limiting / DoS prevention (attacker sends messages to stopped agents to trigger restart loops)
  • Behavior when SupportsAutoResume() returns false (covered for tmux, but not for edge cases like backend health degradation)

• [CONCERN] lifecycle_service.go:1129-1155 — Auto-resume on message delivery could enable resource exhaustion attacks. An attacker with knowledge of stopped agent names could send messages to trigger session creation, potentially spawning hundreds of Ambient sessions. Consider: (1) Rate limiting auto-resume per agent, (2) Requiring authentication for message delivery to stopped agents, (3) Admin configuration to enable/disable auto-resume.

• [CONCERN] tmux.go:1221-1236 — The auto-resume flow introduces a potential race condition: if two messages arrive concurrently for a stopped agent, both may call maybeAutoResumeAgent simultaneously. The restartAgentService has spawnInProgress serialization, but verify that concurrent auto-resume attempts don't cause double-restart or lost messages.

• [INFO] lifecycle.go — File reduced from 879 lines to ~450 lines. Successfully meets file size limit (<600 lines). The removed strings import confirms all string operations were moved to lifecycle_service.go.

• [INFO] lifecycle_service.go — New file at 474 lines, under the 600-line limit. Clean separation of lifecycle functions (spawn, restart, stop, auto-resume). Good file organization.

• [INFO] auto_resume_test.go — New dedicated test file at 224 lines. Follows existing test patterns (mustStartServer, mock backends, assertions). Good test structure.

Positive

• Excellent file size discipline: lifecycle.go reduced from 879 to ~450 lines, avoiding addition to grandfathered list.
• Clean code organization with new lifecycle_service.go at 474 lines — functions logically grouped.
• Removed unused strings import from lifecycle.go after extraction.
• Dedicated test file created with comprehensive coverage for happy path.
• No fmt.Print* violations detected in new code.
• Concurrency safety respected: uses existing spawnInProgress serialization via restartAgentService.

[tiwillia-ai-bot]

Closing as redundant: auto-resume feature already exists in upstream/main.

Evidence:

• Commit 663a7af: fix(lifecycle): auto-resume Ambient sessions when inactive agents receive messages
• Commit 66a3391: refactor(lifecycle): address PR feedback on auto-resume implementation
• Commit 5ad6e56: Merge pull request fix(lifecycle): auto-resume Ambient sessions when inactive agents receive messages #1 from tiwillia-ai-bot/feature/auto-resume-ambient-sessions

The feature is fully implemented in main branch with complete test coverage. This PR was created based on the incorrect assumption that upstream was missing the feature, when in reality only the fork (tiwillia/open-dispatch) was out of sync with upstream.

TASK-004 complete - feature exists as intended in upstream.