From 19095e51488c4618f0ab21ed239fe51d095bc25f Mon Sep 17 00:00:00 2001 From: Axel Delafosse Date: Tue, 24 Feb 2026 00:12:33 -0800 Subject: [PATCH] feat: follow up plan-review-pass changes --- README.md | 5 +- src/loop/args.ts | 19 ++- src/loop/codex-app-server.ts | 15 ++- src/loop/codex-render.ts | 226 +++++++++++++++++++++++++++++++ src/loop/constants.ts | 2 +- src/loop/runner.ts | 232 ++------------------------------ src/loop/task.ts | 9 +- src/loop/types.ts | 2 +- tests/loop/args.test.ts | 12 ++ tests/loop/codex-render.test.ts | 91 +++++++++++++ tests/loop/runner.test.ts | 2 +- tests/loop/task.test.ts | 22 +++ 12 files changed, 401 insertions(+), 236 deletions(-) create mode 100644 src/loop/codex-render.ts create mode 100644 tests/loop/codex-render.test.ts diff --git a/README.md b/README.md index a3e6e54..3bd1ff2 100644 --- a/README.md +++ b/README.md @@ -141,7 +141,7 @@ When running from source (`bun src/loop.ts`), auto-update is disabled — use `g - `-d, --done `: done signal string (default: `DONE`) - `--format `: output format (default: `pretty`) - `--review [claude|codex|claudex]`: run a review when done (default: `claudex`; bare `--review` also uses `claudex`). With `claudex`, both reviews run in parallel, then both comments are passed back to the original agent so it can decide what to address. If both reviews found the same issue, that is a stronger signal to fix it. -- `--review-plan [other|claude|codex]`: reviewer for the automatic plan review pass that runs after plain-text prompts create `PLAN.md` (default: `other`, the non-primary model). +- `--review-plan [other|claude|codex|none]`: reviewer for the automatic plan review pass that runs after plain-text prompts create `PLAN.md` (default: `other`, the non-primary model). Use `none` to skip plan review. - `--tmux`: run `loop` in a detached tmux session so it survives SSH disconnects (auto-attaches when interactive). Session name format: `repo-loop-X` - `--worktree`: create and run inside a fresh git worktree + branch automatically. Worktree/branch format: `repo-loop-X` - `-h, --help`: help @@ -161,6 +161,9 @@ loop --proof "Use {skill} to verify your changes" "Implement {feature}" # plain text prompt: override the plan reviewer loop --proof "Use {skill} to verify your changes" --review-plan claude "Implement {feature}" +# plain text prompt: skip automatic plan review +loop --proof "Use {skill} to verify your changes" --review-plan none "Implement {feature}" + # run with claude loop --proof "Use {skill} to verify your changes" --agent claude --prompt PLAN.md diff --git a/src/loop/args.ts b/src/loop/args.ts index f36e5a7..e35e93f 100644 --- a/src/loop/args.ts +++ b/src/loop/args.ts @@ -38,8 +38,13 @@ const parseReviewValue = (value: string): ReviewMode => { throw new Error(`Invalid --review value: ${value}`); }; -const parsePlanReviewValue = (value: string): PlanReviewMode => { - if (value === "other" || value === "claude" || value === "codex") { +const parsePlanReviewValue = (value: string | undefined): PlanReviewMode => { + if ( + value === "other" || + value === "claude" || + value === "codex" || + value === "none" + ) { return value; } throw new Error(`Invalid --review-plan value: ${value}`); @@ -126,13 +131,13 @@ const parsePlanReviewArg = ( } const next = argv[index + 1]; - if (next === "other" || next === "claude" || next === "codex") { - opts.reviewPlan = next; + try { + opts.reviewPlan = parsePlanReviewValue(next); return index + 1; + } catch { + opts.reviewPlan = "other"; + return index; } - - opts.reviewPlan = "other"; - return index; }; const consumeArg = ( diff --git a/src/loop/codex-app-server.ts b/src/loop/codex-app-server.ts index 6167ff8..2b6d2e1 100644 --- a/src/loop/codex-app-server.ts +++ b/src/loop/codex-app-server.ts @@ -5,6 +5,10 @@ import type { Options, RunResult } from "./types"; type ExitSignal = "SIGINT" | "SIGTERM"; type TransportMode = "app-server" | "exec"; type Callback = (text: string) => void; +interface RunCodexTurnCallbacks { + onParsed?: Callback; + onRaw: Callback; +} interface JsonFrame { error?: unknown; @@ -38,6 +42,7 @@ const WS_CONNECT_ATTEMPTS = 40; const WS_CONNECT_DELAY_MS = 150; const USER_INPUT_TEXT_ELEMENTS = "text_elements"; const WAIT_TIMEOUT_MS = 600_000; +const NOOP_CALLBACK: Callback = () => undefined; export const CODEX_TRANSPORT_APP_SERVER: TransportMode = "app-server"; export const CODEX_TRANSPORT_EXEC: TransportMode = "exec"; @@ -887,9 +892,15 @@ export const startAppServer = async (): Promise => { export const runCodexTurn = ( prompt: string, opts: Options, - callbacks: { onParsed: Callback; onRaw: Callback } + // Some callers render directly from raw events and intentionally skip parsed callbacks. + callbacks: RunCodexTurnCallbacks ): Promise => { - return getClient().runTurn(prompt, opts, callbacks.onParsed, callbacks.onRaw); + return getClient().runTurn( + prompt, + opts, + callbacks.onParsed ?? NOOP_CALLBACK, + callbacks.onRaw + ); }; export const interruptAppServer = (signal: ExitSignal): void => { diff --git a/src/loop/codex-render.ts b/src/loop/codex-render.ts new file mode 100644 index 0000000..f946b03 --- /dev/null +++ b/src/loop/codex-render.ts @@ -0,0 +1,226 @@ +import type { Format } from "./types"; + +interface AppServerEvent { + method?: unknown; + params?: unknown; +} + +interface CodexRenderState { + activeItemHasDelta: boolean; + activeItemId: string; + lastCompleted: string; + parsed: string; + pendingMessageBreak: boolean; + wrotePretty: boolean; +} + +interface CodexRendererConfig { + format: Format; + write: (text: string) => void; +} + +interface CodexRenderer { + getParsed: () => string; + onRawLine: (text: string) => void; + wrotePretty: () => boolean; +} + +const asRecord = (value: unknown): Record => + typeof value === "object" && value !== null + ? (value as Record) + : {}; + +const asString = (value: unknown): string | undefined => + typeof value === "string" ? value : undefined; + +const parseJsonLine = (line: string): Record | undefined => { + if (!line.trim().startsWith("{")) { + return undefined; + } + try { + const parsed = JSON.parse(line); + return typeof parsed === "object" && parsed !== null + ? (parsed as Record) + : undefined; + } catch { + return undefined; + } +}; + +const collectText = ( + value: unknown, + out: string[], + primaryField: string, + secondaryField: string +): void => { + if (typeof value === "string") { + out.push(value); + return; + } + if (Array.isArray(value)) { + for (const item of value) { + collectText(item, out, primaryField, secondaryField); + } + return; + } + if (!value || typeof value !== "object") { + return; + } + const record = asRecord(value); + const direct = + asString(record[primaryField]) ?? asString(record[secondaryField]); + if (direct !== undefined) { + out.push(direct); + } + collectText(record.content, out, primaryField, secondaryField); + collectText(record.item, out, primaryField, secondaryField); + collectText(record.payload, out, primaryField, secondaryField); +}; + +const parseDeltaText = (value: unknown): string => { + const parts: string[] = []; + collectText(value, parts, "delta", "text"); + return parts.join(""); +}; + +const parseCompletedMessage = (value: unknown): string => { + const parts: string[] = []; + collectText(value, parts, "text", "delta"); + // Keep completed-message concatenation aligned with delta concatenation. + return parts.join(""); +}; + +const parseItemId = (value: unknown): string => { + const record = asRecord(value); + return ( + asString(record.itemId) ?? + asString(record.item_id) ?? + asString(record.id) ?? + asString(asRecord(record.item).id) ?? + "" + ); +}; + +const appendChunk = ( + state: CodexRenderState, + format: Format, + write: (text: string) => void, + text: string +): void => { + if (!text) { + return; + } + const needsMessageBreak = + state.pendingMessageBreak && + state.parsed && + !state.parsed.endsWith("\n") && + !text.startsWith("\n"); + const parsedChunk = needsMessageBreak ? `\n${text}` : text; + const prettyChunk = needsMessageBreak ? `\n\n${text}` : text; + state.pendingMessageBreak = false; + state.parsed += parsedChunk; + if (format === "pretty") { + write(prettyChunk); + state.wrotePretty = true; + } +}; + +const markMessageBoundary = (state: CodexRenderState): void => { + state.pendingMessageBreak = true; + state.activeItemHasDelta = false; + state.activeItemId = ""; +}; + +const handleDeltaLine = ( + params: Record, + state: CodexRenderState, + format: Format, + write: (text: string) => void +): void => { + const itemId = parseItemId(params); + const itemChanged = + Boolean(state.activeItemId) && + Boolean(itemId) && + itemId !== state.activeItemId; + if (itemChanged) { + markMessageBoundary(state); + } + if (itemId) { + state.activeItemId = itemId; + } + const chunk = parseDeltaText(params.delta ?? params); + if (!chunk) { + return; + } + state.activeItemHasDelta = true; + appendChunk(state, format, write, chunk); +}; + +const handleCompletedLine = ( + params: Record, + state: CodexRenderState, + format: Format, + write: (text: string) => void +): void => { + const item = asRecord(params.item); + const itemType = asString(item.type); + if (itemType !== "agentMessage" && itemType !== "agent_message") { + return; + } + const completedId = parseItemId(params); + const sameActive = + Boolean(completedId) && + Boolean(state.activeItemId) && + completedId === state.activeItemId; + const candidate = parseCompletedMessage(item).trim(); + if ( + candidate && + candidate !== state.lastCompleted && + !(sameActive && state.activeItemHasDelta) + ) { + state.lastCompleted = candidate; + appendChunk(state, format, write, candidate); + } + markMessageBoundary(state); +}; + +export const createCodexRenderer = ( + config: CodexRendererConfig +): CodexRenderer => { + const state: CodexRenderState = { + activeItemHasDelta: false, + activeItemId: "", + lastCompleted: "", + parsed: "", + pendingMessageBreak: false, + wrotePretty: false, + }; + + const onRawLine = (text: string): void => { + if (config.format === "raw") { + config.write(`${text}\n`); + } + + const parsedLine = parseJsonLine(text) as AppServerEvent | undefined; + if (!parsedLine) { + return; + } + const method = asString(parsedLine.method); + const params = asRecord(parsedLine.params); + + if (method === "item/agentMessage/delta") { + handleDeltaLine(params, state, config.format, config.write); + return; + } + + if (method === "item/completed") { + handleCompletedLine(params, state, config.format, config.write); + } + }; + + return { + getParsed: () => state.parsed, + onRawLine, + wrotePretty: () => state.wrotePretty, + }; +}; diff --git a/src/loop/constants.ts b/src/loop/constants.ts index 820fb34..b91850e 100644 --- a/src/loop/constants.ts +++ b/src/loop/constants.ts @@ -24,7 +24,7 @@ Options: --codex-model Override codex model (default: ${DEFAULT_CODEX_MODEL}) --format Log format (default: pretty) --review [claude|codex|claudex] Review on done (default: claudex) - --review-plan [other|claude|codex] Review PLAN.md after plain-text planning (default: other) + --review-plan [other|claude|codex|none] Review PLAN.md after plain-text planning (default: other) --tmux Run in a detached tmux session (name: repo-loop-X) --worktree Create and run in a fresh git worktree (name: repo-loop-X) -v, --version Show loop version diff --git a/src/loop/runner.ts b/src/loop/runner.ts index 6972b30..056f2a3 100644 --- a/src/loop/runner.ts +++ b/src/loop/runner.ts @@ -15,6 +15,7 @@ import { startAppServer, useAppServer, } from "./codex-app-server"; +import { createCodexRenderer } from "./codex-render"; import { DEFAULT_CLAUDE_MODEL } from "./constants"; import type { Agent, Options, RunResult } from "./types"; @@ -24,15 +25,6 @@ interface SpawnConfig { cmd: string; } -interface AppServerEvent { - method?: unknown; - params?: unknown; -} -interface CodexRenderState { - activeItemHasDelta: boolean; - activeItemId: string; - lastCompleted: string; -} type LegacyAgentRunner = ( agent: Agent, prompt: string, @@ -185,155 +177,6 @@ const eventMessage = (line: string): string => { } }; -const asRecord = (value: unknown): Record => - typeof value === "object" && value !== null - ? (value as Record) - : {}; - -const asString = (value: unknown): string | undefined => - typeof value === "string" ? value : undefined; - -const parseJsonLine = (line: string): Record | undefined => { - if (!line.trim().startsWith("{")) { - return undefined; - } - try { - const parsed = JSON.parse(line); - return typeof parsed === "object" && parsed !== null - ? (parsed as Record) - : undefined; - } catch { - return undefined; - } -}; - -const collectDeltaText = (value: unknown, out: string[]): void => { - if (typeof value === "string") { - out.push(value); - return; - } - if (Array.isArray(value)) { - for (const item of value) { - collectDeltaText(item, out); - } - return; - } - if (!value || typeof value !== "object") { - return; - } - const record = asRecord(value); - const direct = asString(record.delta) ?? asString(record.text); - if (direct !== undefined) { - out.push(direct); - } - collectDeltaText(record.content, out); - collectDeltaText(record.item, out); - collectDeltaText(record.payload, out); -}; - -const parseDeltaText = (value: unknown): string => { - const parts: string[] = []; - collectDeltaText(value, parts); - return parts.join(""); -}; - -const collectMessageText = (value: unknown, out: string[]): void => { - if (typeof value === "string") { - out.push(value); - return; - } - if (Array.isArray(value)) { - for (const item of value) { - collectMessageText(item, out); - } - return; - } - if (!value || typeof value !== "object") { - return; - } - const record = asRecord(value); - const direct = asString(record.text) ?? asString(record.delta); - if (direct !== undefined) { - out.push(direct); - } - collectMessageText(record.content, out); - collectMessageText(record.item, out); - collectMessageText(record.payload, out); -}; - -const parseCompletedMessage = (value: unknown): string => { - const parts: string[] = []; - collectMessageText(value, parts); - return parts - .map((part) => part.trim()) - .filter(Boolean) - .join("\n"); -}; - -const parseItemId = (value: unknown): string => { - const record = asRecord(value); - return ( - asString(record.itemId) ?? - asString(record.item_id) ?? - asString(record.id) ?? - asString(asRecord(record.item).id) ?? - "" - ); -}; - -const handleDeltaLine = ( - params: Record, - renderState: CodexRenderState, - appendChunk: (text: string) => void, - markMessageBoundary: () => void -): void => { - const itemId = parseItemId(params); - const itemChanged = - Boolean(renderState.activeItemId) && - Boolean(itemId) && - itemId !== renderState.activeItemId; - if (itemChanged) { - markMessageBoundary(); - } - if (itemId) { - renderState.activeItemId = itemId; - } - const chunk = parseDeltaText(params.delta ?? params); - if (!chunk) { - return; - } - renderState.activeItemHasDelta = true; - appendChunk(chunk); -}; - -const handleCompletedLine = ( - params: Record, - renderState: CodexRenderState, - appendChunk: (text: string) => void, - markMessageBoundary: () => void -): void => { - const item = asRecord(params.item); - const itemType = asString(item.type); - if (itemType !== "agentMessage" && itemType !== "agent_message") { - return; - } - const completedId = parseItemId(params); - const sameActive = - Boolean(completedId) && - Boolean(renderState.activeItemId) && - completedId === renderState.activeItemId; - const candidate = parseCompletedMessage(item); - if ( - candidate && - candidate !== renderState.lastCompleted && - !(sameActive && renderState.activeItemHasDelta) - ) { - renderState.lastCompleted = candidate; - appendChunk(candidate); - } - markMessageBoundary(); -}; - const consume = async ( stream: ReadableStream, onText: (text: string) => void @@ -397,66 +240,12 @@ const runCodexAgent = async ( return runnerState.runLegacyAgent("codex", prompt, opts); } - const renderState: CodexRenderState = { - activeItemHasDelta: false, - activeItemId: "", - lastCompleted: "", - }; - let parsed = ""; - let pendingMessageBreak = false; - let wrotePretty = false; - const appendChunk = (text: string): void => { - if (!text) { - return; - } - const needsMessageBreak = - pendingMessageBreak && - parsed && - !parsed.endsWith("\n") && - !text.startsWith("\n"); - const parsedChunk = needsMessageBreak ? `\n${text}` : text; - const prettyChunk = needsMessageBreak ? `\n\n${text}` : text; - pendingMessageBreak = false; - parsed += parsedChunk; - if (opts.format === "pretty") { - process.stdout.write(prettyChunk); - wrotePretty = true; - } - }; - const markMessageBoundary = (): void => { - pendingMessageBreak = true; - renderState.activeItemHasDelta = false; - renderState.activeItemId = ""; - }; - const onParsed = (_text: string): void => { - // app-server parsing is ignored; client renders from raw events. - }; - const onRaw = (text: string): void => { - if (opts.format === "raw") { - process.stdout.write(`${text}\n`); - } - - const parsedLine = parseJsonLine(text) as AppServerEvent | undefined; - if (!parsedLine) { - return; - } - const method = asString(parsedLine.method); - const params = asRecord(parsedLine.params); - - if (method === "item/agentMessage/delta") { - handleDeltaLine(params, renderState, appendChunk, markMessageBoundary); - return; - } - - if (method === "item/completed") { - handleCompletedLine( - params, - renderState, - appendChunk, - markMessageBoundary - ); - } - }; + const renderer = createCodexRenderer({ + format: opts.format, + write: (text) => { + process.stdout.write(text); + }, + }); activeAppServerRuns += 1; syncSignalHandlers(); @@ -473,13 +262,12 @@ const runCodexAgent = async ( } const result = await runCodexTurn(prompt, opts, { - onParsed, - onRaw, + onRaw: renderer.onRawLine, }); - const finalParsed = result.parsed || parsed; + const finalParsed = result.parsed || renderer.getParsed(); if ( opts.format === "pretty" && - wrotePretty && + renderer.wrotePretty() && !finalParsed.endsWith("\n") ) { process.stdout.write("\n"); diff --git a/src/loop/task.ts b/src/loop/task.ts index 7a335b3..3158ba4 100644 --- a/src/loop/task.ts +++ b/src/loop/task.ts @@ -14,8 +14,11 @@ const isMarkdownInput = (input: string): boolean => const resolvePlanReviewer = ( reviewPlan: PlanReviewMode | undefined, agent: Agent -): Agent => { +): Agent | undefined => { const mode = reviewPlan ?? "other"; + if (mode === "none") { + return undefined; + } if (mode === "other") { return agent === "codex" ? "claude" : "codex"; } @@ -38,6 +41,10 @@ const runPlanMode = async (opts: Options, task: string): Promise => { } const reviewer = resolvePlanReviewer(opts.reviewPlan, opts.agent); + if (!reviewer) { + console.log("\n[loop] skipping PLAN.md review (--review-plan none)."); + return; + } console.log(`\n[loop] reviewing PLAN.md with ${reviewer}.`); const reviewPrompt = buildPlanReviewPrompt(task); const review = await runAgent(reviewer, reviewPrompt, opts); diff --git a/src/loop/types.ts b/src/loop/types.ts index dbd2c69..08ba9c2 100644 --- a/src/loop/types.ts +++ b/src/loop/types.ts @@ -1,7 +1,7 @@ export type Agent = "claude" | "codex"; export type Format = "pretty" | "raw"; export type ReviewMode = Agent | "claudex"; -export type PlanReviewMode = Agent | "other"; +export type PlanReviewMode = Agent | "other" | "none"; export type ValueFlag = | "agent" | "prompt" diff --git a/tests/loop/args.test.ts b/tests/loop/args.test.ts index 17033cf..3f54985 100644 --- a/tests/loop/args.test.ts +++ b/tests/loop/args.test.ts @@ -159,12 +159,24 @@ test("parseArgs uses reviewer after --review-plan when valid", () => { expect(opts.reviewPlan).toBe("claude"); }); +test("parseArgs accepts none after --review-plan", () => { + const opts = parseArgs(["--review-plan", "none", "--proof", "verify"]); + + expect(opts.reviewPlan).toBe("none"); +}); + test("parseArgs supports equals form for --review-plan", () => { const opts = parseArgs(["--review-plan=codex", "--proof", "verify"]); expect(opts.reviewPlan).toBe("codex"); }); +test("parseArgs supports equals form for --review-plan=none", () => { + const opts = parseArgs(["--review-plan=none", "--proof", "verify"]); + + expect(opts.reviewPlan).toBe("none"); +}); + test("parseArgs rejects invalid --review-plan value", () => { expect(() => parseArgs(["--review-plan=claudex", "--proof", "verify"]) diff --git a/tests/loop/codex-render.test.ts b/tests/loop/codex-render.test.ts new file mode 100644 index 0000000..6c4c4a1 --- /dev/null +++ b/tests/loop/codex-render.test.ts @@ -0,0 +1,91 @@ +import { expect, test } from "bun:test"; +import { createCodexRenderer } from "../../src/loop/codex-render"; + +const makeRenderer = (format: "pretty" | "raw" = "pretty") => { + const writes: string[] = []; + const renderer = createCodexRenderer({ + format, + write: (text) => { + writes.push(text); + }, + }); + return { renderer, writes }; +}; + +test("completed message concatenates content parts without inserting newlines", () => { + const { renderer, writes } = makeRenderer(); + renderer.onRawLine( + JSON.stringify({ + method: "item/completed", + params: { + item: { + type: "agentMessage", + content: [{ text: "Hello " }, { text: "world" }], + }, + }, + }) + ); + + expect(renderer.getParsed()).toBe("Hello world"); + expect(writes.join("")).toBe("Hello world"); +}); + +test("delta extraction prioritizes delta over text", () => { + const { renderer } = makeRenderer(); + renderer.onRawLine( + JSON.stringify({ + method: "item/agentMessage/delta", + params: { + delta: { + delta: "delta-first", + text: "text-fallback", + }, + }, + }) + ); + + expect(renderer.getParsed()).toBe("delta-first"); +}); + +test("completed extraction prioritizes text over delta", () => { + const { renderer } = makeRenderer(); + renderer.onRawLine( + JSON.stringify({ + method: "item/completed", + params: { + item: { + delta: "delta-fallback", + text: "text-first", + type: "agentMessage", + }, + }, + }) + ); + + expect(renderer.getParsed()).toBe("text-first"); +}); + +test("completed whitespace-only content is ignored", () => { + const { renderer, writes } = makeRenderer(); + renderer.onRawLine( + JSON.stringify({ + method: "item/completed", + params: { + item: { + type: "agentMessage", + content: [{ text: " " }, { text: "" }], + }, + }, + }) + ); + + expect(renderer.getParsed()).toBe(""); + expect(writes).toEqual([]); +}); + +test("raw mode writes each incoming line with a trailing newline", () => { + const { renderer, writes } = makeRenderer("raw"); + renderer.onRawLine('{"method":"ping"}'); + + expect(writes).toEqual(['{"method":"ping"}\n']); +}); diff --git a/tests/loop/runner.test.ts b/tests/loop/runner.test.ts index ed65708..0759782 100644 --- a/tests/loop/runner.test.ts +++ b/tests/loop/runner.test.ts @@ -48,7 +48,7 @@ const runCodexTurn: MockFn< _prompt: string, _opts: Options, _callbacks: { - onParsed: (text: string) => void; + onParsed?: (text: string) => void; onRaw: (text: string) => void; } ) => Promise diff --git a/tests/loop/task.test.ts b/tests/loop/task.test.ts index 6e4d314..8af3d37 100644 --- a/tests/loop/task.test.ts +++ b/tests/loop/task.test.ts @@ -206,6 +206,28 @@ test("resolveTask uses codex as review-plan=other reviewer when primary is claud ); }); +test("resolveTask skips plan review when review-plan is none", async () => { + const { runAgentMock, resolveTask } = await loadResolveTask({ + isFile: (path) => path === "PLAN.md", + readPrompt: async () => "generated plan task", + runAgent: async () => ({ + combined: "", + exitCode: 0, + parsed: "", + }), + }); + + await resolveTask(makeOptions("ship feature", { reviewPlan: "none" })); + + expect(runAgentMock).toHaveBeenCalledTimes(1); + expect(runAgentMock).toHaveBeenNthCalledWith( + 1, + "codex", + expect.any(String), + expect.any(Object) + ); +}); + test("resolveTask throws when plan review exits non-zero", async () => { let calls = 0; const { resolveTask } = await loadResolveTask({