From eee547d8913618f8137a0533f26dccff96f1cf07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 2 May 2026 22:57:13 +0100 Subject: [PATCH] refactor(handlers): use GetOrEnsureExecution for workspace ops (#786) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(handlers): use GetOrEnsureExecution for workspace ops Per CLAUDE.md, workspace-oriented handlers should use GetOrEnsureExecution so they survive backend restarts. Two git ops in this file (wsGitCommits, wsCumulativeDiff) already followed this pattern; the shared helper and the shell.subscribe handler did not. Changes: - git_handlers.getAgentCtlClient: now takes ctx and uses GetOrEnsureExecution. Propagates to all 14 git ops routing through the helper (pull, push, rebase, merge, abort, commit, stage, unstage, discard, createPR, revertCommit, renameBranch, reset, commitDiff). - shell_handlers.wsShellSubscribe: matches wsShellStatus in the same file. Left alone (intentional): - shell_handlers.wsShellInput: the 5s workspace-stream wait is too short to absorb a cold-start of agentctl, so we still require an existing execution. - passthrough_handlers: stdin/resize require a live agent process per the rule. - git_handlers post-success/failure callbacks: just look up TaskID; ensuring would be wasteful. Co-Authored-By: Claude Opus 4.7 * test(handlers): cover wsShellSubscribe no-instance path Mirrors TestWsShellInput_NoInstanceFound. Closes a coverage gap flagged in PR review — the handler was switched to GetOrEnsureExecution but had no direct tests for the lookup-failure path. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- .../internal/agent/handlers/git_handlers.go | 40 ++++++++++--------- .../internal/agent/handlers/shell_handlers.go | 13 +++--- .../agent/handlers/shell_handlers_test.go | 17 ++++++++ 3 files changed, 46 insertions(+), 24 deletions(-) diff --git a/apps/backend/internal/agent/handlers/git_handlers.go b/apps/backend/internal/agent/handlers/git_handlers.go index 3fae5190fb..4135e65a1a 100644 --- a/apps/backend/internal/agent/handlers/git_handlers.go +++ b/apps/backend/internal/agent/handlers/git_handlers.go @@ -247,7 +247,7 @@ func (h *GitHandlers) wsPull(ctx context.Context, msg *ws.Message) (*ws.Message, return nil, fmt.Errorf("session_id is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -272,7 +272,7 @@ func (h *GitHandlers) wsPush(ctx context.Context, msg *ws.Message) (*ws.Message, return nil, fmt.Errorf("session_id is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -300,7 +300,7 @@ func (h *GitHandlers) wsRebase(ctx context.Context, msg *ws.Message) (*ws.Messag return nil, fmt.Errorf("base_branch is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -328,7 +328,7 @@ func (h *GitHandlers) wsMerge(ctx context.Context, msg *ws.Message) (*ws.Message return nil, fmt.Errorf("base_branch is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -356,7 +356,7 @@ func (h *GitHandlers) wsAbort(ctx context.Context, msg *ws.Message) (*ws.Message return nil, fmt.Errorf("operation must be 'merge' or 'rebase'") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -383,7 +383,7 @@ func (h *GitHandlers) wsCommit(ctx context.Context, msg *ws.Message) (*ws.Messag return nil, fmt.Errorf("message is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -411,7 +411,7 @@ func (h *GitHandlers) wsRenameBranch(ctx context.Context, msg *ws.Message) (*ws. return nil, fmt.Errorf("new_name is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -445,7 +445,7 @@ func (h *GitHandlers) wsReset(ctx context.Context, msg *ws.Message) (*ws.Message return nil, fmt.Errorf("invalid reset mode: %s (must be soft, mixed, or hard)", req.Mode) } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -469,7 +469,7 @@ func (h *GitHandlers) wsStage(ctx context.Context, msg *ws.Message) (*ws.Message return nil, fmt.Errorf("session_id is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -493,7 +493,7 @@ func (h *GitHandlers) wsUnstage(ctx context.Context, msg *ws.Message) (*ws.Messa return nil, fmt.Errorf("session_id is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -521,7 +521,7 @@ func (h *GitHandlers) wsDiscard(ctx context.Context, msg *ws.Message) (*ws.Messa return nil, fmt.Errorf("paths are required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -548,7 +548,7 @@ func (h *GitHandlers) wsCreatePR(ctx context.Context, msg *ws.Message) (*ws.Mess return nil, fmt.Errorf("title is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -594,7 +594,7 @@ func (h *GitHandlers) wsRevertCommit(ctx context.Context, msg *ws.Message) (*ws. return nil, fmt.Errorf("commit_sha is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { return nil, err } @@ -622,7 +622,7 @@ func (h *GitHandlers) wsCommitDiff(ctx context.Context, msg *ws.Message) (*ws.Me return nil, fmt.Errorf("commit_sha is required") } - client, err := h.getAgentCtlClient(req.SessionID) + client, err := h.getAgentCtlClient(ctx, req.SessionID) if err != nil { if isSessionNotReadyError(err) { return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{ @@ -652,11 +652,13 @@ func isSessionNotReadyError(err error) bool { strings.Contains(msg, "agent client not available for session") } -// getAgentCtlClient gets the agentctl client for a session -func (h *GitHandlers) getAgentCtlClient(sessionID string) (*client.Client, error) { - execution, ok := h.lifecycleMgr.GetExecutionBySessionID(sessionID) - if !ok { - return nil, fmt.Errorf("no agent running for session %s", sessionID) +// getAgentCtlClient gets the agentctl client for a session. +// Uses GetOrEnsureExecution so git operations survive backend restarts — +// they're workspace-oriented and don't require a running agent process. +func (h *GitHandlers) getAgentCtlClient(ctx context.Context, sessionID string) (*client.Client, error) { + execution, err := h.lifecycleMgr.GetOrEnsureExecution(ctx, sessionID) + if err != nil { + return nil, fmt.Errorf("no agent running for session %s: %w", sessionID, err) } c := execution.GetAgentCtlClient() diff --git a/apps/backend/internal/agent/handlers/shell_handlers.go b/apps/backend/internal/agent/handlers/shell_handlers.go index 7247cffb82..13b9a407ca 100644 --- a/apps/backend/internal/agent/handlers/shell_handlers.go +++ b/apps/backend/internal/agent/handlers/shell_handlers.go @@ -130,10 +130,10 @@ func (h *ShellHandlers) wsShellSubscribe(ctx context.Context, msg *ws.Message) ( return nil, fmt.Errorf("session_id is required") } - // Verify the agent execution exists for this session - execution, ok := h.lifecycleMgr.GetExecutionBySessionID(req.SessionID) - if !ok { - return nil, fmt.Errorf("no agent running for session %s", req.SessionID) + // Get or create execution on-demand (survives backend restart) + execution, err := h.lifecycleMgr.GetOrEnsureExecution(ctx, req.SessionID) + if err != nil { + return nil, fmt.Errorf("no agent running for session %s: %w", req.SessionID, err) } // Get buffered output to include in response @@ -164,7 +164,10 @@ func (h *ShellHandlers) wsShellInput(ctx context.Context, msg *ws.Message) (*ws. return nil, fmt.Errorf("session_id is required") } - // Get the agent execution for this session + // Get the agent execution for this session. + // Use GetExecutionBySessionID (not GetOrEnsureExecution): writing input + // requires the workspace stream to already be live, and the 5s wait below + // is too short to absorb a cold-start of agentctl. execution, ok := h.lifecycleMgr.GetExecutionBySessionID(req.SessionID) if !ok { return nil, fmt.Errorf("no agent running for session %s", req.SessionID) diff --git a/apps/backend/internal/agent/handlers/shell_handlers_test.go b/apps/backend/internal/agent/handlers/shell_handlers_test.go index 56329c2457..93852ae070 100644 --- a/apps/backend/internal/agent/handlers/shell_handlers_test.go +++ b/apps/backend/internal/agent/handlers/shell_handlers_test.go @@ -242,6 +242,23 @@ func TestWsShellStatus_NoClientAvailable(t *testing.T) { } } +func TestWsShellSubscribe_NoInstanceFound(t *testing.T) { + log := newTestLogger() + mgr := newTestManager() + handlers := NewShellHandlers(mgr, nil, log) + + msg, _ := ws.NewRequest("test-1", ws.ActionShellSubscribe, ShellSubscribeRequest{SessionID: "session-1"}) + + _, err := handlers.wsShellSubscribe(context.Background(), msg) + if err == nil { + t.Fatal("expected error for non-existent session") + } + expectedPrefix := "no agent running for session session-1" + if !strings.HasPrefix(err.Error(), expectedPrefix) { + t.Errorf("expected prefix '%s', got: %v", expectedPrefix, err) + } +} + func TestWsShellInput_NoInstanceFound(t *testing.T) { log := newTestLogger() mgr := newTestManager()