fix(agent): complete resumed workspace preparation (#3267)

2026-09-01 23:42:42 +01:00
parent 2e2019aed1
commit e6f98d996d
8 changed files with 356 additions and 4 deletions
@@ -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
 	}

@@ -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
+}
@@ -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<ReturnType<typeof apiClient.getTaskEnvironment>> = null;
    let newBranch = "";
@@ -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<ReturnType<typeof apiClient.getTaskEnvironment>> = null;
    let newBranch = "";
@@ -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`.
@@ -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.
@@ -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

@@ -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