bkinnightskytw
/
agent-orchestrator
mirror of https://github.com/ComposioHQ/agent-orchestrator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: address LCM/SM review blockers R1, RA, R11, RB 

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) <noreply@anthropic.com>
This commit is contained in:
harshitsinghbhandari 2026-05-28 19:49:56 +05:30
parent 2b4af15605
commit 650ecdf08a
9 changed files with 322 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
backend/internal/adapters/workspace/gitworktree/commands.go
View File

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

23
backend/internal/adapters/workspace/gitworktree/workspace.go
View File

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

66
backend/internal/adapters/workspace/gitworktree/workspace_test.go
View File

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

23
backend/internal/lifecycle/manager.go
View File

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

36
backend/internal/lifecycle/reactions.go
View File

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

117
backend/internal/lifecycle/reactions_test.go
View File

@ -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 = &notf.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 = &notf.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 = &notf.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()

6
backend/internal/ports/facts.go
View File

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

28
backend/internal/session/manager.go
View File

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

45
backend/internal/session/manager_test.go
View File

@ -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 <id>".
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")