Merge pull request #3675 from code-yeongyu/fix/ralph-loop-remove-recovery-window
fix(ralph-loop): remove redundant recovery window that stalls loop after errors (fixes #3235)
This commit is contained in:
@@ -386,8 +386,8 @@ describe("ralph-loop", () => {
|
||||
expect(hook.getState()).not.toBeNull()
|
||||
})
|
||||
|
||||
test("should skip injection during recovery", async () => {
|
||||
// given - active loop and session in recovery
|
||||
test("should continue after non-abort session error", async () => {
|
||||
// given - active loop and non-abort session error
|
||||
const hook = createRalphLoopHook(createMockPluginInput())
|
||||
hook.startLoop("session-123", "Test task")
|
||||
|
||||
@@ -398,7 +398,7 @@ describe("ralph-loop", () => {
|
||||
},
|
||||
})
|
||||
|
||||
// when - session goes idle immediately
|
||||
// when - session goes idle immediately after the error
|
||||
await hook.event({
|
||||
event: {
|
||||
type: "session.idle",
|
||||
@@ -406,8 +406,9 @@ describe("ralph-loop", () => {
|
||||
},
|
||||
})
|
||||
|
||||
// then - no continuation injected
|
||||
expect(promptCalls.length).toBe(0)
|
||||
// then - continuation is injected without a recovery skip
|
||||
expect(promptCalls.length).toBe(1)
|
||||
expect(hook.getState()?.iteration).toBe(2)
|
||||
})
|
||||
|
||||
test("should clear state on session deletion", async () => {
|
||||
|
||||
@@ -1,33 +0,0 @@
|
||||
type SessionState = {
|
||||
isRecovering?: boolean
|
||||
}
|
||||
|
||||
export function createLoopSessionRecovery(options?: { recoveryWindowMs?: number }) {
|
||||
const recoveryWindowMs = options?.recoveryWindowMs ?? 5000
|
||||
const sessions = new Map<string, SessionState>()
|
||||
|
||||
function getSessionState(sessionID: string): SessionState {
|
||||
let state = sessions.get(sessionID)
|
||||
if (!state) {
|
||||
state = {}
|
||||
sessions.set(sessionID, state)
|
||||
}
|
||||
return state
|
||||
}
|
||||
|
||||
return {
|
||||
isRecovering(sessionID: string): boolean {
|
||||
return getSessionState(sessionID).isRecovering === true
|
||||
},
|
||||
markRecovering(sessionID: string): void {
|
||||
const state = getSessionState(sessionID)
|
||||
state.isRecovering = true
|
||||
setTimeout(() => {
|
||||
state.isRecovering = false
|
||||
}, recoveryWindowMs)
|
||||
},
|
||||
clear(sessionID: string): void {
|
||||
sessions.delete(sessionID)
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,96 @@
|
||||
/// <reference types="bun-types" />
|
||||
import { afterEach, beforeEach, describe, expect, test } from "bun:test"
|
||||
import { existsSync, mkdirSync, rmSync } from "node:fs"
|
||||
import { tmpdir } from "node:os"
|
||||
import { join } from "node:path"
|
||||
import { createRalphLoopHook } from "./index"
|
||||
import { clearState } from "./storage"
|
||||
|
||||
describe("ralph-loop non-abort error continuation", () => {
|
||||
const testDirectory = join(tmpdir(), `ralph-loop-non-abort-error-${Date.now()}`)
|
||||
let promptCalls: Array<{ sessionID: string; text: string }>
|
||||
let messagesCalls: Array<{ sessionID: string }>
|
||||
|
||||
beforeEach(() => {
|
||||
promptCalls = []
|
||||
messagesCalls = []
|
||||
mkdirSync(testDirectory, { recursive: true })
|
||||
clearState(testDirectory)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
clearState(testDirectory)
|
||||
if (existsSync(testDirectory)) {
|
||||
rmSync(testDirectory, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test("continues on next idle after non-abort session error", async () => {
|
||||
// given - an active Ralph Loop receives a recoverable command error
|
||||
const hook = createRalphLoopHook({
|
||||
directory: testDirectory,
|
||||
project: testDirectory,
|
||||
worktree: testDirectory,
|
||||
serverUrl: "http://localhost:4096",
|
||||
$: async () => ({}),
|
||||
client: {
|
||||
session: {
|
||||
messages: async (options: { path: { id: string } }) => {
|
||||
messagesCalls.push({ sessionID: options.path.id })
|
||||
return { data: [] }
|
||||
},
|
||||
promptAsync: async (options: {
|
||||
path: { id: string }
|
||||
body: { parts: Array<{ type: string; text: string }> }
|
||||
}) => {
|
||||
promptCalls.push({
|
||||
sessionID: options.path.id,
|
||||
text: options.body.parts[0]?.text ?? "",
|
||||
})
|
||||
return {}
|
||||
},
|
||||
prompt: async (options: {
|
||||
path: { id: string }
|
||||
body: { parts: Array<{ type: string; text: string }> }
|
||||
}) => {
|
||||
promptCalls.push({
|
||||
sessionID: options.path.id,
|
||||
text: options.body.parts[0]?.text ?? "",
|
||||
})
|
||||
return {}
|
||||
},
|
||||
},
|
||||
tui: {
|
||||
showToast: async () => ({}),
|
||||
},
|
||||
},
|
||||
} as never)
|
||||
|
||||
hook.startLoop("session-123", "Keep working", {
|
||||
messageCountAtStart: 0,
|
||||
maxIterations: 5,
|
||||
})
|
||||
|
||||
await hook.event({
|
||||
event: {
|
||||
type: "session.error",
|
||||
properties: {
|
||||
sessionID: "session-123",
|
||||
error: { name: "CommandFailedError" },
|
||||
},
|
||||
},
|
||||
})
|
||||
|
||||
// when - OpenCode emits the idle event caused by that failed command
|
||||
await hook.event({
|
||||
event: { type: "session.idle", properties: { sessionID: "session-123" } },
|
||||
})
|
||||
|
||||
// then - the loop should continue instead of skipping idle as recovery
|
||||
expect(promptCalls).toHaveLength(1)
|
||||
expect(promptCalls[0]?.sessionID).toBe("session-123")
|
||||
expect(promptCalls[0]?.text).toContain("Keep working")
|
||||
expect(messagesCalls.length).toBeGreaterThan(0)
|
||||
expect(hook.getState()?.iteration).toBe(2)
|
||||
})
|
||||
})
|
||||
@@ -11,11 +11,6 @@ import { continueIteration } from "./iteration-continuation"
|
||||
import { handlePendingVerification } from "./pending-verification-handler"
|
||||
import { handleDeletedLoopSession, handleErroredLoopSession } from "./session-event-handler"
|
||||
|
||||
type SessionRecovery = {
|
||||
isRecovering: (sessionID: string) => boolean
|
||||
markRecovering: (sessionID: string) => void
|
||||
clear: (sessionID: string) => void
|
||||
}
|
||||
type LoopStateController = {
|
||||
getState: () => RalphLoopState | null
|
||||
clear: () => boolean
|
||||
@@ -25,7 +20,7 @@ type LoopStateController = {
|
||||
setVerificationSessionID: (sessionID: string, verificationSessionID: string) => RalphLoopState | null
|
||||
restartAfterFailedVerification: (sessionID: string, messageCountAtStart?: number) => RalphLoopState | null
|
||||
}
|
||||
type RalphLoopEventHandlerOptions = { directory: string; apiTimeoutMs: number; getTranscriptPath: (sessionID: string) => string | undefined; checkSessionExists?: RalphLoopOptions["checkSessionExists"]; backgroundManager?: RalphLoopOptions["backgroundManager"]; sessionRecovery: SessionRecovery; loopState: LoopStateController }
|
||||
type RalphLoopEventHandlerOptions = { directory: string; apiTimeoutMs: number; getTranscriptPath: (sessionID: string) => string | undefined; checkSessionExists?: RalphLoopOptions["checkSessionExists"]; backgroundManager?: RalphLoopOptions["backgroundManager"]; loopState: LoopStateController }
|
||||
|
||||
export function createRalphLoopEventHandler(
|
||||
ctx: PluginInput,
|
||||
@@ -48,12 +43,6 @@ export function createRalphLoopEventHandler(
|
||||
inFlightSessions.add(sessionID)
|
||||
|
||||
try {
|
||||
|
||||
if (options.sessionRecovery.isRecovering(sessionID)) {
|
||||
log(`[${HOOK_NAME}] Skipped: in recovery`, { sessionID })
|
||||
return
|
||||
}
|
||||
|
||||
const state = options.loopState.getState()
|
||||
if (!state || !state.active) {
|
||||
return
|
||||
@@ -229,12 +218,12 @@ export function createRalphLoopEventHandler(
|
||||
}
|
||||
|
||||
if (event.type === "session.deleted") {
|
||||
if (!handleDeletedLoopSession(props, options.loopState, options.sessionRecovery)) return
|
||||
if (!handleDeletedLoopSession(props, options.loopState)) return
|
||||
return
|
||||
}
|
||||
|
||||
if (event.type === "session.error") {
|
||||
handleErroredLoopSession(props, options.loopState, options.sessionRecovery)
|
||||
handleErroredLoopSession(props, options.loopState)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
import type { PluginInput } from "@opencode-ai/plugin"
|
||||
import type { RalphLoopOptions, RalphLoopState } from "./types"
|
||||
import { getTranscriptPath as getDefaultTranscriptPath } from "../claude-code-hooks/transcript"
|
||||
import { createLoopSessionRecovery } from "./loop-session-recovery"
|
||||
import { createLoopStateController } from "./loop-state-controller"
|
||||
import { createRalphLoopEventHandler } from "./ralph-loop-event-handler"
|
||||
|
||||
@@ -53,7 +52,6 @@ export function createRalphLoopHook(
|
||||
stateDir,
|
||||
config,
|
||||
})
|
||||
const sessionRecovery = createLoopSessionRecovery()
|
||||
|
||||
const event = createRalphLoopEventHandler(ctx, {
|
||||
directory: ctx.directory,
|
||||
@@ -61,7 +59,6 @@ export function createRalphLoopHook(
|
||||
getTranscriptPath,
|
||||
checkSessionExists,
|
||||
backgroundManager,
|
||||
sessionRecovery,
|
||||
loopState,
|
||||
})
|
||||
|
||||
|
||||
@@ -7,15 +7,9 @@ type LoopStateController = {
|
||||
clear: () => boolean
|
||||
}
|
||||
|
||||
type SessionRecovery = {
|
||||
clear: (sessionID: string) => void
|
||||
markRecovering: (sessionID: string) => void
|
||||
}
|
||||
|
||||
export function handleDeletedLoopSession(
|
||||
props: Record<string, unknown> | undefined,
|
||||
loopState: LoopStateController,
|
||||
sessionRecovery: SessionRecovery,
|
||||
): boolean {
|
||||
const sessionInfo = props?.info as { id?: string } | undefined
|
||||
if (!sessionInfo?.id) return false
|
||||
@@ -25,14 +19,12 @@ export function handleDeletedLoopSession(
|
||||
loopState.clear()
|
||||
log(`[${HOOK_NAME}] Session deleted, loop cleared`, { sessionID: sessionInfo.id })
|
||||
}
|
||||
sessionRecovery.clear(sessionInfo.id)
|
||||
return true
|
||||
}
|
||||
|
||||
export function handleErroredLoopSession(
|
||||
props: Record<string, unknown> | undefined,
|
||||
loopState: LoopStateController,
|
||||
sessionRecovery: SessionRecovery,
|
||||
): boolean {
|
||||
const sessionID = props?.sessionID as string | undefined
|
||||
const error = props?.error as { name?: string } | undefined
|
||||
@@ -44,13 +36,12 @@ export function handleErroredLoopSession(
|
||||
loopState.clear()
|
||||
log(`[${HOOK_NAME}] User aborted, loop cleared`, { sessionID })
|
||||
}
|
||||
sessionRecovery.clear(sessionID)
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
if (sessionID) {
|
||||
sessionRecovery.markRecovering(sessionID)
|
||||
log(`[${HOOK_NAME}] Session error ignored, loop remains active`, { sessionID })
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user