diff --git a/.changeset/loop-guard.md b/.changeset/loop-guard.md new file mode 100644 index 000000000000..866a85ac028d --- /dev/null +++ b/.changeset/loop-guard.md @@ -0,0 +1,8 @@ +--- +"@reddb-io/redcode": patch +"@reddb-io/redcode-core": patch +--- + +Notice when the model is repeating itself, and say so instead of asking the user + +The old detector compared the last three parts of a single assistant message and required byte-identical serialized input, so one interleaved reasoning part — which reasoning models emit constantly — reset it permanently, a loop spanning steps was invisible, and when it did fire it asked a question whose wait had no bound: the only defence against a loop was itself a way to hang. It now looks across the whole turn, counts only calls that returned the same result (identical calls with different results are polling, and are left alone), and answers the repeated call itself with a correction quoting the model's own arguments and the answer it keeps ignoring. If the correction changes nothing, the turn ends. Nobody is asked anything. Configurable via `experimental.loop_guard`; a `doom_loop: "allow"` permission rule still turns it off. diff --git a/packages/core/src/v1/config/config.ts b/packages/core/src/v1/config/config.ts index 5c4ad95369d4..cf084ede2e74 100644 --- a/packages/core/src/v1/config/config.ts +++ b/packages/core/src/v1/config/config.ts @@ -182,6 +182,18 @@ export const Info = Schema.Struct({ mcp_timeout: Schema.optional(PositiveInt).annotate({ description: "Timeout in milliseconds for model context protocol (MCP) requests", }), + loop_guard: Schema.optional( + Schema.Union([ + Schema.Literal(false), + Schema.Struct({ + correct_at: Schema.optional(PositiveInt), + stop_at: Schema.optional(PositiveInt), + }), + ]), + ).annotate({ + description: + "How many identical tool calls in a row - same arguments, same result - before the model is told it is repeating itself (correct_at, default 3) and before the turn ends (stop_at, default 5). Set to false to disable.", + }), tool_timeout: Schema.optional(Schema.Union([Schema.Literal(false), PositiveInt])).annotate({ description: "Milliseconds a tool may run before it is stopped and reported to the model as a failure (default: 600000). Tools that carry their own deadline, wait for a person, or run a whole child turn are not affected. Set to false to disable.", diff --git a/packages/redcode/src/session/loop-guard.ts b/packages/redcode/src/session/loop-guard.ts new file mode 100644 index 000000000000..979987e103cb --- /dev/null +++ b/packages/redcode/src/session/loop-guard.ts @@ -0,0 +1,140 @@ +/** + * Noticing when the model has stopped making progress and is just repeating itself. + * + * The detector this replaces compared the last three *parts* of one assistant message and required + * byte-identical serialized input. A single interleaved text or reasoning part — which reasoning + * models emit constantly — reset it permanently, it could not see a loop that spanned steps, and + * when it did fire it asked the user a question whose wait had no bound: the only defence against + * a loop was itself a way to hang. + * + * What counts as a loop here is narrower and more honest: the same tool, the same arguments, and + * the same result, several times running. Identical calls that return *different* results are how + * polling looks, and are left alone. Nothing about this needs a person. + */ + +export interface Limits { + /** Calls in a row before the model is told, in its own transcript, that it is repeating itself. */ + readonly correctAt: number + /** Calls in a row before the turn ends. Reached only if the correction was ignored. */ + readonly stopAt: number +} + +export const LIMITS: Limits = { correctAt: 3, stopAt: 5 } + +export function limits(config?: false | { correct_at?: number; stop_at?: number }): Limits | undefined { + if (config === false) return undefined + const correctAt = config?.correct_at ?? LIMITS.correctAt + const stopAt = config?.stop_at ?? LIMITS.stopAt + if (correctAt <= 1) return undefined + return { correctAt, stopAt: Math.max(stopAt, correctAt) } +} + +/** The shape this needs from a message part. Anything that is not a settled tool call is skipped. */ +export interface Part { + readonly type: string + readonly tool?: string + readonly state?: { readonly status: string; readonly input?: unknown; readonly output?: string; readonly error?: string } +} + +export type Decision = + | { readonly type: "ok" } + | { readonly type: "correct"; readonly streak: number; readonly message: string } + | { readonly type: "stop"; readonly streak: number; readonly message: string } + +/** + * A call this guard already refused. + * + * Its result is the correction, not the tool's answer, so it must not be compared against the + * answers around it — otherwise the guard's own message would look like the world changing and + * would reset the streak it just started. + */ +const refused = (text: string) => text.startsWith(REFUSAL) + +const REFUSAL = "This is call " + +const settled = (part: Part) => part.type === "tool" && (part.state?.status === "completed" || part.state?.status === "error") +const result = (part: Part) => part.state?.output ?? part.state?.error ?? "" + +/** + * How many times in a row this exact call has already been made and answered the same way. + * + * Walks backwards over settled tool calls only, so text and reasoning between calls do not break + * the chain, and a loop that spans several steps is still visible. Stops at the first call that + * differs in tool, arguments, or result — a different result means the world moved, which is + * polling rather than repetition. + */ +export function streak(parts: readonly Part[], next: { tool: string; input: unknown }): number { + const wanted = JSON.stringify(next.input ?? null) + let count = 0 + let last: string | undefined + for (let i = parts.length - 1; i >= 0; i--) { + const part = parts[i]! + if (!settled(part)) continue + if (part.tool !== next.tool) break + if (JSON.stringify(part.state?.input ?? null) !== wanted) break + const out = result(part) + if (refused(out)) { + count++ + continue + } + if (last !== undefined && out !== last) break + last = out + count++ + } + return count +} + +export function assess(input: { + parts: readonly Part[] + next: { tool: string; input: unknown } + limits?: Limits +}): Decision { + if (!input.limits) return { type: "ok" } + // The call about to be made is part of the run, so a streak of two prior calls makes this the third. + const count = streak(input.parts, input.next) + 1 + if (count >= input.limits.stopAt) return { type: "stop", streak: count, message: stopped(input.next, count) } + if (count >= input.limits.correctAt) return { type: "correct", streak: count, message: correction(input.parts, input.next, count) } + return { type: "ok" } +} + +const args = (input: unknown) => { + const text = JSON.stringify(input ?? null) + return text.length > 400 ? text.slice(0, 400) + "…" : text +} + +const lastResult = (parts: readonly Part[], next: { tool: string }) => { + for (let i = parts.length - 1; i >= 0; i--) { + const part = parts[i]! + if (!settled(part) || part.tool !== next.tool) continue + const text = result(part) + // Quote the tool's own answer, never this guard's earlier correction. + if (refused(text)) continue + return text.length > 400 ? text.slice(0, 400) + "…" : text + } + return "" +} + +/** + * Quote the model back to itself. + * + * A bare "you are looping" leaves the model to guess what it did; naming the arguments and the + * answer it keeps getting, and saying plainly what the ways out are, is what turns the notice into + * something it can act on. + */ +export function correction(parts: readonly Part[], next: { tool: string; input: unknown }, count: number) { + const answer = lastResult(parts, next) + return [ + `${REFUSAL}${count} of \`${next.tool}\` with identical arguments, and every one of them returned the same thing.`, + `arguments: ${args(next.input)}`, + answer ? `result: ${answer}` : undefined, + `The call was not run this time, because running it again cannot produce anything new. Change the arguments, use a different tool, or tell the user what is blocking you and stop.`, + ] + .filter(Boolean) + .join("\n") +} + +export function stopped(next: { tool: string }, count: number) { + return `Stopped: \`${next.tool}\` was called ${count} times in a row with the same arguments and the same result, and the earlier warning did not change anything.` +} + +export * as LoopGuard from "./loop-guard" diff --git a/packages/redcode/src/session/processor.ts b/packages/redcode/src/session/processor.ts index 668639bdb86e..2109a23d4193 100644 --- a/packages/redcode/src/session/processor.ts +++ b/packages/redcode/src/session/processor.ts @@ -6,7 +6,7 @@ import { Cause, DateTime, Deferred, Effect, Exit, Layer, Context, Scope, Schema import * as Stream from "effect/Stream" import { Agent } from "@/agent/agent" import { Config } from "@/config/config" -import { Permission } from "@/permission" +import { Permission, evaluate } from "@/permission" import { Plugin } from "@/plugin" import { Snapshot } from "@/snapshot" import { Session } from "./session" @@ -23,13 +23,15 @@ import { Question } from "@/question" import { errorMessage } from "@/util/error" import { isRecord } from "@/util/record" import { EventV2Bridge } from "@/event-v2-bridge" +import { LoopGuard } from "./loop-guard" import { SessionEvent } from "@reddb-io/redcode-core/session/event" import { Database } from "@reddb-io/redcode-core/database/database" import { Usage, type LLMEvent } from "@reddb-io/redcode-llm" import { OperationHook } from "@reddb-io/redcode-core/operation-hook" import { OperationHookBridge } from "@/operation-hook-bridge" -const DOOM_LOOP_THRESHOLD = 3 +/** Steps of one turn to look back over. Comfortably more than any sane `stop_at`. */ +const LOOP_WINDOW = 16 export type Result = "compact" | "stop" | "continue" export interface Handle { @@ -55,6 +57,13 @@ export interface Handle { attachments?: SessionV1.FilePart[] }, ) => Effect.Effect + /** + * Whether this call has already been made, with these arguments, to the same answer. + * + * Asked before the tool runs, so a call that cannot produce anything new is never run at all. + * A `stop` decision also ends the turn after this step. + */ + readonly guardLoop: (input: { tool: string; input: unknown }) => Effect.Effect readonly process: (streamInput: LLM.StreamInput) => Effect.Effect } @@ -366,33 +375,8 @@ const layer = Layer.effect( : value.providerMetadata, })) - const parts = yield* MessageV2.parts(ctx.assistantMessage.id).pipe( - Effect.provideService(Database.Service, database), - ) - const recentParts = parts.slice(-DOOM_LOOP_THRESHOLD) - - if ( - recentParts.length !== DOOM_LOOP_THRESHOLD || - !recentParts.every( - (part) => - part.type === "tool" && - part.tool === value.name && - part.state.status !== "pending" && - JSON.stringify(part.state.input) === JSON.stringify(input), - ) - ) { - return - } - - const agent = yield* agents.get(ctx.assistantMessage.agent) - yield* permission.ask({ - permission: "doom_loop", - patterns: [value.name], - sessionID: ctx.assistantMessage.sessionID, - metadata: { tool: value.name, input }, - always: [value.name], - ruleset: agent.permission, - }) + // Repetition is judged in guardLoop, before the tool runs, so the model reads the + // correction as an ordinary tool result instead of the user being asked a question. return } @@ -710,6 +694,40 @@ const layer = Layer.effect( }) }) + const guardLoop = Effect.fn("SessionProcessor.guardLoop")(function* (input: { + tool: string + input: unknown + }) { + const configured = (yield* config.get()).experimental?.loop_guard + const bounds = LoopGuard.limits(configured) + if (!bounds) return { type: "ok" } as LoopGuard.Decision + // The `doom_loop` permission predates this guard and is how people already say "let it + // repeat"; allowing it keeps meaning that, rather than becoming a dead config key. + const agent = yield* agents.get(ctx.assistantMessage.agent) + if (evaluate("doom_loop", input.tool, agent.permission).action === "allow") { + return { type: "ok" } as LoopGuard.Decision + } + // Every step of a turn is its own assistant message, so looking at the current message + // alone can never see a loop that spans steps — which is what a loop actually looks like. + // Read back a bounded window and cut it at the last thing the user said. + const recent = yield* session.messages({ sessionID: ctx.sessionID, limit: LOOP_WINDOW }).pipe(Effect.orElseSucceed(() => [])) + const turn = recent.slice(recent.findLastIndex((item) => item.info.role === "user") + 1) + const parts = turn.flatMap((item) => item.parts) + const decision = LoopGuard.assess({ parts, next: input, limits: bounds }) + if (decision.type === "ok") return decision + yield* Effect.logWarning("model is repeating itself", { + sessionID: ctx.sessionID, + tool: input.tool, + streak: decision.streak, + action: decision.type, + }) + // A loop that survived its own correction ends the turn: continuing only spends money to + // reach the same place. Unlike a denied permission this is not the user's call, so it does + // not go through `shouldBreak`. + if (decision.type === "stop") ctx.blocked = true + return decision + }) + return { /** Read by the turn loop's watchdog: silence here is what a stall looks like. */ get lastEventAt() { @@ -723,6 +741,7 @@ const layer = Layer.effect( }, updateToolCall, completeToolCall, + guardLoop, process, } satisfies Handle }) diff --git a/packages/redcode/src/session/tools.ts b/packages/redcode/src/session/tools.ts index f71097c4ad2b..ff32a69d061c 100644 --- a/packages/redcode/src/session/tools.ts +++ b/packages/redcode/src/session/tools.ts @@ -48,7 +48,7 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: { agent: Agent.Info model: Provider.Model session: Session.Info - processor: Pick + processor: Pick bypassAgentCheck: boolean messages: SessionV1.WithParts[] promptOps: TaskPromptOps @@ -114,6 +114,14 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: { // tool in flight is deliberately counted as work. A timeout here lands in the same // failure branch as any other tool error, so the model reads it and can react. const deadline = ToolDeadline.deadlineMs({ tool: toolID, configured: toolTimeout }) + // Asked before the call is made: a call whose answer is already known cannot become + // useful by being made again, and the correction reaches the model as this tool's + // own result, so it can change course without anyone being asked a question. + const loop = yield* input.processor.guardLoop({ tool: toolID, input: decided.args }) + if (loop.type !== "ok") { + yield* publishPost({ error: loop.message }, true).pipe(Effect.ignoreCause) + return yield* Effect.fail(new Error(loop.message)) + } const call = Effect.promise(() => Promise.resolve(execute(decided.args, options))) const executed = yield* (deadline === undefined ? call diff --git a/packages/redcode/test/session/compaction.test.ts b/packages/redcode/test/session/compaction.test.ts index 597af8e7b3b4..1342a03ff06b 100644 --- a/packages/redcode/test/session/compaction.test.ts +++ b/packages/redcode/test/session/compaction.test.ts @@ -206,6 +206,7 @@ function fake( activeToolCount: 0, updateToolCall: Effect.fn("TestSessionProcessor.updateToolCall")(() => Effect.succeed(undefined)), completeToolCall: Effect.fn("TestSessionProcessor.completeToolCall")(() => Effect.void), + guardLoop: Effect.fn("TestSessionProcessor.guardLoop")(() => Effect.succeed({ type: "ok" as const })), process: Effect.fn("TestSessionProcessor.process")(() => Effect.succeed(result)), } satisfies SessionProcessorModule.SessionProcessor.Handle } diff --git a/packages/redcode/test/session/loop-guard.test.ts b/packages/redcode/test/session/loop-guard.test.ts new file mode 100644 index 000000000000..ed1fd241a983 --- /dev/null +++ b/packages/redcode/test/session/loop-guard.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, test } from "bun:test" +import { assess, LIMITS, limits, streak, type Part } from "@/session/loop-guard" + +const call = (tool: string, input: unknown, output: string, status = "completed"): Part => ({ + type: "tool", + tool, + state: { status, input, ...(status === "error" ? { error: output } : { output }) }, +}) +const text = (value: string): Part => ({ type: "text", state: { status: "completed", output: value } }) +const reasoning: Part = { type: "reasoning" } + +describe("loop guard", () => { + test("sees a repetition that reasoning and text are interleaved with", () => { + // The old detector compared the last three parts, so one reasoning part between calls hid the + // loop completely — and reasoning models emit them constantly. + const parts = [ + call("read", { path: "/gone" }, "ENOENT"), + reasoning, + text("let me try that again"), + call("read", { path: "/gone" }, "ENOENT"), + reasoning, + ] + expect(streak(parts, { tool: "read", input: { path: "/gone" } })).toBe(2) + }) + + test("leaves polling alone", () => { + // Same call, different answers: the world is moving, so this is waiting, not repeating. + const parts = [ + call("read", { path: "/log" }, "line 1"), + call("read", { path: "/log" }, "line 1\nline 2"), + call("read", { path: "/log" }, "line 1\nline 2\nline 3"), + ] + expect(assess({ parts, next: { tool: "read", input: { path: "/log" } }, limits: LIMITS })).toEqual({ type: "ok" }) + }) + + test("counts a repeated failure, not just a repeated success", () => { + const parts = [call("edit", { file: "a" }, "not found", "error"), call("edit", { file: "a" }, "not found", "error")] + const decision = assess({ parts, next: { tool: "edit", input: { file: "a" } }, limits: LIMITS }) + expect(decision.type).toBe("correct") + }) + + test("a different call in between is a fresh start", () => { + const parts = [ + call("read", { path: "/a" }, "x"), + call("read", { path: "/a" }, "x"), + call("grep", { pattern: "y" }, "no matches"), + ] + expect(streak(parts, { tool: "read", input: { path: "/a" } })).toBe(0) + }) + + test("corrects first and only stops if the correction changed nothing", () => { + const repeat = (n: number) => Array.from({ length: n }, () => call("read", { path: "/gone" }, "ENOENT")) + const next = { tool: "read", input: { path: "/gone" } } + expect(assess({ parts: repeat(1), next, limits: LIMITS }).type).toBe("ok") + expect(assess({ parts: repeat(2), next, limits: LIMITS }).type).toBe("correct") + expect(assess({ parts: repeat(4), next, limits: LIMITS }).type).toBe("stop") + }) + + test("the correction quotes the model's own arguments and the answer it keeps ignoring", () => { + const parts = [call("read", { path: "/gone" }, "ENOENT: no such file"), call("read", { path: "/gone" }, "ENOENT: no such file")] + const decision = assess({ parts, next: { tool: "read", input: { path: "/gone" } }, limits: LIMITS }) + expect(decision.type).toBe("correct") + if (decision.type !== "correct") return + expect(decision.message).toContain('"path":"/gone"') + expect(decision.message).toContain("ENOENT: no such file") + // Naming the ways out is what makes the notice actionable rather than a scolding. + expect(decision.message).toMatch(/different tool|tell the user/) + }) + + test("a call this guard already refused still counts toward stopping", () => { + // The correction is not the tool's answer, so it must not read as the world having changed — + // otherwise the guard resets the very streak it just started and never stops anything. + const first = call("read", { path: "/gone" }, "ENOENT") + const next = { tool: "read", input: { path: "/gone" } } + const corrected = assess({ parts: [first, first], next, limits: LIMITS }) + expect(corrected.type).toBe("correct") + if (corrected.type !== "correct") return + const refusal = call("read", { path: "/gone" }, corrected.message, "error") + expect(streak([first, first, refusal, refusal], next)).toBe(4) + expect(assess({ parts: [first, first, refusal, refusal], next, limits: LIMITS }).type).toBe("stop") + }) + + test("can be turned off, and nonsense thresholds turn it off rather than firing constantly", () => { + expect(limits(false)).toBeUndefined() + expect(limits({ correct_at: 1 })).toBeUndefined() + expect(limits()).toEqual(LIMITS) + // A stop threshold below the warning would abort without ever correcting. + expect(limits({ correct_at: 4, stop_at: 2 })).toEqual({ correctAt: 4, stopAt: 4 }) + }) +}) diff --git a/packages/redcode/test/session/prompt.test.ts b/packages/redcode/test/session/prompt.test.ts index 65ec21523df9..abf4c5b21e34 100644 --- a/packages/redcode/test/session/prompt.test.ts +++ b/packages/redcode/test/session/prompt.test.ts @@ -1461,6 +1461,42 @@ it.instance("stops a tool that never returns and hands the failure to the model" 30_000, ) +it.instance("corrects a model that repeats itself, then ends the turn if nothing changes", () => + Effect.gen(function* () { + // The old detector needed three byte-identical parts in a row, so one reasoning part hid the + // loop, and when it did fire it asked the user a question that could wait forever. + const { llm, dir } = yield* useServerConfig((url) => ({ + ...providerCfg(url), + experimental: { loop_guard: { correct_at: 2, stop_at: 3 } }, + })) + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const chat = yield* sessions.create({ title: "Pinned" }) + yield* seed(chat.id) + + // Inside the instance directory: an external path would stop on a permission prompt instead. + const same = { filePath: path.join(dir, "not-here.txt") } + yield* llm.tool("read", same) + yield* llm.tool("read", same) + yield* llm.tool("read", same) + yield* llm.text("giving up") + yield* user(chat.id, "read that file") + + yield* awaitWithTimeout(prompt.loop({ sessionID: chat.id }), "the turn never finished", "30 seconds") + + const parts = (yield* sessions.messages({ sessionID: chat.id })).flatMap((item) => item.parts) + const errors = parts.flatMap((part) => + part.type === "tool" && part.state.status === "error" ? [part.state.error] : [], + ) + // The second identical call is answered by the guard, not by running the tool again, and the + // model is told exactly what it repeated. + expect(errors.some((text) => text.includes("identical arguments"))).toBe(true) + // The third ends the turn rather than asking anyone whether to keep going. + expect(errors.some((text) => text.startsWith("Stopped:"))).toBe(true) + }), + 60_000, +) + it.instance("cancel records MessageAbortedError on interrupted process", () => Effect.gen(function* () { const { llm } = yield* useServerConfig(providerCfg)