feat(session): Session Manager — spawn/kill/list/get/send/restore/cleanup

[harshitsinghbhandari]

Summary

Implements ports.SessionManager (the explicit-mutation half of the LCM+SM lane) in backend/internal/session/, against fakes for the outbound ports. The SM drives the Runtime/Agent/Workspace plugins, seeds the initial lifecycle record, and routes mutation outcomes to the LCM (OnSpawnCompleted / OnKillRequested). It never derives observed state and is the single producer of the derived display status (attached on read via DeriveLegacyStatus, never persisted).

Base is feat/lcm-sm-contracts (the integration branch), not main.

Pipelines

• Spawn: Workspace.Create → Runtime.Create (env AO_SESSION_ID/AO_PROJECT_ID/AO_ISSUE_ID layered over the agent env) → Seed → LCM.OnSpawnCompleted. Eager rollback of completed steps on any failure (the record is seeded late, so an unseeded orphan would be invisible to Cleanup). If OnSpawnCompleted fails after seeding, best-effort routes the orphan to errored via OnKillRequested(KillError) since the store has no delete.
• Kill: LCM.OnKillRequested first → Runtime.Destroy → Workspace.Destroy, honoring the worktree-remove safety — a refusal (path still registered after prune ⇒ may hold uncommitted work) surfaces as an error with WorkspaceFreed:false and is never forced.
• List/Get: read records, attach derived status; single producer, never persisted.
• Send: via AgentMessenger.
• Restore: fallible I/O first (workspace restore + runtime create, never destroys the worktree on failure) → re-seed/reopen lifecycle (not_started/spawn_requested, PR cleared_on_restore) → relaunch via Agent.GetRestoreCommand → OnSpawnCompleted.
• Cleanup: reclaims terminal sessions' workspaces, skips paths holding uncommitted work → CleanupResult{Cleaned, Skipped}.

⚠️ Store-contract additions (need @tom's review)

OnSpawnCompleted requires a pre-seeded record, but LifecycleStore had no way to create one with identity, and single-id reads (Get/Kill) had no record-with-identity accessor (Load is lifecycle-only). Added two methods, discussed and approved with the lane owner but flagged here since they touch the persistence contract:

• Seed(ctx, SessionRecord) error — explicit create-with-identity; errors if the id already exists (re-seed on Restore goes through PatchLifecycle).
• Get(ctx, SessionID) (SessionRecord, bool, error) — single full record read (symmetric with List).

The lifecycle package's test fake was updated to satisfy the wider interface; LCM tests remain green. No other LCM / decide-core / contract changes.

Out of scope / flags

• preflight appears in the spec but no outbound port exposes it → treated as out of scope.
• "Agent stop" in the kill pipeline has no port method → subsumed by Runtime.Destroy (the agent runs inside the runtime).
• Real tmux/claude-code/worktree adapters, full 3-layer prompt assembly, orchestrator-as-session, real CDC — all out of scope per the lane plan.

Test plan

Tests route through the real LCM Manager (wrapped to record call order):

• Spawn happy path — record seeded with identity, OnSpawnCompleted flips runtime→alive, AO_* env wired, handles in metadata, status spawning
• Spawn runtime-create failure — eager workspace rollback, no record seeded, no OnSpawnCompleted
• Kill ordering (OnKillRequested < Runtime.Destroy < Workspace.Destroy) + terminal killed status
• Kill worktree-remove refusal surfaced (WorkspaceFreed:false, not force-deleted)
• List/Get derive status (table) + Get not-found
• Send routes to messenger
• Restore relaunch via resume command + reopened lifecycle
• Cleanup skips uncommitted-work worktrees, ignores live sessions

Gates: gofmt -l . empty · go build ./... · go vet ./... · go test -race ./... — all green.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Implements a new ports.SessionManager in backend/internal/session/ to own explicit session mutations (spawn/kill/send/restore/cleanup) while delegating canonical lifecycle transitions to the existing LCM, and deriving the display Status only on reads.

Changes:

• Added session.Manager implementing the SM pipelines for Spawn/Kill/List/Get/Send/Restore/Cleanup with rollback behavior on spawn failures.
• Extended ports.LifecycleStore with Seed (create-with-identity) and Get (single record read) to support SM needs beyond lifecycle-only Load.
• Added an in-package fake harness and tests that exercise SM behavior against the real lifecycle manager plus faked outbound ports.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
backend/internal/session/manager.go	New Session Manager implementation wiring outbound ports, store seeding, LCM notifications, and read-model derivation.
backend/internal/session/manager_test.go	SM tests covering spawn/kill/list/get/send/restore/cleanup flows using a harness.
backend/internal/session/fakes_test.go	Fakes for store/runtime/agent/workspace/messenger + recording LCM wrapper to assert pipeline ordering.
backend/internal/ports/outbound.go	Adds Seed and Get to LifecycleStore contract with explanatory docs.
backend/internal/lifecycle/fakes_test.go	Updates lifecycle test fake store to satisfy the expanded LifecycleStore interface.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[harshitsinghbhandari]

Review — Session Manager (reviewed via subagent + independent verification)

Verified in a throwaway worktree: gofmt clean, build/vet pass, go test -race green; internal/session at 67.5% coverage. Spec-faithful and the invariants hold — display status is derived only on read (toSession) and never persisted, kills route through LCM.OnKillRequested, and the tests exercise the real lifecycle.Manager (not a fake LCM) for the round-trips, which is exactly right. Two should-fix items before this is clean; the rest is minor/doc-level.

1. LifecycleStore.Seed + Get — clean ✅

Seed taking a full SessionRecord and rejecting an existing id (create-only), with Restore routed through PatchLifecycle instead, cleanly separates create from reopen and avoids a double-seed footgun. Get is not redundant with Load (lifecycle-only, hot path) or List (project-scoped) — the SM needs identity by id for teardown/read-model. Both are ordinary CRUD for a real store. Minor: a one-line doc noting Load = hot-path lifecycle-only vs Get = full record would help Tom.

2. Spawn rollback + orphan handling — logic right, path untested (should-fix)

Eager rollback is correct (late seed; each step unwinds the prior). The OnSpawnCompleted-failure orphan route (seed + runtime up, then route to terminal-errored via OnKillRequested(KillError) + teardown) is the right call — but there is no test for it, and it's the most subtle path flagged. Add a test (fake LCM whose OnSpawnCompleted errors) asserting terminal-errored + OnKillRequested(KillError) fired + runtime/workspace torn down. Also untested: workspace-create-failure rollback, and Seed duplicate-id rejection.

3. Kill ordering + worktree-remove refusal — correct & tested ✅

OnKillRequested → Runtime.Destroy → Workspace.Destroy, asserted by call-order indices; refusal surfaces as an error with WorkspaceFreed=false and the path is never force-deleted. Note: the spec's explicit "Agent stop" step is folded into Runtime.Destroy (agent runs inside the runtime) — defensible, but call it out in the PR body so the Agent port's stop semantics aren't assumed elsewhere. Minor: Kill reconstructs handles from metadata; a seeded-but-never-completed session yields empty handle/path to Destroy — should be a safe no-op but isn't guarded/tested.

4. Restore — no rollback → runtime leak + stranded state (should-fix)

Restore does Workspace.Restore → Runtime.Create → PatchLifecycle(reopen) → OnSpawnCompleted, but on a failure at the reopen or OnSpawnCompleted step it returns the error without destroying the runtime it just created → orphan runtime. Worse, if OnSpawnCompleted fails after the reopen patch landed, the record is left not_started (display: spawning) with a live runtime and no recovery path. Spawn handles exactly this; Restore should mirror it (roll back the runtime on reopen/OnSpawnCompleted failure). No Restore failure path is tested. The reopen itself (PR → none/cleared_on_restore, session → not_started/spawn_requested, via PatchLifecycle not Seed) is correct and tested on the happy path.

5. Tests against the real LCM — yes ✅

recordingLCM wraps the real lifecycle.New(...) and only logs call order, so OnSpawnCompleted/OnKillRequested run the real load→decide→persist pipeline and the kill test asserts the LCM produced terminated/manually_killed. Coverage gaps are the error paths above (orphan route, all Restore-failure paths) — that's what's holding 67.5%.

Minor

• Send has no existence/terminal guard — forwards blindly to AgentMessenger. Probably fine (messenger owns busy-detect/verify), but confirm it's intentional.
• Cleanup drops a runtime-destroy error silently even when the workspace is later reclaimed — fine for terminal sessions, undocumented.
• buildPrompt is a documented stub for the 3-layer assembly — fine for this split.

Recommendation: Approve-with-follow-ups. Sound design, invariants hold, real-LCM testing is exactly right. Two before-clean items: (4) give Restore the same rollback Spawn has, and (2) test the OnSpawnCompleted-failure orphan route + Restore error paths.

[harshitsinghbhandari]

Re-review — 4c331d9

Verified in a throwaway worktree: gofmt clean, build/vet pass, go test -race green; internal/session coverage 67.5% → 72.7%. Both should-fix items resolved.

• #4 Restore rollback ✅ — rollbackRuntime(handle) now fires on both the reopen-patch failure and the OnSpawnCompleted failure, tearing down the freshly-created runtime while preserving the workspace (right call — it may hold uncommitted work). The early missing-agent-session-id guard is a good addition: it bails before any I/O instead of launching an ambiguous "resume nothing."
• #2 tests ✅ — TestSpawn_OnSpawnCompletedFailure_RoutesOrphanToErrored (seeded orphan → terminal/errored, since the store has no delete) and TestRestore_OnSpawnCompletedFailure_RollsBackRuntime (runtime destroyed, workspace preserved) cover exactly the previously-untested paths.
• Minors — Send-delegates-to-messenger (by design), Cleanup best-effort runtime-destroy (documented), buildPrompt stub: all acknowledged, agreed as-is.

LGTM — approving. Clean work, and the real-LCM round-trip testing throughout this PR was the right call.