diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index 0cd92b8e62..b6ba8691e7 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -1148,7 +1148,7 @@ func (m *Manager) launchApplyPrepareResult( } func (m *Manager) publishLaunchPrepareCompleted(req *LaunchRequest, result *EnvPrepareResult, recorder *prepareProgressRecorder, workspacePath string, success bool, err error) { - if req.ACPSessionID != "" { + if req.ACPSessionID != "" && !shouldPrepareEnvironment(req) { return } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_prepare_events_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_prepare_events_test.go new file mode 100644 index 0000000000..2040109a7d --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_prepare_events_test.go @@ -0,0 +1,84 @@ +package lifecycle + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/agent/executor" + "github.com/kandev/kandev/internal/task/models" + "github.com/stretchr/testify/require" +) + +func TestLaunch_WorktreeResumePublishesPrepareCompleted(t *testing.T) { + mgr, eventBus := newPrepareEventsTestManager(t, "profile-worktree-resume") + mgr.preparerRegistry = NewPreparerRegistry(mgr.logger) + mgr.preparerRegistry.Register(models.ExecutorTypeWorktree, &progressPreparer{}) + + _, err := mgr.Launch(context.Background(), &LaunchRequest{ + TaskID: "task-worktree-resume", + SessionID: "session-worktree-resume", + ACPSessionID: "acp-session-worktree-resume", + AgentProfileID: "profile-worktree-resume", + ExecutorType: string(models.ExecutorTypeWorktree), + RepositoryPath: "/tmp/repo", + UseWorktree: true, + BaseBranch: "main", + }) + require.NoError(t, err) + require.NotEmpty(t, prepareProgressPayloads(eventBus)) + + completed := prepareCompletedPayloads(eventBus) + require.Len(t, completed, 1) + require.True(t, completed[0].Success) + requirePrepareStep(t, completed[0].Steps, "Validate Docker") +} + +func TestLaunch_ResumeWithoutPreparationPublishesNoPrepareEvents(t *testing.T) { + mgr, eventBus := newPrepareEventsTestManager(t, "profile-resume-without-preparation") + + _, err := mgr.Launch(context.Background(), &LaunchRequest{ + TaskID: "task-resume-without-preparation", + SessionID: "session-resume-without-preparation", + ACPSessionID: "acp-session-resume-without-preparation", + AgentProfileID: "profile-resume-without-preparation", + ExecutorType: string(models.ExecutorTypeLocal), + IsEphemeral: true, + }) + require.NoError(t, err) + require.Empty(t, prepareProgressPayloads(eventBus)) + require.Empty(t, prepareCompletedPayloads(eventBus)) +} + +func newPrepareEventsTestManager(t *testing.T, profileID string) (*Manager, *MockEventBusWithTracking) { + t.Helper() + log := newTestLogger() + execRegistry := NewExecutorRegistry(log) + execRegistry.Register(&createInstanceExecutor{ + MockExecutor: MockExecutor{name: executor.NameStandalone}, + client: newReadyAgentctlClient(t, log), + }) + eventBus := &MockEventBusWithTracking{} + mgr := NewManager( + newTestRegistry(), eventBus, execRegistry, + &MockCredentialsManager{}, &countingProfileResolver{info: &AgentProfileInfo{ + ProfileID: profileID, + AgentName: "auggie", + }}, nil, + ExecutorFallbackWarn, "", log, + ) + cleanupManagerStopCh(t, mgr) + return mgr, eventBus +} + +func prepareProgressPayloads(eventBus *MockEventBusWithTracking) []*PrepareProgressEventPayload { + eventBus.mu.Lock() + defer eventBus.mu.Unlock() + var out []*PrepareProgressEventPayload + for _, tracked := range eventBus.PublishedEvents { + payload, ok := tracked.Event.Data.(*PrepareProgressEventPayload) + if ok { + out = append(out, payload) + } + } + return out +} diff --git a/apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts b/apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts index 0f8b3551ed..cb17f49269 100644 --- a/apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts +++ b/apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts @@ -86,6 +86,10 @@ test.describe("mobile: worktree branch resume recovery", () => { await fixture.session.recoveryNewBranchButton().tap(); await expect(fixture.session.branchRecreatedWarning()).toBeVisible({ timeout: 60_000 }); await fixture.session.waitForChatIdle({ timeout: 30_000 }); + await expect(fixture.session.agentStatus()).toHaveCount(0); + await expect( + fixture.session.activeChat().locator('[data-placeholder="Preparing workspace..."]'), + ).toHaveCount(0); let afterEnvironment: Awaited> = null; let newBranch = ""; diff --git a/apps/web/e2e/tests/session/session-resume-recovery.spec.ts b/apps/web/e2e/tests/session/session-resume-recovery.spec.ts index 2e9e5ec162..7240fe8e29 100644 --- a/apps/web/e2e/tests/session/session-resume-recovery.spec.ts +++ b/apps/web/e2e/tests/session/session-resume-recovery.spec.ts @@ -79,6 +79,10 @@ test.describe("worktree branch resume recovery", () => { await fixture.session.recoveryNewBranchButton().click(); await expect(fixture.session.branchRecreatedWarning()).toBeVisible({ timeout: 60_000 }); await fixture.session.waitForChatIdle({ timeout: 30_000 }); + await expect(fixture.session.agentStatus()).toHaveCount(0); + await expect( + fixture.session.activeChat().locator('[data-placeholder="Preparing workspace..."]'), + ).toHaveCount(0); let afterEnvironment: Awaited> = null; let newBranch = ""; diff --git a/docs/plans/resume-preparation-completion/plan.md b/docs/plans/resume-preparation-completion/plan.md new file mode 100644 index 0000000000..cacae00d92 --- /dev/null +++ b/docs/plans/resume-preparation-completion/plan.md @@ -0,0 +1,114 @@ +--- +created: 2026-09-01 +status: implemented +requirements: + - REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001 +system_design: + - ../../specs/agents/system-design/agent-resume-runtime-recovery.md +legacy_specs: [] +--- + +# Implementation Plan: Complete Resumed Workspace Preparation + +## Overview + +Restore the terminal preparation event for worktree-backed ACP resumes. Add a +backend regression at the lifecycle event boundary, then extend the existing +desktop and mobile session-recovery scenarios to prove that the chat settles to +its real idle state. + +## Confirmed root cause + +PR #3216 correctly made worktree ACP resumes call environment preparation when +Git recovery may be required. The older `publishLaunchPrepareCompleted` guard +still returns for every request with an ACP session ID. The resulting resume +publishes `executor.prepare.progress` without `executor.prepare.completed`. +The web store remains `preparing`, `useSessionState` folds that into +`isWorking`, and a `WAITING_FOR_INPUT` session is mislabeled as background work. + +## Scope + +### In scope + +- Pair every resumed worktree preparation progress stream with one terminal + preparation event on success or failure. +- Keep ordinary ACP resumes that skip preparation free of synthetic preparation + events. +- Add a failing Go regression for the exact ACP worktree resume path. +- Prove desktop and mobile chats no longer show **Preparing workspace** or + **Background work is running** after recovery becomes idle. +- Keep the existing shared chat composition and mobile interaction model. + +### Out of scope + +- Changing ACP provider session identity, resume-token persistence, or branch + replacement authorization. +- Redesigning chat status labels or the composer. +- Changing genuine detached background-work presentation. +- Reworking preparation snapshot hydration or live-event precedence. + +## Technical approach + +Update `publishLaunchPrepareCompleted` in +`apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` so its resume +suppression uses the same `shouldPrepareEnvironment` decision that gates +environment and runtime progress. This preserves the no-event behavior for ACP +resumes that reuse a prepared non-worktree workspace while allowing worktree +resumes to publish their terminal result. + +Add a lifecycle test in `manager_launch_test.go` that uses an ACP session ID, +worktree preparation, and a tracked event bus. It must fail before the +correction because no terminal payload is published, then pass with one +successful payload containing the preparation step. Extend the existing +session-resume recovery Playwright scenarios after their idle wait to assert +the shared chat surface has no stale preparation or background-work status. + +The mobile surface already uses the same chat state and composition as desktop. +No layout, navigation, touch target, safe-area, or scroll behavior changes. The +existing `mobile-session-resume-recovery.spec.ts` is the nearest shipped mobile +exemplar and provides the rendered mobile regression. + +## Tests + +- `AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.9`: add + `TestLaunch_WorktreeResumePublishesPrepareCompleted` in + `apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go`. +- Preserve the existing fresh-launch completion test and cover the + no-preparation ACP resume case in the same focused lifecycle suite. + +## E2E tests + +- `AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.9`: extend + `apps/web/e2e/tests/session/session-resume-recovery.spec.ts` to assert the + recovered desktop chat becomes idle without stale preparation or background + status. +- Extend + `apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts` with the + same user outcome in the `mobile-chrome` project. No mobile geometry changes + are expected. + +## Work orders + +- [x] [Task 01: Balance resumed preparation events](task-01-balance-resumed-preparation-events.md) + +## Verification results + +- The lifecycle regression failed before the fix with preparation progress but + zero completion events, then passed after terminal suppression was aligned + with `shouldPrepareEnvironment`. +- The focused lifecycle suite passes for resumed worktree preparation, ordered + runtime progress, terminal error publication, and a launch-level event-free + ACP resume fast path. +- The existing desktop and mobile branch-recovery flows pass with post-idle + assertions for both stale preparation and false background work. +- Specification lint and diff validation pass. + +## Risks + +- Removing the ACP guard entirely would emit synthetic completion for resumes + that deliberately skip preparation. Reuse `shouldPrepareEnvironment` instead. +- A unit test that invokes only the publication helper could miss divergence in + launch gating. Exercise the real launch path with tracked events. +- E2E assertions made before the recovered session settles could mistake a + legitimate transient status for the defect. Assert only after + `waitForChatIdle`. diff --git a/docs/plans/resume-preparation-completion/task-01-balance-resumed-preparation-events.md b/docs/plans/resume-preparation-completion/task-01-balance-resumed-preparation-events.md new file mode 100644 index 0000000000..d853322960 --- /dev/null +++ b/docs/plans/resume-preparation-completion/task-01-balance-resumed-preparation-events.md @@ -0,0 +1,108 @@ +--- +id: "01-balance-resumed-preparation-events" +title: "Balance resumed preparation events" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001 +acceptance_criteria: + - AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.9 +system_design: + - ../../specs/agents/system-design/agent-resume-runtime-recovery.md +--- + +# Task 01: Balance Resumed Preparation Events + +## Summary + +Make worktree ACP resume preparation publish its terminal event and prove that +desktop and mobile chat return to their actual idle state. Preserve the current +event-free fast path for ACP resumes that skip preparation. + +## In scope + +- Start with a failing lifecycle regression that launches a worktree resume with + an ACP session ID and observes preparation events through the real event bus. +- Gate terminal-event suppression with the same preparation decision used by + environment and runtime progress. +- Cover success, failure, and skipped-preparation behavior where the existing + test harness supports them without duplicating launch setup. +- Extend the existing desktop and mobile session-resume recovery scenarios with + post-idle assertions for stale preparation and background-work status. +- Record final command results in this work order and `plan.md`. + +## Out of scope + +- Frontend state-model or component changes unless the balanced backend event + does not satisfy the documented contract. +- New responsive composition, copy, or localization. +- ACP protocol changes, branch recovery policy, or task-state transitions. + +## Acceptance + +- A worktree ACP resume that emits preparation progress emits exactly one + terminal preparation result for the same launch attempt. +- An ACP resume that skips preparation continues to emit no preparation + progress or completion. +- After successful recovery settles at `WAITING_FOR_INPUT`, desktop and mobile + chat show neither stale workspace preparation nor false background work. + +## Verification + +```bash +# From apps/backend: +rtk go test ./internal/agent/runtime/lifecycle -run 'TestLaunch_(WorktreeResumePublishesPrepareCompleted|ResumeWithoutPreparationPublishesNoPrepareEvents|PublishesPrepareCompletedAfterRuntimeProgress|PublishesPrepareCompletionOnLegacyRouteEnvError)$' -count=1 + +# From apps/web: +rtk pnpm e2e:run tests/session/session-resume-recovery.spec.ts -- --retries=0 +rtk pnpm e2e:run --project mobile-chrome tests/session/mobile-session-resume-recovery.spec.ts -- --retries=0 + +# From the repository root: +rtk python3 scripts/lint-spec-files.py --all +rtk git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch_prepare_events_test.go` +- `apps/web/e2e/pages/session-page.ts` +- `apps/web/e2e/tests/session/session-resume-recovery.spec.ts` +- `apps/web/e2e/tests/session/mobile-session-resume-recovery.spec.ts` +- `docs/plans/resume-preparation-completion/plan.md` +- `docs/plans/resume-preparation-completion/task-01-balance-resumed-preparation-events.md` + +## Dependencies + +None. + +## Risks + +- The completion predicate can drift from progress gating if it duplicates the + resume rule instead of calling `shouldPrepareEnvironment`. +- The existing E2E recovery flow performs real Git branch replacement and is + slower than a store-only test, but it is the closest production path to PR + #3216 and the reported incident. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001` and AC 001.9. +- `docs/specs/agents/system-design/agent-resume-runtime-recovery.md`. +- PR #3216 and its `session-resume-recovery` unit and Playwright patterns. +- Incident diagnostics for session `9dd7b17f-0c66-4c73-8e1e-ff5c7eb67c35`. + +## Results + +- Added a real launch-path regression that reproduced the missing terminal + event for an ACP worktree resume before the fix. +- Reused `shouldPrepareEnvironment` when deciding whether to suppress terminal + events, which preserves the ordinary ACP resume fast path. +- Added launch-level coverage for skipped preparation and terminal failure, and + extended desktop/mobile branch-recovery E2E tests with idle-state assertions. +- All commands in **Verification** pass. diff --git a/docs/specs/agents/requirements/agent-resume-runtime-recovery.md b/docs/specs/agents/requirements/agent-resume-runtime-recovery.md index 1ef62290d2..1b1ddc71c0 100644 --- a/docs/specs/agents/requirements/agent-resume-runtime-recovery.md +++ b/docs/specs/agents/requirements/agent-resume-runtime-recovery.md @@ -2,6 +2,7 @@ status: active system: agents created: 2026-07-27 +updated: 2026-09-01 owners: - Kandev --- @@ -27,6 +28,7 @@ Preserve the observable behavior documented for Agent Resume and Runtime Recover - **AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.6:** A completed turn remains represented by the task's review state while its persisted response and session lifecycle state settle. After a backend restart and automatic resume, the prior transcript remains visible and the task returns to the Turn Finished review bucket once the session is again `WAITING_FOR_INPUT`; it does not settle in Backlog or Running. - **AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.7:** The explicit managed-runtime update path may invalidate only the deterministic `_npx` execution directory for the selected built-in package after an initial update failure, then retry once and run the normal ACP capability probe. - **AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.8:** **GIVEN** a valid OpenCode resume token, **WHEN** the OpenCode child exits before answering ACP `initialize`, **THEN** Kandev shows the normal recovery action and retains the same token for the next Resume attempt. +- **AC-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001.9:** When a resumed session re-enters workspace preparation, Kandev shall publish a terminal preparation result after preparation finishes. A session that has returned to `WAITING_FOR_INPUT` without detached activity shall not remain visibly preparing or appear to have background work. ### REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-002: Visible resume failure feedback @@ -113,6 +115,9 @@ without repairing it. - A confirmed missing branch offers explicit continuation on a new branch from the task base branch. The same conversation continues, but lost code does not return. +- A resumed session that performs workspace preparation leaves the preparing + state when preparation finishes. An idle resumed session does not display + background activity that is not running. ## Persistence and security constraints @@ -205,6 +210,10 @@ without repairing it. - **GIVEN** replacement materializes before provider startup or readiness fails, **WHEN** the resume attempt reaches a terminal path, **THEN** one warning is persisted or remains retryable without changing the session identity. +- **GIVEN** an ACP worktree resume emits workspace preparation progress, + **WHEN** preparation and agent reconnection finish successfully, **THEN** the + preparation state becomes terminal and the idle chat shows neither + **Preparing workspace** nor **Background work is running**. ## Out of scope diff --git a/docs/specs/agents/system-design/agent-resume-runtime-recovery.md b/docs/specs/agents/system-design/agent-resume-runtime-recovery.md index d9184d56c5..85b78627e7 100644 --- a/docs/specs/agents/system-design/agent-resume-runtime-recovery.md +++ b/docs/specs/agents/system-design/agent-resume-runtime-recovery.md @@ -16,8 +16,9 @@ also defines visible recovery errors and explicit continuation after confirmed Git branch loss. The orchestrator owns recovery authorization, session identity, and warning -persistence. The worktree manager owns branch verification and fresh branch -creation. The web client owns error presentation and recovery choices. +persistence. The agent lifecycle manager owns balanced preparation progress and +completion publication. The worktree manager owns branch verification and fresh +branch creation. The web client owns error presentation and recovery choices. The task environment remains the owner of worktrees. A task session refers to that environment and does not acquire its own worktree lifecycle. @@ -26,7 +27,7 @@ that environment and does not acquire its own worktree lifecycle. | Requirement | Design section | | --- | --- | -| `REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001` | [Session identity](#session-identity) | +| `REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-001` | [Session identity](#session-identity), [Resume preparation lifecycle](#resume-preparation-lifecycle) | | `REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-002` | [Visible recovery errors](#visible-recovery-errors) | | `REQ-AGENTS-AGENT-RESUME-RUNTIME-RECOVERY-003` | [Explicit branch replacement](#explicit-branch-replacement), [Warning persistence](#warning-persistence) | @@ -57,6 +58,31 @@ The web client currently converts WebSocket failures to a plain `Error` and several resume call sites then discard that error. Automatic resume also hides the resume error when read-only workspace restore succeeds. +## Resume preparation lifecycle + +Native ACP resume normally reuses an already prepared workspace. A worktree +resume is different: `shouldPrepareEnvironment` can re-enter environment and +runtime preparation so Git recovery can validate, recreate, or replace the +task-owned worktree. + +Preparation event publication follows one balanced lifecycle. If a launch path +can publish `executor.prepare.progress`, it publishes exactly one terminal +`executor.prepare.completed` event for that attempt after environment and +runtime preparation succeeds or fails. The presence of an ACP session ID can +skip both event kinds when no preparation runs; it cannot suppress only the +terminal event after a worktree resume has emitted progress. + +The web preparation handler projects progress as `preparing` and the terminal +event as `completed` or `failed`. `useSessionState` can treat live preparation +as working while the durable session is temporarily `STARTING` or +`WAITING_FOR_INPUT`, but that derived state ends with the terminal event. A +resumed session that settles at `WAITING_FOR_INPUT` without foreground or +detached activity therefore renders as idle and keeps the composer available. + +Session snapshots remain recovery evidence, not a substitute for balancing the +live stream. They can hydrate a missing preparation projection, while a live +in-flight projection remains authoritative until its matching terminal event. + ## Recovery protocol `session.recover` accepts these actions: @@ -246,6 +272,9 @@ not translated strings. User-facing copy does not use a Unicode em dash. Backend tests start with the current failure: +- An ACP worktree resume that publishes preparation progress also publishes one + terminal completion event. An ACP resume that skips preparation publishes + neither event. - Normal resume returns an error that matches `ErrBranchUnrecoverable` and does not mutate the branch. - Explicit replacement keeps the session and resume token, creates a unique