From 2540db27ffc591453d4eee30607a054c9317ad59 Mon Sep 17 00:00:00 2001 From: youwang Date: Thu, 24 Sep 2026 00:11:01 +0800 Subject: [PATCH] fix(coding-agent): add opt-in non-interactive denial recovery Without a UI, an unapprovable confirmation (hazardous command, incomplete shell analysis, refused or unconfigured policy) returned block+terminate from StepPermissionController.handleToolCall, ending the whole run. In unattended benchmark runs this turned every mandatory-confirmation command (recursive force remove, destructive SQL, unparseable shell) into a fatal error for the trial. Add nonInteractiveDenial: "terminate" | "continue" (CLI --non-interactive-denial, env STEP_NON_INTERACTIVE_DENIAL; default terminate keeps current behavior). Under continue the call stays blocked and is reported as a failed tool result with recovery guidance, so the agent can take a safer route. Explicit deny decisions (read-only mode, per-tool deny overrides) still terminate; nothing new is approved. --- apps/cli/src/main.ts | 1 + docs/command-permissions.md | 6 ++ docs/step-configuration.md | 6 ++ packages/coding-agent/src/cli/args.ts | 30 ++++++- packages/coding-agent/src/step/permissions.ts | 39 +++++++++ packages/coding-agent/test/args.test.ts | 17 ++++ .../test/step-permissions.test.ts | 85 +++++++++++++++++++ .../test/suite/step-command-approval.test.ts | 22 ++++- 8 files changed, 203 insertions(+), 3 deletions(-) diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 4b4f2b15..19223261 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -187,6 +187,7 @@ const stepMainOptions: MainOptions = { permission: { approvalMode: stepPermissionArgs.approvalMode, nonInteractiveApproval: stepPermissionArgs.nonInteractiveApproval, + nonInteractiveDenial: stepPermissionArgs.nonInteractiveDenial, toolOverrides: stepPermissionArgs.toolOverride ?? stepPermissionArgs.toolOverrides, }, traceHeaderPolicy: observability.traceHeaderPolicy(), diff --git a/docs/command-permissions.md b/docs/command-permissions.md index 74badc59..b073b2fb 100644 --- a/docs/command-permissions.md +++ b/docs/command-permissions.md @@ -14,6 +14,12 @@ Unresolved analysis is not reported as a detected dangerous command. Neither outcome can use an automatic tool override or unattended `allow`. Explicit user approval applies only to that call. +Without an approval channel, a deny ends the run by default. The opt-in +`nonInteractiveDenial: "continue"` (CLI `--non-interactive-denial continue`, +env `STEP_NON_INTERACTIVE_DENIAL=continue`) keeps the call blocked but returns +the block as a failed tool result so the agent can continue; explicit per-tool +denial and read-only mode still terminate. + The product policy in `packages/coding-agent/src/step/permissions.ts` runs through the existing `tool_call` hook, before foreground/background execution. Clients render the existing confirmation request; they do not implement another policy. diff --git a/docs/step-configuration.md b/docs/step-configuration.md index 69a75765..637a7140 100644 --- a/docs/step-configuration.md +++ b/docs/step-configuration.md @@ -28,6 +28,12 @@ and `/tmp/cache`. Bypass, auto, and autopilot still ask for each call. Read-only mode blocks it, and runs without an approval channel cannot execute it even with `nonInteractiveApproval = "allow"`. +By default, a run without an approval channel terminates after such a block. +`--non-interactive-denial continue` (or `STEP_NON_INTERACTIVE_DENIAL=continue`) +keeps the call blocked but reports it as a failed tool result, so an unattended +agent can take a safer route instead of ending the run. Explicit denials — +read-only mode and `deny` tool overrides — still terminate. + See [command permissions](command-permissions.md) for matching behavior and how to extend the built-in rules. diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index 4f38275c..ded6b85e 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -8,7 +8,12 @@ import { APP_NAME, CONFIG_DIR_NAME, ENV_AGENT_DIR, ENV_SESSION_DIR, IS_STEP_ENTR import type { ExtensionFlag } from "../core/extensions/types.ts"; import type { TuiMode } from "../core/settings-manager.ts"; import { getStepDefaultProvider } from "../step/defaults.ts"; -import type { StepNonInteractiveApproval, StepPermissionMode, StepToolPermissionMode } from "../step/permissions.ts"; +import type { + StepNonInteractiveApproval, + StepNonInteractiveDenial, + StepPermissionMode, + StepToolPermissionMode, +} from "../step/permissions.ts"; export type Mode = "text" | "json" | "rpc"; @@ -61,6 +66,8 @@ export interface Args { approvalMode?: StepPermissionMode; /** Fallback for approval requests when no interactive UI is available. */ nonInteractiveApproval?: StepNonInteractiveApproval; + /** Outcome of a blocked call when no interactive UI is available. */ + nonInteractiveDenial?: StepNonInteractiveDenial; /** Repeated per-tool approval overrides (canonical runtime option name). */ toolOverride?: Record; /** Backward-compatible plural alias for callers that used the TUI vocabulary. */ @@ -236,6 +243,24 @@ export function parseArgs(args: string[]): Args { }); } } + } else if (arg === "--non-interactive-denial" || arg.startsWith("--non-interactive-denial=")) { + const value = arg === "--non-interactive-denial" ? args[i + 1] : arg.slice("--non-interactive-denial=".length); + if (arg === "--non-interactive-denial" && (value === undefined || value.startsWith("-"))) { + result.diagnostics.push({ + type: "error", + message: "--non-interactive-denial requires terminate or continue", + }); + } else { + if (arg === "--non-interactive-denial") i++; + if (value === "terminate" || value === "continue") { + result.nonInteractiveDenial = value; + } else { + result.diagnostics.push({ + type: "error", + message: `Invalid non-interactive denial mode "${value}". Valid values: terminate, continue`, + }); + } + } } else if (arg === "--tool-override" || arg.startsWith("--tool-override=")) { const value = arg === "--tool-override" ? args[i + 1] : arg.slice("--tool-override=".length); if (arg === "--tool-override" && (value === undefined || value.startsWith("-"))) { @@ -518,11 +543,12 @@ export function printHelp(extensionFlags?: ExtensionFlag[]): void { " STEPCODE_DISABLE_PI_SERVICES Disable upstream update/catalog services (enabled by step)", " STEP_APPROVAL_MODE Default tool approval mode (confirm|auto|strict)", " STEP_NON_INTERACTIVE_APPROVAL Fallback when no approval UI is available (allow|deny)", + " STEP_NON_INTERACTIVE_DENIAL Blocked call without a UI (terminate|continue)", " STEP_AUTOPILOT Enable bounded model-error auto-resume", ].join("\n") : ""; const stepPermissionOptionsText = IS_STEP_ENTRYPOINT - ? "\n --approval-mode Tool approval mode: confirm, auto, or strict\n --non-interactive-approval Fallback without a UI: allow or deny\n --tool-override Per-tool override (repeatable; mode: allow, confirm, deny)" + ? "\n --approval-mode Tool approval mode: confirm, auto, or strict\n --non-interactive-approval Fallback without a UI: allow or deny\n --non-interactive-denial Blocked call without a UI: terminate the run or continue\n --tool-override Per-tool override (repeatable; mode: allow, confirm, deny)" : ""; const stepAuthCommandsText = IS_STEP_ENTRYPOINT ? `\n ${APP_NAME} login Sign in with the Step account (OAuth)\n ${APP_NAME} logout Remove the stored Step credential` diff --git a/packages/coding-agent/src/step/permissions.ts b/packages/coding-agent/src/step/permissions.ts index 31b945c9..f22ca34f 100644 --- a/packages/coding-agent/src/step/permissions.ts +++ b/packages/coding-agent/src/step/permissions.ts @@ -23,6 +23,7 @@ export { containsDangerousLifecycleCommand, isDangerousCommand } from "./command export type StepPermissionPresetId = "ask" | "read-only" | "bypass" | "autopilot"; export type StepPermissionMode = "confirm" | "strict" | "auto"; export type StepNonInteractiveApproval = "allow" | "deny"; +export type StepNonInteractiveDenial = "terminate" | "continue"; export type StepToolPermissionMode = "allow" | "confirm" | "deny"; export interface StepPermissionPreset { @@ -133,6 +134,13 @@ export interface StepPermissionControllerOptions { approvalMode?: StepPermissionMode; /** Fallback for confirmation requests when no interactive UI exists. */ nonInteractiveApproval?: StepNonInteractiveApproval; + /** + * What an unapprovable confirmation does to an unattended run: `terminate` + * (default) ends the run after the blocked batch, `continue` reports the + * block as a failed tool result and lets the agent keep working. The command + * itself is never executed either way. + */ + nonInteractiveDenial?: StepNonInteractiveDenial; /** Enables the bounded continuation ladder when the mode permits it. */ autoResume?: boolean; /** Per-tool overrides (CLI `--tool-override` is merged here). */ @@ -430,11 +438,13 @@ export class StepPermissionController { private state: StepPermissionState; private toolOverrides: Record; private readonly shellContext: () => ShellExecutionContext; + private readonly nonInteractiveDenial: StepNonInteractiveDenial; constructor(options: StepPermissionControllerOptions = {}) { this.shellContext = options.shellContext ?? (() => ({})); this.state = resolveInitialStepPermissionState(options); this.toolOverrides = cloneToolOverrides(options.toolOverrides ?? {}); + this.nonInteractiveDenial = resolveNonInteractiveDenial(options); if (Object.keys(this.toolOverrides).length > 0) this.state.toolOverrides = { ...this.toolOverrides }; } @@ -538,6 +548,12 @@ export class StepPermissionController { // fail closed. The default remains deny. if (!decision.hazardous && !decision.analysisIncomplete && state.nonInteractiveApproval === "allow") return undefined; + // Opt-in recovery: the call stays blocked, but the block is reported as + // a failed tool result instead of ending the run, so the agent can take + // a safer route. Explicit `deny` decisions above keep terminating. + if (this.nonInteractiveDenial === "continue") { + return { block: true, reason: formatDenialRecoveryReason(decision) }; + } return { block: true, terminate: true, @@ -595,6 +611,29 @@ function formatUnattendedBlockReason(decision: StepToolDecision, cause: Unattend return `${decision.reason} (no interactive approval is available).`; } +/** + * Explain a non-terminating unattended block to the model. + * + * The audience differs from formatUnattendedBlockReason: recovery mode feeds + * this text back as a failed tool result, so it needs "what to do instead" + * guidance rather than CLI flags the model cannot change. + */ +function formatDenialRecoveryReason(decision: StepToolDecision): string { + return ( + `${decision.reason} No interactive approval is available in this run, so the call was not executed. ` + + "Do not retry it verbatim; use a safer equivalent or continue the task without it." + ); +} + +/** Resolve the unattended denial outcome from options, then the environment. */ +function resolveNonInteractiveDenial(options: StepPermissionControllerOptions): StepNonInteractiveDenial { + if (options.nonInteractiveDenial === "terminate" || options.nonInteractiveDenial === "continue") { + return options.nonInteractiveDenial; + } + const env = options.env ?? process.env; + return env.STEP_NON_INTERACTIVE_DENIAL?.trim().toLowerCase() === "continue" ? "continue" : "terminate"; +} + function cloneToolOverrides(overrides: Record): Record { const result: Record = {}; for (const [name, mode] of Object.entries(overrides)) { diff --git a/packages/coding-agent/test/args.test.ts b/packages/coding-agent/test/args.test.ts index 9e74d826..6e966ebc 100644 --- a/packages/coding-agent/test/args.test.ts +++ b/packages/coding-agent/test/args.test.ts @@ -376,6 +376,23 @@ describe("parseArgs", () => { }); }); + test("parses the non-interactive denial outcome", () => { + expect(parseArgs(["--non-interactive-denial", "continue"])).toMatchObject({ + nonInteractiveDenial: "continue", + }); + expect(parseArgs(["--non-interactive-denial=terminate"])).toMatchObject({ + nonInteractiveDenial: "terminate", + }); + expect(parseArgs(["--non-interactive-denial", "abort"]).diagnostics).toContainEqual({ + type: "error", + message: 'Invalid non-interactive denial mode "abort". Valid values: terminate, continue', + }); + expect(parseArgs(["--non-interactive-denial"]).diagnostics).toContainEqual({ + type: "error", + message: "--non-interactive-denial requires terminate or continue", + }); + }); + test("supports equals syntax and repeated per-tool overrides", () => { expect( parseArgs([ diff --git a/packages/coding-agent/test/step-permissions.test.ts b/packages/coding-agent/test/step-permissions.test.ts index 53943a97..d70d70c6 100644 --- a/packages/coding-agent/test/step-permissions.test.ts +++ b/packages/coding-agent/test/step-permissions.test.ts @@ -196,6 +196,91 @@ describe("Step permission presets", () => { expect(dangerous).toMatchObject({ block: true, terminate: true }); }); + describe("non-interactive denial recovery", () => { + const noUI = { hasUI: false } as never; + const hazardousCall = { + toolName: "run_command", + input: { command: "rm -rf ./build" }, + } as never; + + it("blocks a hazardous command without terminating when denial is continue", async () => { + const controller = new StepPermissionController({ + approvalMode: "auto", + nonInteractiveApproval: "allow", + nonInteractiveDenial: "continue", + env: {}, + }); + const result = await controller.handleToolCall(hazardousCall, noUI); + expect(result).toMatchObject({ block: true }); + expect((result as { terminate?: boolean }).terminate).toBeUndefined(); + expect((result as { reason: string }).reason).toContain("was not executed"); + }); + + it("resolves continue from the environment", async () => { + const controller = new StepPermissionController({ + approvalMode: "auto", + nonInteractiveApproval: "allow", + env: { STEP_NON_INTERACTIVE_DENIAL: "continue" }, + }); + const result = await controller.handleToolCall(hazardousCall, noUI); + expect(result).toMatchObject({ block: true }); + expect((result as { terminate?: boolean }).terminate).toBeUndefined(); + }); + + it("still terminates by default and on unrecognized values", async () => { + for (const env of [{}, { STEP_NON_INTERACTIVE_DENIAL: "recover" }]) { + const controller = new StepPermissionController({ + approvalMode: "auto", + nonInteractiveApproval: "allow", + env, + }); + expect(await controller.handleToolCall(hazardousCall, noUI)).toMatchObject({ + block: true, + terminate: true, + }); + } + }); + + it("does not approve anything: incomplete analysis stays blocked", async () => { + const controller = new StepPermissionController({ + approvalMode: "auto", + nonInteractiveApproval: "allow", + nonInteractiveDenial: "continue", + env: {}, + }); + const result = await controller.handleToolCall( + { toolName: "run_command", input: { command: "eval $unresolved" } } as never, + noUI, + ); + expect(result).toMatchObject({ block: true }); + expect((result as { terminate?: boolean }).terminate).toBeUndefined(); + }); + + it("keeps explicit denials terminating even with continue", async () => { + const readOnly = new StepPermissionController({ + initialPreset: "read-only", + nonInteractiveDenial: "continue", + env: {}, + }); + expect( + await readOnly.handleToolCall( + { toolName: "write_file", input: { path: "x", content: "y" } } as never, + noUI, + ), + ).toMatchObject({ block: true, terminate: true }); + const overridden = new StepPermissionController({ + initialPreset: "bypass", + nonInteractiveDenial: "continue", + toolOverrides: { run_command: "deny" }, + env: {}, + }); + expect(await overridden.handleToolCall(hazardousCall, noUI)).toMatchObject({ + block: true, + terminate: true, + }); + }); + }); + // Feedback issue-287bfff1a5fe7668: with the approval config removed entirely, // a non-interactive run inherited the interactive Bypass default and deleted // files with nobody watching. diff --git a/packages/coding-agent/test/suite/step-command-approval.test.ts b/packages/coding-agent/test/suite/step-command-approval.test.ts index efe308b8..cc63659e 100644 --- a/packages/coding-agent/test/suite/step-command-approval.test.ts +++ b/packages/coding-agent/test/suite/step-command-approval.test.ts @@ -6,7 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createStepExtension } from "../../src/features/step.ts"; import type { StepPermissionControllerOptions } from "../../src/step/permissions.ts"; import { createStepToolProfile } from "../../src/step/tool-profile.ts"; -import { createHarness, type Harness } from "./harness.ts"; +import { createHarness, getAssistantTexts, type Harness } from "./harness.ts"; const modes: Array<{ name: string; permission: StepPermissionControllerOptions }> = [ { name: "ask", permission: { initialPreset: "ask" } }, @@ -113,6 +113,26 @@ describe("Step command approval through the agent loop", () => { }); }); + it("continues past a refused unattended deletion when denial recovery is enabled", async () => { + const session = await setup({ + approvalMode: "auto", + nonInteractiveApproval: "allow", + nonInteractiveDenial: "continue", + toolOverrides: { run_command: "allow" }, + }); + await session.session.bindExtensions({ mode: "print" }); + const marker = prepareRemoval(session, "recovered-removal"); + await session.session.prompt("Remove the test directory"); + expect(existsSync(marker)).toBe(true); + expect(session.session.messages.find((message) => message.role === "toolResult")).toMatchObject({ + isError: true, + }); + // The run keeps going: the follow-up assistant turn is consumed instead + // of the batch terminating after the blocked call. + expect(session.getPendingResponseCount()).toBe(0); + expect(getAssistantTexts(session)).toContain("done"); + }); + it.each([ { name: "read-only", permission: { initialPreset: "read-only" } }, { name: "explicit deny", permission: { initialPreset: "bypass", toolOverrides: { run_command: "deny" } } },