From 650ecdf08a0a48c1d2c314e6cf93afc9e7032f9d Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Thu, 28 May 2026 19:49:56 +0530 Subject: [PATCH] fix: address LCM/SM review blockers R1, RA, R11, RB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four narrowly-scoped fixes against the LCM + Session Manager lane from an external review of the current backend state. R2 (failed-restore lifecycle stranding) is intentionally deferred to PR #15, which already closes it via the new OnSpawnInitiated path; R3 also stays on that PR. - R1 (BLOCKER): Manager.Spawn never persisted AgentSessionID, so Manager.Restore's hard-required metadata key was always missing and every restore failed. Persist the assembled launch prompt as MetaPrompt at spawn time and add a fresh-launch fallback to Restore that uses Agent.GetLaunchCommand with the seeded prompt when the captured agent session id is absent (the id-capture hook is a separate path that may never have run). Restore still fails fast when neither the id nor a prompt is on hand — there is nothing to relaunch from. - RA (BLOCKER): adapters/workspace/gitworktree/commands.go's worktreeRemoveForceArgs passed --force, which deletes uncommitted agent work. Renamed to worktreeRemoveArgs and dropped --force so the post-prune "still registered" guard in Workspace.Destroy surfaces the refusal to Manager.Cleanup, which routes the session to Skipped instead of destroying in-progress changes. - R11 (SHOULD-FIX): reactions.go's two Notifier.Notify call sites (executeReaction's notify and escalate) built OrchestratorEvent without ProjectID. Captured projectID on the transition (via a store.Get in mutate) and on reactionTracker (so TickEscalations can still populate it on duration-based escalations), and threaded it through executeReaction/sendToAgent/escalate. - RB (SHOULD-FIX): gitworktree.Workspace.managedPath used filepath.Join which cleans .. segments before validateManagedPath ran, so session=\"../other\" stayed inside managedRoot while breaking per-project isolation. validateConfig now rejects path separators and the . / .. components on ProjectID and SessionID at the source. go build ./..., go vet ./..., and go test -race ./... all pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../workspace/gitworktree/commands.go | 9 +- .../workspace/gitworktree/workspace.go | 23 +++- .../workspace/gitworktree/workspace_test.go | 66 +++++++++- backend/internal/lifecycle/manager.go | 23 +++- backend/internal/lifecycle/reactions.go | 36 ++++-- backend/internal/lifecycle/reactions_test.go | 117 ++++++++++++++++++ backend/internal/ports/facts.go | 6 + backend/internal/session/manager.go | 28 +++-- backend/internal/session/manager_test.go | 45 ++++++- 9 files changed, 322 insertions(+), 31 deletions(-) diff --git a/backend/internal/adapters/workspace/gitworktree/commands.go b/backend/internal/adapters/workspace/gitworktree/commands.go index 739616c94..5a417dd7f 100644 --- a/backend/internal/adapters/workspace/gitworktree/commands.go +++ b/backend/internal/adapters/workspace/gitworktree/commands.go @@ -16,8 +16,13 @@ func worktreeAddNewBranchArgs(repo, branch, path, baseRef string) []string { return []string{"-C", repo, "worktree", "add", "-b", branch, path, baseRef} } -func worktreeRemoveForceArgs(repo, path string) []string { - return []string{"-C", repo, "worktree", "remove", "--force", path} +// worktreeRemoveArgs intentionally omits --force: a dirty worktree (uncommitted +// agent work) MUST cause `git worktree remove` to fail, so the post-prune +// "still registered" check in Destroy surfaces the refusal to the Session +// Manager's Cleanup, which routes the session to Skipped rather than deleting +// the agent's in-progress changes. +func worktreeRemoveArgs(repo, path string) []string { + return []string{"-C", repo, "worktree", "remove", path} } func worktreePruneArgs(repo string) []string { diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index e90db12c9..da6d2d832 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -119,7 +119,7 @@ func (w *Workspace) Destroy(ctx context.Context, info ports.WorkspaceInfo) error if err != nil { return err } - _, removeErr := w.run(ctx, w.binary, worktreeRemoveForceArgs(repo, path)...) + _, removeErr := w.run(ctx, w.binary, worktreeRemoveArgs(repo, path)...) if _, err := w.run(ctx, w.binary, worktreePruneArgs(repo)...); err != nil { return fmt.Errorf("gitworktree: worktree prune: %w", err) } @@ -304,15 +304,36 @@ func validateConfig(cfg ports.WorkspaceConfig) error { if cfg.ProjectID == "" { return errors.New("gitworktree: project id is required") } + if err := validatePathComponent("project id", string(cfg.ProjectID)); err != nil { + return err + } if cfg.SessionID == "" { return errors.New("gitworktree: session id is required") } + if err := validatePathComponent("session id", string(cfg.SessionID)); err != nil { + return err + } if cfg.Branch == "" { return errors.New("gitworktree: branch is required") } return nil } +// validatePathComponent rejects id values that could escape the managed root +// once joined into a path. filepath.Join cleans `..` before validateManagedPath +// runs, so a session id of "../other" would otherwise resolve back inside +// managedRoot while breaking per-project isolation. Reject any path separator +// or the special `.`/`..` components at the source. +func validatePathComponent(name, value string) error { + if strings.ContainsAny(value, `/\`) { + return fmt.Errorf("%w: %s %q must not contain path separators", ErrUnsafePath, name, value) + } + if value == "." || value == ".." { + return fmt.Errorf("%w: %s %q must not be a path-traversal component", ErrUnsafePath, name, value) + } + return nil +} + func (w *Workspace) managedPath(project domain.ProjectID, session domain.SessionID) (string, error) { path := filepath.Join(w.managedRoot, string(project), string(session)) return w.validateManagedPath(path) diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index 7e56529de..afa7872f4 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -27,7 +27,10 @@ func TestCommandArgs(t *testing.T) { {"rev parse", revParseVerifyArgs(repo, "origin/main"), []string{"-C", repo, "rev-parse", "--verify", "--quiet", "origin/main"}}, {"add existing", chooseWorktreeAddArgs(repo, path, branch, "", true), []string{"-C", repo, "worktree", "add", path, branch}}, {"add new", chooseWorktreeAddArgs(repo, path, branch, "origin/main", false), []string{"-C", repo, "worktree", "add", "-b", branch, path, "origin/main"}}, - {"remove", worktreeRemoveForceArgs(repo, path), []string{"-C", repo, "worktree", "remove", "--force", path}}, + // No --force: a dirty worktree must cause `git worktree remove` to fail so + // the post-prune safety check surfaces the refusal instead of deleting + // uncommitted agent work (review item RA). + {"remove", worktreeRemoveArgs(repo, path), []string{"-C", repo, "worktree", "remove", path}}, {"prune", worktreePruneArgs(repo), []string{"-C", repo, "worktree", "prune"}}, {"list", worktreeListPorcelainArgs(repo), []string{"-C", repo, "worktree", "list", "--porcelain"}}, } @@ -126,6 +129,57 @@ func TestManagedPathSafety(t *testing.T) { } } +// TestValidateConfigRejectsPathEscapingIDs covers review item RB: filepath.Join +// in managedPath cleans `..` segments before validateManagedPath sees them, so a +// session id of "../other" would stay inside managedRoot while jumping projects. +// validateConfig must reject these at the source — before any path is composed. +func TestValidateConfigRejectsPathEscapingIDs(t *testing.T) { + root := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": root}}) + if err != nil { + t.Fatalf("new: %v", err) + } + cases := []struct { + name string + cfg ports.WorkspaceConfig + }{ + {"session contains slash escapes project root", ports.WorkspaceConfig{ProjectID: "proj", SessionID: "../other", Branch: "main"}}, + {"session is .. is rejected", ports.WorkspaceConfig{ProjectID: "proj", SessionID: "..", Branch: "main"}}, + {"session is . is rejected", ports.WorkspaceConfig{ProjectID: "proj", SessionID: ".", Branch: "main"}}, + {"session contains backslash is rejected", ports.WorkspaceConfig{ProjectID: "proj", SessionID: `evil\sess`, Branch: "main"}}, + {"project contains slash escapes managed root", ports.WorkspaceConfig{ProjectID: "../proj", SessionID: "sess", Branch: "main"}}, + {"project is .. is rejected", ports.WorkspaceConfig{ProjectID: "..", SessionID: "sess", Branch: "main"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Create rejects it directly through validateConfig. + if _, err := ws.Create(context.Background(), tc.cfg); !errors.Is(err, ErrUnsafePath) { + t.Fatalf("Create err = %v, want ErrUnsafePath", err) + } + // Restore also goes through validateConfig, so the same guarantee holds. + if _, err := ws.Restore(context.Background(), tc.cfg); !errors.Is(err, ErrUnsafePath) { + t.Fatalf("Restore err = %v, want ErrUnsafePath", err) + } + }) + } +} + +// TestValidateConfigAcceptsBenignIDs is a positive guard so the rejection rule +// above does not creep into normal session/project naming. Hyphens, underscores, +// dots inside (e.g. "foo.bar"), and digits all stay allowed. +func TestValidateConfigAcceptsBenignIDs(t *testing.T) { + cases := []ports.WorkspaceConfig{ + {ProjectID: "proj-1", SessionID: "sess_2", Branch: "main"}, + {ProjectID: "foo.bar", SessionID: "abc-42", Branch: "main"}, + {ProjectID: "p", SessionID: "..hidden", Branch: "main"}, // leading dots != ".." + } + for i, cfg := range cases { + if err := validateConfig(cfg); err != nil { + t.Errorf("case %d %+v: unexpected error: %v", i, cfg, err) + } + } +} + func TestRestoreRefusesNonEmptyUnregisteredPath(t *testing.T) { root := t.TempDir() repo := t.TempDir() @@ -157,10 +211,12 @@ func TestDestroyRefusesStillRegisteredPathAndPreservesDirectory(t *testing.T) { if err := mkdirFile(path, "keep.txt"); err != nil { t.Fatalf("seed path: %v", err) } + var removeArgs []string ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { joined := strings.Join(args, " ") switch { case strings.Contains(joined, "worktree remove"): + removeArgs = append([]string{}, args...) return []byte("locked"), errors.New("remove failed") case strings.Contains(joined, "worktree prune"): return nil, nil @@ -177,6 +233,14 @@ func TestDestroyRefusesStillRegisteredPathAndPreservesDirectory(t *testing.T) { if _, statErr := os.Stat(filepath.Join(path, "keep.txt")); statErr != nil { t.Fatalf("expected directory to be preserved: %v", statErr) } + // Belt-and-braces: --force must NEVER be passed to `git worktree remove` from + // Destroy. If it ever is, dirty worktrees would be deleted instead of routed + // to Skipped by the Session Manager's Cleanup (review item RA). + for _, a := range removeArgs { + if a == "--force" || a == "-f" { + t.Fatalf("git worktree remove was called with %q; --force must never be passed", a) + } + } } func mkdirFile(dir, name string) error { diff --git a/backend/internal/lifecycle/manager.go b/backend/internal/lifecycle/manager.go index bedfb3b1f..23d6a9b90 100644 --- a/backend/internal/lifecycle/manager.go +++ b/backend/internal/lifecycle/manager.go @@ -22,12 +22,17 @@ import ( ) // Metadata keys OnSpawnCompleted records for the spawned session's handles. +// +// MetaPrompt is the assembled launch prompt, persisted so a Restore that finds +// no captured agent session id can still fall back to a fresh launch with the +// same prompt rather than failing. const ( MetaBranch = "branch" MetaWorkspacePath = "workspacePath" MetaRuntimeHandleID = "runtimeHandleId" MetaRuntimeName = "runtimeName" MetaAgentSessionID = "agentSessionId" + MetaPrompt = "prompt" ) // Manager is the LCM. The Apply* pipeline persists a transition and then fires @@ -114,9 +119,15 @@ func (m *Manager) withLock(id domain.SessionID, fn func() error) error { // transition is what a persisted write produced: the canonical before and after // the full-row upsert. The ACT layer (react) derives the reaction from these. It // is nil when the pipeline made no write. +// +// projectID is captured so reaction events fired downstream (Notifier.Notify in +// executeReaction and escalate) can populate OrchestratorEvent.ProjectID — the +// human-facing event router groups events by project. Empty when the record has +// no ProjectID (e.g. test-only seeded records that omit identity). type transition struct { - beforeLC domain.CanonicalSessionLifecycle - afterLC domain.CanonicalSessionLifecycle + beforeLC domain.CanonicalSessionLifecycle + afterLC domain.CanonicalSessionLifecycle + projectID domain.ProjectID } // mutate runs the shared pipeline: load full row -> build next canonical -> @@ -150,7 +161,10 @@ func (m *Manager) mutate( if err := m.store.Upsert(ctx, rec, classifyEventType(cur, rec.Lifecycle, false)); err != nil { return err } - tr = &transition{beforeLC: cur, afterLC: rec.Lifecycle} + // ProjectID is captured straight from the record we already loaded at the + // top of this closure — identity is set once at OnSpawnInitiated and never + // mutated, so no second store roundtrip is needed for reaction events. + tr = &transition{beforeLC: cur, afterLC: rec.Lifecycle, projectID: rec.ProjectID} return nil }) return tr, err @@ -484,5 +498,8 @@ func spawnMetadata(o ports.SpawnOutcome) map[string]string { if o.AgentSessionID != "" { meta[MetaAgentSessionID] = o.AgentSessionID } + if o.Prompt != "" { + meta[MetaPrompt] = o.Prompt + } return meta } diff --git a/backend/internal/lifecycle/reactions.go b/backend/internal/lifecycle/reactions.go index 761ac4a4a..26dea5627 100644 --- a/backend/internal/lifecycle/reactions.go +++ b/backend/internal/lifecycle/reactions.go @@ -190,10 +190,16 @@ type trackerKey struct { // a few extra agent retries before re-escalating — never a missed human // notification. Keeping it out of the canonical store preserves the // truth-vs-policy split (the store holds session truth; this is ACT policy). +// +// projectID is captured at first attempt so TickEscalations — which fires from +// the reaper and has no transition on hand — can still populate ProjectID on +// the escalation event. It is set once and never overwritten; reaction-bearing +// transitions for a given session id always carry the same projectID. type reactionTracker struct { attempts int escalated bool firstAttemptAt time.Time + projectID domain.ProjectID } // react fires the ACT layer after a persisted transition: clear the tracker for @@ -239,7 +245,7 @@ func (m *Manager) react(ctx context.Context, id domain.SessionID, tr *transition } if hasAfter && (!hadBefore || changed) { - return m.executeReaction(ctx, id, afterKey, rc) + return m.executeReaction(ctx, id, tr.projectID, afterKey, rc) } return nil } @@ -272,7 +278,7 @@ func recovered(l domain.CanonicalSessionLifecycle) bool { } } -func (m *Manager) executeReaction(ctx context.Context, id domain.SessionID, key reactionKey, rc reactionContext) error { +func (m *Manager) executeReaction(ctx context.Context, id domain.SessionID, projectID domain.ProjectID, key reactionKey, rc reactionContext) error { cfg := defaultReactions[key] switch cfg.action { case actionNotify: @@ -282,6 +288,7 @@ func (m *Manager) executeReaction(ctx context.Context, id domain.SessionID, key Type: cfg.eventType, Priority: cfg.priority, SessionID: id, + ProjectID: projectID, Message: cfg.message, }) case actionAutoMerge: @@ -289,7 +296,7 @@ func (m *Manager) executeReaction(ctx context.Context, id domain.SessionID, key // later PR. An opt-in config could route a reaction here. return nil case actionSendToAgent: - return m.sendToAgent(ctx, id, key, cfg, rc) + return m.sendToAgent(ctx, id, projectID, key, cfg, rc) } return nil } @@ -297,9 +304,16 @@ func (m *Manager) executeReaction(ctx context.Context, id domain.SessionID, key // sendToAgent runs the escalation engine for an auto send-to-agent reaction: // count the attempt, escalate when the numeric cap or duration is exceeded // (silencing further auto-dispatch), else inject the message via the messenger. -func (m *Manager) sendToAgent(ctx context.Context, id domain.SessionID, key reactionKey, cfg reactionConfig, rc reactionContext) error { +func (m *Manager) sendToAgent(ctx context.Context, id domain.SessionID, projectID domain.ProjectID, key reactionKey, cfg reactionConfig, rc reactionContext) error { m.trackerMu.Lock() tk := m.trackerFor(id, key) + // Capture projectID once so the duration-based TickEscalations path — which + // has no transition on hand — can still populate ProjectID on the escalation + // event. A non-empty incoming projectID always wins, in case the tracker was + // first created from an observation that lacked one. + if projectID != "" { + tk.projectID = projectID + } if tk.escalated { m.trackerMu.Unlock() return nil // silenced until the condition clears the tracker @@ -313,7 +327,7 @@ func (m *Manager) sendToAgent(ctx context.Context, id domain.SessionID, key reac if shouldEscalate(tk, cfg, now) { tk.escalated = true m.trackerMu.Unlock() - return m.escalate(ctx, id, key) + return m.escalate(ctx, id, tk.projectID, key) } m.trackerMu.Unlock() @@ -349,11 +363,12 @@ func shouldEscalate(tk *reactionTracker, cfg reactionConfig, now time.Time) bool // escalate emits reaction.escalated and notifies the human. The caller has // already set tracker.escalated under the lock, which silences further // auto-dispatch for this reaction until the tracker clears. -func (m *Manager) escalate(ctx context.Context, id domain.SessionID, key reactionKey) error { +func (m *Manager) escalate(ctx context.Context, id domain.SessionID, projectID domain.ProjectID, key reactionKey) error { return m.notifier.Notify(ctx, ports.OrchestratorEvent{ Type: "reaction.escalated", Priority: ports.PriorityUrgent, SessionID: id, + ProjectID: projectID, Message: fmt.Sprintf("auto-handling of %q is exhausted and needs a human.", key), Data: map[string]any{"reaction": string(key)}, }) @@ -403,8 +418,9 @@ func (m *Manager) clearSessionTrackers(id domain.SessionID) { // sent outside the lock so agent/notifier latency never blocks tracker access. func (m *Manager) TickEscalations(ctx context.Context, now time.Time) error { type due struct { - id domain.SessionID - key reactionKey + id domain.SessionID + projectID domain.ProjectID + key reactionKey } var fire []due @@ -416,13 +432,13 @@ func (m *Manager) TickEscalations(ctx context.Context, now time.Time) error { cfg := defaultReactions[k.key] if cfg.escalateAfter > 0 && !tk.firstAttemptAt.IsZero() && now.Sub(tk.firstAttemptAt) >= cfg.escalateAfter { tk.escalated = true - fire = append(fire, due{id: k.id, key: k.key}) + fire = append(fire, due{id: k.id, projectID: tk.projectID, key: k.key}) } } m.trackerMu.Unlock() for _, d := range fire { - if err := m.escalate(ctx, d.id, d.key); err != nil { + if err := m.escalate(ctx, d.id, d.projectID, d.key); err != nil { return err } } diff --git a/backend/internal/lifecycle/reactions_test.go b/backend/internal/lifecycle/reactions_test.go index 942bc339b..637b1e5bd 100644 --- a/backend/internal/lifecycle/reactions_test.go +++ b/backend/internal/lifecycle/reactions_test.go @@ -446,6 +446,123 @@ func TestReaction_IncidentOverClearsAllSessionTrackers(t *testing.T) { } } +// ---- ProjectID propagation (review R11) ---- + +// TestReaction_ProjectIDOnNotifyAndEscalateEvents asserts that both Notify call +// sites in reactions.go (executeReaction's notify and escalate) carry the +// record's ProjectID. The human-facing event router groups by project, so a +// missing id would land events in the wrong bucket. +func TestReaction_ProjectIDOnNotifyAndEscalateEvents(t *testing.T) { + const proj domain.ProjectID = "acme" + + t.Run("notify path -> ProjectID populated", func(t *testing.T) { + m, store, notf, _ := newReactive() + // Seed via Upsert (not the lifecycle-only seed helper) so the record carries + // the ProjectID that mutate's transition then propagates to react. + if err := store.Upsert(ctx(), domain.SessionRecord{ + ID: sid, ProjectID: proj, Lifecycle: lcOpenPR(domain.PRReasonReviewPending), + }, ports.EventSessionCreated); err != nil { + t.Fatalf("upsert: %v", err) + } + + // approved-and-green is a notify reaction; it fires once via executeReaction. + err := m.ApplySCMObservation(ctx(), sid, ports.SCMFacts{ + Fetched: true, PRState: domain.PROpen, ReviewDecision: ports.ReviewApproved, + Mergeability: ports.Mergeability{Mergeable: true}, PRNumber: 7, + }) + if err != nil { + t.Fatalf("apply: %v", err) + } + + notf.mu.Lock() + defer notf.mu.Unlock() + var got *ports.OrchestratorEvent + for i := range notf.events { + if notf.events[i].Type == "reaction.approved-and-green" { + got = ¬f.events[i] + break + } + } + if got == nil { + t.Fatalf("expected approved-and-green notify, got events: %+v", notf.events) + } + if got.ProjectID != proj { + t.Errorf("notify ProjectID = %q, want %q", got.ProjectID, proj) + } + if got.SessionID != sid { + t.Errorf("notify SessionID = %q, want %q", got.SessionID, sid) + } + }) + + t.Run("escalate path -> ProjectID populated (numeric cap)", func(t *testing.T) { + m, store, notf, _ := newReactive() + if err := store.Upsert(ctx(), domain.SessionRecord{ + ID: sid, ProjectID: proj, Lifecycle: lcOpenPR(domain.PRReasonReviewPending), + }, ports.EventSessionCreated); err != nil { + t.Fatalf("upsert: %v", err) + } + + // Drain the ci-failed budget to numeric escalation (sendToAgent -> escalate). + for i := 0; i < 4; i++ { + failCI(t, m) + pendingCI(t, m) + } + + notf.mu.Lock() + defer notf.mu.Unlock() + var got *ports.OrchestratorEvent + for i := range notf.events { + if notf.events[i].Type == "reaction.escalated" { + got = ¬f.events[i] + break + } + } + if got == nil { + t.Fatalf("expected reaction.escalated event, got events: %+v", notf.events) + } + if got.ProjectID != proj { + t.Errorf("escalate ProjectID = %q, want %q", got.ProjectID, proj) + } + }) + + t.Run("escalate path -> ProjectID populated (TickEscalations duration)", func(t *testing.T) { + m, store, notf, _ := newReactive() + if err := store.Upsert(ctx(), domain.SessionRecord{ + ID: sid, ProjectID: proj, Lifecycle: lcOpenPR(domain.PRReasonReviewPending), + }, ports.EventSessionCreated); err != nil { + t.Fatalf("upsert: %v", err) + } + + // changes-requested creates a duration-based tracker on the first send; + // TickEscalations fires escalate from a path with no transition on hand, + // so the tracker's captured ProjectID is what must surface on the event. + if err := m.ApplySCMObservation(ctx(), sid, ports.SCMFacts{ + Fetched: true, PRState: domain.PROpen, ReviewDecision: ports.ReviewChangesRequested, PRNumber: 7, + }); err != nil { + t.Fatalf("apply: %v", err) + } + if err := m.TickEscalations(ctx(), t0.Add(30*time.Minute)); err != nil { + t.Fatalf("tick: %v", err) + } + + notf.mu.Lock() + defer notf.mu.Unlock() + var got *ports.OrchestratorEvent + for i := range notf.events { + if notf.events[i].Type == "reaction.escalated" { + got = ¬f.events[i] + break + } + } + if got == nil { + t.Fatalf("expected duration-escalated event, got events: %+v", notf.events) + } + if got.ProjectID != proj { + t.Errorf("tick-escalate ProjectID = %q, want %q", got.ProjectID, proj) + } + }) +} + func sessionTrackerCount(m *Manager, id domain.SessionID) int { m.trackerMu.Lock() defer m.trackerMu.Unlock() diff --git a/backend/internal/ports/facts.go b/backend/internal/ports/facts.go index f1b0c702e..e1854facc 100644 --- a/backend/internal/ports/facts.go +++ b/backend/internal/ports/facts.go @@ -123,11 +123,17 @@ const ( // SpawnOutcome is what the Session Manager reports to the LCM after a spawn. // RuntimeHandle is the same structured handle the Runtime port returns, so no // ad-hoc string encoding is needed for later Destroy/SendMessage calls. +// +// Prompt is the assembled launch prompt persisted as metadata so Restore can +// fall back to a fresh launch (Agent.GetLaunchCommand) when the agent's native +// session id was never captured — without it Restore would have nothing to +// resume and nothing to re-seed a fresh run with. type SpawnOutcome struct { Branch string WorkspacePath string RuntimeHandle RuntimeHandle AgentSessionID string + Prompt string } // KillReason is what the Session Manager reports to the LCM when a kill is diff --git a/backend/internal/session/manager.go b/backend/internal/session/manager.go index 4d9157398..e764f6a31 100644 --- a/backend/internal/session/manager.go +++ b/backend/internal/session/manager.go @@ -134,7 +134,10 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess return domain.Session{}, fmt.Errorf("spawn %s: on spawn initiated: %w", id, err) } - outcome := ports.SpawnOutcome{Branch: ws.Branch, WorkspacePath: ws.Path, RuntimeHandle: handle} + // Prompt is persisted via OnSpawnCompleted -> spawnMetadata so a later Restore + // can fall back to a fresh launch if the agent's native session id was never + // captured (the capture path is a separate hook that may never have run). + outcome := ports.SpawnOutcome{Branch: ws.Branch, WorkspacePath: ws.Path, RuntimeHandle: handle, Prompt: agentCfg.Prompt} if err := m.lcm.OnSpawnCompleted(ctx, id, outcome); err != nil { // The record is seeded but the runtime/workspace are about to be torn // down. The store has no delete, so route the orphan to a terminal @@ -270,13 +273,15 @@ func (m *Manager) Restore(ctx context.Context, id domain.SessionID) (domain.Sess return domain.Session{}, fmt.Errorf("restore %s: metadata: %w", id, err) } - // Resume is only possible with the agent's captured session id. Without it, - // GetRestoreCommand would produce an ambiguous "resume nothing" launch, and - // we have no stored prompt to fall back to a fresh launch — so fail early, - // before any I/O. + // Resume is only possible with the agent's captured session id; without it we + // fall back to a fresh launch using the seeded prompt persisted at spawn time + // (the agent's id-capture path is a separate hook that may never have run, so + // "no id" is the common case rather than an error). If neither is available + // there is nothing to relaunch from — fail early, before any I/O. agentSessionID := meta[lifecycle.MetaAgentSessionID] - if agentSessionID == "" { - return domain.Session{}, fmt.Errorf("restore %s: missing agent session id (cannot resume)", id) + seededPrompt := meta[lifecycle.MetaPrompt] + if agentSessionID == "" && seededPrompt == "" { + return domain.Session{}, fmt.Errorf("restore %s: no agent session id or seeded prompt (cannot resume or relaunch)", id) } ws, err := m.workspace.Restore(ctx, ports.WorkspaceConfig{ @@ -288,11 +293,15 @@ func (m *Manager) Restore(ctx context.Context, id domain.SessionID) (domain.Sess return domain.Session{}, fmt.Errorf("restore %s: workspace restore: %w", id, err) } - agentCfg := ports.AgentConfig{SessionID: id, WorkspacePath: ws.Path} + agentCfg := ports.AgentConfig{SessionID: id, WorkspacePath: ws.Path, Prompt: seededPrompt} + launchCommand := m.agent.GetRestoreCommand(agentSessionID) + if agentSessionID == "" { + launchCommand = m.agent.GetLaunchCommand(agentCfg) + } handle, err := m.runtime.Create(ctx, ports.RuntimeConfig{ SessionID: id, WorkspacePath: ws.Path, - LaunchCommand: m.agent.GetRestoreCommand(agentSessionID), + LaunchCommand: launchCommand, Env: spawnEnv(m.agent.GetEnvironment(agentCfg), id, rec.ProjectID, rec.IssueID), }) if err != nil { @@ -317,6 +326,7 @@ func (m *Manager) Restore(ctx context.Context, id domain.SessionID) (domain.Sess WorkspacePath: ws.Path, RuntimeHandle: handle, AgentSessionID: agentSessionID, + Prompt: seededPrompt, } if err := m.lcm.OnSpawnCompleted(ctx, id, outcome); err != nil { m.rollbackRuntime(ctx, handle) diff --git a/backend/internal/session/manager_test.go b/backend/internal/session/manager_test.go index 381fb57e0..5bb20d07b 100644 --- a/backend/internal/session/manager_test.go +++ b/backend/internal/session/manager_test.go @@ -82,13 +82,16 @@ func TestSpawn_HappyPath(t *testing.T) { } } - // Handles persisted to metadata for later teardown/restore. + // Handles persisted to metadata for later teardown/restore. The prompt is + // persisted too so a later Restore that finds no captured agent session id + // can still fall back to a fresh launch using the same prompt. meta, _ := h.store.GetMetadata(ctx, "sess-1") for k, want := range map[string]string{ lifecycle.MetaBranch: "feat/42", lifecycle.MetaWorkspacePath: "/tmp/ws/sess-1", lifecycle.MetaRuntimeHandleID: "rt-sess-1", lifecycle.MetaRuntimeName: "tmux", + lifecycle.MetaPrompt: "do the thing\n\nbe careful", } { if meta[k] != want { t.Errorf("meta[%q] = %q, want %q", k, meta[k], want) @@ -431,7 +434,7 @@ func TestRestore_RelaunchesWithResumeCommand(t *testing.T) { } } -func TestRestore_MissingAgentSessionID_Errors(t *testing.T) { +func TestRestore_NoAgentSessionID_FreshLaunchFallback(t *testing.T) { h := newHarness("sess-1") ctx := context.Background() if _, err := h.sm.Spawn(ctx, spawnCfg()); err != nil { @@ -440,13 +443,45 @@ func TestRestore_MissingAgentSessionID_Errors(t *testing.T) { if _, err := h.sm.Kill(ctx, "sess-1", ports.KillOptions{Reason: ports.KillManual}); err != nil { t.Fatalf("kill: %v", err) } - // No agent session id was ever captured (spawn leaves it empty) — resume is - // impossible, so Restore must fail early without touching workspace/runtime. + // No agent session id was ever captured (the capture hook is a separate + // path that may never have run), but Spawn persisted the prompt, so Restore + // must fall back to a fresh launch instead of failing. + createdBefore := len(h.runtime.created) + + sess, err := h.sm.Restore(ctx, "sess-1") + if err != nil { + t.Fatalf("restore: %v", err) + } + if sess.Status != domain.StatusSpawning { + t.Errorf("status = %q, want spawning", sess.Status) + } + if len(h.runtime.created) != createdBefore+1 { + t.Fatalf("runtime.created grew by %d, want 1 (fresh-launch fallback)", len(h.runtime.created)-createdBefore) + } + // Fresh launch uses GetLaunchCommand (returns "claude" in the fake) — not + // the resume command, which would have read "claude --resume ". + if got := h.runtime.created[createdBefore].LaunchCommand; got != "claude" { + t.Errorf("restore launch command = %q, want fresh-launch %q", got, "claude") + } +} + +func TestRestore_NoIDAndNoPrompt_Errors(t *testing.T) { + h := newHarness("sess-1") + ctx := context.Background() + // Seed a terminal record directly without any metadata — no agent session id, + // no prompt. Restore has nothing to resume and nothing to relaunch from, so + // it must fail early without touching workspace/runtime. + if err := h.store.Upsert(ctx, domain.SessionRecord{ + ID: "sess-1", ProjectID: testProject, + Lifecycle: lc(domain.SessionTerminated, domain.ReasonManuallyKilled, domain.PRNone, ""), + }, ports.EventSessionCreated); err != nil { + t.Fatalf("upsert: %v", err) + } beforeRestores := len(h.workspace.restoredID) beforeCreated := len(h.runtime.created) if _, err := h.sm.Restore(ctx, "sess-1"); err == nil { - t.Fatal("restore: want error for missing agent session id, got nil") + t.Fatal("restore: want error for missing agent session id and prompt, got nil") } if len(h.workspace.restoredID) != beforeRestores { t.Error("workspace was touched despite a doomed restore")