From 58ec9fd4fcc7e1929d527c7ee76ebd73337eae61 Mon Sep 17 00:00:00 2001 From: youwang <61931019+MelodyVAR@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:26:57 +0800 Subject: [PATCH 1/5] feat: expose structured external plan approvals --- apps/cli/test/plan-review-tool-row.test.ts | 12 ++ docs/plan-approval.md | 100 ++++++++++ .../src/features/plan-mode-tools.ts | 29 +-- .../coding-agent/src/render/plan-review.ts | 19 +- .../test/plan-review-dialog.test.ts | 19 ++ .../test/step-plan-extension.test.ts | 176 ++++++++++++++---- 6 files changed, 303 insertions(+), 52 deletions(-) create mode 100644 docs/plan-approval.md diff --git a/apps/cli/test/plan-review-tool-row.test.ts b/apps/cli/test/plan-review-tool-row.test.ts index 9eb22332..3aeaef46 100644 --- a/apps/cli/test/plan-review-tool-row.test.ts +++ b/apps/cli/test/plan-review-tool-row.test.ts @@ -135,6 +135,18 @@ describe("exit_plan_mode tool row", () => { } }); + it("renders an external approval handoff without implying approval or clipping the plan", () => { + const component = createRow({ ...dismissed, outcome: "external_approval_required", planModeExited: true }); + const out = rows(component); + expect(out.some((row) => row.includes("External approval required"))).toBe(true); + expect(out.some((row) => row.includes("Plan approved"))).toBe(false); + expect(out.some((row) => row.includes("# Plan: tank game"))).toBe(true); + expect(out.some((row) => row.includes("Delete the directory."))).toBe(true); + expect(out.some((row) => row.includes("to expand"))).toBe(false); + component.setExpanded(true); + expect(rows(component)).toEqual(out); + }); + it("reports the approved and feedback outcomes too", () => { const approved = rows(createRow({ ...dismissed, outcome: "approved" })); expect(approved.some((row) => row.includes("Plan approved"))).toBe(true); diff --git a/docs/plan-approval.md b/docs/plan-approval.md new file mode 100644 index 00000000..4aabb7be --- /dev/null +++ b/docs/plan-approval.md @@ -0,0 +1,100 @@ +# Plan approval handoff + +`exit_plan_mode` submits a written proposal for review. Leaving plan mode and +approving the proposal are separate events. In headless and RPC sessions, a valid +proposal exits plan mode and restores the previous tools, while the caller remains +responsible for obtaining approval before execution. This also applies to RPC +contexts with `hasUI: true`; their UI bridge does not provide interactive approval. + +Successful headless and RPC exits return the existing model-facing text plus +structured `details`, for example: + +```json +{ + "outcome": "external_approval_required", + "approvalSource": "external", + "planFilePath": "/workspace/.stepcode/plans/session-example.md", + "planContents": "# Plan\n\n1. Update the parser.\n2. Run focused tests.\n", + "planHash": "64a6a6c86ef0197c43f9bfc081792cca8d33807fbe33390d2439308a790f7520", + "planModeExited": true +} +``` + +`outcome: "external_approval_required"` identifies a handoff awaiting the caller's +decision. `planModeExited: true` describes the resulting mode state; it does not +mean approval was granted. The result still has `terminate: false`, so the host +must gate execution through its own approval flow. These details do not add an +approval command or enforce an execution barrier. + +`approvalSource` identifies who owns the decision: `"user"` for native interactive +review, including feedback and dismissal, and `"external"` for the headless/RPC +caller's approval flow. It does not indicate that approval has occurred. Source +and outcome are independent: a future caller-owned `"approved"` outcome can keep +`approvalSource: "external"` without claiming the user clicked the TUI. Native +headless/RPC exits currently emit `"external_approval_required"` only. + +`planHash` is the lowercase hexadecimal SHA-256 digest of `planContents` encoded +as UTF-8. It covers the complete snapshot read from the file, including any BOM, +line endings, leading or trailing whitespace, and the end of a long proposal. +Nothing is trimmed, normalized, or truncated before hashing. The path identifies +the source file; the hash identifies the returned contents, even if that file +later changes. + +A caller can verify the returned snapshot before presenting it for approval: + +```js +import { createHash } from "node:crypto"; + +const details = toolResult.details; +if (details?.outcome === "external_approval_required") { + const expectedHash = createHash("sha256") + .update(details.planContents, "utf8") + .digest("hex"); + if (details.planModeExited !== true || details.planHash !== expectedHash) { + throw new Error("Incomplete or inconsistent plan handoff"); + } + // Present details.planContents and bind the caller's decision to details.planHash. + // Allow execution only after that caller-owned approval flow succeeds. +} +``` + +A caller-owned extension can use the existing `tool_result` hook to apply its +approval policy before the result reaches the runner. After that policy actually +approves the snapshot, it can retain the hash and contents, set +`outcome: "approved"` with `approvalSource: "external"`, and deliver an explicit +caller-approval message to the model. Merely changing metadata is not a substitute +for obtaining approval. Execution-time permission checks continue to apply. + +This also lets an opted-in completion check run after a caller-approved plan. +Once an unapproved handoff has already reached the runner, automatic feedback +stays suppressed for that invocation; explicit caller messages are still delivered. +The runner does not infer an approval decision from their wording. + +If the proposal changes, obtain a new review and approval for the new hash. A +matching hash identifies content; it is not an approval token. + +Native outcomes keep their existing behavior: + +| Outcome | `approvalSource` | `planModeExited` | Behavior | +| --- | --- | --- | --- | +| `approved` | `user` | `true` | The user selected execution; restore tools and continue. | +| `feedback` | `user` | `false` | Keep planning and steer the user's notes into the next response. | +| `dismissed` | `user` | `false` | Keep planning and terminate the turn to wait for the user. | +| `external_approval_required` | `external` | `true` | Headless/RPC handoff; the caller must gate execution. | + +The TUI still selects execution by default, and Enter confirms that selection. +Interactive result details also include the hash of the reviewed snapshot. +Transcripts display external handoffs as **External approval required**, with the +complete plan, rather than displaying an approved result. + +Missing, empty, whitespace-only, unreadable, and non-regular plan files retain +their existing explanatory text and leave plan mode active. Their `details` are +`undefined`; they never receive a successful handoff outcome or hash. Calling +`exit_plan_mode` outside plan mode likewise returns no handoff details. A +non-error tool result alone is therefore insufficient to recognize a handoff. + +`approvalSource`, `planHash`, and `planModeExited` are optional in `PlanReviewDetails` +for compatibility with existing producers and older transcripts. Native results +with a valid plan populate all three fields. Consumers should handle their absence +explicitly and must not infer approval or its source from missing fields, or infer +approval from plan-mode exit alone. diff --git a/packages/coding-agent/src/features/plan-mode-tools.ts b/packages/coding-agent/src/features/plan-mode-tools.ts index 54c7da07..d8e46fad 100644 --- a/packages/coding-agent/src/features/plan-mode-tools.ts +++ b/packages/coding-agent/src/features/plan-mode-tools.ts @@ -6,6 +6,7 @@ * through {@link StepPlanModeController}, so the tools stay stateless. */ +import { createHash } from "node:crypto"; import { existsSync, readFileSync, statSync } from "node:fs"; import path from "node:path"; import type { AgentToolResult } from "@step-harness/agent-core"; @@ -56,7 +57,7 @@ function controlResult(text: string, terminate = false): AgentToolResult ({ + planFilePath, + planContents, + planHash, + planModeExited: !planMode.isPlanModeActive(), + approvalSource: ctx.mode === "rpc" || !ctx.hasUI ? "external" : "user", + outcome, + ...(feedback === undefined ? {} : { feedback }), + }); if (ctx.mode === "rpc") { // An rpc child has a dialog bridge (hasUI is true) but no real user // behind it, so its extension_ui_request select can be auto-cancelled - // to "Stay", leaving the child idle in plan mode forever. Auto-approve - // the exit instead and hand the approval to the caller. + // to "Stay", leaving the child idle in plan mode forever. Exit plan mode + // and hand approval to the caller. planMode.exitPlanMode("auto_rpc", ctx); ctx.ui.notify("rpc child auto-approved plan mode exit — user oversight was not gated", "warning"); - return controlResult( + return reviewedResult( `Cannot gate approval in rpc child context. Plan file at ${planFilePath}; caller should approve externally. Plan mode exited and file mutations are re-enabled.\n\nPlan contents:\n${planContents}`, + reviewed("external_approval_required"), ); } if (!ctx.hasUI) { planMode.exitPlanMode("auto_headless", ctx); - return controlResult( + return reviewedResult( `Plan mode exited. Plan file kept at ${planFilePath} for reference.\n\nPlan contents:\n${planContents}\n\nCannot show interactive prompt; caller must gate approval externally.`, + reviewed("external_approval_required"), ); } const review = await reviewPlanInteractively(ctx, planFilePath, planContents); - const reviewed = (outcome: PlanReviewOutcome, feedback?: string): PlanReviewDetails => ({ - planFilePath, - planContents, - outcome, - ...(feedback === undefined ? {} : { feedback }), - }); if (review?.action === "execute") { planMode.exitPlanMode("approved", ctx); return reviewedResult( diff --git a/packages/coding-agent/src/render/plan-review.ts b/packages/coding-agent/src/render/plan-review.ts index 440689f6..dfdb772a 100644 --- a/packages/coding-agent/src/render/plan-review.ts +++ b/packages/coding-agent/src/render/plan-review.ts @@ -25,20 +25,27 @@ import type { ToolRenderResultOptions } from "../core/extensions/types.ts"; import { getMarkdownTheme, type Theme } from "../theme/theme.ts"; import { formatKeyText } from "./keybinding-hints.ts"; -/** How the review ended, as recorded on the tool result for the transcript. */ -export type PlanReviewOutcome = "approved" | "feedback" | "dismissed"; +/** How review ended or was handed off, as recorded on the tool result. */ +export type PlanReviewOutcome = "approved" | "feedback" | "dismissed" | "external_approval_required"; /** * Tool-result details for exit_plan_mode. * * The dialog is an inline component that is torn down the moment it closes, so * the plan it displayed leaves no trace on screen. Carrying the reviewed text - * here lets the tool row render it back into the transcript. + * here lets the tool row render it back into the transcript. Headless and RPC + * results carry the same snapshot for callers to approve externally. */ export interface PlanReviewDetails { planFilePath: string; planContents: string; outcome: PlanReviewOutcome; + /** Who owns the approval decision, independent of whether approval has occurred. */ + approvalSource?: "user" | "external"; + /** SHA-256 (hex) of the exact planContents encoded as UTF-8, without normalization. */ + planHash?: string; + /** Whether plan mode is inactive after this result; this does not itself grant approval. */ + planModeExited?: boolean; /** The note the user typed, for the "feedback" outcome. */ feedback?: string; } @@ -243,7 +250,10 @@ function isPlanReviewDetails(value: unknown): value is PlanReviewDetails { return ( typeof candidate.planFilePath === "string" && typeof candidate.planContents === "string" && - (candidate.outcome === "approved" || candidate.outcome === "feedback" || candidate.outcome === "dismissed") + (candidate.outcome === "approved" || + candidate.outcome === "feedback" || + candidate.outcome === "dismissed" || + candidate.outcome === "external_approval_required") ); } @@ -254,6 +264,7 @@ const OUTCOME_SUMMARIES: Record< approved: { marker: "✓", color: "success", text: "Plan approved" }, feedback: { marker: "✎", color: "warning", text: "Changes requested" }, dismissed: { marker: "•", color: "muted", text: "Review dismissed — still planning" }, + external_approval_required: { marker: "•", color: "warning", text: "External approval required" }, }; /** diff --git a/packages/coding-agent/test/plan-review-dialog.test.ts b/packages/coding-agent/test/plan-review-dialog.test.ts index 41a44990..0e75b2e3 100644 --- a/packages/coding-agent/test/plan-review-dialog.test.ts +++ b/packages/coding-agent/test/plan-review-dialog.test.ts @@ -260,6 +260,25 @@ describe("plan review dialog", () => { } }); + test.each([false, true])( + "external approval stays pending and shows the whole plan (expanded: %s)", + (expanded) => { + const handoff: PlanReviewDetails = { + ...details("external_approval_required"), + planContents: PLAN_LINES.join("\n"), + planModeExited: true, + }; + const component = renderPlanReviewResult(result(handoff), { expanded, isPartial: false }, theme); + const out = lines(component); + expect(out[0]).toContain("External approval required"); + expect(out[0]).toContain("/tmp/plan.md"); + expect(component.render(80)[0]).toContain(colorPrefix("warning")); + expect(out.some((row) => row.includes("Plan approved"))).toBe(false); + expect(out.some((row) => row.includes("line 1 of the plan"))).toBe(true); + expect(out.some((row) => row.includes("line 40 of the plan"))).toBe(true); + }, + ); + test("feedback: the user's note is shown under the summary", () => { const out = lines( renderPlanReviewResult( diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index 56cc67e7..73ef5ee3 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -1,3 +1,4 @@ +import { createHash } from "node:crypto"; import * as fs from "node:fs"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; @@ -11,6 +12,7 @@ import { SessionManager } from "../src/core/session-manager.ts"; import { getPlanFilePath } from "../src/features/plan-mode-tools.ts"; import { createStepPlanExtension } from "../src/features/step-plan.ts"; import { createStepTasksExtension, type StepTask } from "../src/features/step-tasks.ts"; +import type { PlanReviewDetails } from "../src/render/plan-review.ts"; import type { StepTelemetryReporter } from "../src/step/telemetry.ts"; // Preserve real file I/O while making the ESM read export spyable for EACCES tests. @@ -500,30 +502,46 @@ test("plan telemetry carries the source dim across enter, update, and exit", asy // rpc children // --------------------------------------------------------------------------- -test("rpc-mode exit_plan_mode auto-approves without the UI select", async () => { +test.each([true, false])("rpc-mode exit_plan_mode hands approval to the caller (hasUI: %s)", async (hasUI) => { const cwd = makeWorkspace(); const { telemetry, events } = createTelemetryRecorder(); const harness = createHarness({ cwd, review: [REVIEW_KEYS.enter] }); + const originalTools = harness.activeTools(); createStepPlanExtension({ telemetry })(harness.api); - // rpc children have a dialog bridge (hasUI true) but no real user. - const context = harness.ctx({ mode: "rpc" }); + // RPC can expose a dialog bridge (hasUI true), but approval still belongs to its caller. + const context = harness.ctx({ mode: "rpc", hasUI }); await runTool(harness, "enter_plan_mode", {}, context); const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); mkdirSync(path.dirname(planPath), { recursive: true }); - writeFileSync(planPath, "# The plan\n1. Do the thing\n"); + const planContents = "# The plan\n1. Do the thing\n"; + writeFileSync(planPath, planContents); const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { content: Array<{ type: string; text: string }>; + details?: PlanReviewDetails; + terminate?: boolean; }; - const text = result.content[0]!.text; - expect(text).toContain("Cannot gate approval in rpc child context."); - expect(text).toContain(planPath); - expect(text).toContain("caller should approve externally"); + expect(result.content).toEqual([ + { + type: "text", + text: `Cannot gate approval in rpc child context. Plan file at ${planPath}; caller should approve externally. Plan mode exited and file mutations are re-enabled.\n\nPlan contents:\n${planContents}`, + }, + ]); + expect(result.details).toEqual({ + outcome: "external_approval_required", + approvalSource: "external", + planFilePath: planPath, + planContents, + planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), + planModeExited: true, + }); + expect(result.terminate).toBe(false); expect(harness.customCalls()).toBe(0); + expect(harness.userMessages).toEqual([]); // Plan mode actually exited: tools restored, chip cleared, state persisted off. - expect(harness.activeTools()).toContain("write_file"); + expect(harness.activeTools()).toEqual(originalTools); expect(harness.statuses.get("plan-mode")).toBeUndefined(); expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); expect( @@ -562,41 +580,46 @@ test("tui-mode exit_plan_mode still gates through the review dialog", async () = expect(result.content[0]!.text).toContain("do not call exit_plan_mode again"); }); -test("only a dismissed review ends the turn; approval and notes let the agent keep going", async () => { - const cwd = makeWorkspace(); - const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); - const cases = [ - { keys: [REVIEW_KEYS.enter], terminate: false, outcome: "approved" }, - { - keys: [REVIEW_KEYS.down, ...typeReview("use canvas"), REVIEW_KEYS.enter], - terminate: false, - outcome: "feedback", - feedback: "use canvas", - }, - { keys: [REVIEW_KEYS.escape], terminate: true, outcome: "dismissed" }, - ]; - for (const { keys, terminate, outcome, feedback } of cases) { +test.each([ + { keys: [REVIEW_KEYS.enter], terminate: false, outcome: "approved" }, + { + keys: [REVIEW_KEYS.down, ...typeReview("use canvas"), REVIEW_KEYS.enter], + terminate: false, + outcome: "feedback", + feedback: "use canvas", + }, + { keys: [REVIEW_KEYS.escape], terminate: true, outcome: "dismissed" }, + { keys: [], terminate: true, outcome: "dismissed" }, +])( + "interactive $outcome reports its exit state and termination (keys: $keys)", + async ({ keys, terminate, outcome, feedback }) => { + const cwd = makeWorkspace(); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + const planContents = "# The plan\n"; const harness = createHarness({ cwd, review: keys }); createStepPlanExtension()(harness.api); await runTool(harness, "enter_plan_mode", {}, harness.ctx()); mkdirSync(path.dirname(planPath), { recursive: true }); - writeFileSync(planPath, "# The plan\n"); + writeFileSync(planPath, planContents); const result = (await runTool(harness, "exit_plan_mode", {}, harness.ctx())) as { terminate?: boolean; - details?: unknown; + details?: PlanReviewDetails; }; expect(result.terminate).toBe(terminate); - // Every reviewed outcome carries the plan so the tool row can replay it - // after the dialog — the only place it was drawn — has been torn down. + // Every reviewed outcome carries the complete snapshot after the dialog closes. expect(result.details).toEqual({ planFilePath: planPath, - planContents: "# The plan\n", + planContents, + planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), + planModeExited: outcome === "approved", + approvalSource: "user", outcome, ...(feedback === undefined ? {} : { feedback }), }); - } -}); + expect(result.details?.planModeExited).toBe(!lastPlanEntry(harness).enabled); + }, +); // --------------------------------------------------------------------------- // Shared entry and branch-local restoration @@ -1075,17 +1098,35 @@ test.each(["session_start", "session_tree"] as const)( // Proposal validation and caller-owned approval // --------------------------------------------------------------------------- -for (const mode of ["tui", "print", "rpc"] as const) { +for (const { mode, hasUI } of [ + { mode: "tui", hasUI: true }, + { mode: "print", hasUI: false }, + { mode: "rpc", hasUI: true }, + { mode: "rpc", hasUI: false }, +] as const) { + test(`exit_plan_mode without active planning has no handoff in ${mode} (hasUI: ${hasUI})`, async () => { + const harness = createHarness({ cwd: makeWorkspace() }); + createStepPlanExtension()(harness.api); + expect(await runTool(harness, "exit_plan_mode", {}, harness.ctx({ mode, hasUI }))).toEqual({ + content: [{ type: "text", text: "Not in plan mode." }], + details: undefined, + terminate: false, + }); + expect(harness.appended).toEqual([]); + expect(harness.customCalls()).toBe(0); + }); + test.each(["missing", "empty", "whitespace", "directory", "unreadable"])( - `exit_plan_mode refuses %s proposal files in ${mode} before approval or exit`, + `exit_plan_mode refuses %s proposal files in ${mode} (hasUI: ${hasUI}) before approval or exit`, async (invalid) => { const cwd = makeWorkspace(); const { telemetry, events } = createTelemetryRecorder(); const select = vi.fn(async () => "Execute the plan"); const harness = createHarness({ cwd, select }); createStepPlanExtension({ telemetry })(harness.api); - const context = harness.ctx({ mode, hasUI: mode !== "print" }); + const context = harness.ctx({ mode, hasUI }); await runTool(harness, "enter_plan_mode", {}, context); + const planningTools = harness.activeTools(); const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); mkdirSync(path.dirname(planPath), { recursive: true }); if (invalid === "directory") mkdirSync(planPath); @@ -1100,7 +1141,13 @@ for (const mode of ["tui", "print", "rpc"] as const) { const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { content: Array<{ text: string }>; + details?: PlanReviewDetails; + terminate?: boolean; }; + expect(result.details).toBeUndefined(); + expect(result.terminate).toBe(false); + expect(harness.customCalls()).toBe(0); + expect(harness.activeTools()).toEqual(planningTools); expect(select).not.toHaveBeenCalled(); expect(result.content[0].text).toContain(planPath); expect(result.content[0].text).toContain("exit_plan_mode"); @@ -1209,7 +1256,7 @@ test("the review dialog shows the whole plan, however long", async () => { expect(rendered).not.toContain("truncated"); }); -test("a valid headless proposal exits with external approval and the whole plan", async () => { +test("a valid headless proposal exits requiring external approval with the whole plan", async () => { const cwd = makeWorkspace(); const { telemetry, events } = createTelemetryRecorder(); const select = vi.fn(async () => "Execute the plan"); @@ -1220,14 +1267,36 @@ test("a valid headless proposal exits with external approval and the whole plan" await runTool(harness, "enter_plan_mode", {}, context); const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); mkdirSync(path.dirname(planPath), { recursive: true }); - writeFileSync(planPath, `${"p".repeat(2000)}tail of the plan`); + const planContents = `${"p".repeat(2000)}tail of the plan`; + writeFileSync(planPath, planContents); - const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { content: Array<{ text: string }> }; - expect(result.content[0].text).toContain("caller must gate approval externally"); + const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { + content: Array<{ text: string }>; + details?: PlanReviewDetails; + terminate?: boolean; + }; + expect(result.content).toEqual([ + { + type: "text", + text: `Plan mode exited. Plan file kept at ${planPath} for reference.\n\nPlan contents:\n${planContents}\n\nCannot show interactive prompt; caller must gate approval externally.`, + }, + ]); + expect(result.details).toEqual({ + outcome: "external_approval_required", + approvalSource: "external", + planFilePath: planPath, + planContents, + planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), + planModeExited: true, + }); + expect(result.terminate).toBe(false); // A caller that has to gate approval externally cannot do it on a clipped plan. expect(result.content[0].text).toContain("tail of the plan"); expect(result.content[0].text).not.toContain("truncated"); expect(select).not.toHaveBeenCalled(); + expect(harness.customCalls()).toBe(0); + expect(harness.userMessages).toEqual([]); + expect(harness.statuses.get("plan-mode")).toBeUndefined(); expect(lastPlanEntry(harness).enabled).toBe(false); expect(harness.activeTools()).toEqual(originalTools); expect(events.at(-1)).toEqual({ @@ -1236,6 +1305,39 @@ test("a valid headless proposal exits with external approval and the whole plan" }); }); +test.each(["tui", "print", "rpc"] as const)( + "%s plan hashes bind the complete UTF-8 content without normalization", + async (mode) => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd, review: [REVIEW_KEYS.enter] }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode, hasUI: mode !== "print" }); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + const proposal = `\uFEFF# Plan — café 🧪\r\n\r\n${"detail ".repeat(1000)}\r\nlast line \t\r\n`; + const variants = [ + proposal, + proposal.replace("last line", "changed tail"), + proposal.trim(), + proposal.replaceAll("\r\n", "\n"), + ]; + const hashes: string[] = []; + + for (const planContents of variants) { + await runTool(harness, "enter_plan_mode", {}, context); + writeFileSync(planPath, planContents); + const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { details?: PlanReviewDetails }; + expect(result.details).toMatchObject({ + planContents, + planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), + planModeExited: true, + }); + hashes.push(result.details!.planHash!); + } + expect(new Set(hashes).size).toBe(variants.length); + }, +); + test("enter_plan_mode describes writing a proposal rather than tracking todos", () => { const harness = createHarness(); createStepPlanExtension()(harness.api); From 5c3e6cfcaaf5ce21cdbee6e62f026e70d3cc0c28 Mon Sep 17 00:00:00 2001 From: youwang <61931019+MelodyVAR@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:26:57 +0800 Subject: [PATCH 2/5] feat: let extensions declare unavailable tools --- packages/coding-agent/docs/extensions.md | 25 +- .../coding-agent/src/core/agent-session.ts | 38 ++- .../src/core/extensions/loader.ts | 12 + .../src/core/extensions/runner.ts | 9 + .../coding-agent/src/core/extensions/types.ts | 9 + .../test/extension-tool-availability.test.ts | 265 ++++++++++++++++++ ...ension-tool-availability-next-turn.test.ts | 57 ++++ 7 files changed, 401 insertions(+), 14 deletions(-) create mode 100644 packages/coding-agent/test/extension-tool-availability.test.ts create mode 100644 packages/coding-agent/test/suite/regressions/extension-tool-availability-next-turn.test.ts diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index 0ca4bf05..eb77b04c 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -1557,6 +1557,29 @@ pi.registerCommand("deploy", { }); ``` +### pi.setUnavailableTools(names) + +Declare tool names unavailable for this extension's current runtime. This works during extension loading and after startup, and also applies to matching tools registered later. + +```typescript +export default function (pi: ExtensionAPI) { + pi.setUnavailableTools(["remote_lookup"]); + + pi.registerCommand("enable-remote-lookup", { + handler: async () => { + // Clear this extension's declaration after its prerequisite is satisfied. + pi.setUnavailableTools([]); + }, + }); +} +``` + +Declarations combine across extensions: clearing one list cannot release another extension's restriction or override `--exclude-tools`. Unavailable tools are omitted from `getAllTools()`, the model's active tools, and the default generated prompt. Explicit system-prompt replacements remain owned by the extension that supplies them. + +The requested active selection is retained while availability changes. Releasing a restriction restores a tool only if it is selected; an explicit `setActiveTools()` call can change that selection while the tool is unavailable. Declarations belong to the loaded extension instance, so failed loads do not apply them and reload rebuilds them from the new instances. Refresh and reload preserve selections for previously registered tools; genuinely new tool registrations become active by default within the CLI allowlist. Captured APIs from a replaced or disposed runtime reject further changes. + +Use this for tools whose prerequisites or session policy make them unavailable. Tools that require conditional confirmation can stay available. Availability controls discovery and selection; keep execution-time permission checks in the tool or `tool_call` hook. A failed tool call does not automatically change availability. + ### pi.getCommands() Get the slash commands available for invocation via `prompt` in the current session. Includes extension commands, prompt templates, and skill commands. @@ -1676,7 +1699,7 @@ const result = await pi.exec("git", ["status"], { signal, timeout: 5000 }); ### pi.getActiveTools() / pi.getAllTools() / pi.setActiveTools(names) -Manage active tools. This works for both built-in tools and dynamically registered tools. `pi.getActiveTools()` returns the active tool names as `string[]`; `pi.getAllTools()` returns metadata for all configured tools. +Manage active tools. This works for both built-in tools and dynamically registered tools. `pi.getActiveTools()` returns the active tool names as `string[]`; `pi.getAllTools()` returns metadata for configured tools that remain available after CLI selectors and extension availability declarations. ```typescript const active = pi.getActiveTools(); // ["read", "bash", ...] diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 2b16828d..a3fe3a83 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -382,6 +382,8 @@ export class AgentSession { private _initialActiveToolNames?: string[]; private _allowedToolNames?: Set; private _excludedToolNames?: Set; + /** Requested selection includes tools temporarily hidden by extension availability. */ + private _requestedActiveToolNames: string[] = []; private _baseToolsOverride?: Record; private _sessionStartEvent: SessionStartEvent; private _extensionUIContext?: ExtensionUIContext; @@ -1444,13 +1446,16 @@ export class AgentSession { * Get all configured tools with name, description, parameter schema, prompt guidelines, and source metadata. */ getAllTools(): ToolInfo[] { - return Array.from(this._toolDefinitions.values()).map(({ definition, sourceInfo }) => ({ - name: definition.name, - description: definition.description, - parameters: definition.parameters, - promptGuidelines: definition.promptGuidelines, - sourceInfo, - })); + const unavailable = this._extensionRunner.getUnavailableToolNames(); + return Array.from(this._toolDefinitions.values()) + .filter(({ definition }) => !unavailable.has(definition.name)) + .map(({ definition, sourceInfo }) => ({ + name: definition.name, + description: definition.description, + parameters: definition.parameters, + promptGuidelines: definition.promptGuidelines, + sourceInfo, + })); } getToolDefinition(name: string): ToolDefinition | undefined { @@ -1466,7 +1471,7 @@ export class AgentSession { * This compares the selectors against the unfiltered universe (built-in + * extension + SDK tools) so the caller can warn about names that will be ignored. * `knownTools` is that same unfiltered universe — `getAllTools()` reports the - * post-filter set, which is empty precisely when every selector was misspelled. + * post-filter available set, which may also be empty due to extension availability. * Call it only after extensions have finished registering, otherwise * not-yet-registered tools look unknown. */ @@ -1494,13 +1499,18 @@ export class AgentSession { setActiveToolsByName(toolNames: string[]): void { const tools: AgentTool[] = []; const validToolNames: string[] = []; + const requestedToolNames: string[] = []; + const unavailable = this._extensionRunner.getUnavailableToolNames(); for (const name of toolNames) { const tool = this._toolRegistry.get(name); if (tool) { + requestedToolNames.push(name); + if (unavailable.has(name)) continue; tools.push(tool); validToolNames.push(name); } } + this._requestedActiveToolNames = requestedToolNames; this.agent.state.tools = tools; // Rebuild base system prompt with new tool set @@ -3316,7 +3326,7 @@ export class AgentSession { private _refreshToolRegistry(options?: { activeToolNames?: string[]; includeAllExtensionTools?: boolean }): void { const previousRegistryNames = new Set(this._toolRegistry.keys()); - const previousActiveToolNames = this.getActiveToolNames(); + const previousActiveToolNames = this._requestedActiveToolNames; const allowedToolNames = this._allowedToolNames; const excludedToolNames = this._excludedToolNames; const isAllowedTool = (name: string): boolean => @@ -3386,7 +3396,7 @@ export class AgentSession { options?.activeToolNames ? [...options.activeToolNames] : [...previousActiveToolNames] ).filter((name) => isAllowedTool(name)); - if (allowedToolNames) { + if (allowedToolNames && options?.includeAllExtensionTools) { for (const toolName of this._toolRegistry.keys()) { if (allowedToolNames.has(toolName)) { nextActiveToolNames.push(toolName); @@ -3396,7 +3406,9 @@ export class AgentSession { for (const tool of wrappedExtensionTools) { nextActiveToolNames.push(tool.name); } - } else if (!options?.activeToolNames) { + } else { + // Preserve selections for known tools during refresh/reload; only genuinely + // new registrations opt in automatically after initial runtime creation. for (const toolName of this._toolRegistry.keys()) { if (!previousRegistryNames.has(toolName)) { nextActiveToolNames.push(toolName); @@ -3472,9 +3484,9 @@ export class AgentSession { resetApiProviders(); await this._resourceLoader.reload(); this._buildRuntime({ - activeToolNames: this.getActiveToolNames(), + activeToolNames: this._requestedActiveToolNames, flagValues: previousFlagValues, - includeAllExtensionTools: true, + includeAllExtensionTools: false, }); const hasBindings = diff --git a/packages/coding-agent/src/core/extensions/loader.ts b/packages/coding-agent/src/core/extensions/loader.ts index b0a62784..1ce609c8 100644 --- a/packages/coding-agent/src/core/extensions/loader.ts +++ b/packages/coding-agent/src/core/extensions/loader.ts @@ -441,6 +441,18 @@ function createExtensionAPI( runtime.setActiveTools(toolNames); }, + setUnavailableTools(toolNames: readonly string[]): void { + assertActive(); + if (!Array.isArray(toolNames) || toolNames.some((name) => typeof name !== "string" || !name.trim())) { + throw new Error("Unavailable tools must be an array of nonempty tool names."); + } + const names = new Set(toolNames); + const previous = extension.unavailableTools; + if ((previous?.size ?? 0) === names.size && [...names].every((name) => previous?.has(name) === true)) return; + extension.unavailableTools = names; + runtime.refreshTools(); + }, + getCommands() { assertActive(); return runtime.getCommands(); diff --git a/packages/coding-agent/src/core/extensions/runner.ts b/packages/coding-agent/src/core/extensions/runner.ts index 819747e4..37768a5a 100644 --- a/packages/coding-agent/src/core/extensions/runner.ts +++ b/packages/coding-agent/src/core/extensions/runner.ts @@ -501,6 +501,15 @@ export class ExtensionRunner { return this.extensions.map((e) => e.path); } + /** Availability contributions are independent of tool registration ownership. */ + getUnavailableToolNames(): ReadonlySet { + const names = new Set(); + for (const extension of this.extensions) { + for (const name of extension.unavailableTools ?? []) names.add(name); + } + return names; + } + /** Get all registered tools from all extensions (first registration per name wins). */ getAllRegisteredTools(): RegisteredTool[] { const toolsByName = new Map(); diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index ad76bd3f..eed77e31 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -1476,6 +1476,13 @@ export interface ExtensionAPI { /** Set the active tools by name. */ setActiveTools(toolNames: string[]): void; + /** + * Declare tools unavailable for this extension's current runtime. Contributions + * combine across extensions; an empty list clears only this extension's list. + * Availability filters discovery and model tools, not execution permission checks. + * Valid during extension loading; reset when the extension is reloaded. + */ + setUnavailableTools(toolNames: readonly string[]): void; /** Get available slash commands in the current session. */ getCommands(): SlashCommandInfo[]; @@ -1842,6 +1849,8 @@ export interface Extension { sourceInfo: SourceInfo; handlers: Map; tools: Map; + /** Session-local availability contribution owned by this extension instance. */ + unavailableTools?: ReadonlySet; messageRenderers: Map; markdownTransformer?: MarkdownTransformer; entryRenderers?: Map; diff --git a/packages/coding-agent/test/extension-tool-availability.test.ts b/packages/coding-agent/test/extension-tool-availability.test.ts new file mode 100644 index 00000000..b3f50fc1 --- /dev/null +++ b/packages/coding-agent/test/extension-tool-availability.test.ts @@ -0,0 +1,265 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Type } from "typebox"; +import { afterEach, describe, expect, it } from "vitest"; +import type { AgentSession } from "../src/core/agent-session.ts"; +import type { ExtensionAPI, ExtensionFactory, ToolDefinition } from "../src/core/extensions/types.ts"; +import { DefaultResourceLoader } from "../src/core/resource-loader.ts"; +import { createAgentSession } from "../src/core/sdk.ts"; +import { SessionManager } from "../src/core/session-manager.ts"; +import { SettingsManager } from "../src/core/settings-manager.ts"; +import { stepModel } from "./utilities.ts"; + +function tool(name = "restricted_tool"): ToolDefinition { + return { + name, + label: "Local check", + description: "Run a local check", + promptSnippet: "Run the restricted local check", + promptGuidelines: [`Use ${name} for the local check.`], + parameters: Type.Object({}), + execute: async () => ({ content: [{ type: "text", text: "ok" }], details: {} }), + }; +} + +describe("extension-owned tool availability", () => { + const sessions: AgentSession[] = []; + const directories: string[] = []; + + afterEach(() => { + for (const session of sessions.splice(0)) session.dispose(); + for (const directory of directories.splice(0)) rmSync(directory, { recursive: true, force: true }); + }); + + async function setup( + policies: ExtensionFactory[], + options: { tools?: string[]; excludeTools?: string[]; allowLoadErrors?: boolean } = {}, + ) { + const cwd = mkdtempSync(join(tmpdir(), "step-tool-availability-")); + directories.push(cwd); + const agentDir = join(cwd, "agent"); + const settingsManager = SettingsManager.create(cwd, agentDir); + let registrar!: ExtensionAPI; + const resourceLoader = new DefaultResourceLoader({ + cwd, + agentDir, + settingsManager, + extensionFactories: [ + (pi) => { + registrar = pi; + pi.registerTool(tool()); + }, + ...policies, + ], + }); + await resourceLoader.reload(); + if (!options.allowLoadErrors) expect(resourceLoader.getExtensions().errors).toEqual([]); + const { session } = await createAgentSession({ + cwd, + agentDir, + model: stepModel(), + settingsManager, + sessionManager: SessionManager.inMemory(cwd), + resourceLoader, + tools: options.tools, + excludeTools: options.excludeTools, + }); + sessions.push(session); + await session.bindExtensions({}); + return { session, registrar, resourceLoader }; + } + + function expectUnavailable(session: AgentSession, name = "restricted_tool") { + expect(session.getActiveToolNames()).not.toContain(name); + expect(session.getAllTools().map((entry) => entry.name)).not.toContain(name); + expect(session.agent.state.tools.map((entry) => entry.name)).not.toContain(name); + expect(session.systemPrompt).not.toContain(`${name}:`); + expect(session.systemPrompt).not.toContain(`Use ${name} for the local check.`); + } + + it("filters a load-time declaration from the schema, catalog, and prompt", async () => { + const { session } = await setup([(pi) => pi.setUnavailableTools(["restricted_tool"])]); + expectUnavailable(session); + expect(session.getActiveToolNames()).toContain("read"); + }); + + it("combines independent declarations and releases only the caller's restriction", async () => { + let first!: ExtensionAPI; + let second!: ExtensionAPI; + const { session } = await setup([ + (pi) => { + first = pi; + }, + (pi) => { + second = pi; + }, + ]); + expect(session.getActiveToolNames()).toContain("restricted_tool"); + first.setUnavailableTools(["restricted_tool"]); + second.setUnavailableTools(["restricted_tool"]); + expectUnavailable(session); + first.setUnavailableTools([]); + expectUnavailable(session); + second.setUnavailableTools([]); + expect(session.getActiveToolNames()).toContain("restricted_tool"); + expect(session.systemPrompt).toContain("restricted_tool:"); + }); + + it("does not activate a previously inactive tool when a restriction is released", async () => { + let policy!: ExtensionAPI; + const { session } = await setup([ + (pi) => { + policy = pi; + }, + ]); + policy.setActiveTools(["read"]); + policy.setUnavailableTools(["restricted_tool"]); + policy.setUnavailableTools([]); + expect(session.getActiveToolNames()).toEqual(["read"]); + expect(session.getAllTools().map((entry) => entry.name)).toContain("restricted_tool"); + }); + + it("does not reactivate a deselected CLI-allowed tool after availability changes", async () => { + let policy!: ExtensionAPI; + const { session } = await setup( + [ + (pi) => { + policy = pi; + }, + ], + { tools: ["read", "restricted_tool"] }, + ); + policy.setActiveTools(["read"]); + policy.setUnavailableTools(["restricted_tool"]); + policy.setUnavailableTools([]); + expect(session.getActiveToolNames()).toEqual(["read"]); + }); + + it("preserves an explicit selection made while a tool is unavailable", async () => { + let policy!: ExtensionAPI; + const { session } = await setup([ + (pi) => { + policy = pi; + }, + ]); + policy.setUnavailableTools(["restricted_tool"]); + policy.setActiveTools(["restricted_tool"]); + expect(session.getActiveToolNames()).toEqual([]); + policy.setUnavailableTools([]); + expect(session.getActiveToolNames()).toEqual(["restricted_tool"]); + policy.setUnavailableTools(["restricted_tool"]); + policy.setActiveTools(["read"]); + policy.setUnavailableTools([]); + expect(session.getActiveToolNames()).toEqual(["read"]); + }); + + it("applies declarations to tools registered later", async () => { + let policy!: ExtensionAPI; + const { session, registrar } = await setup([ + (pi) => { + policy = pi; + pi.setUnavailableTools(["late_tool"]); + }, + ]); + registrar.registerTool(tool("late_tool")); + expectUnavailable(session, "late_tool"); + policy.setUnavailableTools([]); + expect(session.getActiveToolNames()).toContain("late_tool"); + }); + + it("retains explicit CLI exclusions when extensions release restrictions", async () => { + let policy!: ExtensionAPI; + const { session } = await setup( + [ + (pi) => { + policy = pi; + }, + ], + { excludeTools: ["restricted_tool"] }, + ); + policy.setUnavailableTools(["restricted_tool"]); + policy.setUnavailableTools([]); + policy.setActiveTools(["read", "restricted_tool"]); + expectUnavailable(session); + expect(session.getUnknownToolSelectors().excludeTools).toEqual([]); + }); + + it("does not retain a declaration from an extension that fails to load", async () => { + const { session, resourceLoader } = await setup( + [ + (pi) => { + pi.setUnavailableTools(["restricted_tool"]); + throw new Error("test load failure"); + }, + ], + { allowLoadErrors: true }, + ); + expect(resourceLoader.getExtensions().errors).toHaveLength(1); + expect(resourceLoader.getExtensions().errors[0].error).toContain("test load failure"); + expect(session.getActiveToolNames()).toContain("restricted_tool"); + }); + + it("restores a selected tool across reload and rejects the old extension API", async () => { + let restrict = true; + let policy!: ExtensionAPI; + const { session } = await setup([ + (pi) => { + policy = pi; + pi.setUnavailableTools(restrict ? ["write"] : []); + }, + ]); + const oldPolicy = policy; + expectUnavailable(session, "write"); + restrict = false; + await session.reload(); + expect(session.getActiveToolNames()).toContain("write"); + expect(() => oldPolicy.setUnavailableTools(["read"])).toThrow(/stale/i); + expect(session.getActiveToolNames()).toContain("read"); + }); + + it("preserves inactive tools across reload while activating genuinely new registrations", async () => { + let policy!: ExtensionAPI; + let restricted = true; + let addNewTool = false; + const { session } = await setup([ + (pi) => { + policy = pi; + pi.setUnavailableTools(restricted ? ["restricted_tool"] : []); + if (addNewTool) pi.registerTool(tool("new_tool")); + }, + ]); + policy.setActiveTools(["read"]); + restricted = false; + addNewTool = true; + await session.reload(); + expect(session.getActiveToolNames()).toEqual(["read", "new_tool"]); + expect(session.getAllTools().map((entry) => entry.name)).toContain("restricted_tool"); + }); + + it("copies the declaration and preserves it when invalid updates are rejected", async () => { + let policy!: ExtensionAPI; + const { session } = await setup([ + (pi) => { + policy = pi; + }, + ]); + const names = ["restricted_tool"]; + policy.setUnavailableTools(names); + names.length = 0; + expectUnavailable(session); + expect(() => policy.setUnavailableTools([""])).toThrow(/nonempty tool names/); + expectUnavailable(session); + }); + + it("rejects changes from a disposed extension runtime", async () => { + let policy!: ExtensionAPI; + const { session } = await setup([ + (pi) => { + policy = pi; + }, + ]); + session.dispose(); + expect(() => policy.setUnavailableTools(["read"])).toThrow(/stale/i); + }); +}); diff --git a/packages/coding-agent/test/suite/regressions/extension-tool-availability-next-turn.test.ts b/packages/coding-agent/test/suite/regressions/extension-tool-availability-next-turn.test.ts new file mode 100644 index 00000000..4e707aa5 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/extension-tool-availability-next-turn.test.ts @@ -0,0 +1,57 @@ +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { Type } from "typebox"; +import { expect, it } from "vitest"; +import { createHarness } from "../harness.ts"; + +it("removes unavailable schemas and prompt guidance before the next model request", async () => { + const harness = await createHarness({ + extensionFactories: [ + (pi) => { + pi.registerTool({ + name: "disable_remote", + label: "Disable remote lookup", + description: "Mark the remote lookup unavailable", + promptSnippet: "Disable the remote lookup", + parameters: Type.Object({}), + execute: async () => { + pi.setUnavailableTools(["remote_lookup"]); + return { content: [{ type: "text", text: "disabled" }], details: {} }; + }, + }); + pi.registerTool({ + name: "remote_lookup", + label: "Remote lookup", + description: "Look up remote data", + promptSnippet: "Look up remote data", + parameters: Type.Object({}), + execute: async () => ({ content: [{ type: "text", text: "data" }], details: {} }), + }); + }, + ], + }); + + try { + harness.session.setActiveToolsByName(["disable_remote", "remote_lookup"]); + const schemas: string[][] = []; + const prompts: string[] = []; + harness.setResponses([ + (context) => { + schemas.push((context.tools ?? []).map((tool) => tool.name).sort()); + prompts.push(context.systemPrompt ?? ""); + return fauxAssistantMessage(fauxToolCall("disable_remote", {}), { stopReason: "toolUse" }); + }, + (context) => { + schemas.push((context.tools ?? []).map((tool) => tool.name).sort()); + prompts.push(context.systemPrompt ?? ""); + return fauxAssistantMessage("done"); + }, + ]); + await harness.session.prompt("Disable remote access and finish."); + expect(schemas).toEqual([["disable_remote", "remote_lookup"], ["disable_remote"]]); + expect(prompts[0]).toContain("remote_lookup:"); + expect(prompts[1]).not.toContain("remote_lookup:"); + expect(harness.getPendingResponseCount()).toBe(0); + } finally { + harness.cleanup(); + } +}); From 4d4eae0c2f97314bb86228a313181c6878f2574a Mon Sep 17 00:00:00 2001 From: youwang <61931019+MelodyVAR@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:26:57 +0800 Subject: [PATCH 3/5] feat: support standalone task completion review --- apps/cli/src/main.ts | 13 +- docs/completion-check.md | 105 ++- packages/coding-agent/src/cli/args.ts | 9 +- packages/coding-agent/src/main.ts | 8 + .../src/modes/completion-check.ts | 27 +- packages/coding-agent/src/modes/print-mode.ts | 66 +- .../test/completion-check-args.test.ts | 24 +- .../test/completion-check-cli.test.ts | 288 +++++--- .../test/completion-check.test.ts | 28 +- .../test/suite/completion-check.test.ts | 638 +++++++++++++++--- .../test/suite/plan-storage.test.ts | 52 +- 11 files changed, 966 insertions(+), 292 deletions(-) diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 27fe6b34..5c06e7e8 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -623,20 +623,13 @@ try { let shouldLaunchMain = true; const parsedInteractiveArgs = parseArgs(compatibility?.args ?? stepCodeArgs); if ( - parsedInteractiveArgs.completionReview && - !parsedInteractiveArgs.completionCheck && - !parsedInteractiveArgs.help && - !parsedInteractiveArgs.version - ) { - throw new Error("--completion-review requires --completion-check git-committed"); - } - if ( - parsedInteractiveArgs.completionCheck && + (parsedInteractiveArgs.completionCheck || parsedInteractiveArgs.completionReview) && !parsedInteractiveArgs.help && !parsedInteractiveArgs.version && resolveAppMode(parsedInteractiveArgs, process.stdin.isTTY, process.stdout.isTTY) === "interactive" ) { - throw new Error("--completion-check requires print or JSON mode; use --print or --mode json."); + const flag = parsedInteractiveArgs.completionCheck ? "--completion-check" : "--completion-review"; + throw new Error(`${flag} requires print or JSON mode; use --print or --mode json.`); } const interactiveStartup = isStepInteractiveLoginStartup({ stdinIsTTY: process.stdin.isTTY, diff --git a/docs/completion-check.md b/docs/completion-check.md index d77af815..e6d4f4e4 100644 --- a/docs/completion-check.md +++ b/docs/completion-check.md @@ -1,26 +1,48 @@ -# Print-mode completion check +# Print-mode completion checks and task self-review -The Step CLI can perform a bounded completion check in the existing session: +The Step CLI can request a task self-review or a bounded Git completion check in +the existing session. Both features default off and support print and JSON modes: ```sh +step --print --completion-review "Complete the task." +step --mode json --completion-review "Complete the task." step --print --completion-check git-committed --completion-check-attempts 2 "Complete the task and commit the changes." step --mode json --completion-check git-committed "Complete the task and commit the changes." step --mode json --completion-check git-committed --completion-review "Complete the task and commit the changes." ``` -The feature is off unless `--completion-check git-committed` is supplied. The -attempts option counts **additional prompts**, defaults to 2, and accepts integers -1 through 3. Both flags accept `--flag=value` syntax. Attempts without the check, -unsupported values, interactive mode, RPC, and SDK stdio are rejected. Piped -print mode is supported. Direct `runPrintMode` callers can supply the same -`completionCheck` and `completionCheckAttempts` options. The boolean -`--completion-review` flag takes no value, requires `--completion-check -git-committed`, and defaults off. Direct callers can set `completionReview: true`. +`--completion-review` alone requests exactly one additional semantic review +prompt after the initial prompt, its images, and all additional user messages. +It does not require a Git repository, a new commit, or a clean worktree. The review +asks the same session and model to compare the work with the original visible +task, check task coverage, public interfaces and types, boundary cases and the +final diff, and confirm relevant tests and checks ran after the last edit. It asks +the assistant to run missing verification, fix issues, and provide a brief final +answer while preserving unrelated user changes and respecting permission denials. +It adds no requirement to commit. Original task instructions still apply. + +The boolean review flag takes no value. Its standalone budget is fixed at **one +additional prompt**; `--completion-check-attempts` requires the Git check and cannot +extend standalone review. Direct `runPrintMode` callers can set +`completionReview: true` without a `completionCheck` option. + +`--completion-check git-committed` enables the Git conditions described below. +`--completion-check-attempts` counts **additional prompts**, defaults to 2, and +accepts integers 1 through 3. Both value-taking flags accept `--flag=value` syntax. +Direct callers can supply `completionCheck` and `completionCheckAttempts` options. +Combining `--completion-review` with the Git check uses one slot of that existing +budget for review; it does not add a separate review call or increase the budget. + +Attempts without the Git check, unsupported values, interactive mode, RPC, and +SDK stdio are rejected. Piped print mode is supported. Neither option creates a +new session or resets the invocation's time budget. A review is not a correctness +score, a grader, or independent verification; the canonical task verifier still +owns correctness. -Before binding extensions or sending the first prompt, the check requires a Git -worktree with an existing HEAD commit and saves that HEAD. It then sends the -initial prompt, its images, and all additional user messages in their original -order. Once those prompts finish, completion requires all of these conditions: +When the Git check is enabled, before binding extensions or sending the first +prompt, the check requires a Git worktree with an existing HEAD commit and saves +that HEAD. It then sends the initial prompt, its images, and all additional user +messages in their original order. Once those prompts finish, completion requires all of these conditions: - `starting-HEAD..HEAD` contains at least one commit. A preexisting commit or moving HEAD backwards is insufficient. @@ -43,10 +65,10 @@ not separately to each user message. These prompts consume the original trial's time budget; no trial timeout is extended or reset, and no new attempt is started. The checker neither changes source files nor commits changes or runs hidden tests. -With `--completion-review`, the first eligible completion follow-up requests one -generic self-review even if the Git conditions and final text already pass. It -asks the same session and model to compare the work with the original visible -task, check public interfaces and types, boundary cases and the final diff, and +With both `--completion-check git-committed` and `--completion-review`, the first +eligible completion follow-up requests one generic self-review even if the Git +conditions and final text already pass. It asks the same session and model to +compare the work with the original visible task, check public interfaces and types, boundary cases and the final diff, and confirm relevant tests and checks ran after the last edit. It asks the assistant to fix issues, commit remaining task changes, and provide a final answer while preserving unrelated user changes and permission denials. @@ -60,12 +82,24 @@ optional experiment, not a grader; its benefit to task scores is unproven. It introduces no hidden tests, external grading feedback, automatic commits, or independent correctness judgment. +A successful `exit_plan_mode` tool result with structured details containing +`outcome: "external_approval_required"` is a handoff to the caller, not permission +to execute the plan. The headless result also carries `planHash` and +`planModeExited: true`. This handoff suppresses automatically generated review or +completion feedback for the rest of the invocation. Explicit caller messages, +including approval, cancellation, refinements, or new requests, are still delivered +in their original order, subject to the terminal failure and cancellation guards +below. The automatic guard remains sticky even after those messages: completion +control does not infer approval from prose or an arbitrary non-error plan-exit +call. Ordinary recoverable tool errors retain their existing behavior and do not +trigger this guard. + An explicit terminating tool denial, or an assistant error/abort observed during this invocation, prevents further prompts from the checker, including when a native retry subsequently succeeds. Pending user messages also stop at such a -terminal outcome when the check is enabled. Native provider retry policies are -unchanged. Explicit runtime session replacement continues to rebind listeners and -extensions, but the checker does not carry automatic feedback into another +terminal outcome when either completion feature is enabled. Native provider retry +policies are unchanged. Explicit runtime session replacement continues to rebind +listeners and extensions, but the checker does not carry automatic feedback into another session or working directory. These rules also suppress review, including after an error recovered by native retry. If cancellation or replacement occurs while waiting for stdout, a planned @@ -76,20 +110,33 @@ code **0** even if Git conditions remain unsatisfied. The canonical task verifie owns the score; an ordinary failed task must not become an infrastructure error that resamples the attempt. Missing/thinking-only final output returns **2** with an explicit incomplete diagnostic. Existing terminal denials and final assistant -errors keep exit code **1**. Invalid configuration or failed Git preflight returns -**1** before a model call. If Git becomes unreadable after the model runs, the -checker stops adding prompts and reports that state; final text still returns 0, -and missing final text returns 2. +errors keep exit code **1**. A successful external-approval handoff produced by the +latest prompt keeps exit code **0** even when the last message is its tool result. +An earlier handoff does not excuse missing output from a later explicit prompt: +that prompt must provide final text or a fresh handoff, otherwise the exit code is +**2**. Automatic follow-ups stay blocked for the rest of the invocation. +Invalid configuration or failed Git preflight returns **1** before a model call. +If Git becomes unreadable after the model runs, the checker stops adding prompts +and reports that state; final text still returns 0, and missing final text returns 2. Text stdout contains only the last assistant answer. Diagnostics use stderr. JSON mode keeps the ordinary session event stream, including the added user -prompts, and adds `completion_check` events. Each successful inspection includes -`check`, `attempt` (follow-ups already used, starting at 0), `maxAttempts`, +prompts, and adds `completion_check` events. For the Git check, each successful +inspection includes `check`, `attempt` (follow-ups already used, starting at 0), `maxAttempts`, `hasNewCommit`, `hasCommittedChanges`, `trackedDirty`, `untrackedFiles`, `hasFinalText`, `status` (`passed`, `follow_up`, or `exhausted`), and `willFollowUp`. A failed inspection emits `status: "unavailable"` and `willFollowUp: false`. No filenames, file contents, diffs, commit messages, or Git stderr appear in check feedback/events. +Standalone review emits `completion_check` events with `check: "task-review"`, +`maxAttempts: 1`, `attempt`, `hasFinalText`, `status`, and `willFollowUp`. It includes +no Git-state fields. The status is `follow_up` when scheduling review, `completed` +when final assistant text is present after the single review opportunity, or +`exhausted` when final text is still missing. `completed` records the end of this +bounded flow, not successful verification or a correct task result. It does not +prove that an extension allowed delivery of the review; use `review.sent` for that. +These events contain no correctness score. + Only when review is enabled, completion events also contain `review: { requested: boolean, sent: boolean }`. `requested` becomes true when the first eligible inspection schedules review. `sent` becomes true only when the @@ -107,8 +154,8 @@ messages rather than the number of events. Subsequent inspections retain the review state. With review disabled, these fields and the receipt are absent and the existing event shape is unchanged. -Git is invoked directly with fixed argument arrays, no shell, and only a -validated starting object ID as a variable argument. Reads use `rev-parse`, +Only `--completion-check git-committed` invokes Git. It uses fixed argument arrays, +no shell, and only a validated starting object ID as a variable argument. Reads use `rev-parse`, `rev-list --max-count=1`, `diff --quiet HEAD --`, and NUL-delimited porcelain `status --no-renames` with normal untracked-directory reporting. The tree diff disables external diffs, text conversion, and rename detection. Only diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index 85ac3316..86324354 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -49,7 +49,7 @@ export interface Args { completionCheck?: "git-committed"; /** Maximum completion follow-up prompts, 1..3 (default 2). */ completionCheckAttempts?: number; - /** Opt-in self-review sharing the completion-check follow-up budget. */ + /** One opt-in task self-review; shares the Git follow-up budget when enabled. */ completionReview?: boolean; export?: string; noSkills?: boolean; @@ -578,10 +578,10 @@ export function parseArgs(args: string[]): Args { message: "--completion-check-attempts requires --completion-check git-committed", }); } - if (result.completionReview && !result.completionCheck) { + if (result.completionReview && !result.completionCheck && (result.mode === "rpc" || result.sdkStdio)) { result.diagnostics.push({ type: "error", - message: "--completion-review requires --completion-check git-committed", + message: "--completion-review is only supported in print or JSON mode", }); } @@ -665,7 +665,8 @@ ${stepPermissionOptionsText} --print, -p Non-interactive mode: process prompt and exit --completion-check Opt-in print/json completion check: git-committed --completion-check-attempts Maximum same-session follow-ups: 1..3 (default: 2) - --completion-review Opt-in self-review; requires git-committed and shares its follow-up budget + --completion-review One print/json task self-review; no Git required + With git-committed, shares its follow-up budget --continue, -c Continue previous session --resume, -r [path|id] Resume a session: with a path/id resume it directly, without opens a selector --session Use specific session file or partial UUID diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index a311f784..20b6eda2 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -971,6 +971,11 @@ export async function prepareMain(args: string[], options?: MainOptions): Promis } let appMode = resolveAppMode(parsed, process.stdin.isTTY, process.stdout.isTTY); + if ((parsed.completionCheck || parsed.completionReview) && !parsed.help && appMode === "interactive") { + const flag = parsed.completionCheck ? "--completion-check" : "--completion-review"; + console.error(chalk.red(`Error: ${flag} requires print or JSON mode; use --print or --mode json.`)); + return { kind: "completed", exitCode: 1 }; + } // A bare --resume opens the interactive session selector (createSessionManager checks // parsed.resume before parsed.continue). Without a terminal it can never receive a // selection and would hang until killed, so reject it before the picker opens. @@ -1432,6 +1437,9 @@ async function dispatchAppMode( } else { const exitCode = await runPrintMode(runtimeHost, { mode: toPrintOutputMode(appMode), + completionCheck: prep.parsed.completionCheck, + completionCheckAttempts: prep.parsed.completionCheckAttempts, + completionReview: prep.parsed.completionReview, messages: prep.parsed.messages, initialMessage: prep.initialMessage, initialImages: prep.initialImages, diff --git a/packages/coding-agent/src/modes/completion-check.ts b/packages/coding-agent/src/modes/completion-check.ts index 375a7cf2..cb40b8b7 100644 --- a/packages/coding-agent/src/modes/completion-check.ts +++ b/packages/coding-agent/src/modes/completion-check.ts @@ -5,7 +5,7 @@ export interface CompletionCheckOptions { completionCheck?: "git-committed"; /** Maximum additional prompts, 1..3 (default 2). */ completionCheckAttempts?: number; - /** Request one self-review within the completion follow-up budget (default off). */ + /** Request one task self-review; shares the Git follow-up budget when enabled (default off). */ completionReview?: boolean; } @@ -18,13 +18,10 @@ export interface GitCompletionState { export function getCompletionCheckAttempts(options: CompletionCheckOptions): number | undefined { if (options.completionCheck === undefined) { - if (options.completionReview) { - throw new Error("--completion-review requires --completion-check git-committed"); - } if (options.completionCheckAttempts !== undefined) { throw new Error("--completion-check-attempts requires --completion-check git-committed"); } - return undefined; + return options.completionReview ? 1 : undefined; } if (options.completionCheck !== "git-committed") { throw new Error("--completion-check must be git-committed"); @@ -136,12 +133,18 @@ export async function createGitCompletionCheck( return check; } -export function completionCheckFeedback(git: GitCompletionState, hasFinalText: boolean, review = false): string { +export function completionCheckFeedback( + git: GitCompletionState | undefined, + hasFinalText: boolean, + review = false, +): string { const missing: string[] = []; - if (!git.hasNewCommit) missing.push("no new commit since the starting HEAD"); - if (!git.hasCommittedChanges) missing.push("no committed tree changes from the starting HEAD"); - if (git.trackedDirty) missing.push("tracked changes remain"); - if (git.untrackedFiles) missing.push("unignored untracked files remain"); + if (git) { + if (!git.hasNewCommit) missing.push("no new commit since the starting HEAD"); + if (!git.hasCommittedChanges) missing.push("no committed tree changes from the starting HEAD"); + if (git.trackedDirty) missing.push("tracked changes remain"); + if (git.untrackedFiles) missing.push("unignored untracked files remain"); + } if (!hasFinalText) missing.push("final answer text is missing"); if (review) { return ( @@ -149,7 +152,9 @@ export function completionCheckFeedback(git: GitCompletionState, hasFinalText: b "Final self-review: Compare your work with the original visible task. " + "Check task coverage, public interfaces and types, boundary cases, and the final diff. " + "Confirm relevant tests and checks were run after the last edit; run any missing verification. " + - "Fix issues you find, commit any remaining task changes, then provide a brief final answer. " + + (git + ? "Fix issues you find, commit any remaining task changes, then provide a brief final answer. " + : "Fix issues you find, then provide a brief final answer. ") + "Preserve unrelated user changes and respect permission denials." ); } diff --git a/packages/coding-agent/src/modes/print-mode.ts b/packages/coding-agent/src/modes/print-mode.ts index a5f337e1..457b8a59 100644 --- a/packages/coding-agent/src/modes/print-mode.ts +++ b/packages/coding-agent/src/modes/print-mode.ts @@ -71,6 +71,19 @@ function getTerminatingBlock(event: AgentSessionEvent): TerminatingBlock | undef return { toolName: event.toolName, reason: reason || "no reason given" }; } +/** A successful plan handoff is not approval to send automatic execution prompts. */ +function requiresExternalPlanApproval(event: AgentSessionEvent): boolean { + if (event.type !== "tool_execution_end" || event.isError || event.toolName !== "exit_plan_mode") return false; + const result: unknown = event.result; + if (typeof result !== "object" || result === null) return false; + const details = (result as { details?: unknown }).details; + return ( + typeof details === "object" && + details !== null && + (details as { outcome?: unknown }).outcome === "external_approval_required" + ); +} + function getAssistantFailure(message: AgentMessage | undefined): AssistantMessage | undefined { if (message?.role !== "assistant") return undefined; const assistant = message as AssistantMessage; @@ -106,6 +119,10 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options // Sticky across native retries and runtime rebinds: the completion check // must never resume past a terminal denial, assistant error, or abort. let assistantFailure: AssistantMessage | undefined; + // Sticky for automatic feedback, including after explicit caller messages. + let externalApprovalRequired = false; + // Only a handoff from the latest prompt excuses missing final output. + let latestPromptHasExternalHandoff = false; const completionAbort = new AbortController(); let completionAttempts: number | undefined; let reviewRequested = false; @@ -155,6 +172,10 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options unsubscribe = session.subscribe((event) => { const block = getTerminatingBlock(event); if (block) terminatingBlocks.push(block); + if (requiresExternalPlanApproval(event)) { + externalApprovalRequired = true; + latestPromptHasExternalHandoff = true; + } if (event.type === "message_end") assistantFailure ??= getAssistantFailure(event.message); if (mode === "json") { writeRawStdout(`${JSON.stringify(toJsonEvent(event))}\n`); @@ -200,14 +221,15 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options try { completionAttempts = getCompletionCheckAttempts(options); if (completionAttempts !== undefined && mode !== "text" && mode !== "json") { - throw new Error("--completion-check is only supported in print or JSON mode"); + const flag = options.completionCheck ? "--completion-check" : "--completion-review"; + throw new Error(`${flag} is only supported in print or JSON mode`); } const completionSession = session; const completionCwd = runtimeHost.cwd; const checkGit = - completionAttempts === undefined - ? undefined - : await createGitCompletionCheck(completionCwd, completionAbort.signal); + options.completionCheck === "git-committed" + ? await createGitCompletionCheck(completionCwd, completionAbort.signal) + : undefined; if (mode === "json") { const header = session.sessionManager.getHeader(); @@ -236,7 +258,10 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options } const terminalOutcome = () => disposed || terminatingBlocks.length > 0 || assistantFailure !== undefined; - if (initialMessage && !(checkGit && terminalOutcome())) { + // A plan handoff suppresses automatic feedback, not explicit caller input. + const automaticCompletionBlocked = () => terminalOutcome() || externalApprovalRequired; + if (initialMessage && !(completionAttempts !== undefined && terminalOutcome())) { + latestPromptHasExternalHandoff = false; await session.prompt(initialMessage, { images: initialImages }); assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); } @@ -244,20 +269,21 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options // Keep all explicit user messages in order; the completion budget is for // the entire invocation, not a fresh budget after each user message. for (const message of messages) { - if (checkGit && terminalOutcome()) break; + if (completionAttempts !== undefined && terminalOutcome()) break; + latestPromptHasExternalHandoff = false; await session.prompt(message); assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); } - if (checkGit && completionAttempts !== undefined) { + if (completionAttempts !== undefined) { assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); - for (let attempt = 0; attempt <= completionAttempts && !terminalOutcome(); attempt++) { + for (let attempt = 0; attempt <= completionAttempts && !automaticCompletionBlocked(); attempt++) { // Extension commands may explicitly replace the runtime. Keep normal // rebinding intact, but never carry automatic feedback to a new session. if (session !== completionSession || runtimeHost.cwd !== completionCwd) break; - let git: GitCompletionState; + let git: GitCompletionState | undefined; try { - git = await checkGit(); + git = await checkGit?.(); } catch (error) { console.error(error instanceof Error ? error.message : "Completion check: Git state unavailable."); if (mode === "json") { @@ -269,26 +295,26 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options // infrastructure error after the model has already run. break; } - if (terminalOutcome() || session !== completionSession || runtimeHost.cwd !== completionCwd) break; + if (automaticCompletionBlocked() || session !== completionSession || runtimeHost.cwd !== completionCwd) + break; const hasFinalText = hasFinalAssistantText(session.state.messages.at(-1)); const requestReview = options.completionReview === true && !reviewRequested; const passed = - git.hasNewCommit && - git.hasCommittedChanges && - !git.trackedDirty && - !git.untrackedFiles && + (!git || (git.hasNewCommit && git.hasCommittedChanges && !git.trackedDirty && !git.untrackedFiles)) && hasFinalText && !requestReview; const willFollowUp = !passed && attempt < completionAttempts; if (willFollowUp && requestReview) reviewRequested = true; const completionEvent = { type: "completion_check", - check: "git-committed", + check: options.completionCheck ?? "task-review", attempt, maxAttempts: completionAttempts, ...git, hasFinalText, - status: passed ? "passed" : willFollowUp ? "follow_up" : "exhausted", + // Standalone review records completion of the bounded follow-up, + // never a judgment that the task itself passed verification. + status: passed ? (checkGit ? "passed" : "completed") : willFollowUp ? "follow_up" : "exhausted", willFollowUp, ...(options.completionReview ? { @@ -306,7 +332,8 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options break; } // Backpressure may yield to a signal or a runtime replacement. - if (terminalOutcome() || session !== completionSession || runtimeHost.cwd !== completionCwd) break; + if (automaticCompletionBlocked() || session !== completionSession || runtimeHost.cwd !== completionCwd) + break; const feedback = completionCheckFeedback(git, hasFinalText, requestReview); // A requested prompt can still fail preflight or be intercepted by an // extension. A user-message event is evidence of delivery to this session, @@ -331,6 +358,7 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options }) : undefined; try { + latestPromptHasExternalHandoff = false; await session.prompt(feedback, { expandPromptTemplates: false }); } finally { unsubscribeReview?.(); @@ -369,7 +397,7 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options if (assistantFailure) { console.error(assistantFailure.errorMessage || `Request ${assistantFailure.stopReason}`); exitCode = 1; - } else { + } else if (!latestPromptHasExternalHandoff) { console.error("Completion check incomplete: no final answer text after bounded follow-up."); exitCode = 2; } diff --git a/packages/coding-agent/test/completion-check-args.test.ts b/packages/coding-agent/test/completion-check-args.test.ts index 65f0884e..8573521b 100644 --- a/packages/coding-agent/test/completion-check-args.test.ts +++ b/packages/coding-agent/test/completion-check-args.test.ts @@ -70,9 +70,25 @@ describe("completion-check CLI options", () => { ]); }); - it("requires the check when requesting review", () => { - expect(parseArgs(["--completion-review", "-p", "task"]).diagnostics).toEqual([ - { type: "error", message: "--completion-review requires --completion-check git-committed" }, + it.each([["-p"], ["--mode", "json"], []])("accepts standalone review without a Git budget: %j", (...flags) => { + const parsed = parseArgs(["--completion-review", ...flags, "first", "second"]); + expect(parsed.completionReview).toBe(true); + expect(parsed.completionCheck).toBeUndefined(); + expect(parsed.completionCheckAttempts).toBeUndefined(); + expect(parsed.messages).toEqual(["first", "second"]); + expect(parsed.unknownFlags.size).toBe(0); + expect(parsed.diagnostics).toEqual([]); + }); + + it("does not let standalone review configure a Git follow-up budget", () => { + expect(parseArgs(["-p", "--completion-review", "--completion-check-attempts=2"]).diagnostics).toEqual([ + { type: "error", message: "--completion-check-attempts requires --completion-check git-committed" }, + ]); + }); + + it.each([["--mode", "rpc"], ["--sdk-stdio"]])("rejects standalone review in incompatible mode %j", (...flags) => { + expect(parseArgs(["--completion-review", ...flags]).diagnostics).toEqual([ + { type: "error", message: "--completion-review is only supported in print or JSON mode" }, ]); }); @@ -127,7 +143,9 @@ describe("completion-check CLI options", () => { expect(help).toContain("--completion-check-attempts "); expect(help).toContain("1..3 (default: 2)"); expect(help).toContain("--completion-review"); + expect(help).toContain("One print/json task self-review; no Git required"); expect(help).toContain("shares its follow-up budget"); + expect(help).not.toContain("requires git-committed"); } finally { log.mockRestore(); } diff --git a/packages/coding-agent/test/completion-check-cli.test.ts b/packages/coding-agent/test/completion-check-cli.test.ts index ea902e81..53fd6447 100644 --- a/packages/coding-agent/test/completion-check-cli.test.ts +++ b/packages/coding-agent/test/completion-check-cli.test.ts @@ -6,12 +6,14 @@ import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; const roots: string[] = []; -// test.sh and CI build workspace entrypoints before running tests. Exercise that -// CLI without recompiling its complete TypeScript graph for every isolated fixture. -const cli = fileURLToPath(new URL("../../../apps/cli/dist/main.js", import.meta.url)); +// Exercise the live source dispatchers with the repository's tsx loader. This +// keeps CLI flag tests independent of stale or missing ignored build outputs. +const cli = new URL("../../../apps/cli/src/main.ts", import.meta.url).href; +const legacyMain = new URL("../src/main.ts", import.meta.url).href; +const tsconfig = fileURLToPath(new URL("../../../tsconfig.json", import.meta.url)); const fixture = fileURLToPath(new URL("./fixtures/completion-check-provider.ts", import.meta.url)); -function runCli(flags: string[], repository: boolean) { +function runCli(entrypoint: "step" | "legacy", flags: string[], repository: boolean, tty = false) { const root = mkdtempSync(join(tmpdir(), "completion-cli-")); roots.push(root); const cwd = join(root, "worktree"); @@ -51,6 +53,7 @@ function runCli(flags: string[], repository: boolean) { NODE_ENV: "test", FORCE_COLOR: "0", COMPLETION_CHECK_CALL_LOG: callLog, + TSX_TSCONFIG_PATH: tsconfig, // Keep child runtime caches in the fixture as well as the outer test temp root. TMPDIR: root, // A temporary directory may be inside the enclosing source checkout. @@ -63,10 +66,30 @@ function runCli(flags: string[], repository: boolean) { // offline by explicitly disabling the built-in server in its isolated config. mkdirSync(join(root, "config"), { recursive: true }); writeFileSync(join(root, "config", "config.toml"), "[mcp_servers.steppage__steppage]\nenabled = false\n"); + const entry = join(root, "entry.mjs"); + writeFileSync( + entry, + [ + ...(tty + ? [ + 'Object.defineProperty(process.stdin, "isTTY", { value: true });', + 'Object.defineProperty(process.stdout, "isTTY", { value: true });', + ] + : []), + ...(entrypoint === "step" + ? [`await import(${JSON.stringify(cli)});`] + : [ + `const { main } = await import(${JSON.stringify(legacyMain)});`, + "await main(process.argv.slice(2), { disableBackgroundServices: true });", + ]), + ].join("\n"), + ); const result = spawnSync( process.execPath, [ - cli, + "--import", + import.meta.resolve("tsx"), + entry, "--provider", "completion-offline", "--model", @@ -95,102 +118,173 @@ afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); -describe("real CLI completion-check dispatch with an offline provider", () => { - it("forwards both flags and retains valid failed-task exit 0 after the configured bound", () => { - const result = runCli(["-p", "--completion-check", "git-committed", "--completion-check-attempts", "1"], true); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(0); - expect(result.calls).toBe(2); - expect(result.stdout).toBe("CLI offline final\n"); - expect(result.stderr).toContain("Completion check incomplete after 1 follow-up(s)."); - expect(result.stderr).not.toContain("steppage"); - }); +describe.each(["step", "legacy"] as const)( + "%s CLI completion-check dispatch with an offline provider", + (entrypoint) => { + const run = (flags: string[], repository: boolean, tty = false) => runCli(entrypoint, flags, repository, tty); + it("forwards both flags and retains valid failed-task exit 0 after the configured bound", () => { + const result = run(["-p", "--completion-check", "git-committed", "--completion-check-attempts", "1"], true); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(2); + expect(result.stdout).toBe("CLI offline final\n"); + expect(result.stderr).toContain("Completion check incomplete after 1 follow-up(s)."); + expect(result.stderr).not.toContain("steppage"); + }); - it("records both tree and commit checks in JSON with the default two follow-ups", () => { - const result = runCli(["--mode", "json", "--completion-check=git-committed"], true); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(0); - expect(result.calls).toBe(3); - const checks = result.stdout - .trim() - .split("\n") - .map((line) => JSON.parse(line)) - .filter((event) => event.type === "completion_check"); - expect(checks).toHaveLength(3); - for (const check of checks) { - expect(check).not.toHaveProperty("review"); - expect(check).not.toHaveProperty("followUpKind"); - } - expect(checks.at(-1)).toMatchObject({ - hasNewCommit: false, - hasCommittedChanges: false, - status: "exhausted", - willFollowUp: false, + it("records both tree and commit checks in JSON with the default two follow-ups", () => { + const result = run(["--mode", "json", "--completion-check=git-committed"], true); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(3); + const checks = result.stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line)) + .filter((event) => event.type === "completion_check"); + expect(checks).toHaveLength(3); + for (const check of checks) { + expect(check).not.toHaveProperty("review"); + expect(check).not.toHaveProperty("followUpKind"); + } + expect(checks.at(-1)).toMatchObject({ + hasNewCommit: false, + hasCommittedChanges: false, + status: "exhausted", + willFollowUp: false, + }); }); - }); - it("forwards review and combines it with repair within the existing two-prompt budget", () => { - const result = runCli(["--mode", "json", "--completion-check=git-committed", "--completion-review"], true); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(0); - expect(result.calls).toBe(3); - const events = result.stdout - .trim() - .split("\n") - .map((line) => JSON.parse(line)); - const checks = events.filter((event) => event.type === "completion_check"); - expect(checks.map((event) => [event.attempt, event.status, event.followUpKind, event.review])).toEqual([ - [0, "follow_up", "review", { requested: true, sent: false }], - [0, "follow_up", "review", { requested: true, sent: true }], - [1, "follow_up", "completion", { requested: true, sent: true }], - [2, "exhausted", undefined, { requested: true, sent: true }], - ]); - const users = events.filter((event) => event.type === "message_end" && event.message.role === "user"); - expect(users).toHaveLength(3); - expect(JSON.stringify(users[1])).toContain("Final self-review:"); - expect(JSON.stringify(users[1])).toContain("no new commit since the starting HEAD"); - expect(JSON.stringify(users[2])).not.toContain("Final self-review:"); - expect(result.stderr).toContain("Completion check incomplete after 2 follow-up(s)."); - }); + it("forwards review and combines it with repair within the existing two-prompt budget", () => { + const result = run(["--mode", "json", "--completion-check=git-committed", "--completion-review"], true); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(3); + const events = result.stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line)); + const checks = events.filter((event) => event.type === "completion_check"); + expect(checks.map((event) => [event.attempt, event.status, event.followUpKind, event.review])).toEqual([ + [0, "follow_up", "review", { requested: true, sent: false }], + [0, "follow_up", "review", { requested: true, sent: true }], + [1, "follow_up", "completion", { requested: true, sent: true }], + [2, "exhausted", undefined, { requested: true, sent: true }], + ]); + const users = events.filter((event) => event.type === "message_end" && event.message.role === "user"); + expect(users).toHaveLength(3); + expect(JSON.stringify(users[1])).toContain("Final self-review:"); + expect(JSON.stringify(users[1])).toContain("no new commit since the starting HEAD"); + expect(JSON.stringify(users[2])).not.toContain("Final self-review:"); + expect(result.stderr).toContain("Completion check incomplete after 2 follow-up(s)."); + }); - it("rejects review without the checker before invoking the provider", () => { - const result = runCli(["-p", "--completion-review"], false); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(1); - expect(result.calls).toBe(0); - expect(result.stderr).toContain("--completion-review requires --completion-check git-committed"); - }); + it.each([["-p"], []])("reviews a standalone task once without Git in print/piped mode: %j", (...flags) => { + const result = run([...flags, "--completion-review"], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(2); + expect(result.stdout).toBe("CLI offline final\n"); + expect(result.stderr).not.toContain("Completion check incomplete"); + }); - it("fails a non-repository before the provider is invoked", () => { - const result = runCli(["-p", "--completion-check", "git-committed"], false); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(1); - expect(result.calls).toBe(0); - expect(result.stderr).toContain("existing HEAD commit"); - expect(result.stdout).toBe(""); - }); + it("runs one standalone JSON review after all input messages without Git", () => { + const result = run(["--mode", "json", "--completion-review", "first", "second"], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(4); + const events = result.stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line)); + const users = events.filter((event) => event.type === "message_end" && event.message.role === "user"); + expect(users).toHaveLength(4); + expect(users.slice(0, 3).map((event) => event.message.content[0].text)).toEqual(["first", "second", "task"]); + expect(JSON.stringify(users[3])).toContain("Final self-review:"); + expect(JSON.stringify(users[3])).not.toMatch(/commit|Git/); + const checks = events.filter((event) => event.type === "completion_check"); + expect(checks.map((event) => [event.attempt, event.status, event.review])).toEqual([ + [0, "follow_up", { requested: true, sent: false }], + [0, "follow_up", { requested: true, sent: true }], + [1, "completed", { requested: true, sent: true }], + ]); + for (const event of checks) { + expect(event).toMatchObject({ check: "task-review", maxAttempts: 1 }); + expect(event).not.toHaveProperty("hasNewCommit"); + } + }); + + it.each([false, true])("documents standalone review and the optional shared Git budget with tty=%s", (tty) => { + const result = run(["--completion-review", "--help"], false, tty); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(0); + expect(result.stdout).toContain("One print/json task self-review; no Git required"); + expect(result.stdout).toContain("shares its follow-up budget"); + expect(result.stdout).not.toContain("requires git-committed"); + }); - it("keeps default-off CLI behavior independent of Git", () => { - const result = runCli(["-p"], false); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(0); - expect(result.calls).toBe(1); - expect(result.stdout).toBe("CLI offline final\n"); - }); + it.each([["--completion-review"], ["--completion-check", "git-committed"]])( + "rejects interactive completion before dispatch: %j", + (...flags) => { + const result = run(flags, false, true); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(1); + expect(result.calls).toBe(0); + expect(result.stderr).toContain(`${flags[0]} requires print or JSON mode; use --print or --mode json.`); + }, + ); - it.each([ - ["--completion-check-attempts", "4"], - ["--mode", "rpc"], - ["--sdk-stdio"], - ["--completion-review", "--completion-check-attempts", "4"], - ["--completion-review", "--mode", "rpc"], - ["--completion-review", "--sdk-stdio"], - ["--completion-review=true"], - ])("rejects invalid CLI options before the provider is invoked: %j", (...invalid) => { - const result = runCli(["--completion-check", "git-committed", ...invalid], false); - expect(result.error).toBeUndefined(); - expect(result.status, result.stderr).toBe(1); - expect(result.calls).toBe(0); - expect(result.stderr).toContain("--completion-"); - }); -}); + it.each([["--mode", "rpc"], ["--sdk-stdio"]])( + "rejects incompatible standalone review before dispatch: %j", + (...flags) => { + const result = run(["--completion-review", ...flags], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(1); + expect(result.calls).toBe(0); + expect(result.stderr).toContain("--completion-review is only supported in print or JSON mode"); + }, + ); + + it("rejects a standalone review budget override before invoking the provider", () => { + const result = run(["-p", "--completion-review", "--completion-check-attempts=2"], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(1); + expect(result.calls).toBe(0); + expect(result.stderr).toContain("--completion-check-attempts requires --completion-check git-committed"); + }); + + it("fails a non-repository before the provider is invoked", () => { + const result = run(["-p", "--completion-check", "git-committed"], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(1); + expect(result.calls).toBe(0); + expect(result.stderr).toContain("existing HEAD commit"); + expect(result.stdout).toBe(""); + }); + + it("keeps default-off CLI behavior independent of Git", () => { + const result = run(["-p"], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(0); + expect(result.calls).toBe(1); + expect(result.stdout).toBe("CLI offline final\n"); + }); + + it.each([ + ["--completion-check-attempts", "4"], + ["--mode", "rpc"], + ["--sdk-stdio"], + ["--completion-review", "--completion-check-attempts", "4"], + ["--completion-review", "--mode", "rpc"], + ["--completion-review", "--sdk-stdio"], + ["--completion-review=true"], + ])("rejects invalid CLI options before the provider is invoked: %j", (...invalid) => { + const result = run(["--completion-check", "git-committed", ...invalid], false); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr).toBe(1); + expect(result.calls).toBe(0); + expect(result.stderr).toContain("--completion-"); + }); + }, +); diff --git a/packages/coding-agent/test/completion-check.test.ts b/packages/coding-agent/test/completion-check.test.ts index 2194e70f..477693e3 100644 --- a/packages/coding-agent/test/completion-check.test.ts +++ b/packages/coding-agent/test/completion-check.test.ts @@ -193,14 +193,36 @@ describe("completion-check option validation for direct print-mode callers", () expect(() => getCompletionCheckAttempts({ completionCheckAttempts: 2 })).toThrow("requires --completion-check"); }); - it("rejects review without an enabled check", () => { - expect(() => getCompletionCheckAttempts({ completionReview: true })).toThrow( - "--completion-review requires --completion-check git-committed", + it("allows exactly one standalone review without a Git check", () => { + expect(getCompletionCheckAttempts({ completionReview: true })).toBe(1); + }); + + it("rejects an attempts override for standalone review", () => { + expect(() => getCompletionCheckAttempts({ completionReview: true, completionCheckAttempts: 2 })).toThrow( + "--completion-check-attempts requires --completion-check git-committed", ); }); }); describe("completion self-review feedback", () => { + it("reviews the visible task without requiring Git or a commit", () => { + const feedback = completionCheckFeedback(undefined, true, true); + for (const requirement of [ + "original visible task", + "task coverage", + "public interfaces and types", + "boundary cases", + "final diff", + "after the last edit", + "run any missing verification", + "Preserve unrelated user changes and respect permission denials.", + ]) + expect(feedback).toContain(requirement); + expect(feedback).not.toMatch(/commit|Git|score|passed/i); + expect(feedback).not.toContain("Completion check:"); + expect(completionCheckFeedback(undefined, false, true)).toContain("final answer text is missing"); + }); + it("reviews visible requirements without inventing a missing Git condition", () => { const feedback = completionCheckFeedback( { hasNewCommit: true, hasCommittedChanges: true, trackedDirty: false, untrackedFiles: false }, diff --git a/packages/coding-agent/test/suite/completion-check.test.ts b/packages/coding-agent/test/suite/completion-check.test.ts index 898eec29..cd05ba3d 100644 --- a/packages/coding-agent/test/suite/completion-check.test.ts +++ b/packages/coding-agent/test/suite/completion-check.test.ts @@ -13,6 +13,13 @@ import { createHarness, getUserTexts, type Harness, type HarnessOptions } from " const harnesses: Harness[] = []; let stdout = ""; +const completionConfigurations = [ + { label: "Git completion", options: { completionCheck: "git-committed", completionReview: false } }, + { label: "combined review", options: { completionCheck: "git-committed", completionReview: true } }, + { label: "standalone review", options: { completionCheck: undefined, completionReview: true } }, +] as const; +const reviewConfigurations = completionConfigurations.filter(({ options }) => options.completionReview); + function getCompletionEvents(): Record[] { return stdout .trim() @@ -278,19 +285,23 @@ describe("runPrintMode same-session completion check", () => { }); it.each([ - { stopReason: "error", completionReview: false }, - { stopReason: "error", completionReview: true }, - { stopReason: "aborted", completionReview: false }, - { stopReason: "aborted", completionReview: true }, + { stopReason: "error", completionCheck: "git-committed", completionReview: false }, + { stopReason: "error", completionCheck: "git-committed", completionReview: true }, + { stopReason: "aborted", completionCheck: "git-committed", completionReview: false }, + { stopReason: "aborted", completionCheck: "git-committed", completionReview: true }, + { stopReason: "error", completionCheck: undefined, completionReview: true }, + { stopReason: "aborted", completionCheck: undefined, completionReview: true }, ] as const)( - "never prompts after assistant $stopReason with review=$completionReview", - async ({ stopReason, completionReview }) => { - const { harness, run } = await setup(); + "never prompts after assistant $stopReason with check=$completionCheck review=$completionReview", + async ({ stopReason, completionCheck, completionReview }) => { + const { harness, run } = await setup({}, completionCheck !== undefined); harness.setResponses([ fauxAssistantMessage("", { stopReason, errorMessage: "terminal failure" }), fauxAssistantMessage("must not be used"), ]); - expect(await run({ mode: "json", completionReview, messages: ["pending user prompt"] })).toBe(1); + expect(await run({ mode: "json", completionCheck, completionReview, messages: ["pending user prompt"] })).toBe( + 1, + ); expect(harness.faux.state.callCount).toBe(1); expect(getUserTexts(harness)).toHaveLength(1); expect(console.error).toHaveBeenCalledWith("terminal failure"); @@ -298,9 +309,9 @@ describe("runPrintMode same-session completion check", () => { }, ); - it.each([false, true])( - "does not add prompts after native retry recovery with review=%s", - async (completionReview) => { + it.each(completionConfigurations)( + "does not add prompts after native retry recovery with $label", + async ({ options }) => { const { harness, run } = await setup({ settings: { retry: { enabled: true, maxRetries: 1, baseDelayMs: 1 } }, }); @@ -309,7 +320,7 @@ describe("runPrintMode same-session completion check", () => { fauxAssistantMessage("native retry recovered"), fauxAssistantMessage("must not be used"), ]); - expect(await run({ mode: "json", completionReview })).toBe(0); + expect(await run({ mode: "json", ...options, messages: ["pending user prompt"] })).toBe(0); expect(harness.faux.state.callCount).toBe(2); expect(getUserTexts(harness)).toHaveLength(1); expect(harness.getPendingResponseCount()).toBe(1); @@ -317,33 +328,36 @@ describe("runPrintMode same-session completion check", () => { }, ); - it.each([false, true])("never follows a terminating permission denial with review=%s", async (completionReview) => { - const execute = vi.fn(async () => ({ content: [{ type: "text" as const, text: "unsafe" }], details: {} })); - const tool: AgentTool = { - name: "blocked_tool", - label: "Blocked tool", - description: "test tool", - parameters: Type.Object({}), - execute, - }; - const { harness, run } = await setup({ - tools: [tool], - extensionFactories: [ - (pi) => { - pi.on("tool_call", () => ({ block: true, reason: "explicit permission denial", terminate: true })); - }, - ], - }); - harness.setResponses([ - fauxAssistantMessage(fauxToolCall("blocked_tool", {}), { stopReason: "toolUse" }), - fauxAssistantMessage("must not be used"), - ]); - expect(await run({ mode: "json", completionReview, messages: ["pending user prompt"] })).toBe(1); - expect(harness.faux.state.callCount).toBe(1); - expect(execute).not.toHaveBeenCalled(); - expect(stdout).toContain("explicit permission denial"); - expect(stdout).not.toContain('"type":"completion_check"'); - }); + it.each(completionConfigurations)( + "never follows a terminating permission denial with $label", + async ({ options }) => { + const execute = vi.fn(async () => ({ content: [{ type: "text" as const, text: "unsafe" }], details: {} })); + const tool: AgentTool = { + name: "blocked_tool", + label: "Blocked tool", + description: "test tool", + parameters: Type.Object({}), + execute, + }; + const { harness, run } = await setup({ + tools: [tool], + extensionFactories: [ + (pi) => { + pi.on("tool_call", () => ({ block: true, reason: "explicit permission denial", terminate: true })); + }, + ], + }); + harness.setResponses([ + fauxAssistantMessage(fauxToolCall("blocked_tool", {}), { stopReason: "toolUse" }), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", ...options, messages: ["pending user prompt"] })).toBe(1); + expect(harness.faux.state.callCount).toBe(1); + expect(execute).not.toHaveBeenCalled(); + expect(stdout).toContain("explicit permission denial"); + expect(stdout).not.toContain('"type":"completion_check"'); + }, + ); it("leaves default-off behavior unchanged without Git or a final answer", async () => { const { harness, run } = await setup({}, false); @@ -429,9 +443,9 @@ describe("runPrintMode same-session completion check", () => { expect(stdout).toContain('"text":"last"'); }); - it.each([false, true])( - "preserves explicit rebinding without automatic continuation with review=%s", - async (completionReview) => { + it.each(completionConfigurations)( + "preserves explicit rebinding without automatic continuation with $label", + async ({ options }) => { const first = await setup(); const second = await setup(); first.harness.setResponses([fauxAssistantMessage("before replacement")]); @@ -443,7 +457,7 @@ describe("runPrintMode same-session completion check", () => { first.host.cwd = second.harness.tempDir; await first.host.setRebindSession.mock.calls[0]?.[0]?.(second.harness.session); }); - expect(await first.run({ mode: "json", completionReview, messages: ["explicit next message"] })).toBe(0); + expect(await first.run({ mode: "json", ...options, messages: ["explicit next message"] })).toBe(0); expect(getUserTexts(second.harness)).toEqual(["explicit next message"]); expect(stdout).toContain('"text":"after replacement"'); expect(stdout).not.toContain('"type":"completion_check"'); @@ -599,15 +613,7 @@ describe("opt-in completion self-review", () => { expect(harness.faux.state.callCount).toBe(3); }); - it("rejects review without the check before binding extensions or prompting", async () => { - const { harness, run } = await setup({}, false); - const bind = vi.spyOn(harness.session, "bindExtensions"); - expect(await run({ completionCheck: undefined, completionReview: true })).toBe(1); - expect(bind).not.toHaveBeenCalled(); - expect(harness.faux.state.callCount).toBe(0); - }); - - it("records requested but not sent when the review prompt fails preflight", async () => { + it.each(reviewConfigurations)("records requested but not sent when $label fails preflight", async ({ options }) => { const { harness, run } = await setup(); harness.setResponses([ () => { @@ -619,86 +625,494 @@ describe("opt-in completion self-review", () => { vi.spyOn(harness.session, "prompt") .mockImplementationOnce(originalPrompt) .mockRejectedValueOnce(new Error("review preflight failed")); - expect(await run({ mode: "json", completionReview: true })).toBe(1); + expect(await run({ mode: "json", ...options })).toBe(1); expect(harness.faux.state.callCount).toBe(1); expect(getUserTexts(harness)).toHaveLength(1); expect(getCompletionEvents()).toHaveLength(1); expect(getCompletionEvents()[0].review).toEqual({ requested: true, sent: false }); }); - it.each(["error", "aborted", "recovered retry"] as const)( - "retains a sent receipt if review ends with %s", - async (outcome) => { - const { harness, run } = await setup({ - settings: { retry: { enabled: true, maxRetries: 1, baseDelayMs: 1 } }, - }); + it.each( + reviewConfigurations.flatMap(({ label, options }) => + (["error", "aborted", "recovered retry"] as const).map((outcome) => ({ label, options, outcome })), + ), + )("retains a sent receipt if $label ends with $outcome", async ({ options, outcome }) => { + const { harness, run } = await setup({ + settings: { retry: { enabled: true, maxRetries: 1, baseDelayMs: 1 } }, + }); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage("initial final"); + }, + fauxAssistantMessage("", { + stopReason: outcome === "aborted" ? "aborted" : "error", + errorMessage: outcome === "recovered retry" ? "overloaded_error" : "terminal review failure", + }), + fauxAssistantMessage("recovered final"), + fauxAssistantMessage("unused"), + ]); + expect(await run({ mode: "json", ...options })).toBe(outcome === "recovered retry" ? 0 : 1); + expect(harness.faux.state.callCount).toBe(outcome === "recovered retry" ? 3 : 2); + expect(getUserTexts(harness)).toHaveLength(2); + expect(getCompletionEvents().map((event) => event.review)).toEqual([ + { requested: true, sent: false }, + { requested: true, sent: true }, + ]); + }); + + it.each( + reviewConfigurations.flatMap(({ label, options }) => + (["signal", "session", "cwd"] as const).map((interruption) => ({ label, options, interruption })), + ), + )("does not send $label after $interruption changes while stdout is blocked", async ({ options, interruption }) => { + const first = await setup(); + first.harness.setResponses([ + () => { + commitTask(first.harness); + return fauxAssistantMessage("initial final"); + }, + fauxAssistantMessage("must not be used"), + ]); + let release: () => void = () => {}; + const blocked = new Promise((resolve) => { + release = resolve; + }); + vi.mocked(output.waitForRawStdoutBackpressure).mockImplementation(async () => { + if (stdout.includes('"followUpKind":"review"') && first.harness.faux.state.callCount === 1) await blocked; + }); + const running = first.run({ mode: "json", ...options }); + try { + await vi.waitFor(() => expect(getCompletionEvents()).toHaveLength(1)); + expect(getCompletionEvents()[0].review).toEqual({ requested: true, sent: false }); + expect(getUserTexts(first.harness)).toHaveLength(1); + if (interruption === "signal") { + const exit = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); + process.emit("SIGINT"); + await vi.waitFor(() => expect(exit).toHaveBeenCalledWith(130)); + } else if (interruption === "session") { + const second = await setup(); + second.harness.session.agent.state.messages = [fauxAssistantMessage("replacement final")]; + first.host.session = second.harness.session; + first.host.cwd = second.harness.tempDir; + await first.host.setRebindSession.mock.calls[0]?.[0]?.(second.harness.session); + expect(second.harness.faux.state.callCount).toBe(0); + } else { + first.host.cwd = join(first.harness.tempDir, "other-workspace"); + } + } finally { + release(); + } + expect(await running).toBe(0); + expect(first.harness.faux.state.callCount).toBe(1); + expect(first.harness.getPendingResponseCount()).toBe(1); + expect(getCompletionEvents()).toHaveLength(1); + expect(getCompletionEvents()[0].review).toEqual({ requested: true, sent: false }); + }); +}); + +describe("standalone task self-review", () => { + it.each(["text", "json"] as const)( + "reviews once after all explicit prompts without Git in %s mode", + async (mode) => { + const { harness, host, run } = await setup({}, false); + const sessionId = harness.session.sessionId; + const model = harness.session.model; + const prompt = vi.spyOn(harness.session, "prompt"); + const messages = ["Cover the public interface.", "Verify the boundary case."]; + const images: ImageContent[] = [{ type: "image", mimeType: "image/png", data: "abc" }]; harness.setResponses([ - () => { - commitTask(harness); - return fauxAssistantMessage("initial final"); - }, - fauxAssistantMessage("", { - stopReason: outcome === "aborted" ? "aborted" : "error", - errorMessage: outcome === "recovered retry" ? "overloaded_error" : "terminal review failure", + fauxAssistantMessage("first"), + fauxAssistantMessage("second"), + fauxAssistantMessage("third"), + fauxAssistantMessage("reviewed final"), + fauxAssistantMessage("must not be used"), + ]); + expect( + await run({ + mode, + completionCheck: undefined, + completionReview: true, + initialMessage: "Implement the task.", + initialImages: images, + messages, }), - fauxAssistantMessage("recovered final"), + ).toBe(0); + expect(harness.faux.state.callCount).toBe(4); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness).slice(0, 3)).toEqual(["Implement the task.", ...messages]); + const review = getUserTexts(harness)[3]; + expect(review).toContain("Final self-review:"); + expect(review).toContain("original visible task"); + expect(review).toContain("public interfaces and types"); + expect(review).toContain("boundary cases"); + expect(review).toContain("final diff"); + expect(review).toContain("run any missing verification"); + expect(review).not.toMatch(/commit|Git|score/i); + expect(prompt.mock.calls[0]).toEqual(["Implement the task.", { images }]); + expect(prompt.mock.calls[3]?.[1]).toEqual({ expandPromptTemplates: false }); + expect(harness.session.sessionId).toBe(sessionId); + expect(harness.session.model).toBe(model); + expect(host.newSession).not.toHaveBeenCalled(); + expect(host.fork).not.toHaveBeenCalled(); + expect(host.switchSession).not.toHaveBeenCalled(); + expect(host.dispose).toHaveBeenCalledTimes(1); + expect(console.error).not.toHaveBeenCalled(); + if (mode === "text") expect(stdout).toBe("reviewed final\n"); + else { + const events = getCompletionEvents(); + expect(events.map((event) => [event.attempt, event.status, event.review])).toEqual([ + [0, "follow_up", { requested: true, sent: false }], + [0, "follow_up", { requested: true, sent: true }], + [1, "completed", { requested: true, sent: true }], + ]); + for (const event of events) { + expect(event).toMatchObject({ check: "task-review", maxAttempts: 1 }); + expect(event).not.toHaveProperty("hasNewCommit"); + expect(event).not.toHaveProperty("trackedDirty"); + expect(event).not.toHaveProperty("score"); + } + } + }, + ); + + it("keeps a valid incomplete task at exit 0 without requiring a clean tree or new commit", async () => { + const { harness, run } = await setup(); + const head = git(harness.tempDir, "rev-parse", "HEAD"); + harness.setResponses([ + () => { + writeFileSync(join(harness.tempDir, "source.txt"), "uncommitted task change\n"); + return fauxAssistantMessage("initial final"); + }, + fauxAssistantMessage("Unable to finish; verification failed."), + ]); + expect(await run({ completionCheck: undefined, completionReview: true })).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(git(harness.tempDir, "rev-parse", "HEAD")).toBe(head); + expect(git(harness.tempDir, "status", "--porcelain")).toContain("source.txt"); + expect(stdout).toBe("Unable to finish; verification failed.\n"); + expect(console.error).not.toHaveBeenCalled(); + }); + + it.each([[], [fauxThinking("reasoning only")], [{ type: "text" as const, text: " \n " }]])( + "returns incomplete status 2 after one review with missing final text: %j", + async (...content) => { + const { harness, run } = await setup({}, false); + harness.setResponses([ + fauxAssistantMessage(content), + fauxAssistantMessage(content), fauxAssistantMessage("unused"), ]); - expect(await run({ mode: "json", completionReview: true })).toBe(outcome === "recovered retry" ? 0 : 1); - expect(harness.faux.state.callCount).toBe(outcome === "recovered retry" ? 3 : 2); + expect(await run({ mode: "json", completionCheck: undefined, completionReview: true })).toBe(2); + expect(harness.faux.state.callCount).toBe(2); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness)[1]).toContain("final answer text is missing"); + expect(getCompletionEvents().at(-1)).toMatchObject({ attempt: 1, status: "exhausted", willFollowUp: false }); + expect(console.error).toHaveBeenCalledWith( + "Completion check incomplete: no final answer text after bounded follow-up.", + ); + }, + ); + + it("does not retry a review intercepted by an extension or claim it was sent", async () => { + const { harness, run } = await setup( + { + extensionFactories: [ + (pi) => { + pi.on("input", (event) => + event.text.startsWith("Final self-review:") ? { action: "handled" } : undefined, + ); + }, + ], + }, + false, + ); + harness.setResponses([fauxAssistantMessage("initial final"), fauxAssistantMessage("unused")]); + expect(await run({ mode: "json", completionCheck: undefined, completionReview: true })).toBe(0); + expect(harness.faux.state.callCount).toBe(1); + expect(getUserTexts(harness)).toHaveLength(1); + expect(getCompletionEvents().every((event) => (event.review as { sent: boolean }).sent === false)).toBe(true); + }); +}); + +const externalApprovalDetails = { + outcome: "external_approval_required", + planHash: "a".repeat(64), + planModeExited: true, + planFilePath: "/plans/task.md", + planContents: "Implement only after approval.", +}; + +describe("external plan approval handoff", () => { + it.each(completionConfigurations)( + "stops automatic $label without treating a handoff as failure", + async ({ options }) => { + const tool: AgentTool = { + name: "exit_plan_mode", + label: "Exit plan mode", + description: "handoff fixture", + parameters: Type.Object({}), + execute: async () => ({ + content: [{ type: "text", text: "Handoff recorded." }], + details: externalApprovalDetails, + terminate: true, + }), + }; + const { harness, run } = await setup({ tools: [tool] }, options.completionCheck !== undefined); + harness.setResponses([ + fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", ...options })).toBe(0); + expect(harness.faux.state.callCount).toBe(1); + expect(getUserTexts(harness)).toHaveLength(1); + expect(getCompletionEvents()).toEqual([]); + expect(harness.eventsOfType("tool_execution_end")[0]).toMatchObject({ + isError: false, + result: { details: externalApprovalDetails }, + }); + expect(console.error).not.toHaveBeenCalled(); + }, + ); + + it.each(completionConfigurations)( + "delivers explicit messages after a handoff while suppressing automatic $label", + async ({ options }) => { + const tool: AgentTool = { + name: "exit_plan_mode", + label: "Exit plan mode", + description: "handoff fixture", + parameters: Type.Object({}), + execute: async () => ({ + content: [{ type: "text", text: "Handoff recorded." }], + details: externalApprovalDetails, + terminate: true, + }), + }; + const { harness, run } = await setup({ tools: [tool] }, options.completionCheck !== undefined); + const sessionId = harness.session.sessionId; + const messages = [ + "Approved. Proceed with the plan.", + "Cancel execution.", + "Refine the boundary cases in the proposal.", + "Now describe the public interface.", + ]; + harness.setResponses([ + fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }), + ...messages.map(() => fauxAssistantMessage("explicit response")), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", ...options, initialMessage: "Prepare a plan.", messages })).toBe(0); + expect(getUserTexts(harness)).toEqual(["Prepare a plan.", ...messages]); + expect(harness.faux.state.callCount).toBe(5); + expect(harness.getPendingResponseCount()).toBe(1); + expect(harness.session.sessionId).toBe(sessionId); + expect(getCompletionEvents()).toEqual([]); + expect(console.error).not.toHaveBeenCalled(); + }, + ); + + it.each( + completionConfigurations.flatMap(({ label, options }) => + [false, true].map((freshHandoff) => ({ label, options, freshHandoff })), + ), + )( + "uses the latest input's handoff for final output with $label, fresh=$freshHandoff", + async ({ options, freshHandoff }) => { + const tool: AgentTool = { + name: "exit_plan_mode", + label: "Exit plan mode", + description: "handoff fixture", + parameters: Type.Object({}), + execute: async () => ({ + content: [{ type: "text", text: "Handoff recorded." }], + details: externalApprovalDetails, + terminate: true, + }), + }; + const { harness, run } = await setup({ tools: [tool] }, options.completionCheck !== undefined); + harness.setResponses([ + fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }), + freshHandoff + ? fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }) + : fauxAssistantMessage([]), + fauxAssistantMessage("must not be used"), + ]); + expect( + await run({ + mode: "json", + ...options, + initialMessage: "Prepare a plan.", + messages: ["Approved. Continue."], + }), + ).toBe(freshHandoff ? 0 : 2); + expect(getUserTexts(harness)).toEqual(["Prepare a plan.", "Approved. Continue."]); + expect(harness.faux.state.callCount).toBe(2); + expect(harness.getPendingResponseCount()).toBe(1); + expect(harness.eventsOfType("tool_execution_end")).toHaveLength(freshHandoff ? 2 : 1); + expect(getCompletionEvents()).toEqual([]); + if (freshHandoff) expect(console.error).not.toHaveBeenCalled(); + else + expect(console.error).toHaveBeenCalledWith( + "Completion check incomplete: no final answer text after bounded follow-up.", + ); + }, + ); + + it.each( + completionConfigurations.flatMap(({ label, options }) => + (["error", "aborted", "denial"] as const).map((outcome) => ({ label, options, outcome })), + ), + )("stops pending explicit messages on $outcome after a handoff with $label", async ({ options, outcome }) => { + let handoffRecorded = false; + const tool: AgentTool = { + name: "exit_plan_mode", + label: "Exit plan mode", + description: "handoff fixture", + parameters: Type.Object({}), + execute: async () => { + handoffRecorded = true; + return { + content: [{ type: "text", text: "Handoff recorded." }], + details: externalApprovalDetails, + terminate: true, + }; + }, + }; + const { harness, run } = await setup( + { + tools: [tool], + extensionFactories: [ + (pi) => { + pi.on("tool_call", () => + handoffRecorded + ? { block: true, reason: "explicit permission denial", terminate: true } + : undefined, + ); + }, + ], + }, + options.completionCheck !== undefined, + ); + harness.setResponses([ + fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }), + outcome === "denial" + ? fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }) + : fauxAssistantMessage("", { stopReason: outcome, errorMessage: "explicit prompt failed" }), + fauxAssistantMessage("must not be used"), + ]); + expect( + await run({ + mode: "json", + ...options, + initialMessage: "Prepare a plan.", + messages: ["Explicit next request.", "Must remain unsent after a terminal failure."], + }), + ).toBe(1); + expect(getUserTexts(harness)).toEqual(["Prepare a plan.", "Explicit next request."]); + expect(harness.faux.state.callCount).toBe(2); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getCompletionEvents()).toEqual([]); + expect(console.error).toHaveBeenCalledWith( + outcome === "denial" ? "Blocked exit_plan_mode: explicit permission denial" : "explicit prompt failed", + ); + }); + + it.each(completionConfigurations)( + "does not clear pending approval on an arbitrary successful exit before $label", + async ({ options }) => { + let exits = 0; + const tool: AgentTool = { + name: "exit_plan_mode", + label: "Exit plan mode", + description: "handoff fixture", + parameters: Type.Object({}), + execute: async () => ({ + content: [{ type: "text", text: "The user approved the plan." }], + details: ++exits === 1 ? externalApprovalDetails : {}, + }), + }; + const { harness, run } = await setup({ tools: [tool] }, options.completionCheck !== undefined); + harness.setResponses([ + fauxAssistantMessage([fauxToolCall("exit_plan_mode", {}), fauxToolCall("exit_plan_mode", {})], { + stopReason: "toolUse", + }), + fauxAssistantMessage("Plan ready for external approval."), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", ...options })).toBe(0); + expect(exits).toBe(2); + expect(harness.faux.state.callCount).toBe(2); + expect(getUserTexts(harness)).toHaveLength(1); + expect(getCompletionEvents()).toEqual([]); + expect(console.error).not.toHaveBeenCalled(); + }, + ); + + it.each(reviewConfigurations)( + "stops automatic prompts if $label itself requests external approval", + async ({ options }) => { + const tool: AgentTool = { + name: "exit_plan_mode", + label: "Exit plan mode", + description: "handoff fixture", + parameters: Type.Object({}), + execute: async () => ({ + content: [{ type: "text", text: "Handoff recorded." }], + details: externalApprovalDetails, + terminate: true, + }), + }; + const { harness, run } = await setup({ tools: [tool] }, options.completionCheck !== undefined); + harness.setResponses([ + fauxAssistantMessage("initial final"), + fauxAssistantMessage(fauxToolCall("exit_plan_mode", {}), { stopReason: "toolUse" }), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", ...options })).toBe(0); + expect(harness.faux.state.callCount).toBe(2); expect(getUserTexts(harness)).toHaveLength(2); expect(getCompletionEvents().map((event) => event.review)).toEqual([ { requested: true, sent: false }, { requested: true, sent: true }, ]); + expect(console.error).not.toHaveBeenCalled(); }, ); - it.each(["signal", "session", "cwd"] as const)( - "does not send review after %s changes while stdout is blocked", - async (interruption) => { - const first = await setup(); - first.harness.setResponses([ - () => { - commitTask(first.harness); - return fauxAssistantMessage("initial final"); + it.each(["read_file", "exit_plan_mode"])( + "keeps a recoverable %s failure recoverable before review", + async (toolName) => { + const tool: AgentTool = { + name: toolName, + label: toolName, + description: "recoverable fixture", + parameters: Type.Object({}), + execute: async () => { + throw new Error("recoverable failure"); }, - fauxAssistantMessage("must not be used"), + }; + const { harness, run } = await setup( + { + tools: [tool], + extensionFactories: [ + (pi) => { + pi.on("tool_result", () => ({ details: externalApprovalDetails })); + }, + ], + }, + false, + ); + harness.setResponses([ + fauxAssistantMessage(fauxToolCall(toolName, {}), { stopReason: "toolUse" }), + fauxAssistantMessage("recovered final"), + fauxAssistantMessage("reviewed final"), ]); - let release: () => void = () => {}; - const blocked = new Promise((resolve) => { - release = resolve; - }); - vi.mocked(output.waitForRawStdoutBackpressure).mockImplementation(async () => { - if (stdout.includes('"followUpKind":"review"') && first.harness.faux.state.callCount === 1) await blocked; - }); - const running = first.run({ mode: "json", completionReview: true }); - try { - await vi.waitFor(() => expect(getCompletionEvents()).toHaveLength(1)); - expect(getCompletionEvents()[0].review).toEqual({ requested: true, sent: false }); - expect(getUserTexts(first.harness)).toHaveLength(1); - if (interruption === "signal") { - const exit = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); - process.emit("SIGINT"); - await vi.waitFor(() => expect(exit).toHaveBeenCalledWith(130)); - } else if (interruption === "session") { - const second = await setup(); - second.harness.session.agent.state.messages = [fauxAssistantMessage("replacement final")]; - first.host.session = second.harness.session; - first.host.cwd = second.harness.tempDir; - await first.host.setRebindSession.mock.calls[0]?.[0]?.(second.harness.session); - expect(second.harness.faux.state.callCount).toBe(0); - } else { - first.host.cwd = join(first.harness.tempDir, "other-workspace"); - } - } finally { - release(); - } - expect(await running).toBe(0); - expect(first.harness.faux.state.callCount).toBe(1); - expect(first.harness.getPendingResponseCount()).toBe(1); - expect(getCompletionEvents()).toHaveLength(1); - expect(getCompletionEvents()[0].review).toEqual({ requested: true, sent: false }); + expect(await run({ completionCheck: undefined, completionReview: true })).toBe(0); + expect(harness.faux.state.callCount).toBe(3); + expect(getUserTexts(harness)).toHaveLength(2); + expect(stdout).toBe("reviewed final\n"); + expect(console.error).not.toHaveBeenCalled(); }, ); }); diff --git a/packages/coding-agent/test/suite/plan-storage.test.ts b/packages/coding-agent/test/suite/plan-storage.test.ts index 4e3d9b38..90d49cc7 100644 --- a/packages/coding-agent/test/suite/plan-storage.test.ts +++ b/packages/coding-agent/test/suite/plan-storage.test.ts @@ -1,4 +1,5 @@ import { execFileSync } from "node:child_process"; +import { createHash } from "node:crypto"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -9,6 +10,7 @@ import type { AgentSessionRuntimeHost } from "../../src/core/agent-session-runti import * as output from "../../src/core/output-guard.ts"; import { createStepPlanExtension } from "../../src/features/step-plan.ts"; import { runPrintMode } from "../../src/modes/print-mode.ts"; +import type { PlanReviewDetails } from "../../src/render/plan-review.ts"; import { readStepConfig, resolveStepConfigPath } from "../../src/step/config-toml.ts"; import { createStepToolProfile } from "../../src/step/tool-profile.ts"; import { initTheme } from "../../src/theme/theme.ts"; @@ -27,7 +29,7 @@ function git(cwd: string, ...args: string[]): string { }); } -async function setup(external: boolean) { +async function setup(external: boolean, approvePlan: boolean) { const storage = mkdtempSync(join(tmpdir(), "step-plan-storage-")); roots.push(storage); const harness = await createHarness({ @@ -40,6 +42,24 @@ async function setup(external: boolean) { } }, createStepPlanExtension(), + (pi) => { + if (!approvePlan) return; + // This fixture's caller explicitly approves the reviewed snapshot so these + // tests can exercise Git completion independently of external approval. + pi.on("tool_result", (event) => { + if (event.toolName !== "exit_plan_mode" || event.isError) return; + const details = event.details as PlanReviewDetails | undefined; + if (details?.outcome !== "external_approval_required" || details.planModeExited !== true) return; + expect(details.planHash).toBe(createHash("sha256").update(details.planContents, "utf8").digest("hex")); + return { + content: [ + ...event.content, + { type: "text", text: "The external test caller approved execution of this reviewed plan." }, + ], + details: { ...details, outcome: "approved", approvalSource: "external" }, + }; + }); + }, ], }); harnesses.push(harness); @@ -115,9 +135,9 @@ afterEach(() => { }); test.each([false, true])( - "native planning keeps Git completion sensitive to plan location (external: %s)", + "approved native planning keeps Git completion sensitive to plan location (external: %s)", async (external) => { - const { harness, cwd, planPath, run } = await setup(external); + const { harness, cwd, planPath, run } = await setup(external, true); expect(await run()).toBe(0); const tools = harness.eventsOfType("tool_execution_end"); expect(tools.map((event) => [event.toolName, event.isError])).toEqual([ @@ -129,6 +149,7 @@ test.each([false, true])( expect(getMessageText(tools[0].result)).toContain(planPath); expect(getMessageText(tools[3].result)).toContain("caller must gate approval externally"); expect(getMessageText(tools[3].result)).toContain("Check the parser and tests."); + expect(tools[3].result.details).toMatchObject({ outcome: "approved", approvalSource: "external" }); expect(readFileSync(planPath, "utf8")).toBe("# Proposal\nCheck the parser and tests.\n"); expect(existsSync(join(cwd, ".gitignore"))).toBe(false); // Physical absence catches accidental project writes even if Git ignores them. @@ -165,7 +186,7 @@ test.each([false, true])( ); test("external planning preserves project settings, tracked plans and untracked user files", async () => { - const { harness, cwd, planPath, run } = await setup(true); + const { harness, cwd, planPath, run } = await setup(true, true); git(cwd, "sparse-checkout", "set", "--no-cone", "/package.json", "/.stepcode/"); const oldPlanPath = join(cwd, ".stepcode", "plans", `session-${harness.session.sessionId}.md`); const configPath = join(cwd, ".stepcode", "config.toml"); @@ -198,3 +219,26 @@ test("external planning preserves project settings, tracked plans and untracked expect(getUserTexts(harness)[1]).toContain("unignored untracked files remain"); expect(existsSync(join(cwd, ".gitignore"))).toBe(false); }); + +test.each([false, true])( + "unapproved native planning does not send Git completion feedback (external: %s)", + async (external) => { + const { harness, planPath, run } = await setup(external, false); + expect(await run()).toBe(0); + expect(getUserTexts(harness)).toHaveLength(1); + expect(harness.getPendingResponseCount()).toBe(1); + expect(readFileSync(planPath, "utf8")).toBe("# Proposal\nCheck the parser and tests.\n"); + const exit = harness.eventsOfType("tool_execution_end").find((event) => event.toolName === "exit_plan_mode"); + expect(exit?.result.details).toMatchObject({ + outcome: "external_approval_required", + approvalSource: "external", + planModeExited: true, + }); + const completionEvents = stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line)) + .filter((event) => event.type === "completion_check"); + expect(completionEvents).toEqual([]); + }, +); From ca04ec57d3b8a0f7170f67e3f20eeb25e0f0f7d1 Mon Sep 17 00:00:00 2001 From: youwang <61931019+MelodyVAR@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:26:58 +0800 Subject: [PATCH 4/5] feat: add opt-in runtime information extension --- docs/runtime-info.md | 144 +++++ .../examples/extensions/runtime-info.ts | 173 +++++ .../test/runtime-info-extension.test.ts | 589 ++++++++++++++++++ 3 files changed, 906 insertions(+) create mode 100644 docs/runtime-info.md create mode 100644 packages/coding-agent/examples/extensions/runtime-info.ts create mode 100644 packages/coding-agent/test/runtime-info-extension.test.ts diff --git a/docs/runtime-info.md b/docs/runtime-info.md new file mode 100644 index 00000000..b6292dd8 --- /dev/null +++ b/docs/runtime-info.md @@ -0,0 +1,144 @@ +# Runtime diagnostics example + +`packages/coding-agent/examples/extensions/runtime-info.ts` is an opt-in extension +for inspecting the active runtime and the output-token ceilings seen at provider +request construction. Loading it alone produces no diagnostics and does not +inspect requests. It registers `/runtime-info` and its own `--runtime-info` flag; +there is no built-in runtime diagnostics flag. + +## Usage + +Run these commands from the repository root with `step` on your path, or replace +`step` with `pnpm step` when running from source. + +```sh +step -e ./packages/coding-agent/examples/extensions/runtime-info.ts +``` + +Enter `/runtime-info` to display a snapshot and enable automatic reports for the +current session. Subsequent `/runtime-info` commands show fresh runtime values +and the most recent applicable request observation. `/runtime-info off` silently +stops reporting and clears that observation. Command opt-in ends on session +replacement or reload. + +For headless inspection without submitting a model prompt: + +```sh +step --no-session --print \ + -e ./packages/coding-agent/examples/extensions/runtime-info.ts \ + '/runtime-info' 2>runtime-info.log +``` + +The command uses the existing extension command path. It does not call a model, +resolve credentials, append conversation entries, start an execution turn, or +change model settings. Before an observed request, its request limits are unknown. + +To enable reports from startup, including during an otherwise normal task: + +```sh +step --mode json \ + -e ./packages/coding-agent/examples/extensions/runtime-info.ts \ + --runtime-info 'Explain the project structure' \ + >events.jsonl 2>runtime-info.log +``` + +That normal task can call a model. Adding `--runtime-info` only observes the work +already requested. The flag opts in again at every session start, including new, +resumed, forked, and reloaded sessions. `/runtime-info off` disables reports until +the next opt-in or session start when the flag is set. + +The example uses the existing extension loader and type-only API imports. The +examples' native TypeScript loading behavior is unchanged; no loader flags, +transpilation setup, or dependency installation are added by this extension. + +## Reports + +TUI and RPC modes use `ctx.ui.notify`. Print and JSON modes emit one JSON line to +stderr per report, at most 2 KiB including the newline. The extension does not +write to stdout, so JSON events remain separate. Stderr may also contain other +CLI diagnostics; the extension's JSON objects have `"type":"runtime-info"`. + +Automatic reports occur on session start, context preparation, provider request +construction, model/thinking changes, successful compaction, and tree navigation. +The schema is fixed; unavailable or rejected values are `null`: + +```json +{ + "type": "runtime-info", + "event": "before_provider_request", + "model": { + "id": "example-model", + "provider": "example-provider", + "api": "openai-responses", + "contextWindow": 128000, + "configuredMaxTokens": 32000 + }, + "thinkingLevel": "medium", + "contextUsage": { "tokens": 96000, "contextWindow": 128000, "percent": 75 }, + "lastRequest": { + "max_output_tokens": 4096, + "max_tokens": null, + "max_completion_tokens": null + } +} +``` + +`model` describes the resolved `ctx.model`; `configuredMaxTokens` is its +`maxTokens` metadata. It is not a prediction of the next request's limit. +`thinkingLevel` is the effective level exposed by the runtime, restricted to +`off`, `minimal`, `low`, `medium`, `high`, `xhigh`, and `max`. It is not provider +reasoning content or an inferred reasoning budget. + +`contextUsage` comes from `ctx.getContextUsage()` and retains its estimation and +availability limits. In particular, usage can be unknown after compaction until +a later response. The extension neither tokenizes messages nor substitutes zero +for unknown usage. + +`lastRequest` holds only these positive safe-integer fields, read directly from +the payload at `before_provider_request`: + +| Resolved API | Allowed request fields | +| --- | --- | +| `openai-responses` | `max_output_tokens` | +| `anthropic-messages` | `max_tokens` | +| `openai-completions` | `max_tokens`, `max_completion_tokens` | + +The values include adjustments already made by the provider adapter, such as +context clamping or a protocol minimum. They describe requested ceilings, not +output actually produced or a guarantee about how much text a provider returns. +If both completion fields are present, both are shown without choosing precedence. +Other APIs and absent, nested, inherited, accessor, nonnumeric, nonfinite, +nonpositive, fractional, or unsafe-integer values remain unknown. No request cap +is inferred from configured `maxTokens`. + +A `null` `lastRequest` means there is no applicable observation. An object with +null fields means a request hook was observed but those fields were unavailable +or unsupported. New context preparation, model/thinking changes, compaction, +tree navigation, session start, shutdown, and explicit disable discard old +observations. Live session/model identity is also checked before reporting. A +runtime change between context preparation and request construction invalidates +that pending request observation. + +## Privacy and API limits + +The extension constructs reports from an explicit primitive allowlist. Model, +provider, and API identifiers must be at most 128 ASCII characters drawn from +letters, digits, `.`, `_`, `:`, `/`, and `-`, starting with a letter or digit; +URL-shaped identifiers containing `://` are rejected. Invalid identifiers are +omitted as `null`, rather than truncated or converted from objects. + +It never prints headers, credentials, endpoints, prompts, message bodies, +reasoning content, arbitrary payload fields, session IDs, or session paths. +Request objects are neither enumerated nor serialized, and their getters and +`toJSON` methods are not invoked. Only the small current observation is kept in +memory; there is no persistence, telemetry collector, network client, environment +inspection, or automatic installer. + +`before_provider_request` handlers run in extension load order. The ceiling is +what this extension saw at its hook position; a later handler or transport may +still alter the request. Load the example after payload-rewriting extensions +when practical. The hook does not provide a request ID, request model, session +ID, or a final-wire-payload callback, so this is a runtime snapshot, not a wire +trace. It cannot correlate concurrent custom provider calls or recover limits +from requests made before opt-in, skipped hooks, or previous sessions. It does +not intercept traffic or reconstruct requests to fill those gaps. diff --git a/packages/coding-agent/examples/extensions/runtime-info.ts b/packages/coding-agent/examples/extensions/runtime-info.ts new file mode 100644 index 00000000..78c9cca5 --- /dev/null +++ b/packages/coding-agent/examples/extensions/runtime-info.ts @@ -0,0 +1,173 @@ +/** + * Opt-in, local runtime diagnostics. Load with: + * step -e ./packages/coding-agent/examples/extensions/runtime-info.ts + * Then use /runtime-info, or add --runtime-info for automatic reports. + * See docs/runtime-info.md for the observation limits and output schema. + */ +import type { ExtensionAPI, ExtensionContext } from "@step-harness/coding-agent"; + +type ResetEvent = "context" | "model_select" | "thinking_level_select" | "session_compact" | "session_tree"; +type ReportEvent = ResetEvent | "command" | "session_start" | "before_provider_request"; +type RequestLimits = { + max_output_tokens: number | null; + max_tokens: number | null; + max_completion_tokens: number | null; +}; + +// Read only named, own data properties. Never enumerate, stringify, or invoke +// accessors on provider payloads (including an arbitrary toJSON implementation). +function ownValue(object: unknown, key: string): unknown { + try { + if (typeof object !== "object" || object === null || Array.isArray(object)) return undefined; + const descriptor = Object.getOwnPropertyDescriptor(object, key); + return descriptor && "value" in descriptor ? descriptor.value : undefined; + } catch { + return undefined; + } +} + +function identifier(value: unknown): string | null { + return typeof value === "string" && + value.length <= 128 && + /^[A-Za-z0-9][A-Za-z0-9._:/-]*$/.test(value) && + !value.includes("://") + ? value + : null; +} + +function tokenCount(value: unknown, minimum = 1): number | null { + return typeof value === "number" && Number.isSafeInteger(value) && value >= minimum ? value : null; +} + +function thinkingLevel(value: unknown): string | null { + switch (value) { + case "off": + case "minimal": + case "low": + case "medium": + case "high": + case "xhigh": + case "max": + return value; + default: + return null; + } +} + +function requestLimits(payload: unknown, api: string | null): RequestLimits { + return { + max_output_tokens: api === "openai-responses" ? tokenCount(ownValue(payload, "max_output_tokens")) : null, + max_tokens: + api === "anthropic-messages" || api === "openai-completions" + ? tokenCount(ownValue(payload, "max_tokens")) + : null, + max_completion_tokens: + api === "openai-completions" ? tokenCount(ownValue(payload, "max_completion_tokens")) : null, + }; +} + +export default function runtimeInfo(pi: ExtensionAPI) { + let enabled = false; + let lastRequest: RequestLimits | null = null; + let lastIdentity: string | undefined; + // undefined: no pending context; null: the pending context was invalidated. + let pendingIdentity: string | null | undefined; + + pi.registerFlag("runtime-info", { + description: "Report local runtime metadata and observed request token ceilings", + type: "boolean", + default: false, + }); + + function report(event: ReportEvent, ctx: ExtensionContext, payload?: unknown): void { + const currentModel = ctx.model; + const model = { + id: identifier(ownValue(currentModel, "id")), + provider: identifier(ownValue(currentModel, "provider")), + api: identifier(ownValue(currentModel, "api")), + contextWindow: tokenCount(ownValue(currentModel, "contextWindow")), + configuredMaxTokens: tokenCount(ownValue(currentModel, "maxTokens")), + }; + const level = thinkingLevel(ctx.thinkingLevel); + // Session identity is used only to invalidate the in-memory observation; + // neither session IDs nor paths are included in diagnostics. + const sessionId = identifier(ctx.sessionManager.getSessionId()); + const identity = JSON.stringify([sessionId, model, level]); + if (!sessionId || !model.id || !model.provider || !model.api || identity !== lastIdentity) lastRequest = null; + lastIdentity = identity; + if (event === "context") pendingIdentity = sessionId && model.id && model.provider && model.api ? identity : null; + if (event === "before_provider_request") { + // The hook has no request model or session ID. If the runtime changed + // since context preparation, do not attribute that payload to it. + lastRequest = + pendingIdentity === undefined || pendingIdentity === identity ? requestLimits(payload, model.api) : null; + pendingIdentity = undefined; + } + + const usage = ctx.getContextUsage(); + const percent = ownValue(usage, "percent"); + const diagnostic = { + type: "runtime-info", + event, + model, + thinkingLevel: level, + contextUsage: { + tokens: tokenCount(ownValue(usage, "tokens"), 0), + contextWindow: tokenCount(ownValue(usage, "contextWindow")), + percent: typeof percent === "number" && Number.isFinite(percent) && percent >= 0 ? percent : null, + }, + lastRequest, + }; + // All strings above are fixed labels or bounded ASCII identifiers. Keep + // each line under 2 KiB, including its newline, even if fields are added. + const line = JSON.stringify(diagnostic); + if (line.length + 1 > 2048) return; + if (ctx.hasUI) ctx.ui.notify(line, "info"); + else process.stderr.write(`${line}\n`); + } + + pi.registerCommand("runtime-info", { + description: "Inspect runtime metadata and enable reports for this session; use off to stop", + handler: async (args, ctx) => { + if (args.trim() === "off") { + enabled = false; + lastRequest = null; + lastIdentity = undefined; + pendingIdentity = undefined; + return; + } + enabled = true; + report("command", ctx); + }, + }); + + pi.on("session_start", (_event, ctx) => { + lastRequest = null; + lastIdentity = undefined; + pendingIdentity = undefined; + // CLI values are available only after the extension factory has run. + enabled = pi.getFlag("runtime-info") === true; + if (enabled) report("session_start", ctx); + }); + pi.on("session_shutdown", () => { + enabled = false; + lastRequest = null; + lastIdentity = undefined; + pendingIdentity = undefined; + }); + + const resetAndReport = (event: { type: ResetEvent }, ctx: ExtensionContext): void => { + lastRequest = null; + if (event.type !== "context" && pendingIdentity !== undefined) pendingIdentity = null; + if (enabled) report(event.type, ctx); + }; + pi.on("context", resetAndReport); + pi.on("model_select", resetAndReport); + pi.on("thinking_level_select", resetAndReport); + pi.on("session_compact", resetAndReport); + pi.on("session_tree", resetAndReport); + pi.on("before_provider_request", (event, ctx) => { + if (enabled) report("before_provider_request", ctx, event.payload); + // Returning anything other than undefined would replace the request. + }); +} diff --git a/packages/coding-agent/test/runtime-info-extension.test.ts b/packages/coding-agent/test/runtime-info-extension.test.ts new file mode 100644 index 00000000..734709b3 --- /dev/null +++ b/packages/coding-agent/test/runtime-info-extension.test.ts @@ -0,0 +1,589 @@ +import type { Api, Context, Model, SimpleStreamOptions } from "@step-harness/providers"; +import { afterEach, describe, expect, test, vi } from "vitest"; +import { streamSimple as messagesStream } from "../../providers/src/api/anthropic-messages.ts"; +import { streamSimple as responsesStream } from "../../providers/src/api/openai-responses.ts"; +import runtimeInfo from "../examples/extensions/runtime-info.ts"; +import type { + ContextUsage, + ExtensionAPI, + ExtensionCommandContext, + ExtensionContext, + ExtensionEvent, + SessionCompactEvent, +} from "../src/core/extensions/types.ts"; + +type Hook = (event: ExtensionEvent, ctx: ExtensionContext) => unknown; +type Command = Parameters[1]; + +type Report = { + type: "runtime-info"; + event: string; + model: { + id: string | null; + provider: string | null; + api: string | null; + contextWindow: number | null; + configuredMaxTokens: number | null; + }; + thinkingLevel: string | null; + contextUsage: { tokens: number | null; contextWindow: number | null; percent: number | null }; + lastRequest: { + max_output_tokens: number | null; + max_tokens: number | null; + max_completion_tokens: number | null; + } | null; +}; + +function model(overrides: Partial> = {}): Model { + return { + id: "test-model", + name: "Test model", + provider: "test-provider", + api: "openai-responses", + baseUrl: "https://model.invalid/secret-endpoint", + headers: { Authorization: "Bearer model-secret" }, + reasoning: true, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 128_000, + maxTokens: 32_000, + ...overrides, + }; +} + +function harness(options: { flag?: boolean | string; mode?: ExtensionContext["mode"] } = {}) { + const hooks = new Map(); + const commands = new Map(); + const flags = new Map[1]>(); + const state = { + model: model() as Model | undefined, + thinkingLevel: "medium" as ExtensionContext["thinkingLevel"], + usage: { tokens: 96_000, contextWindow: 128_000, percent: 75 } as ContextUsage | undefined, + sessionId: "session-one", + flag: options.flag, + }; + const stderr: string[] = []; + const notifications: string[] = []; + const stdout = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + vi.spyOn(process.stderr, "write").mockImplementation((text) => { + stderr.push(String(text)); + return true; + }); + const forbidden = { + sendMessage: vi.fn(), + sendUserMessage: vi.fn(), + appendEntry: vi.fn(), + setModel: vi.fn(), + setThinkingLevel: vi.fn(), + setActiveTools: vi.fn(), + registerTool: vi.fn(), + registerTools: vi.fn(), + exec: vi.fn(), + compact: vi.fn(), + abort: vi.fn(), + shutdown: vi.fn(), + newSession: vi.fn(), + fork: vi.fn(), + navigateTree: vi.fn(), + switchSession: vi.fn(), + reload: vi.fn(), + getSystemPrompt: vi.fn(), + getSystemPromptOptions: vi.fn(), + }; + const getContextUsage = vi.fn(() => state.usage); + const getSessionId = vi.fn(() => state.sessionId); + const getThinkingLevel = vi.fn(() => state.thinkingLevel); + const getFlag = vi.fn(() => state.flag); + const api = { + ...forbidden, + on(event: string, handler: Hook) { + hooks.set(event, [...(hooks.get(event) ?? []), handler]); + }, + registerCommand(name: string, command: Command) { + commands.set(name, command); + }, + registerFlag(name: string, flag: Parameters[1]) { + flags.set(name, flag); + }, + getFlag, + getThinkingLevel, + } as unknown as ExtensionAPI; + const mode = options.mode ?? "json"; + const ctx: ExtensionCommandContext = { + ...forbidden, + mode, + hasUI: mode === "tui" || mode === "rpc", + ui: { + notify(text: string) { + notifications.push(text); + }, + } as ExtensionContext["ui"], + cwd: "/synthetic-workspace", + sessionManager: { getSessionId } as unknown as ExtensionContext["sessionManager"], + modelRegistry: {} as ExtensionContext["modelRegistry"], + get model() { + return state.model; + }, + get thinkingLevel() { + return state.thinkingLevel; + }, + scopedModels: [], + isIdle: () => true, + isProjectTrusted: () => true, + signal: undefined, + hasPendingMessages: () => false, + waitForIdle: async () => {}, + getContextUsage, + }; + runtimeInfo(api); + const emit = async (event: ExtensionEvent) => { + const results: unknown[] = []; + for (const handler of hooks.get(event.type) ?? []) results.push(await handler(event, ctx)); + return results; + }; + return { + state, + ctx, + hooks, + flags, + commands, + stderr, + notifications, + stdout, + forbidden, + getContextUsage, + getSessionId, + getThinkingLevel, + getFlag, + emit, + start: () => emit({ type: "session_start", reason: "startup" }), + request: (payload: unknown) => emit({ type: "before_provider_request", payload }), + async command(args = "") { + const command = commands.get("runtime-info"); + expect(command, "extension registers /runtime-info").toBeDefined(); + await command!.handler(args, ctx); + }, + reports: (): Report[] => [...stderr, ...notifications].map((line) => JSON.parse(line)), + last: (): Report => { + const line = [...stderr, ...notifications].at(-1); + expect(line, "an opted-in diagnostic report was emitted").toBeDefined(); + return JSON.parse(line!); + }, + }; +} + +const compacted: SessionCompactEvent = { + type: "session_compact", + compactionEntry: { + type: "compaction", + id: "compaction-one", + parentId: null, + timestamp: "2026-10-11T00:00:00.000Z", + summary: "private compaction summary", + firstKeptEntryId: "entry-two", + tokensBefore: 96_000, + }, + fromExtension: false, + reason: "manual", + willRetry: false, +}; + +afterEach(() => vi.restoreAllMocks()); + +describe("runtime-info example extension", () => { + test("registers only extension-local opt-ins without reading runtime state in the factory", () => { + const h = harness(); + expect([...h.commands.keys()]).toEqual(["runtime-info"]); + expect([...h.flags.entries()]).toEqual([ + ["runtime-info", { description: expect.any(String), type: "boolean", default: false }], + ]); + expect(h.getFlag).not.toHaveBeenCalled(); + expect(h.getContextUsage).not.toHaveBeenCalled(); + expect(h.getThinkingLevel).not.toHaveBeenCalled(); + expect(h.reports()).toEqual([]); + }); + + test.each([undefined, false, "true"])("stays inactive without a boolean opt-in (%s)", async (flag) => { + const h = harness({ flag }); + await h.start(); + const payload = Object.defineProperty({}, "max_output_tokens", { + get: () => { + throw new Error("inactive diagnostics must not inspect the payload"); + }, + }); + await h.emit({ type: "context", messages: [] }); + await h.request(payload); + await h.emit(compacted); + await h.emit({ type: "model_select", model: h.state.model!, previousModel: undefined, source: "set" }); + expect(h.reports()).toEqual([]); + expect(h.getContextUsage).not.toHaveBeenCalled(); + expect(h.getSessionId).not.toHaveBeenCalled(); + expect(h.getThinkingLevel).not.toHaveBeenCalled(); + expect(h.stdout).not.toHaveBeenCalled(); + }); + + test("reports the resolved runtime after flags are available at session_start", async () => { + const h = harness(); + h.state.flag = true; + await h.start(); + expect(h.reports()).toEqual([ + { + type: "runtime-info", + event: "session_start", + model: { + id: "test-model", + provider: "test-provider", + api: "openai-responses", + contextWindow: 128_000, + configuredMaxTokens: 32_000, + }, + thinkingLevel: "medium", + contextUsage: { tokens: 96_000, contextWindow: 128_000, percent: 75 }, + lastRequest: null, + }, + ]); + }); + + test.each(["tui", "rpc", "print", "json"] as const)( + "/runtime-info inspects locally in %s without a model call or execution turn", + async (mode) => { + const h = harness({ mode }); + await h.start(); + const fetch = vi.spyOn(globalThis, "fetch").mockRejectedValue(new Error("unexpected network call")); + await h.command(); + expect(h.reports()).toHaveLength(1); + expect(h.last().event).toBe("command"); + expect(h.last().lastRequest).toBeNull(); + expect(h.notifications).toHaveLength(mode === "tui" || mode === "rpc" ? 1 : 0); + expect(h.stderr).toHaveLength(mode === "print" || mode === "json" ? 1 : 0); + for (const method of Object.values(h.forbidden)) expect(method).not.toHaveBeenCalled(); + expect(fetch).not.toHaveBeenCalled(); + expect(h.stdout).not.toHaveBeenCalled(); + }, + ); + + test.each([ + ["openai-responses", "max_output_tokens", 16], + ["anthropic-messages", "max_tokens", 4096], + ["openai-completions", "max_completion_tokens", 2048], + ["openai-completions", "max_tokens", 1024], + ] as const)("reports %s request %s separately from the configured ceiling", async (api, field, ceiling) => { + const h = harness({ flag: true }); + h.state.model = model({ api }); + await h.start(); + const payload = Object.freeze({ model: "wire-model-alias", [field]: ceiling, stream: true }); + expect(await h.request(payload)).toEqual([undefined]); + expect(h.last().model.configuredMaxTokens).toBe(32_000); + expect(h.last().model.id).toBe("test-model"); + expect(h.last().lastRequest).toEqual({ + max_output_tokens: null, + max_tokens: null, + max_completion_tokens: null, + [field]: ceiling, + }); + await h.command(); + expect(h.last().lastRequest?.[field]).toBe(ceiling); + expect(payload[field]).toBe(ceiling); + }); + + test("a command opts into future hooks and off stops observation", async () => { + const h = harness(); + await h.start(); + await h.request({ max_output_tokens: 10_000 }); + await h.command(); + expect(h.last().lastRequest).toBeNull(); + await h.request({ max_output_tokens: 2000 }); + expect(h.last().lastRequest?.max_output_tokens).toBe(2000); + await h.command("off"); + const count = h.reports().length; + h.getContextUsage.mockClear(); + await h.request({ max_output_tokens: 1000 }); + await h.emit({ type: "context", messages: [] }); + expect(h.reports()).toHaveLength(count); + expect(h.getContextUsage).not.toHaveBeenCalled(); + await h.command(); + expect(h.last().lastRequest).toBeNull(); + }); + + test("clears the last request for a new context without reading or changing messages", async () => { + const h = harness({ flag: true }); + await h.start(); + await h.request({ max_output_tokens: 2000 }); + const event = Object.defineProperty({ type: "context" }, "messages", { + get: () => { + throw new Error("private message bodies must not be read"); + }, + }) as ExtensionEvent; + expect(await h.emit(event)).toEqual([undefined]); + expect(h.last().event).toBe("context"); + expect(h.last().lastRequest).toBeNull(); + }); + + test("missing model, reasoning and context values stay unknown", async () => { + const h = harness({ flag: true }); + h.state.model = undefined; + h.state.thinkingLevel = undefined; + h.state.usage = undefined; + await h.start(); + expect(h.last().model).toEqual({ + id: null, + provider: null, + api: null, + contextWindow: null, + configuredMaxTokens: null, + }); + expect(h.last().thinkingLevel).toBeNull(); + expect(h.last().contextUsage).toEqual({ tokens: null, contextWindow: null, percent: null }); + await h.request({ max_output_tokens: 2000 }); + expect(h.last().lastRequest).toEqual({ max_output_tokens: null, max_tokens: null, max_completion_tokens: null }); + }); + + test("compaction and tree navigation discard request info and preserve unknown usage", async () => { + const h = harness({ flag: true }); + await h.start(); + await h.request({ max_output_tokens: 2000 }); + h.state.usage = { tokens: null, contextWindow: 128_000, percent: null }; + expect(await h.emit(compacted)).toEqual([undefined]); + expect(h.last().event).toBe("session_compact"); + expect(h.last().contextUsage).toEqual({ tokens: null, contextWindow: 128_000, percent: null }); + expect(h.last().lastRequest).toBeNull(); + await h.request({ max_output_tokens: 1000 }); + await h.emit({ type: "session_tree", oldLeafId: "one", newLeafId: "two" }); + expect(h.last().lastRequest).toBeNull(); + }); + + test.each(["new", "resume", "fork", "reload"] as const)( + "session_start (%s) clears request state", + async (reason) => { + const h = harness({ flag: true }); + await h.start(); + await h.request({ max_output_tokens: 2000 }); + await h.emit({ type: "session_start", reason, previousSessionFile: "/private/session.jsonl" }); + expect(h.last().lastRequest).toBeNull(); + await h.command(); + expect(h.last().lastRequest).toBeNull(); + }, + ); + + test("command opt-in does not survive session replacement or shutdown", async () => { + const h = harness(); + await h.start(); + await h.command(); + await h.request({ max_output_tokens: 2000 }); + await h.emit({ type: "session_shutdown", reason: "new" }); + const count = h.reports().length; + await h.request({ max_output_tokens: 1000 }); + h.state.sessionId = "session-two"; + await h.emit({ type: "session_start", reason: "new" }); + await h.request({ max_output_tokens: 1000 }); + expect(h.reports()).toHaveLength(count); + await h.command(); + expect(h.last().lastRequest).toBeNull(); + }); + + test("model selection clears old requests even when the same model is selected again", async () => { + const h = harness({ flag: true }); + await h.start(); + await h.request({ max_output_tokens: 2000 }); + await h.emit({ type: "model_select", model: h.state.model!, previousModel: h.state.model, source: "set" }); + expect(h.last().lastRequest).toBeNull(); + }); + + test.each(["id", "provider", "api", "contextWindow", "maxTokens", "session", "thinking"] as const)( + "live %s changes cannot inherit stale request info even without a hook", + async (field) => { + const h = harness({ flag: true }); + await h.start(); + await h.request({ max_output_tokens: 2000 }); + if (field === "session") h.state.sessionId = "session-two"; + else if (field === "thinking") h.state.thinkingLevel = "high"; + else { + const changed = { + id: "other-model", + provider: "other-provider", + api: "anthropic-messages", + contextWindow: 64_000, + maxTokens: 8000, + }; + h.state.model = model({ [field]: changed[field] }); + } + await h.command(); + expect(h.last().lastRequest).toBeNull(); + }, + ); + + test.each([undefined, null, "2000", 0, -1, 1.5, NaN, Infinity, Number.MAX_SAFE_INTEGER + 1, true, {}, 2000n])( + "does not coerce invalid request ceilings (%s) or reuse a previous value", + async (value) => { + const h = harness({ flag: true }); + await h.start(); + await h.request({ max_output_tokens: 2000 }); + await h.request({ max_output_tokens: value }); + expect(h.last().lastRequest?.max_output_tokens).toBeNull(); + }, + ); + + test.each([ + null, + undefined, + "private body", + [], + { body: { max_output_tokens: 2000 } }, + Object.create({ max_output_tokens: 2000 }), + ])("unknown payload shapes have unknown ceilings", async (payload) => { + const h = harness({ flag: true }); + await h.start(); + await h.request(payload); + expect(h.last().lastRequest).toEqual({ max_output_tokens: null, max_tokens: null, max_completion_tokens: null }); + }); + + test("ignores accessors and unrecognized APIs instead of guessing a ceiling", async () => { + const h = harness({ flag: true }); + await h.start(); + const getter = vi.fn(() => 2000); + await h.request(Object.defineProperty({}, "max_output_tokens", { get: getter })); + expect(getter).not.toHaveBeenCalled(); + expect(h.last().lastRequest?.max_output_tokens).toBeNull(); + h.state.model = model({ api: "custom-api" }); + await h.request({ max_output_tokens: 2000, max_tokens: 2000, max_completion_tokens: 2000 }); + expect(h.last().lastRequest).toEqual({ max_output_tokens: null, max_tokens: null, max_completion_tokens: null }); + }); + + test("emits only allowlisted primitives and never reads secret-bearing payload or model fields", async () => { + const h = harness({ flag: true }); + const readSecret = vi.fn(() => { + throw new Error("private field read"); + }); + Object.defineProperty(h.state.model!, "baseUrl", { get: readSecret }); + Object.defineProperty(h.state.model!, "headers", { get: readSecret }); + await h.start(); + const payload = { + max_output_tokens: 2000, + model: "secret-wire-model", + apiKey: "secret-api-key", + headers: { Authorization: "Bearer secret-header" }, + baseUrl: "https://secret.invalid/endpoint", + input: [{ content: "secret-input" }], + messages: [{ role: "user", content: "secret-message" }], + prompt: "secret-prompt", + reasoning: { effort: "secret-reasoning" }, + metadata: { user: "secret-user" }, + toJSON: readSecret, + }; + Object.defineProperty(payload, "instructions", { get: readSecret }); + Object.freeze(payload); + expect(await h.request(payload)).toEqual([undefined]); + await h.command(); + expect(readSecret).not.toHaveBeenCalled(); + expect(h.last().lastRequest).toEqual({ max_output_tokens: 2000, max_tokens: null, max_completion_tokens: null }); + expect(h.stderr.join("")).not.toMatch( + /secret|Authorization|apiKey|headers|baseUrl|input|messages|prompt|metadata|session-one/, + ); + for (const method of Object.values(h.forbidden)) expect(method).not.toHaveBeenCalled(); + }); + + test("omits invalid or oversized identifiers and numbers instead of serializing arbitrary values", async () => { + const h = harness({ flag: true }); + h.state.model = { + ...model(), + id: "x".repeat(100_000), + provider: "https://secret.invalid/path", + api: "bad\napi", + contextWindow: -1, + maxTokens: "secret-ceiling", + } as unknown as Model; + h.state.thinkingLevel = "secret-level" as ExtensionContext["thinkingLevel"]; + h.state.usage = { tokens: NaN, contextWindow: 0, percent: Infinity }; + await h.start(); + expect(h.last().model).toEqual({ + id: null, + provider: null, + api: null, + contextWindow: null, + configuredMaxTokens: null, + }); + expect(h.last().thinkingLevel).toBeNull(); + expect(h.last().contextUsage).toEqual({ tokens: null, contextWindow: null, percent: null }); + expect(h.stderr.join("")).not.toContain("secret"); + expect(Buffer.byteLength(h.stderr[0])).toBeLessThanOrEqual(2048); + expect(h.stderr[0].split("\n")).toHaveLength(2); + }); + + test("preserves known zero usage and reads the effective thinking level on every report", async () => { + const h = harness({ flag: true }); + h.state.usage = { tokens: 0, contextWindow: 128_000, percent: 0 }; + h.state.thinkingLevel = "off"; + await h.start(); + expect(h.last().contextUsage).toEqual(h.state.usage); + expect(h.last().thinkingLevel).toBe("off"); + await h.request({ max_output_tokens: 2000 }); + h.state.thinkingLevel = "low"; + await h.emit({ type: "thinking_level_select", level: "low", previousLevel: "off" }); + expect(h.last().thinkingLevel).toBe("low"); + expect(h.last().lastRequest).toBeNull(); + }); + + test.each(["model_select", "thinking_level_select", "live-model"] as const)( + "does not attribute a pending request to a changed runtime (%s)", + async (change) => { + const h = harness({ flag: true }); + await h.start(); + await h.emit({ type: "context", messages: [] }); + if (change === "thinking_level_select") { + h.state.thinkingLevel = "high"; + await h.emit({ type: "thinking_level_select", level: "high", previousLevel: "medium" }); + } else { + h.state.model = model({ id: "new-model" }); + if (change === "model_select") { + await h.emit({ type: "model_select", model: h.state.model, previousModel: model(), source: "set" }); + } + } + await h.request({ max_output_tokens: 2000 }); + expect(h.last().lastRequest).toBeNull(); + await h.emit({ type: "context", messages: [] }); + await h.request({ max_output_tokens: 1000 }); + expect(h.last().lastRequest?.max_output_tokens).toBe(1000); + }, + ); + + test.each([ + { api: "openai-responses", field: "max_output_tokens", contextWindow: 8192, inputTokens: 8192, ceiling: 16 }, + { api: "anthropic-messages", field: "max_tokens", contextWindow: 16384, inputTokens: 10240, ceiling: 2048 }, + ] as const)("observes the real $api serializer's context clamp without changing the request", async (fixture) => { + const h = harness({ flag: true }); + h.state.model = model({ api: fixture.api, contextWindow: fixture.contextWindow }); + h.state.thinkingLevel = "off"; + await h.start(); + const context: Context = { + messages: [{ role: "user", content: "private fixture prompt", timestamp: 0 }], + estimatedInputTokens: fixture.inputTokens, + }; + await h.emit({ type: "context", messages: context.messages }); + let wireBody: Record | undefined; + const fetch = vi.fn(async (_input, init) => { + wireBody = JSON.parse(init!.body as string); + return new Response(JSON.stringify({ error: { message: "fixture request complete" } }), { + status: 400, + headers: { "content-type": "application/json" }, + }); + }); + const options: SimpleStreamOptions = { + apiKey: "fixture-key", + maxRetries: 0, + fetch, + onPayload: async (payload) => { + expect(await h.request(Object.freeze(payload))).toEqual([undefined]); + }, + }; + const stream = + fixture.api === "openai-responses" + ? responsesStream(h.state.model as Model<"openai-responses">, context, options) + : messagesStream(h.state.model as Model<"anthropic-messages">, context, options); + await stream.result(); + expect(fetch).toHaveBeenCalledTimes(1); + expect(wireBody?.[fixture.field]).toBe(fixture.ceiling); + expect(h.last().model.configuredMaxTokens).toBe(32_000); + expect(h.last().lastRequest?.[fixture.field]).toBe(wireBody?.[fixture.field]); + expect(h.stderr.join("")).not.toMatch(/private fixture prompt|fixture-key/); + }); +}); From 7333c6211b9809f20b72f4e8be2a60fbffda8538 Mon Sep 17 00:00:00 2001 From: youwang <61931019+MelodyVAR@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:26:58 +0800 Subject: [PATCH 5/5] fix: avoid persisting unchanged retry preferences --- packages/coding-agent/src/features/step.ts | 6 ++-- .../coding-agent/test/step-extension.test.ts | 31 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/src/features/step.ts b/packages/coding-agent/src/features/step.ts index 3d670edc..d5e1d420 100644 --- a/packages/coding-agent/src/features/step.ts +++ b/packages/coding-agent/src/features/step.ts @@ -222,9 +222,9 @@ export function createStepExtension(options: StepExtensionOptions = {}): Extensi nativeRetryOverridden = false; } if (autoResume) { - if (!nativeRetryOverridden && ctx.autoRetryEnabled !== true) { - nativeRetryOverridden = true; - } + // Do not persist an unchanged default just because a session started. + if (ctx.autoRetryEnabled === true) return; + nativeRetryOverridden = true; try { ctx.setAutoRetryEnabled(true); } catch { diff --git a/packages/coding-agent/test/step-extension.test.ts b/packages/coding-agent/test/step-extension.test.ts index c7b21491..b2d98339 100644 --- a/packages/coding-agent/test/step-extension.test.ts +++ b/packages/coding-agent/test/step-extension.test.ts @@ -56,6 +56,37 @@ describe("Step extension", () => { } }, ); + test.each(["approve-for-me", "full-access"] as const)( + "does not persist an already enabled native retry preference at startup: %s", + (initialPreset) => { + const on = vi.fn(); + const setAutoRetryEnabled = vi.fn(); + createStepExtension({ + permission: { env: {}, initialPreset }, + stepSettings: () => ({ + getStepSettings: () => ({ fullAccessAcknowledged: true }), + setEffectiveStepSettings: vi.fn(), + }), + })({ + on, + registerProvider: vi.fn(), + registerCommand: vi.fn(), + sendUserMessage: vi.fn(), + } as unknown as ExtensionAPI); + const handler = (name: string) => on.mock.calls.filter(([event]) => event === name).at(-1)![1]; + const ctx = { + hasUI: false, + ui: { notify: vi.fn(), setStatus: vi.fn() }, + isIdle: () => true, + hasPendingMessages: () => false, + autoRetryEnabled: true, + setAutoRetryEnabled, + }; + handler("session_start")({}, ctx); + handler("session_shutdown")({}, ctx); + expect(setAutoRetryEnabled).not.toHaveBeenCalled(); + }, + ); test.each([ { name: "selected non-Bash interpreter", shellPath: process.execPath, prefix: undefined }, { name: "executed command prefix", shellPath: undefined, prefix: "rm -rf ./build" },