diff --git a/packages/coding-agent/docs/step-integration.md b/packages/coding-agent/docs/step-integration.md index 1487eb37..00271dbd 100644 --- a/packages/coding-agent/docs/step-integration.md +++ b/packages/coding-agent/docs/step-integration.md @@ -278,21 +278,29 @@ editor refocus/disposal callback is cancelled before mounting. This prevents an orphaned promise, overlay, or timeout. Ordinary dismissal still allows the refocused editor to open the next dialog. -### Fused verification (`then_run`) - -`edit_file` and `write_file` accept an optional `then_run` shell command. It -runs only after the mutation succeeds (including a no-op), from the initial -working directory like `run_command` without `cwd`, with the same timeout and -output cap. Its output and exit status are appended to the mutation receipt -and recorded in `details.thenRun`. A failing check does not mark the call as -an error, because the file change has already been applied. Empty or -whitespace-only values are ignored. - -The Step extension gates `then_run` as an embedded `run_command` call: the -permission decision is the stricter of the mutation and the command -(including `run_command` overrides and dangerous-command analysis), the -confirmation reason names the command, workflow path ACLs check it, and the -call is blocked when `run_command` is not active in the session. SDK +### Fused commands (`then_run`) + +`edit_file` and `write_file` accept an optional `then_run` shell command. After +an edit succeeds (including a no-op), the command uses the same execution path +as `run_command` without `cwd`: the initial working directory, the host's +configured timeout or the default 120 seconds, and the same output cap and +full-output retention. Use a separate `run_command` when a check needs custom +`cwd`, `timeout_ms`, `max_output_chars`, or background execution. Empty or +whitespace-only `then_run` values are ignored. + +The command output and exit status are appended to the mutation receipt and +recorded in `details.thenRun`. When inline output is shortened, its full-log +reference remains visible and is available in `details.thenRun.fullOutputPath`; +read that log instead of rerunning the command to recover its output. A failing +or timed-out check does not mark the combined call as an error or roll back the +file change. Caller cancellation still propagates with the mutation receipt. + +The Step extension gates `then_run` as an embedded `run_command` call before +performing either action: the permission decision is the stricter of the +mutation and the command (including `run_command` overrides and +dangerous-command analysis), the confirmation reason names the command, +workflow path ACLs check it, and the call is blocked when `run_command` is not +active in the session. A denied combined call does not apply the edit. SDK `acceptEdits` does not auto-approve an edit carrying `then_run`. SDK `PreToolUse` hooks and third-party `tool_call` handlers see it as `input.then_run`; embedders that skip the Step extension must gate it diff --git a/packages/coding-agent/src/step/system-prompt.ts b/packages/coding-agent/src/step/system-prompt.ts index 91a22789..c1b426b5 100644 --- a/packages/coding-agent/src/step/system-prompt.ts +++ b/packages/coding-agent/src/step/system-prompt.ts @@ -366,7 +366,7 @@ export function buildStepSystemPromptAppendix( if ((active.has("edit_file") || active.has("write_file")) && active.has("run_command")) { sections.push( - "- To verify a change immediately, pass the check as then_run on edit_file or write_file (for example a focused test or typecheck) instead of a separate run_command call; it runs only after the change succeeds and needs the same approval as run_command.", + "- For a short check that fits the default command settings, use then_run on edit_file or write_file; it is an embedded run_command with the same execution defaults and approval rules. Use run_command separately for custom cwd, timeout, output limits, or background execution. Inspect the appended command status: a successful file change does not imply a successful check.", ); } diff --git a/packages/coding-agent/src/step/tool-profile.ts b/packages/coding-agent/src/step/tool-profile.ts index 0600b72c..fe04f4be 100644 --- a/packages/coding-agent/src/step/tool-profile.ts +++ b/packages/coding-agent/src/step/tool-profile.ts @@ -88,7 +88,7 @@ const READ_FILE_DESCRIPTION = const WRITE_FILE_DESCRIPTION = "Write full content to a file, creating parent directories if missing. Overwrites existing content — for existing files prefer edit_file."; const THEN_RUN_DESCRIPTION = - "Optional shell command to run after the change succeeds, from the initial working directory like run_command (e.g. a focused test or typecheck); its output and exit status are appended to this result, saving a separate run_command call. Requires the same approval as run_command."; + "Optional run_command command after a successful file change, including a no-op. Uses the initial working directory and run_command defaults for timeout and output. Approval covers both actions before editing; a denied call makes no change. If the command fails or times out after editing, the file change remains applied: inspect the appended command status. Use a separate run_command for custom cwd, timeout, output limits, or background execution."; const listDirectorySchema = Type.Object({ path: Type.Optional(Type.String({ description: `Directory path. ${PATH_DESCRIPTION} Defaults to '.'` })), @@ -1079,48 +1079,68 @@ function mapRunCommandArgs(args: RunCommandInput, ctx?: ExtensionContext): { com }; } +/** Preserve command recovery metadata when the Step output cap also shortens an error. */ +class StepCommandError extends ShellToolError { + readonly stepTruncated: boolean; + + constructor(error: ShellToolError, maxChars: number) { + const limited = limitRunCommandText(error.message, maxChars, error.fullOutputPath); + super(limited.text, error.fullOutputPath); + this.cause = error; + this.stepTruncated = limited.truncated; + } +} + export interface StepThenRunDetails { command: string; exitCode: number | null; timedOut: boolean; + /** Whether run_command's Step character cap shortened its inline output. */ stepTruncated: boolean; + /** Complete command output, retained by run_command when inline output is shortened. */ + fullOutputPath?: string; } /** - * Action fusion: run the `then_run` verification command after a successful - * file mutation and fold its outcome into the mutation result. The native - * bash tool throws on non-zero exits; a failing check must not hide the - * already-applied mutation, so those throws become part of the fused output - * and the result is not marked as an error. Caller cancellation still - * propagates, carrying the mutation receipt. + * Run the embedded run_command after a successful file mutation. A failing + * command must not hide or repeat the already-applied mutation. Caller + * cancellation still propagates with the mutation receipt. */ async function withThenRunResult( result: AnyResult, - nativeBash: AnyToolDefinition, + runCommand: ToolDefinition, command: string, signal: AbortSignal | undefined, ctx: ExtensionContext | undefined, ): Promise { - const configuredTimeout = getContextValue(ctx, "commandTimeoutMs"); const receipt = textFromResult(result); let output: string; let status: string | undefined; let exitCode: number | null = 0; let timedOut = false; + let stepTruncated = false; + let fullOutputPath: string | undefined; try { - // No onUpdate: bash partials would reach the edit/write renderers. - const bashResult = await nativeBash.execute( + // No onUpdate: command partials would reach the edit/write renderers. + const commandResult = await runCommand.execute( "step-then-run", - { command, timeout: typeof configuredTimeout === "number" ? configuredTimeout / 1000 : undefined }, + { command }, signal, undefined, ctx as ExtensionContext, ); - output = textFromResult(bashResult); + output = textFromResult(commandResult); + const details = nativeDetails(commandResult); + stepTruncated = details.stepTruncated === true; + fullOutputPath = typeof details.fullOutputPath === "string" ? details.fullOutputPath : undefined; status = "Command exited with code 0"; } catch (error) { const message = error instanceof Error ? error.message : String(error); - if (isCallerAborted(signal)) throw new Error(`${receipt}\n\nthen_run: $ ${command}\n${message}`); + if (isCallerAborted(signal)) { + throw new Error(`${receipt}\n\nthen_run: $ ${command}\n${message}`, { cause: error }); + } + stepTruncated = error instanceof StepCommandError && error.stepTruncated; + fullOutputPath = error instanceof ShellToolError ? error.fullOutputPath : undefined; const match = /(?:^|\n\n)(Command exited with code (\d+)|Command timed out after .+ seconds)$/u.exec(message); if (match) { output = message.slice(0, match.index); @@ -1132,14 +1152,21 @@ async function withThenRunResult( exitCode = null; } } - const limited = withStepTextLimit("run_command", output || "(no output)", resolveStepMaxChars(undefined, ctx)); - const thenRun: StepThenRunDetails = { command, exitCode, timedOut, stepTruncated: limited.truncated }; + // run_command already bounded the output and preserved its log/status footer. + // Limiting it again here could discard the only reference to the full output. + const thenRun: StepThenRunDetails = { + command, + exitCode, + timedOut, + stepTruncated, + ...(fullOutputPath ? { fullOutputPath } : {}), + }; return { ...result, content: [ { type: "text", - text: `${receipt}\n\n${thenRunMarker(command)}${limited.text}${status ? `\n\n${status}` : ""}`, + text: `${receipt}\n\n${thenRunMarker(command)}${output || "(no output)"}${status ? `\n\n${status}` : ""}`, }, ], details: { ...nativeDetails(result), thenRun }, @@ -1583,7 +1610,7 @@ export function createStepToolProfile(cwd: string, options: StepToolProfileOptio ) => { const result = await executeStepWriteFile(args, cwd, options.write, signal, ctx); const thenRun = readThenRunCommand(args); - return thenRun === undefined ? result : withThenRunResult(result, nativeBash, thenRun, signal, ctx); + return thenRun === undefined ? result : withThenRunResult(result, runCommand, thenRun, signal, ctx); }, renderResult: withThenRunRenderResult(writeFileBase.renderResult as Renderer["renderResult"]), } as AnyToolDefinition; @@ -1604,7 +1631,7 @@ export function createStepToolProfile(cwd: string, options: StepToolProfileOptio execute: async (toolCallId, args, signal, onUpdate, ctx) => { const result = await executeStepEdit(nativeEdit, toolCallId, args, cwd, options.edit, signal, onUpdate, ctx); const thenRun = readThenRunCommand(args); - return thenRun === undefined ? result : withThenRunResult(result, nativeBash, thenRun, signal, ctx); + return thenRun === undefined ? result : withThenRunResult(result, runCommand, thenRun, signal, ctx); }, renderCall: nativeEdit.renderCall ? (args, theme, context) => @@ -1653,8 +1680,7 @@ export function createStepToolProfile(cwd: string, options: StepToolProfileOptio return applyStepTextLimit(result, "run_command", maxChars); } catch (error) { if (error instanceof ShellToolError) { - const limited = limitRunCommandText(error.message, maxChars, error.fullOutputPath); - if (limited.truncated) throw new Error(limited.text, { cause: error }); + throw new StepCommandError(error, maxChars); } throw error; } diff --git a/packages/coding-agent/test/step-then-run-command-parity.test.ts b/packages/coding-agent/test/step-then-run-command-parity.test.ts new file mode 100644 index 00000000..471fabb3 --- /dev/null +++ b/packages/coding-agent/test/step-then-run-command-parity.test.ts @@ -0,0 +1,239 @@ +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { ExtensionContext } from "../src/core/extensions/types.ts"; +import type { BashOperations } from "../src/core/tools/bash.ts"; +import { createStepToolProfile, type StepThenRunDetails } from "../src/step/tool-profile.ts"; + +type Tool = ReturnType[number]; +type Result = Awaited>; + +function text(result: Result): string { + return result.content + .filter((part) => part.type === "text") + .map((part) => part.text) + .join("\n"); +} + +function commandDetails(result: Result): StepThenRunDetails { + return (result.details as { thenRun: StepThenRunDetails }).thenRun; +} + +describe("then_run uses run_command execution", () => { + let directory: string; + const logs = new Set(); + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "step-then-run-parity-")); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await rm(directory, { recursive: true, force: true }); + await Promise.all([...logs].map((path) => rm(path, { force: true }))); + logs.clear(); + }); + + function profile(operations?: BashOperations) { + const tools = createStepToolProfile(directory, { + agentDir: join(directory, "agent"), + ...(operations ? { bash: { operations } } : {}), + }); + return { + run: tools.find((tool) => tool.name === "run_command")!, + write: tools.find((tool) => tool.name === "write_file")!, + edit: tools.find((tool) => tool.name === "edit_file")!, + }; + } + + it.each([ + { label: "default", milliseconds: undefined, seconds: 120 }, + { label: "host timeout", milliseconds: 180_500, seconds: 180.5 }, + ])("shares the $label with both write and edit", async ({ milliseconds, seconds }) => { + const exec = vi.fn(async (_command, _cwd, { onData }) => { + onData(Buffer.from("checked\n")); + return { exitCode: 0 }; + }); + const { run, write, edit } = profile({ exec }); + const runSpy = vi.spyOn(run, "execute"); + const context = + milliseconds === undefined + ? (undefined as never) + : ({ cwd: directory, commandTimeoutMs: milliseconds } as unknown as ExtensionContext); + await run.execute("standalone", { command: "check" }, undefined, undefined, context); + await write.execute( + "write", + { path: "sample.txt", content: "before\n", then_run: "check" }, + undefined, + undefined, + context, + ); + await edit.execute( + "edit", + { path: "sample.txt", search: "before", replace: "after", then_run: "check" }, + undefined, + undefined, + context, + ); + + expect(exec.mock.calls.map((call) => call[2].timeout)).toEqual([seconds, seconds, seconds]); + expect(runSpy).toHaveBeenCalledTimes(3); + expect(runSpy.mock.calls.slice(1).map((call) => call[1])).toEqual([{ command: "check" }, { command: "check" }]); + expect(await readFile(join(directory, "sample.txt"), "utf8")).toBe("after\n"); + }); + + it("keeps commands in the initial directory when editing a nested file", async () => { + const exec = vi.fn(async () => ({ exitCode: 0 })); + const { run, write } = profile({ exec }); + await mkdir(join(directory, "nested")); + await mkdir(join(directory, "host")); + const context = { cwd: join(directory, "host"), commandTimeoutMs: 45_000 } as unknown as ExtensionContext; + await run.execute("standalone", { command: "check" }, undefined, undefined, context); + await write.execute( + "write", + { path: join(directory, "nested", "sample.txt"), content: "ok\n", then_run: "check" }, + undefined, + undefined, + context, + ); + expect(exec.mock.calls.map((call) => call[1])).toEqual([directory, directory]); + expect(exec.mock.calls.map((call) => call[2].timeout)).toEqual([45, 45]); + }); + + it.each([ + { chars: 35_000, exitCode: 0 }, + { chars: 35_000, exitCode: 7 }, + { chars: 70_000, exitCode: 0 }, + { chars: 70_000, exitCode: 7 }, + ])("retains the complete $chars-character output after exit $exitCode", async ({ chars, exitCode }) => { + const { write } = profile(); + const command = `printf '%0${chars}d' 0; exit ${exitCode}`; + const result = await write.execute( + "write", + { path: "sample.txt", content: "applied\n", then_run: command }, + undefined, + undefined, + undefined as never, + ); + const output = text(result); + const referencedPath = /\[Full output: ([^\]\n]+)\]/u.exec(output)?.[1]; + if (referencedPath) logs.add(referencedPath); + const details = commandDetails(result); + if (details.fullOutputPath) logs.add(details.fullOutputPath); + + expect(details).toMatchObject({ exitCode, timedOut: false, stepTruncated: true }); + expect(details.fullOutputPath).toBeTruthy(); + expect(referencedPath).toBe(details.fullOutputPath); + expect(await readFile(details.fullOutputPath!, "utf8")).toBe("0".repeat(chars)); + expect(output).toContain(`Command exited with code ${exitCode}`); + expect(output).not.toContain("call run_command again"); + expect(await readFile(join(directory, "sample.txt"), "utf8")).toBe("applied\n"); + expect(result).not.toHaveProperty("isError", true); + }); + + it("applies the host output cap without dropping the log or rerunning the command", async () => { + const { write } = profile(); + const context = { cwd: directory, commandOutputLimit: 100 } as unknown as ExtensionContext; + const command = "printf x >> executed.txt; printf '%035000d' 0"; + const result = await write.execute( + "write", + { path: "sample.txt", content: "applied\n", then_run: command }, + undefined, + undefined, + context, + ); + const details = commandDetails(result); + if (details.fullOutputPath) logs.add(details.fullOutputPath); + const referencedPath = /\[Full output: ([^\]\n]+)\]/u.exec(text(result))?.[1]; + if (referencedPath) logs.add(referencedPath); + + expect(details.fullOutputPath).toBeTruthy(); + expect(await readFile(details.fullOutputPath!, "utf8")).toBe("0".repeat(35_000)); + expect(text(result)).toContain(details.fullOutputPath); + expect(text(result).length).toBeLessThan(600); + expect(await readFile(join(directory, "executed.txt"), "utf8")).toBe("x"); + }); + + it("preserves the edit and command status when a check fails", async () => { + const { edit } = profile(); + await writeFile(join(directory, "sample.txt"), "before\n"); + const result = await edit.execute( + "edit", + { path: "sample.txt", search: "before", replace: "after", then_run: "printf failed; exit 3" }, + undefined, + undefined, + undefined as never, + ); + expect(await readFile(join(directory, "sample.txt"), "utf8")).toBe("after\n"); + expect(commandDetails(result)).toEqual({ + command: "printf failed; exit 3", + exitCode: 3, + timedOut: false, + stepTruncated: false, + }); + expect(text(result)).toMatch(/Command exited with code 3$/u); + expect(result).not.toHaveProperty("isError", true); + }); + + it("keeps timeout failure distinct from caller cancellation", async () => { + const exec = vi.fn(async () => { + throw new Error("timeout:1"); + }); + const { write } = profile({ exec }); + const context = { cwd: directory, commandTimeoutMs: 1_000 } as unknown as ExtensionContext; + const result = await write.execute( + "write", + { path: "sample.txt", content: "applied\n", then_run: "check" }, + undefined, + undefined, + context, + ); + expect(commandDetails(result)).toMatchObject({ exitCode: null, timedOut: true }); + expect(text(result)).toContain("Command timed out after 1 seconds"); + expect(await readFile(join(directory, "sample.txt"), "utf8")).toBe("applied\n"); + }); + + it("propagates caller cancellation with the mutation receipt and performs no retry", async () => { + const controller = new AbortController(); + const exec = vi.fn(async (_command, _cwd, { onData }) => { + onData(Buffer.from("partial output\n")); + controller.abort(); + throw new Error("aborted"); + }); + const { write } = profile({ exec }); + await expect( + write.execute( + "write", + { path: "sample.txt", content: "applied\n", then_run: "check" }, + controller.signal, + undefined, + undefined as never, + ), + ).rejects.toThrow(/Wrote 8 chars[\s\S]*then_run: \$ check[\s\S]*Command aborted/u); + expect(exec).toHaveBeenCalledTimes(1); + expect(await readFile(join(directory, "sample.txt"), "utf8")).toBe("applied\n"); + }); + + it("skips empty checks, but executes a check after a no-op edit", async () => { + const exec = vi.fn(async () => ({ exitCode: 0 })); + const { run, write } = profile({ exec }); + const runSpy = vi.spyOn(run, "execute"); + await write.execute( + "first", + { path: "sample.txt", content: "same\n", then_run: " " }, + undefined, + undefined, + undefined as never, + ); + await write.execute( + "noop", + { path: "sample.txt", content: "same\n", then_run: "check" }, + undefined, + undefined, + undefined as never, + ); + expect(exec).toHaveBeenCalledTimes(1); + expect(runSpy).toHaveBeenCalledTimes(1); + }); +});