mirror of
https://github.com/humanlayer/humanlayer.git
synced 2025-08-20 19:01:22 +03:00
Merge pull request #432 from samdickson22/sam/eng-1891-improve-session-recovery-allow-recovery-of-more-types-of
sam/eng-1891 - fix: enable resuming failed sessions with proper message delivery
This commit is contained in:
@@ -69,7 +69,7 @@ func (c *Client) buildArgs(config SessionConfig) ([]string, error) {
|
||||
args := []string{}
|
||||
|
||||
// Always use print mode for SDK
|
||||
args = append(args, "--print", config.Query)
|
||||
args = append(args, "--print")
|
||||
|
||||
// Title is stored in our database but not passed to Claude CLI
|
||||
// (Claude doesn't support --title flag)
|
||||
@@ -153,6 +153,11 @@ func (c *Client) buildArgs(config SessionConfig) ([]string, error) {
|
||||
args = append(args, "--verbose")
|
||||
}
|
||||
|
||||
// Query must be passed as a positional argument at the end
|
||||
if config.Query != "" {
|
||||
args = append(args, config.Query)
|
||||
}
|
||||
|
||||
return args, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -117,14 +117,14 @@ func TestIntegrationContinueSession(t *testing.T) {
|
||||
return nil, fmt.Errorf("no result in response")
|
||||
}
|
||||
|
||||
t.Run("ContinueSession_RequiresCompletedOrRunningParent", func(t *testing.T) {
|
||||
// Create a parent session that's failed (should be rejected)
|
||||
parentSessionID := "parent-failed"
|
||||
t.Run("ContinueSession_AllowsFailedSessionWithClaudeID", func(t *testing.T) {
|
||||
// Create a parent session that's failed with valid claude_session_id (should be allowed)
|
||||
parentSessionID := "parent-failed-valid"
|
||||
parentSession := &store.Session{
|
||||
ID: parentSessionID,
|
||||
RunID: "run-parent",
|
||||
ClaudeSessionID: "claude-parent",
|
||||
Status: store.SessionStatusFailed, // Neither completed nor running
|
||||
Status: store.SessionStatusFailed,
|
||||
Query: "original query",
|
||||
WorkingDir: "/tmp",
|
||||
CreatedAt: time.Now(),
|
||||
@@ -136,7 +136,44 @@ func TestIntegrationContinueSession(t *testing.T) {
|
||||
t.Fatalf("Failed to create parent session: %v", err)
|
||||
}
|
||||
|
||||
// Try to continue the failed session
|
||||
// Try to continue the failed session - should now succeed (or fail with Claude launch error)
|
||||
req := rpc.ContinueSessionRequest{
|
||||
SessionID: parentSessionID,
|
||||
Query: "continue this",
|
||||
}
|
||||
|
||||
_, err := sendRPC(t, "continueSession", req)
|
||||
if err != nil {
|
||||
// Expected - Claude binary might not exist in test environment
|
||||
expectedErr1 := "failed to continue session: failed to launch resumed Claude session: failed to start claude: exec: \"claude\": executable file not found in $PATH"
|
||||
expectedErr2 := "failed to continue session: failed to launch resumed Claude session: failed to start claude: chdir"
|
||||
if err.Error() != expectedErr1 && !strings.Contains(err.Error(), expectedErr2) {
|
||||
t.Errorf("Unexpected error: %v", err)
|
||||
}
|
||||
// Even if Claude fails to launch, the session should have been created
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("ContinueSession_RejectsFailedSessionWithoutClaudeID", func(t *testing.T) {
|
||||
// Create a parent session that's failed WITHOUT claude_session_id (should still be rejected)
|
||||
parentSessionID := "parent-failed-no-claude"
|
||||
parentSession := &store.Session{
|
||||
ID: parentSessionID,
|
||||
RunID: "run-parent-no-claude",
|
||||
ClaudeSessionID: "", // Missing claude_session_id
|
||||
Status: store.SessionStatusFailed,
|
||||
Query: "original query",
|
||||
WorkingDir: "/tmp",
|
||||
CreatedAt: time.Now(),
|
||||
LastActivityAt: time.Now(),
|
||||
}
|
||||
|
||||
// Insert parent session directly into database
|
||||
if err := d.store.CreateSession(ctx, parentSession); err != nil {
|
||||
t.Fatalf("Failed to create parent session: %v", err)
|
||||
}
|
||||
|
||||
// Try to continue the failed session without claude_session_id
|
||||
req := rpc.ContinueSessionRequest{
|
||||
SessionID: parentSessionID,
|
||||
Query: "continue this",
|
||||
@@ -144,9 +181,43 @@ func TestIntegrationContinueSession(t *testing.T) {
|
||||
|
||||
_, err := sendRPC(t, "continueSession", req)
|
||||
if err == nil {
|
||||
t.Error("Expected error when continuing failed session")
|
||||
t.Error("Expected error when continuing failed session without claude_session_id")
|
||||
}
|
||||
if err.Error() != "cannot continue session with status failed (must be completed, interrupted, or running)" {
|
||||
if err.Error() != "parent session missing claude_session_id (cannot resume)" {
|
||||
t.Errorf("Unexpected error: %v", err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("ContinueSession_RejectsInvalidStatus", func(t *testing.T) {
|
||||
// Create a parent session with an invalid status (e.g., starting)
|
||||
parentSessionID := "parent-invalid-status"
|
||||
parentSession := &store.Session{
|
||||
ID: parentSessionID,
|
||||
RunID: "run-invalid",
|
||||
ClaudeSessionID: "claude-invalid",
|
||||
Status: store.SessionStatusStarting, // Invalid status for continuation
|
||||
Query: "original query",
|
||||
WorkingDir: "/tmp",
|
||||
CreatedAt: time.Now(),
|
||||
LastActivityAt: time.Now(),
|
||||
}
|
||||
|
||||
// Insert parent session
|
||||
if err := d.store.CreateSession(ctx, parentSession); err != nil {
|
||||
t.Fatalf("Failed to create parent session: %v", err)
|
||||
}
|
||||
|
||||
// Try to continue with invalid status
|
||||
req := rpc.ContinueSessionRequest{
|
||||
SessionID: parentSessionID,
|
||||
Query: "continue this",
|
||||
}
|
||||
|
||||
_, err := sendRPC(t, "continueSession", req)
|
||||
if err == nil {
|
||||
t.Error("Expected error when continuing session with invalid status")
|
||||
}
|
||||
if err.Error() != "cannot continue session with status starting (must be completed, interrupted, running, or failed)" {
|
||||
t.Errorf("Unexpected error: %v", err)
|
||||
}
|
||||
})
|
||||
|
||||
@@ -231,14 +231,13 @@ func TestIntegrationResumeDuringRunning(t *testing.T) {
|
||||
{
|
||||
name: "failed session",
|
||||
status: store.SessionStatusFailed,
|
||||
shouldSucceed: false,
|
||||
expectedError: "cannot continue session with status failed (must be completed, interrupted, or running)",
|
||||
shouldSucceed: true, // Now allowed
|
||||
},
|
||||
{
|
||||
name: "starting session",
|
||||
status: store.SessionStatusStarting,
|
||||
shouldSucceed: false,
|
||||
expectedError: "cannot continue session with status starting (must be completed, interrupted, or running)",
|
||||
expectedError: "cannot continue session with status starting (must be completed, interrupted, running, or failed)",
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
@@ -1084,11 +1084,12 @@ func (m *Manager) ContinueSession(ctx context.Context, req ContinueSessionConfig
|
||||
return nil, fmt.Errorf("failed to get parent session: %w", err)
|
||||
}
|
||||
|
||||
// Validate parent session status - allow completed, interrupted, or running sessions
|
||||
// Validate parent session status - allow completed, interrupted, running, or failed sessions
|
||||
if parentSession.Status != store.SessionStatusCompleted &&
|
||||
parentSession.Status != store.SessionStatusInterrupted &&
|
||||
parentSession.Status != store.SessionStatusRunning {
|
||||
return nil, fmt.Errorf("cannot continue session with status %s (must be completed, interrupted, or running)", parentSession.Status)
|
||||
parentSession.Status != store.SessionStatusRunning &&
|
||||
parentSession.Status != store.SessionStatusFailed {
|
||||
return nil, fmt.Errorf("cannot continue session with status %s (must be completed, interrupted, running, or failed)", parentSession.Status)
|
||||
}
|
||||
|
||||
// Validate parent session has claude_session_id (needed for resume)
|
||||
@@ -1277,8 +1278,21 @@ func (m *Manager) ContinueSession(ctx context.Context, req ContinueSessionConfig
|
||||
}
|
||||
|
||||
// Launch resumed Claude session
|
||||
slog.Info("attempting to resume Claude session",
|
||||
"session_id", sessionID,
|
||||
"parent_session_id", req.ParentSessionID,
|
||||
"parent_status", parentSession.Status,
|
||||
"claude_session_id", parentSession.ClaudeSessionID,
|
||||
"query", req.Query)
|
||||
|
||||
claudeSession, err := m.client.Launch(config)
|
||||
if err != nil {
|
||||
slog.Error("failed to resume Claude session from failed parent",
|
||||
"session_id", sessionID,
|
||||
"parent_session_id", req.ParentSessionID,
|
||||
"parent_status", parentSession.Status,
|
||||
"claude_session_id", parentSession.ClaudeSessionID,
|
||||
"error", err)
|
||||
m.updateSessionStatus(ctx, sessionID, StatusFailed, err.Error())
|
||||
return nil, fmt.Errorf("failed to launch resumed Claude session: %w", err)
|
||||
}
|
||||
|
||||
@@ -151,22 +151,26 @@ func TestContinueSession_ValidatesParentStatus(t *testing.T) {
|
||||
testCases := []struct {
|
||||
name string
|
||||
parentStatus string
|
||||
hasWorkingDir bool
|
||||
expectedError string
|
||||
}{
|
||||
{
|
||||
name: "failed session",
|
||||
name: "failed session without working_dir",
|
||||
parentStatus: store.SessionStatusFailed,
|
||||
expectedError: "cannot continue session with status failed (must be completed, interrupted, or running)",
|
||||
hasWorkingDir: false,
|
||||
expectedError: "parent session missing working_dir (cannot resume session without working directory)",
|
||||
},
|
||||
{
|
||||
name: "starting session",
|
||||
parentStatus: store.SessionStatusStarting,
|
||||
expectedError: "cannot continue session with status starting (must be completed, interrupted, or running)",
|
||||
hasWorkingDir: true,
|
||||
expectedError: "cannot continue session with status starting (must be completed, interrupted, running, or failed)",
|
||||
},
|
||||
{
|
||||
name: "waiting input session",
|
||||
parentStatus: store.SessionStatusWaitingInput,
|
||||
expectedError: "cannot continue session with status waiting_input (must be completed, interrupted, or running)",
|
||||
hasWorkingDir: true,
|
||||
expectedError: "cannot continue session with status waiting_input (must be completed, interrupted, running, or failed)",
|
||||
},
|
||||
}
|
||||
|
||||
@@ -180,6 +184,9 @@ func TestContinueSession_ValidatesParentStatus(t *testing.T) {
|
||||
Query: "original query",
|
||||
CreatedAt: time.Now(),
|
||||
}
|
||||
if tc.hasWorkingDir {
|
||||
parentSession.WorkingDir = "/tmp"
|
||||
}
|
||||
mockStore.EXPECT().GetSession(gomock.Any(), "parent-1").Return(parentSession, nil)
|
||||
|
||||
req := ContinueSessionConfig{
|
||||
@@ -197,6 +204,64 @@ func TestContinueSession_ValidatesParentStatus(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestContinueSession_AllowsFailedSessionWithValidRequirements(t *testing.T) {
|
||||
ctrl := gomock.NewController(t)
|
||||
defer ctrl.Finish()
|
||||
|
||||
// Create a context that gets cancelled when test finishes
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer func() {
|
||||
cancel()
|
||||
// Give goroutines a moment to clean up
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
}()
|
||||
|
||||
mockStore := store.NewMockConversationStore(ctrl)
|
||||
manager, _ := NewManager(nil, mockStore, "")
|
||||
|
||||
// Create a failed parent session WITH valid claude_session_id and working_dir
|
||||
failedParentSession := &store.Session{
|
||||
ID: "parent-failed-valid",
|
||||
RunID: "run-failed",
|
||||
ClaudeSessionID: "claude-failed-valid", // Has valid session ID
|
||||
Status: store.SessionStatusFailed,
|
||||
Query: "original query that failed",
|
||||
WorkingDir: "/tmp", // Has valid working directory
|
||||
CreatedAt: time.Now(),
|
||||
}
|
||||
|
||||
mockStore.EXPECT().GetSession(gomock.Any(), "parent-failed-valid").Return(failedParentSession, nil)
|
||||
mockStore.EXPECT().GetMCPServers(gomock.Any(), "parent-failed-valid").Return([]store.MCPServer{}, nil)
|
||||
mockStore.EXPECT().CreateSession(gomock.Any(), gomock.Any()).DoAndReturn(
|
||||
func(ctx interface{}, session *store.Session) error {
|
||||
// Validate the created session
|
||||
if session.ParentSessionID != "parent-failed-valid" {
|
||||
t.Errorf("Expected parent_session_id to be 'parent-failed-valid', got '%s'", session.ParentSessionID)
|
||||
}
|
||||
if session.Query != "retry after failure" {
|
||||
t.Errorf("Expected query 'retry after failure', got '%s'", session.Query)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
mockStore.EXPECT().StoreMCPServers(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
|
||||
mockStore.EXPECT().UpdateSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
|
||||
|
||||
req := ContinueSessionConfig{
|
||||
ParentSessionID: "parent-failed-valid",
|
||||
Query: "retry after failure",
|
||||
}
|
||||
|
||||
// This should now succeed (or fail with Claude launch error, not validation error)
|
||||
_, err := manager.ContinueSession(ctx, req)
|
||||
if err != nil {
|
||||
// Expected - Claude binary might not exist
|
||||
if !containsError(err, "failed to launch resumed Claude session") {
|
||||
t.Errorf("Unexpected error: %v", err)
|
||||
}
|
||||
}
|
||||
// The important thing is we didn't get a validation error about the failed status
|
||||
}
|
||||
|
||||
func TestContinueSession_ValidatesClaudeSessionID(t *testing.T) {
|
||||
ctrl := gomock.NewController(t)
|
||||
defer ctrl.Finish()
|
||||
|
||||
@@ -770,7 +770,7 @@ function SessionDetail({ session, onClose }: SessionDetailProps) {
|
||||
useHotkeys(
|
||||
'enter',
|
||||
() => {
|
||||
if (responseInputRef.current && session.status !== SessionStatus.Failed) {
|
||||
if (responseInputRef.current) {
|
||||
responseInputRef.current.focus()
|
||||
}
|
||||
},
|
||||
@@ -940,7 +940,6 @@ function SessionDetail({ session, onClose }: SessionDetailProps) {
|
||||
onSelectEvent={handleForkSelect}
|
||||
isOpen={forkViewOpen}
|
||||
onOpenChange={setForkViewOpen}
|
||||
sessionStatus={session.status}
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
@@ -1031,7 +1030,6 @@ function SessionDetail({ session, onClose }: SessionDetailProps) {
|
||||
onSelectEvent={handleForkSelect}
|
||||
isOpen={forkViewOpen}
|
||||
onOpenChange={setForkViewOpen}
|
||||
sessionStatus={session.status}
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
@@ -1132,7 +1130,6 @@ function SessionDetail({ session, onClose }: SessionDetailProps) {
|
||||
handleContinueSession={actions.handleContinueSession}
|
||||
handleResponseInputKeyDown={actions.handleResponseInputKeyDown}
|
||||
isForkMode={actions.isForkMode}
|
||||
onOpenForkView={() => setForkViewOpen(true)}
|
||||
/>
|
||||
{/* Session mode indicator - shows either dangerous skip permissions or auto-accept */}
|
||||
<SessionModeIndicator
|
||||
|
||||
@@ -10,7 +10,7 @@ import {
|
||||
DialogTrigger,
|
||||
} from '@/components/ui/dialog'
|
||||
import { Button } from '@/components/ui/button'
|
||||
import { ConversationEvent, SessionStatus } from '@/lib/daemon/types'
|
||||
import { ConversationEvent } from '@/lib/daemon/types'
|
||||
import { cn } from '@/lib/utils'
|
||||
import { useStealHotkeyScope } from '@/hooks/useStealHotkeyScope'
|
||||
|
||||
@@ -22,7 +22,6 @@ interface ForkViewModalProps {
|
||||
onSelectEvent: (index: number | null) => void
|
||||
isOpen: boolean
|
||||
onOpenChange: (open: boolean) => void
|
||||
sessionStatus?: SessionStatus
|
||||
}
|
||||
|
||||
function ForkViewModalContent({
|
||||
@@ -30,7 +29,6 @@ function ForkViewModalContent({
|
||||
selectedEventIndex,
|
||||
onSelectEvent,
|
||||
onClose,
|
||||
sessionStatus,
|
||||
}: Omit<ForkViewModalProps, 'isOpen' | 'onOpenChange'> & { onClose: () => void }) {
|
||||
// Steal hotkey scope when this component mounts
|
||||
useStealHotkeyScope(ForkViewModalHotkeysScope)
|
||||
@@ -57,8 +55,8 @@ function ForkViewModalContent({
|
||||
.filter(({ event }) => event.eventType === 'message' && event.role === 'user')
|
||||
.slice(1) // Exclude first message since it can't be forked
|
||||
|
||||
// Add current option as a special index (-1) only if session is not failed
|
||||
const showCurrentOption = sessionStatus !== SessionStatus.Failed
|
||||
// Add current option as a special index (-1) for all sessions
|
||||
const showCurrentOption = true
|
||||
const allOptions = showCurrentOption
|
||||
? [...userMessageIndices, { event: null, index: -1 }]
|
||||
: userMessageIndices
|
||||
@@ -193,7 +191,7 @@ function ForkViewModalContent({
|
||||
)
|
||||
})}
|
||||
|
||||
{/* Current option - only show if session is not failed */}
|
||||
{/* Current option */}
|
||||
{showCurrentOption && (
|
||||
<div className="border-t mt-2 pt-2">
|
||||
<div
|
||||
@@ -239,7 +237,6 @@ export function ForkViewModal({
|
||||
onSelectEvent,
|
||||
isOpen,
|
||||
onOpenChange,
|
||||
sessionStatus,
|
||||
}: ForkViewModalProps) {
|
||||
return (
|
||||
<Dialog open={isOpen} onOpenChange={onOpenChange}>
|
||||
@@ -272,7 +269,6 @@ export function ForkViewModal({
|
||||
selectedEventIndex={selectedEventIndex}
|
||||
onSelectEvent={onSelectEvent}
|
||||
onClose={() => onOpenChange(false)}
|
||||
sessionStatus={sessionStatus}
|
||||
/>
|
||||
)}
|
||||
</DialogContent>
|
||||
|
||||
@@ -3,12 +3,10 @@ import { Button } from '@/components/ui/button'
|
||||
import { Textarea } from '@/components/ui/textarea'
|
||||
import { Session, SessionStatus } from '@/lib/daemon/types'
|
||||
import {
|
||||
getSessionStatusText,
|
||||
getInputPlaceholder,
|
||||
getHelpText,
|
||||
getForkInputPlaceholder,
|
||||
} from '@/components/internal/SessionDetail/utils/sessionStatus'
|
||||
import { GitBranch } from 'lucide-react'
|
||||
import { ResponseInputLocalStorageKey } from '@/components/internal/SessionDetail/hooks/useSessionActions'
|
||||
|
||||
interface ResponseInputProps {
|
||||
@@ -19,7 +17,6 @@ interface ResponseInputProps {
|
||||
handleContinueSession: () => void
|
||||
handleResponseInputKeyDown: (e: React.KeyboardEvent) => void
|
||||
isForkMode?: boolean
|
||||
onOpenForkView?: () => void
|
||||
}
|
||||
|
||||
export const ResponseInput = forwardRef<HTMLTextAreaElement, ResponseInputProps>(
|
||||
@@ -32,7 +29,6 @@ export const ResponseInput = forwardRef<HTMLTextAreaElement, ResponseInputProps>
|
||||
handleContinueSession,
|
||||
handleResponseInputKeyDown,
|
||||
isForkMode,
|
||||
onOpenForkView,
|
||||
},
|
||||
ref,
|
||||
) => {
|
||||
@@ -64,22 +60,7 @@ export const ResponseInput = forwardRef<HTMLTextAreaElement, ResponseInputProps>
|
||||
// Regular help text
|
||||
return getHelpText(session.status)
|
||||
}
|
||||
// Only show the simple status text if session is failed AND not in fork mode
|
||||
if (session.status === SessionStatus.Failed && !isForkMode) {
|
||||
return (
|
||||
<div className="flex items-center justify-between py-1">
|
||||
<span className="text-sm text-muted-foreground">{getSessionStatusText(session.status)}</span>
|
||||
{onOpenForkView && (
|
||||
<Button variant="ghost" size="sm" onClick={onOpenForkView} className="h-8 gap-2">
|
||||
<GitBranch className="h-4 w-4" />
|
||||
Fork from previous
|
||||
</Button>
|
||||
)}
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
// Otherwise always show the input
|
||||
// Always show the input for all session states
|
||||
return (
|
||||
<div className="space-y-2">
|
||||
{isForkMode && <span className="text-sm font-medium">Fork from this message:</span>}
|
||||
|
||||
@@ -11,19 +11,19 @@ export const Kbd = ({
|
||||
export const getSessionStatusText = (status: string): string => {
|
||||
if (status === 'completed') return 'Continue this conversation with a new message'
|
||||
if (status === 'interrupted') return 'Session was interrupted - continue with a new message'
|
||||
if (status === 'failed') return 'Session failed - continue with a new message to retry'
|
||||
if (status === 'running' || status === 'starting')
|
||||
return 'Claude is working - you can interrupt with a new message'
|
||||
return 'Session must be completed to continue'
|
||||
}
|
||||
|
||||
export const getInputPlaceholder = (status: string): string => {
|
||||
if (status === 'failed') return 'Session failed - cannot continue...'
|
||||
if (status === 'failed') return 'Enter your message to retry from where it failed...'
|
||||
if (status === 'running' || status === 'starting') return 'Enter message to interrupt...'
|
||||
return 'Enter your message to continue the conversation...'
|
||||
}
|
||||
|
||||
export const getHelpText = (status: string): React.ReactNode => {
|
||||
if (status === 'failed') return 'Session failed - cannot continue'
|
||||
const isMac = navigator.platform.includes('Mac')
|
||||
const sendKey = isMac ? '⌘+Enter' : 'Ctrl+Enter'
|
||||
const skipKey = isMac ? '⌥+Y' : 'Alt+Y'
|
||||
|
||||
Reference in New Issue
Block a user