From 5dbbbe49f00c3801bae041c22b2d1bcb4bb236b5 Mon Sep 17 00:00:00 2001 From: kemalelmizan Date: Sat, 17 Oct 2026 10:22:53 +0700 Subject: [PATCH] fix(repl): forward the run's policy instead of re-listing the options it knows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The REPL built its `runAgent` call field by field, and the list was incomplete: `steer` and `stopGuard` were never passed. So `--plan-interval` planned nothing interactively and `--mode tasks` could end a turn with its list unfinished — both worked in a one-shot run, and both did nothing in the REPL, silently. That is the same failure shape as a mode whose tools are wired but whose runner is not (#69): the capability exists, the host drops it on the way to the loop. The fix is structural rather than a third field on the list. `ReplOptions` now extends the loop's `RunAgentOptions` (minus `messages`/`onEvent`, which the REPL owns), and the turn passes its options through wholesale: const result = await runAgent({ ...options, messages: next, onEvent: render }); so a new run seam is forwarded by default and omitting one has to be written down. The `ReplOptions` doc comment says why, because the next person to add an option to the loop should not have to rediscover this. Two unit tests pin the seams that were missing — a `steer` whose text must reach the model, and a `stopGuard` that must earn a second model call — and both fail against the old hand-picked list. One e2e test drives the real REPL over stdin (answer-driven, because `readline.question` drops anything typed while no prompt is pending) and asserts a `--plan-interval 1` turn plans, records the plans, and reports the cost in `/steps`. That last assertion also needed the `/steps` footer to read `planning: plans=…` rather than `planning:plans=…`; the footer could not appear before this fix, so the spacing had no chance to be seen (it landed in #74). Docs: `docs/architecture.md` states the forwarding rule where the opt-in seams are described, `README.md` says run policy applies interactively, and `AGENTS.md` gains the REPL paragraph next to the loop seams. Verified: `pnpm build`, `pnpm typecheck`, `pnpm lint`, `pnpm test:coverage` all green (coverage 90.3 / 84.1 / 89.3 / 90.3). `src/repl.ts`, `src/repl.test.ts` and the touched docs are prettier-clean; the pre-existing drift in `src/cli.e2e.test.ts` is left alone (#49). --- AGENTS.md | 8 ++++ README.md | 8 +++- docs/architecture.md | 8 +++- src/cli.e2e.test.ts | 108 +++++++++++++++++++++++++++++++++++++++++++ src/cli.ts | 2 +- src/repl.test.ts | 65 +++++++++++++++++++++++++- src/repl.ts | 48 ++++++++----------- 7 files changed, 213 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7dca671..3aa3032 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -201,6 +201,14 @@ summary is where it is accounted for (`plans=… plan_tokens=… plan_ms=…`, v `PlanningPassOptions.onCall` → `PlanUsage`), with a `planning:` line on `/steps` for the REPL (#73). +**The REPL is the same host** (`src/repl.ts`): it hands the loop its options +wholesale (`ReplOptions` extends the loop's `RunAgentOptions`), so run policy +applies interactively — `--mode tasks` holds a turn open until its list is done and +`--plan-interval` re-plans here too. A host that re-lists the options it knows +instead of forwarding them silently drops the next seam; that is exactly how +`steer`/`stopGuard` went missing here, working in a one-shot run and doing nothing +interactively. + `AgentRuntime` is the bridge into Adaptive MCP. It wires `AdaptiveRuntime` (telemetry → store → evaluation → derived view), `ThinClient` (approval gate + retry + middleware), and `GraphTrackingMiddleware`, and exposes: diff --git a/README.md b/README.md index 2f76c30..cf315cf 100644 --- a/README.md +++ b/README.md @@ -68,10 +68,16 @@ Defaults (each overridable by a flag or an `AI_*` env var): In the REPL, type a message and press Enter. Assistant text streams as it is generated; tool calls and results print underneath. Commands: `/tools`, `/models`, `/graph` (the per-turn execution DAG), `/steps` (this session's typed -history — tasks, actions, plans), `/metadata`, `/cost`, +history — tasks, actions, plans — plus what planning cost), `/metadata`, `/cost`, `/policy`, `/drift`, `/retrieve [hash]`, `/decoding `, `/decoding-report`, `/decision` (the last System One decision), `/reset`, `/exit`. +The REPL is the same host as a one-shot run, so run policy applies interactively +too: `--mode tasks` holds a turn open until its list is done, and `--plan-interval` +re-plans here exactly as it does there. (It hands the loop its options wholesale, +so a new seam cannot be dropped by omission — that is how `--plan-interval` used to +do nothing at all in the REPL.) + - **Approvals.** Tools the gate marks `require_confirmation` — high-risk from the server's standard MCP annotations, or learned-flaky — prompt `approve ? — (y/N)` in the REPL. **In a non-interactive run diff --git a/docs/architecture.md b/docs/architecture.md index 7774a20..ece263a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -79,7 +79,13 @@ a protocol concept. | **Typed steps** | `src/steps.ts` | One typed history per session: an `ActionStep` for every attempted call (a gate refusal or a pause is an attempt, not a failure), and — with `--plan-interval `, off by default — a **planning pass** that records a `PlanStep` asked for from the _recorded_ state and injects it for the step that follows. `/steps` renders tasks, actions and plans in order. | All of these are opt-in, so a host that enables none behaves exactly as it did -before. Modes and the planning pass are the entries that are not toolsets, because +before. A host must therefore **forward the loop's options rather than re-list the +ones it happens to know**: the REPL did the latter and silently dropped `steer` and +`stopGuard`, so `--plan-interval` planned nothing and a `tasks`-mode turn could end +with its list unfinished — working in a one-shot run and doing nothing +interactively. `ReplOptions` now extends the loop's options, so a new seam is +forwarded by default and omitting one has to be written down. +Modes and the planning pass are the entries that are not toolsets, because each is policy about a _step_ rather than a capability the model calls: `tasks` reaches into the loop with `RunAgentOptions.stopGuard` (`src/agent/loop.ts`) — consulted when the model stops calling tools, so a run cannot end while its list diff --git a/src/cli.e2e.test.ts b/src/cli.e2e.test.ts index e98078d..6190470 100644 --- a/src/cli.e2e.test.ts +++ b/src/cli.e2e.test.ts @@ -41,6 +41,46 @@ function runCli(args: string[]): Promise { }); } +/** + * Drives the interactive REPL: `send` is written once the child's output reaches + * `after`. + * + * It has to be answer-driven rather than a single stdin dump — the REPL reads each + * line with `readline.question`, so a line typed while no prompt is pending is + * dropped, and dumping the whole script up front would run only the first line. The + * matching buffer is consumed as it goes, so the same marker (the `you> ` prompt) + * can be waited for once per turn. + */ +function runReplCli( + args: string[], + script: Array<{ after: string; send: string }>, +): Promise { + return new Promise((resolve) => { + const child = spawn(process.execPath, [TSX, "src/cli.ts", ...args], { cwd: root }); + let stdout = ""; + let unmatched = ""; + let stderr = ""; + const pending = [...script]; + const answerOutput = () => { + const step = pending[0]; + if (!step) return; + const at = unmatched.indexOf(step.after); + if (at < 0) return; + unmatched = unmatched.slice(at + step.after.length); + pending.shift(); + child.stdin.write(step.send); + answerOutput(); + }; + child.stdout.on("data", (chunk: Buffer) => { + stdout += chunk.toString(); + unmatched += chunk.toString(); + answerOutput(); + }); + child.stderr.on("data", (chunk: Buffer) => (stderr += chunk.toString())); + child.on("close", (code) => resolve({ code, stdout, stderr })); + }); +} + /** The JSON `--server` form, so a test can set per-server env (the fixture's delay). */ function serverSpec(env: Record = {}): string { return JSON.stringify({ @@ -340,6 +380,74 @@ describe("cli, end to end and offline", () => { rmSync(dir, { recursive: true, force: true }); }, 30_000); + it("runs the same run policy in the REPL, and reports what planning cost there", async () => { + // The REPL hand-picked the options it gave the loop, so `--plan-interval` did + // nothing interactively while working in a one-shot run. This drives the real + // REPL over stdin: the plan has to reach the model, land in the history, and + // show up in `/steps` with what it cost (#73) — a planning call is invisible to + // telemetry, so the REPL has nowhere else to say it. + const dir = mkdtempSync(join(tmpdir(), "agent-e2e-repl-")); + const db = join(dir, "agent.db"); + model = new StubModelServer({ + respond: (request) => { + const isPlanning = request.messages.some( + (message) => + message.role === "system" && (message.content ?? "").includes("planning pass"), + ); + if (isPlanning) return { text: JSON.stringify({ steps: ["echo something"] }) }; + const answered = request.messages.some((message) => message.role === "tool"); + return answered + ? { text: "done" } + : { toolName: "fixture_fixture_echo", toolInput: { text: "hi" } }; + }, + }); + const baseUrl = await model.start(); + + const run = await runReplCli( + [ + "--plan-interval", + "1", + "--server", + serverSpec(), + "--base-url", + baseUrl, + "--api-key", + "test-key", + "--no-ollama", + "--no-stream", + "--db", + db, + "--session", + "cli-e2e-repl-steps", + "--max-steps", + "4", + ], + [ + { after: "you> ", send: "echo something\n" }, + // The turn is over when its prompt comes back; then read the history back. + { after: "you> ", send: "/steps\n" }, + { after: "planning: plans=", send: "/exit\n" }, + ], + ); + + expect(run.code).toBe(0); + // The history it read back: the plan, the call it led to, and the re-plan. + expect(run.stdout).toContain("plan plan-1: echo something"); + expect(run.stdout).toContain("action"); + // And what the planning cost, which nothing else in a REPL session reports. + expect(run.stdout).toContain("planning: plans="); + + // The planning request went to the model from the REPL, not only from a one-shot. + const planningRequests = model.requests.filter((request) => + request.messages.some( + (message) => message.role === "system" && (message.content ?? "").includes("planning pass"), + ), + ); + expect(planningRequests.length).toBeGreaterThan(0); + + rmSync(dir, { recursive: true, force: true }); + }, 30_000); + it("wires the child runner for a mode that implies delegation", async () => { // `--mode route` implies `--task`, so the tool list has `delegate_task`. The // runner behind it has to be wired from the same decision, or the tool the diff --git a/src/cli.ts b/src/cli.ts index ab8d661..32e8270 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -613,7 +613,7 @@ async function main(): Promise { steps: () => { const history = formatSteps(activeStore?.getSteps?.(sessionId) ?? []); const usage = planUsage.summary(); - return usage ? `${history}\nplanning:${usage.trim()}` : history; + return usage ? `${history}\nplanning:${usage}` : history; }, models: () => catalog.describe(), decodingFor: (tool) => { diff --git a/src/repl.test.ts b/src/repl.test.ts index 02a8228..3e18a0a 100644 --- a/src/repl.test.ts +++ b/src/repl.test.ts @@ -1,7 +1,8 @@ import { describe, it, expect } from "vitest"; import { PassThrough } from "node:stream"; import { runRepl } from "./repl.js"; -import type { ChatModel, ChatStep, DecisionProvider, ToolSpec } from "./types.js"; +import type { ReplOptions } from "./repl.js"; +import type { ChatMessage, ChatModel, ChatStep, DecisionProvider, ToolSpec } from "./types.js"; /** A model that replies with fixed text and no tool calls. */ class ScriptedModel implements ChatModel { @@ -14,6 +15,16 @@ class ScriptedModel implements ChatModel { } } +/** A model that also records every conversation it is asked about. */ +class ObservingModel implements ChatModel { + readonly name = "observing"; + readonly seen: ChatMessage[][] = []; + async step(messages: ChatMessage[]): Promise { + this.seen.push(messages); + return { text: "ok", toolCalls: [], finishReason: "stop" }; + } +} + const NOOP_EXECUTOR = { execute: async () => ({ ok: true, output: {} }) } as never; const DECISION_MODEL = new ScriptedModel("gpt-4o-mini"); @@ -104,3 +115,55 @@ describe("runRepl render switch", () => { expect(stdout).toContain("plan-1: read the parser"); }); }); + +describe("runRepl forwards the loop's run seams", () => { + /** + * One turn with the given seams: the model is asked about "hello", then the + * REPL is told to exit. Returns what the model saw and what the user saw. + */ + async function runTurnWith( + seams: Partial, + ): Promise<{ seen: ChatMessage[][]; stdout: string }> { + const model = new ObservingModel(); + const input = new PassThrough(); + const output = new PassThrough(); + let stdout = ""; + output.on("data", (chunk) => (stdout += String(chunk))); + + const done = runRepl({ model, tools: [], executor: NOOP_EXECUTOR, ...seams, input, output }); + + input.write("hello\n"); + await new Promise((resolve) => setTimeout(resolve, 20)); + input.write("/exit\n"); + await done; + return { seen: model.seen, stdout }; + } + + it("forwards `steer`, so a planning pass runs in the REPL too", async () => { + // The REPL used to hand-pick the options it passed to `runAgent`, and `steer` + // was not in the list: `--plan-interval` did nothing interactively, silently. + const steers: number[] = []; + const { seen, stdout } = await runTurnWith({ + steer: (context) => { + steers.push(context.step); + return "Plan (before step 1):\n1. read the parser"; + }, + }); + + expect(steers).toEqual([0]); + expect(JSON.stringify(seen[0])).toContain("read the parser"); + // And the injection is visible, not a silent prompt change. + expect(stdout).toContain("Plan (before step 1)"); + }); + + it("forwards `stopGuard`, so a `tasks`-mode turn cannot end early", async () => { + let consulted = 0; + const { seen, stdout } = await runTurnWith({ + stopGuard: () => (consulted++ === 0 ? "the list is not finished: t1 is pending" : undefined), + }); + + // Asked twice: once to answer, once because the guard refused to accept it. + expect(seen).toHaveLength(2); + expect(stdout).toContain("t1 is pending"); + }); +}); diff --git a/src/repl.ts b/src/repl.ts index 487b090..7a8f603 100644 --- a/src/repl.ts +++ b/src/repl.ts @@ -1,16 +1,9 @@ import { createInterface } from "node:readline/promises"; import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { runAgent } from "./agent/loop.js"; +import type { RunAgentOptions } from "./agent/loop.js"; import { createReplRenderer, makeColor, preview } from "./render.js"; -import type { - AgentExecutor, - ChatMessage, - ChatModel, - DecisionProvider, - DecodingProvider, - ModelSelector, - ToolSpec, -} from "./types.js"; +import type { AgentExecutor, ChatMessage } from "./types.js"; /** The runtime hooks the REPL uses for human-in-the-loop approvals. */ export interface ApprovalHost { @@ -18,17 +11,21 @@ export interface ApprovalHost { approvalReason?(toolName: string): string | undefined; } -export interface ReplOptions { - model: ChatModel; - tools: ToolSpec[]; +/** + * A REPL is a *host*: it runs the same loop as a one-shot turn, so it takes the + * loop's options rather than re-listing the useful ones. + * + * That is deliberate. A hand-picked list silently dropped `steer` (so + * `--plan-interval` did nothing in the REPL) and `stopGuard` (so `--mode tasks` + * could end a turn with its list unfinished) — the same class of bug as a mode + * whose tools were wired but whose runner was not. Inheriting the loop's options + * means a new seam is forwarded by default, and omitting one has to be written + * down here. + */ +export interface ReplOptions extends Omit { executor: AgentExecutor & Partial; - decoding?: DecodingProvider; - selectModel?: ModelSelector; - /** Fast System-1 decision layer (e.g. `AgentRuntime.systemOneProvider`). */ - decide?: DecisionProvider; + /** System prompt for the turn (the REPL keeps the conversation across turns). */ system?: string; - maxSteps?: number; - stream?: boolean; /** Prompt y/n when the gate returns require_confirmation (default true). */ approvals?: boolean; /** Learned context injected into the system message each turn. */ @@ -249,18 +246,9 @@ export async function runRepl(options: ReplOptions): Promise { reset(); prompt(color.dim("agent> ")); try { - const result = await runAgent({ - model: options.model, - tools: options.tools, - executor: options.executor, - messages: next, - maxSteps: options.maxSteps, - decoding: options.decoding, - selectModel: options.selectModel, - decide: options.decide, - stream: options.stream, - onEvent: render, - }); + // Every run seam is forwarded, not re-listed: a hand-picked list is how + // `steer` and `stopGuard` went missing here (see `ReplOptions`). + const result = await runAgent({ ...options, messages: next, onEvent: render }); messages = result.messages; ensureNewline(); persistHistory();