From 4686fefa0c875ad8f5faca175e3eb2032c7170be Mon Sep 17 00:00:00 2001 From: MelodyVAR <61931019+MelodyVAR@users.noreply.github.com> Date: Sat, 26 Sep 2026 11:29:39 +0800 Subject: [PATCH 1/4] fix(coding-agent): preserve compaction context and check committed completion --- apps/cli/src/main.ts | 12 +- docs/compaction-integrity.md | 75 ++++ docs/completion-check.md | 77 ++++ .../src/harness/compaction/compaction.ts | 15 +- .../test/harness/compaction-integrity.test.ts | 296 ++++++++++++ packages/coding-agent/src/cli/args.ts | 44 ++ .../src/core/compaction/compaction.ts | 21 +- .../src/modes/completion-check.ts | 146 ++++++ packages/coding-agent/src/modes/print-mode.ts | 104 ++++- .../test/completion-check-args.test.ts | 98 ++++ .../test/completion-check-cli.test.ts | 148 ++++++ .../test/completion-check-git-command.test.ts | 108 +++++ .../test/completion-check.test.ts | 193 ++++++++ .../fixtures/completion-check-provider.ts | 17 + .../test/suite/completion-check.test.ts | 422 ++++++++++++++++++ .../regressions/compaction-integrity.test.ts | 324 ++++++++++++++ 16 files changed, 2093 insertions(+), 7 deletions(-) create mode 100644 docs/compaction-integrity.md create mode 100644 docs/completion-check.md create mode 100644 packages/agent-core/test/harness/compaction-integrity.test.ts create mode 100644 packages/coding-agent/src/modes/completion-check.ts create mode 100644 packages/coding-agent/test/completion-check-args.test.ts create mode 100644 packages/coding-agent/test/completion-check-cli.test.ts create mode 100644 packages/coding-agent/test/completion-check-git-command.test.ts create mode 100644 packages/coding-agent/test/completion-check.test.ts create mode 100644 packages/coding-agent/test/fixtures/completion-check-provider.ts create mode 100644 packages/coding-agent/test/suite/completion-check.test.ts create mode 100644 packages/coding-agent/test/suite/regressions/compaction-integrity.test.ts diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 4b4f2b15..55b43bc2 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -71,7 +71,7 @@ import { updateGlobalMcpConfig, withStepDefaults, } from "@step-harness/coding-agent"; -import { parseArgs, toPrintOutputMode } from "#args/index"; +import { parseArgs, resolveAppMode, toPrintOutputMode } from "#args/index"; import { loadStepStartupConfig } from "#bootstrap/config"; import { createStepExtensionFactories } from "#bootstrap/extensions"; import { captureRawStdout, sdkStdioRequested } from "#bootstrap/stdout-capture"; @@ -554,6 +554,14 @@ try { syncStepLoginProfileEndpoint(getStepAuthPath()); let shouldLaunchMain = true; const parsedInteractiveArgs = parseArgs(compatibility?.args ?? stepCodeArgs); + if ( + parsedInteractiveArgs.completionCheck && + !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 interactiveStartup = isStepInteractiveLoginStartup({ stdinIsTTY: process.stdin.isTTY, stdoutIsTTY: process.stdout.isTTY, @@ -721,6 +729,8 @@ async function dispatchStepAppMode(prep: Extract 0` before returning success. Empty +content arrays, empty text, whitespace-only text blocks, and thinking-only +responses fail this check. Thinking mixed with whitespace also fails. Accepted +text retains its original whitespace and formatting. + +Validation happens before combining history and prefix text or appending file +operation metadata. In particular, none of the following can make a missing +generated summary valid: + +- A preserved or newly generated history summary beside an empty turn prefix. +- `No prior history.`, the split-turn heading, or its separators. +- `` and `` metadata. + +Failure of either required generation fails the whole compaction. An empty +history response stops before requesting a turn-prefix summary. An empty prefix +response discards the newly generated history result as a candidate checkpoint. +No partial compaction result is returned. + +The coding-agent helpers throw `Summarization failed: empty summary` or +`Turn prefix summarization failed: empty summary`. Agent-core returns a +`CompactionError` with code `summarization_failed` and the same message. Existing +session callers therefore keep their checkpoint, retained messages, and active +context when generation fails. Both manual and automatic built-in compaction +use these helpers. + +Length-stop and provider-error diagnostics take precedence over the empty-text +check. Cancellation also remains a cancellation: coding-agent throws an +`AbortError` for an aborted summary response, and agent-core returns error code +`aborted`. Existing bounded retries for transient provider errors are unchanged; +an otherwise successful response with empty text fails without an added retry. + +This check prevents missing summaries from being persisted. It does not judge +the factual quality of nonempty model text or validate extension-supplied +compaction results. + +## Offline regression tests + +The tests import the actual compaction and context modules and use the faux +provider. Coding-agent also exercises the real `AgentSession` and in-memory +`SessionManager`, checking that manual and automatic failures do not append a +checkpoint or change the active messages. Automatic cases also cover histories +without file metadata, where an empty generation previously produced either an +empty handoff or only the fixed split-turn boilerplate. No model service is used. + +```sh +# From packages/coding-agent +pnpm exec vitest --run test/suite/regressions/compaction-integrity.test.ts + +# From packages/agent-core +pnpm exec vitest --run test/harness/compaction-integrity.test.ts +``` diff --git a/docs/completion-check.md b/docs/completion-check.md new file mode 100644 index 00000000..1e738006 --- /dev/null +++ b/docs/completion-check.md @@ -0,0 +1,77 @@ +# Print-mode completion check + +The Step CLI can perform a bounded completion check in the existing session: + +```sh +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." +``` + +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. + +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. +- The committed tree differs from the starting HEAD's tree. An empty commit or + a change fully reverted before completion is insufficient. This tests delivery + of a change, not its correctness; the canonical verifier still owns correctness. +- The index and tracked worktree are clean, including submodule changes. +- No unignored untracked files remain. Ignored files do not block completion. +- The final assistant message has non-whitespace text and no pending tool calls. + +If any condition is missing, a short status-only prompt asks the same session to +finish the task's required verification and commit work and give a final answer. +The original conversation and session ID remain in use. Already complete output +costs no extra model calls. The follow-up budget applies to the whole invocation, +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. + +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 +session or working directory. + +After the follow-up budget is exhausted, a valid final answer still returns exit +code **0** even if Git conditions remain unsatisfied. The canonical task verifier +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. + +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`, +`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. + +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`, +`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 +its exit status is used: 0 means no committed changes, 1 means committed changes, +and any other code, timeout, or cancellation makes the check unavailable. Each command has a 5-second timeout, +64-KiB stdout/stderr limits, and SIGKILL termination; optional Git index/cache +writes and fsmonitor are disabled. Lazy fetching and interactive Git prompts are +disabled. Normal disposal and SIGINT/SIGTERM/SIGHUP cancel outstanding Git reads +and retain the existing runtime, detached-child, stdout-backpressure, and signal +cleanup paths. diff --git a/packages/agent-core/src/harness/compaction/compaction.ts b/packages/agent-core/src/harness/compaction/compaction.ts index 4113c294..ab5508c4 100644 --- a/packages/agent-core/src/harness/compaction/compaction.ts +++ b/packages/agent-core/src/harness/compaction/compaction.ts @@ -609,6 +609,10 @@ export async function generateSummaryWithUsage( } const textContent = contentText(response.content); + // Validate model text before split-turn scaffolding or file metadata can make it look nonempty. + if (textContent.trim().length === 0) { + return err(new CompactionError("summarization_failed", "Summarization failed: empty summary")); + } return ok({ text: textContent, usage: response.usage }); } @@ -743,7 +747,8 @@ export async function compact( let summaryUsage: Usage; if (isSplitTurn && turnPrefixMessages.length > 0) { - let historyText = "No prior history."; + // With no new history to summarize, the previous checkpoint still carries the earlier context. + let historyText = previousSummary ?? "No prior history."; let historyUsage: Usage | undefined; if (messagesToSummarize.length > 0) { const historyResult = await generateSummaryWithUsage( @@ -862,8 +867,14 @@ async function generateTurnPrefixSummary( ); } + const textContent = contentText(response.content); + // A valid history summary cannot substitute for a missing turn-prefix summary. + if (textContent.trim().length === 0) { + return err(new CompactionError("summarization_failed", "Turn prefix summarization failed: empty summary")); + } + return ok({ - text: contentText(response.content), + text: textContent, usage: response.usage, }); } diff --git a/packages/agent-core/test/harness/compaction-integrity.test.ts b/packages/agent-core/test/harness/compaction-integrity.test.ts new file mode 100644 index 00000000..f37c710b --- /dev/null +++ b/packages/agent-core/test/harness/compaction-integrity.test.ts @@ -0,0 +1,296 @@ +import { + type AssistantMessage, + type Context, + createModels, + fauxAssistantMessage, + fauxProvider, + fauxToolCall, +} from "@step-harness/providers"; +import { describe, expect, it } from "vitest"; +import { compact, generateSummary, prepareCompaction } from "../../src/harness/compaction/compaction.ts"; +import { buildSessionContext } from "../../src/harness/session/context.ts"; +import type { CompactionEntry, Entry } from "../../src/harness/session/types.ts"; +import { getOrThrow } from "../../src/harness/types.ts"; +import type { AgentMessage } from "../../src/types.ts"; + +const HISTORY_MARKER = "ORIGINAL_GOAL_VERIFY_AND_SUBMIT_9F13"; +const PREVIOUS_SUMMARY = `## User Goal\n${HISTORY_MARKER}\n\nKeep the verified results and submission constraints.\n`; +const HISTORY_SUMMARY = `## User Goal\n${HISTORY_MARKER}\n\nThe earlier investigation is complete.`; +const PREFIX_SUMMARY = "## Next Actions\nInspect the retained output and run the regression test."; +const RETAINED_TEXT = "Retained investigation output. ".repeat(20); + +type Layout = "history" | "prefix" | "history-and-prefix"; + +const emptyContents: { name: string; content: AssistantMessage["content"] }[] = [ + { name: "empty array", content: [] }, + { name: "empty text", content: [{ type: "text", text: "" }] }, + { name: "thinking only", content: [{ type: "thinking", thinking: "I should write the handoff now." }] }, + { + name: "whitespace text blocks", + content: [ + { type: "text", text: " \t\r\n" }, + { type: "text", text: "\u00a0\u2003" }, + ], + }, + { + name: "thinking and whitespace", + content: [ + { type: "thinking", thinking: "Preserve the original goal." }, + { type: "text", text: "\n \t" }, + ], + }, +]; + +const failureScenarios: { + name: string; + layout: Layout; + previousSummary: boolean; + validHistoryFirst: boolean; + label: string; +}[] = [ + { name: "history", layout: "history", previousSummary: true, validHistoryFirst: false, label: "Summarization" }, + { + name: "history before a split turn", + layout: "history-and-prefix", + previousSummary: true, + validHistoryFirst: false, + label: "Summarization", + }, + { + name: "prefix after valid history", + layout: "history-and-prefix", + previousSummary: true, + validHistoryFirst: true, + label: "Turn prefix summarization", + }, + { + name: "prefix with previous summary", + layout: "prefix", + previousSummary: true, + validHistoryFirst: false, + label: "Turn prefix summarization", + }, + { + name: "prefix with no prior history", + layout: "prefix", + previousSummary: false, + validHistoryFirst: false, + label: "Turn prefix summarization", + }, +]; + +function createScenario(layout: Layout, previousSummary = true) { + const models = createModels(); + const faux = fauxProvider(); + models.setProvider(faux.provider); + const prefix: AgentMessage[] = [ + { role: "user", content: previousSummary ? "Continue the investigation." : HISTORY_MARKER, timestamp: 1 }, + fauxAssistantMessage(fauxToolCall("read", { path: "src/retained.ts" }, { id: "read-1" }), { + stopReason: "toolUse", + timestamp: 2, + }), + { + role: "toolResult", + toolCallId: "read-1", + toolName: "read", + content: [{ type: "text", text: "Previously inspected source." }], + isError: false, + timestamp: 3, + }, + ]; + if (layout === "history-and-prefix") { + prefix.push({ role: "user", content: "Check the current turn.", timestamp: 4 }); + } + const entries: Entry[] = previousSummary + ? [ + { + type: "compaction", + id: "previous", + parentId: null, + seq: 1, + timestamp: 4, + summary: PREVIOUS_SUMMARY, + retainedTail: prefix, + tokensBefore: 112000, + details: { readFiles: ["src/old.ts"], modifiedFiles: ["src/fix.ts"] }, + }, + ] + : prefix.map((message, index) => ({ + type: "message", + id: `prefix-${index}`, + parentId: index === 0 ? null : `prefix-${index - 1}`, + seq: index + 1, + timestamp: message.timestamp, + message, + })); + entries.push({ + type: "message", + id: "tail", + parentId: entries.at(-1)!.id, + seq: entries.length + 1, + timestamp: 5, + message: + layout === "history" + ? { role: "user", content: RETAINED_TEXT, timestamp: 5 } + : fauxAssistantMessage(RETAINED_TEXT, { timestamp: 5 }), + }); + const preparation = getOrThrow( + prepareCompaction(entries, { enabled: true, reserveTokens: 16384, keepRecentTokens: 20 }), + )!; + expect(preparation).toBeDefined(); + expect(preparation.isSplitTurn).toBe(layout !== "history"); + expect(preparation.messagesToSummarize.length > 0).toBe(layout !== "prefix"); + expect(preparation.turnPrefixMessages.length > 0).toBe(layout !== "history"); + expect(preparation.fileOps.read.has("src/retained.ts")).toBe(true); + return { models, faux, model: faux.getModel(), entries, preparation }; +} + +describe("harness compaction integrity", () => { + // CP-01: exercise the real retained-tail preparation and context reconstruction. + it("preserves the previous history when only a turn prefix needs summarizing", async () => { + const { models, faux, model, entries, preparation } = createScenario("prefix"); + const requests: Context[] = []; + faux.setResponses([ + (context) => { + requests.push(context); + return fauxAssistantMessage(PREFIX_SUMMARY); + }, + ]); + expect(JSON.stringify(buildSessionContext(entries))).toContain(HISTORY_MARKER); + + const result = getOrThrow(await compact(preparation, models, model)); + + expect(requests).toHaveLength(1); + expect(JSON.stringify(requests)).not.toContain(HISTORY_MARKER); + expect(result.summary).toContain( + `${PREVIOUS_SUMMARY}\n\n---\n\n**Turn Context (split turn):**\n\n${PREFIX_SUMMARY}`, + ); + expect(result.retainedTail).toHaveLength(1); + expect(result.retainedTail[0]).toMatchObject({ + role: "assistant", + content: [{ type: "text", text: RETAINED_TEXT }], + }); + const checkpoint: CompactionEntry = { + type: "compaction", + id: "next", + parentId: "tail", + seq: 3, + timestamp: 6, + ...result, + }; + const context = buildSessionContext([...entries, checkpoint]); + expect(context.messages[0]).toMatchObject({ role: "compactionSummary", summary: result.summary }); + expect(JSON.stringify(context)).toContain(HISTORY_MARKER); + expect(context.messages.slice(1)).toEqual(preparation.retainedTail); + }); + + // CP-02: an error result carries no replacement checkpoint, even if another part or file metadata is nonempty. + describe.each(failureScenarios)("$name", (scenario) => { + it.each(emptyContents)("rejects $name before returning replacement context", async ({ content }) => { + const { models, faux, model, entries, preparation } = createScenario( + scenario.layout, + scenario.previousSummary, + ); + const beforeEntries = structuredClone(entries); + const beforePreparation = structuredClone(preparation); + const beforeContext = structuredClone(buildSessionContext(entries)); + faux.setResponses([ + ...(scenario.validHistoryFirst ? [fauxAssistantMessage(HISTORY_SUMMARY)] : []), + fauxAssistantMessage(content), + ]); + + const result = await compact(preparation, models, model, undefined, undefined, undefined, { + enabled: true, + maxRetries: 2, + baseDelayMs: 0, + }); + + expect(result).toMatchObject({ + ok: false, + error: { code: "summarization_failed", message: `${scenario.label} failed: empty summary` }, + }); + expect(result).not.toHaveProperty("value"); + expect(entries).toEqual(beforeEntries); + expect(preparation).toEqual(beforePreparation); + expect(buildSessionContext(entries)).toEqual(beforeContext); + expect(JSON.stringify(beforeContext)).toContain(HISTORY_MARKER); + expect(faux.state.callCount).toBe(scenario.validHistoryFirst ? 2 : 1); + }); + }); + + it.each(emptyContents)("rejects $name through the public generateSummary helper", async ({ content }) => { + const { models, faux, model, preparation } = createScenario("history"); + faux.setResponses([fauxAssistantMessage(content)]); + + expect(await generateSummary(preparation.messagesToSummarize, models, model, 16384)).toMatchObject({ + ok: false, + error: { code: "summarization_failed", message: "Summarization failed: empty summary" }, + }); + }); + + it.each(["history", "prefix", "history-and-prefix"])( + "accepts valid %s summaries with file metadata", + async (layout) => { + const { models, faux, model, preparation } = createScenario(layout); + const text = ` \n${layout === "prefix" ? PREFIX_SUMMARY : HISTORY_SUMMARY}\n `; + faux.setResponses([ + fauxAssistantMessage([ + { type: "thinking", thinking: "This reasoning is not part of the summary." }, + { type: "text", text }, + ]), + ...(layout === "history-and-prefix" ? [fauxAssistantMessage(PREFIX_SUMMARY)] : []), + ]); + + const result = getOrThrow(await compact(preparation, models, model)); + + expect(result.summary).toContain(text); + expect(result.summary).not.toContain("This reasoning is not part of the summary."); + expect(result.summary).toContain("\nsrc/old.ts\nsrc/retained.ts\n"); + expect(result.summary).toContain("\nsrc/fix.ts\n"); + expect(result.summary).toContain(HISTORY_MARKER); + expect(result.usage!.totalTokens).toBeGreaterThan(0); + expect(result.retainedTail).toEqual(preparation.retainedTail); + expect(faux.state.callCount).toBe(layout === "history-and-prefix" ? 2 : 1); + }, + ); + + describe.each(["history", "prefix"])("%s failure diagnostics", (layout) => { + it.each([ + { + name: "partial length stop", + response: fauxAssistantMessage("partial", { stopReason: "length" }), + code: "summarization_failed", + error: "summary is incomplete", + }, + { + name: "empty length stop", + response: fauxAssistantMessage([], { stopReason: "length" }), + code: "summarization_failed", + error: "summary is incomplete", + }, + { + name: "provider error", + response: fauxAssistantMessage([], { stopReason: "error", errorMessage: "insufficient_quota" }), + code: "summarization_failed", + error: "insufficient_quota", + }, + { + name: "abort", + response: fauxAssistantMessage([], { stopReason: "aborted", errorMessage: "summary cancelled" }), + code: "aborted", + error: "summary cancelled", + }, + ])("preserves $name and the original context", async ({ response, code, error }) => { + const { models, faux, model, entries, preparation } = createScenario(layout); + const before = structuredClone(buildSessionContext(entries)); + faux.setResponses([response]); + + expect(await compact(preparation, models, model)).toMatchObject({ + ok: false, + error: { code, message: expect.stringContaining(error) }, + }); + expect(buildSessionContext(entries)).toEqual(before); + expect(faux.state.callCount).toBe(1); + }); + }); +}); diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index 4f38275c..577dcda5 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -40,6 +40,10 @@ export interface Args { extensions?: string[]; noExtensions?: boolean; print?: boolean; + /** Opt-in completion check in print/json mode. */ + completionCheck?: "git-committed"; + /** Maximum completion follow-up prompts, 1..3 (default 2). */ + completionCheckAttempts?: number; export?: string; noSkills?: boolean; skills?: string[]; @@ -202,6 +206,29 @@ export function parseArgs(args: string[]): Args { result.mode = taken.value; } } + } else if (arg === "--completion-check" || arg.startsWith("--completion-check=")) { + const taken = arg.startsWith("--completion-check=") + ? { value: arg.slice("--completion-check=".length), nextIndex: i } + : takeOptionValue(args, i, "--completion-check", result); + if (taken) { + i = taken.nextIndex; + if (taken.value === "git-committed") result.completionCheck = taken.value; + else result.diagnostics.push({ type: "error", message: "--completion-check must be git-committed" }); + } + } else if (arg === "--completion-check-attempts" || arg.startsWith("--completion-check-attempts=")) { + const taken = arg.startsWith("--completion-check-attempts=") + ? { value: arg.slice("--completion-check-attempts=".length), nextIndex: i } + : takeOptionValue(args, i, "--completion-check-attempts", result); + if (taken) { + i = taken.nextIndex; + if (/^[1-3]$/.test(taken.value)) result.completionCheckAttempts = Number(taken.value); + else { + result.diagnostics.push({ + type: "error", + message: "--completion-check-attempts must be an integer from 1 to 3", + }); + } + } } else if (arg === "--approval-mode" || arg.startsWith("--approval-mode=")) { const value = arg === "--approval-mode" ? args[i + 1] : arg.slice("--approval-mode=".length); if (arg === "--approval-mode" && (value === undefined || value.startsWith("-"))) { @@ -503,6 +530,21 @@ export function parseArgs(args: string[]): Args { } } + if (result.completionCheck) { + result.completionCheckAttempts ??= 2; + if (result.mode === "rpc" || result.sdkStdio) { + result.diagnostics.push({ + type: "error", + message: "--completion-check is only supported in print or JSON mode", + }); + } + } else if (result.completionCheckAttempts !== undefined) { + result.diagnostics.push({ + type: "error", + message: "--completion-check-attempts requires --completion-check git-committed", + }); + } + return result; } @@ -563,6 +605,8 @@ ${chalk.bold("Options:")} ${stepPermissionOptionsText} --sdk-stdio Run the Step Agent SDK length-prefixed stdio host --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) --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/core/compaction/compaction.ts b/packages/coding-agent/src/core/compaction/compaction.ts index 5f3e40f4..2092195c 100644 --- a/packages/coding-agent/src/core/compaction/compaction.ts +++ b/packages/coding-agent/src/core/compaction/compaction.ts @@ -746,6 +746,9 @@ export async function generateSummaryWithUsage( callbacks, ); + if (response.stopReason === "aborted") { + throw new DOMException(response.errorMessage || "Summarization aborted", "AbortError"); + } const failure = getSummarizationFailure(response, "Summarization", maxTokens); if (failure) { throw new Error(failure); @@ -755,6 +758,10 @@ export async function generateSummaryWithUsage( } const textContent = contentText(response.content); + // Validate model text before split-turn scaffolding or file metadata can make it look nonempty. + if (textContent.trim().length === 0) { + throw new Error("Summarization failed: empty summary"); + } return { text: textContent, usage: response.usage }; } @@ -914,7 +921,8 @@ export async function compact( let summaryUsage: Usage; if (isSplitTurn && turnPrefixMessages.length > 0) { - let historyText = "No prior history."; + // With no new history to summarize, the previous checkpoint still carries the earlier context. + let historyText = previousSummary ?? "No prior history."; let historyUsage: Usage | undefined; if (messagesToSummarize.length > 0) { const historyResult = await generateSummaryWithUsage( @@ -1025,6 +1033,9 @@ async function generateTurnPrefixSummary( callbacks, ); + if (response.stopReason === "aborted") { + throw new DOMException(response.errorMessage || "Turn prefix summarization aborted", "AbortError"); + } const failure = getSummarizationFailure(response, "Turn prefix summarization", maxTokens); if (failure) { throw new Error(failure); @@ -1033,8 +1044,14 @@ async function generateTurnPrefixSummary( throw new Error("Turn prefix summarization attempted to call a tool"); } + const textContent = contentText(response.content); + // A valid history summary cannot substitute for a missing turn-prefix summary. + if (textContent.trim().length === 0) { + throw new Error("Turn prefix summarization failed: empty summary"); + } + return { - text: contentText(response.content), + text: textContent, usage: response.usage, }; } diff --git a/packages/coding-agent/src/modes/completion-check.ts b/packages/coding-agent/src/modes/completion-check.ts new file mode 100644 index 00000000..c5284a64 --- /dev/null +++ b/packages/coding-agent/src/modes/completion-check.ts @@ -0,0 +1,146 @@ +import { execFile } from "node:child_process"; + +export interface CompletionCheckOptions { + /** Opt-in check after all user prompts; never starts a new session or attempt. */ + completionCheck?: "git-committed"; + /** Maximum additional prompts, 1..3 (default 2). */ + completionCheckAttempts?: number; +} + +export interface GitCompletionState { + hasNewCommit: boolean; + hasCommittedChanges: boolean; + trackedDirty: boolean; + untrackedFiles: boolean; +} + +export function getCompletionCheckAttempts(options: CompletionCheckOptions): number | undefined { + if (options.completionCheck === undefined) { + if (options.completionCheckAttempts !== undefined) { + throw new Error("--completion-check-attempts requires --completion-check git-committed"); + } + return undefined; + } + if (options.completionCheck !== "git-committed") { + throw new Error("--completion-check must be git-committed"); + } + const attempts = options.completionCheckAttempts ?? 2; + if (!Number.isInteger(attempts) || attempts < 1 || attempts > 3) { + throw new Error("--completion-check-attempts must be an integer from 1 to 3"); + } + return attempts; +} + +const GIT_TIMEOUT_MS = 5_000; +const GIT_MAX_BUFFER = 64 * 1024; + +function readGit( + cwd: string, + args: string[], + signal: AbortSignal, + allowDifference = false, +): Promise<{ stdout: string; exitCode: 0 | 1 }> { + return new Promise((resolve, reject) => { + execFile( + "git", + [ + "--no-pager", + "--no-optional-locks", + "-c", + "core.fsmonitor=false", + "-c", + "core.untrackedCache=false", + ...args, + ], + { + cwd, + encoding: "utf8", + shell: false, + timeout: GIT_TIMEOUT_MS, + maxBuffer: GIT_MAX_BUFFER, + killSignal: "SIGKILL", + signal, + windowsHide: true, + env: { ...process.env, GIT_TERMINAL_PROMPT: "0", GIT_NO_REPLACE_OBJECTS: "1", GIT_NO_LAZY_FETCH: "1" }, + }, + (error, stdout) => { + // Git's stderr can contain paths or config values. Never relay it to + // the model or stdout, including on timeout/output-limit failures. + if (!error) resolve({ stdout, exitCode: 0 }); + else if (allowDifference && error.code === 1 && !error.killed && !error.signal && !signal.aborted) { + // diff --quiet uses exit 1 for a difference. Neither decoded + // stdout nor killed/aborted commands can establish this result. + resolve({ stdout, exitCode: 1 }); + } else reject(new Error(`Completion check: git ${args[0]} failed (limit: 5s / 64 KiB).`)); + }, + ); + }); +} + +/** Capture HEAD and validate every read before extensions can start a model call. */ +export async function createGitCompletionCheck( + cwd: string, + signal: AbortSignal, +): Promise<() => Promise> { + let startHead: string; + try { + const insideWorktree = await readGit(cwd, ["rev-parse", "--is-inside-work-tree"], signal); + if (insideWorktree.stdout.trim() !== "true") throw new Error("not a worktree"); + startHead = (await readGit(cwd, ["rev-parse", "--verify", "HEAD^{commit}"], signal)).stdout.trim(); + if (!/^(?:[a-f0-9]{40}|[a-f0-9]{64})$/.test(startHead)) throw new Error("invalid HEAD"); + } catch { + throw new Error("Completion check requires a readable Git worktree with an existing HEAD commit."); + } + + const check = async (): Promise => { + // The only variable argument is the validated object ID captured above. + // A changed HEAD alone is insufficient: rewinding to an ancestor adds no commit. + const newCommit = await readGit(cwd, ["rev-list", "--max-count=1", `${startHead}..HEAD`, "--"], signal); + const committedDiff = await readGit( + cwd, + [ + "diff", + "--quiet", + "--no-ext-diff", + "--no-textconv", + "--no-renames", + "--ignore-submodules=none", + startHead, + "HEAD", + "--", + ], + signal, + true, + ); + const status = await readGit( + cwd, + ["status", "--porcelain=v1", "-z", "--no-renames", "--untracked-files=normal", "--ignore-submodules=none"], + signal, + ); + // --no-renames gives one NUL-delimited record per path, including paths + // containing newlines. No filenames or file contents leave this function. + const entries = status.stdout.split("\0").filter((entry) => entry.length > 0); + return { + hasNewCommit: newCommit.stdout.trim().length > 0, + hasCommittedChanges: committedDiff.exitCode === 1, + trackedDirty: entries.some((entry) => !entry.startsWith("?? ")), + untrackedFiles: entries.some((entry) => entry.startsWith("?? ")), + }; + }; + await check(); + return check; +} + +export function completionCheckFeedback(git: GitCompletionState, hasFinalText: boolean): 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 (!hasFinalText) missing.push("final answer text is missing"); + return ( + `Completion check: ${missing.join("; ")}. ` + + "Complete the task's required verification and commit any remaining task changes, 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 2040d15c..e87668ef 100644 --- a/packages/coding-agent/src/modes/print-mode.ts +++ b/packages/coding-agent/src/modes/print-mode.ts @@ -6,17 +6,25 @@ * - `pi --mode json "prompt"` - JSON event stream */ +import type { AgentMessage } from "@step-harness/agent-core"; import type { AssistantMessage, ImageContent } from "@step-harness/providers"; import type { AgentSessionEvent } from "../core/agent-session.ts"; import type { AgentSessionRuntimeHost } from "../core/agent-session-runtime.ts"; import { flushRawStdout, waitForRawStdoutBackpressure, writeRawStdout } from "../core/output-guard.ts"; import { killTrackedDetachedChildren } from "../utils/shell.ts"; +import { + type CompletionCheckOptions, + completionCheckFeedback, + createGitCompletionCheck, + type GitCompletionState, + getCompletionCheckAttempts, +} from "./completion-check.ts"; import { toJsonEvent } from "./json-event.ts"; /** * Options for print mode. */ -export interface PrintModeOptions { +export interface PrintModeOptions extends CompletionCheckOptions { /** Output mode: "text" for final response only, "json" for all events */ mode: "text" | "json"; /** Array of additional prompts to send after initialMessage */ @@ -63,6 +71,22 @@ function getTerminatingBlock(event: AgentSessionEvent): TerminatingBlock | undef return { toolName: event.toolName, reason: reason || "no reason given" }; } +function getAssistantFailure(message: AgentMessage | undefined): AssistantMessage | undefined { + if (message?.role !== "assistant") return undefined; + const assistant = message as AssistantMessage; + return assistant.stopReason === "error" || assistant.stopReason === "aborted" ? assistant : undefined; +} + +function hasFinalAssistantText(message: AgentMessage | undefined): boolean { + if (message?.role !== "assistant" || getAssistantFailure(message)) return false; + const assistant = message as AssistantMessage; + return ( + assistant.stopReason !== "toolUse" && + !assistant.content.some((part) => part.type === "toolCall") && + assistant.content.some((part) => part.type === "text" && part.text.trim().length > 0) + ); +} + /** * Run in print (single-shot) mode. * Sends prompts to the agent and outputs the result. @@ -79,10 +103,16 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options // only reach the model, so a denied call looked like the run doing nothing. // Feedback issue-d8b499026f19831c. const terminatingBlocks: TerminatingBlock[] = []; + // 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; + const completionAbort = new AbortController(); + let completionAttempts: number | undefined; const disposeRuntime = async (): Promise => { if (disposed) return; disposed = true; + completionAbort.abort(); unsubscribe?.(); unsubscribeBackpressure?.(); await runtimeHost.dispose(); @@ -123,6 +153,7 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options unsubscribe = session.subscribe((event) => { const block = getTerminatingBlock(event); if (block) terminatingBlocks.push(block); + if (event.type === "message_end") assistantFailure ??= getAssistantFailure(event.message); if (mode === "json") { writeRawStdout(`${JSON.stringify(toJsonEvent(event))}\n`); } @@ -165,6 +196,17 @@ 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 completionSession = session; + const completionCwd = runtimeHost.cwd; + const checkGit = + completionAttempts === undefined + ? undefined + : await createGitCompletionCheck(completionCwd, completionAbort.signal); + if (mode === "json") { const header = session.sessionManager.getHeader(); if (header) { @@ -191,12 +233,60 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options console.error(`Available tools: ${unknownSelectors.knownTools.join(", ") || "(none)"}`); } - if (initialMessage) { + const terminalOutcome = () => disposed || terminatingBlocks.length > 0 || assistantFailure !== undefined; + if (initialMessage && !(checkGit && terminalOutcome())) { await session.prompt(initialMessage, { images: initialImages }); + assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); } + // 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; await session.prompt(message); + assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); + } + + if (checkGit && completionAttempts !== undefined) { + assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); + for (let attempt = 0; attempt <= completionAttempts && !terminalOutcome(); 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; + try { + git = await checkGit(); + } catch (error) { + console.error(error instanceof Error ? error.message : "Completion check: Git state unavailable."); + if (mode === "json") { + writeRawStdout( + `${JSON.stringify({ type: "completion_check", check: "git-committed", attempt, status: "unavailable", willFollowUp: false })}\n`, + ); + } + // Do not turn a task failure with valid final text into a retryable + // infrastructure error after the model has already run. + break; + } + if (terminalOutcome() || session !== completionSession || runtimeHost.cwd !== completionCwd) break; + const hasFinalText = hasFinalAssistantText(session.state.messages.at(-1)); + const passed = + git.hasNewCommit && git.hasCommittedChanges && !git.trackedDirty && !git.untrackedFiles && hasFinalText; + const willFollowUp = !passed && attempt < completionAttempts; + if (mode === "json") { + writeRawStdout( + `${JSON.stringify({ type: "completion_check", check: "git-committed", attempt, maxAttempts: completionAttempts, ...git, hasFinalText, status: passed ? "passed" : willFollowUp ? "follow_up" : "exhausted", willFollowUp })}\n`, + ); + await waitForRawStdoutBackpressure(); + } + if (!willFollowUp) { + if (!passed) console.error(`Completion check incomplete after ${attempt} follow-up(s).`); + break; + } + // Backpressure may yield to a signal or a runtime replacement. + if (terminalOutcome() || session !== completionSession || runtimeHost.cwd !== completionCwd) break; + await session.prompt(completionCheckFeedback(git, hasFinalText), { expandPromptTemplates: false }); + assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); + } } // Exit-code determination applies to both text and json modes so a failed @@ -225,6 +315,16 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options } } + if (completionAttempts !== undefined && exitCode === 0 && !hasFinalAssistantText(lastMessage)) { + if (assistantFailure) { + console.error(assistantFailure.errorMessage || `Request ${assistantFailure.stopReason}`); + exitCode = 1; + } else { + console.error("Completion check incomplete: no final answer text after bounded follow-up."); + exitCode = 2; + } + } + return exitCode; } catch (error: unknown) { console.error(error instanceof Error ? error.message : String(error)); diff --git a/packages/coding-agent/test/completion-check-args.test.ts b/packages/coding-agent/test/completion-check-args.test.ts new file mode 100644 index 00000000..96d1518e --- /dev/null +++ b/packages/coding-agent/test/completion-check-args.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it, vi } from "vitest"; +import { parseArgs, printHelp } from "../src/cli/args.ts"; + +describe("completion-check CLI options", () => { + it("is off by default", () => { + const parsed = parseArgs(["-p", "task"]); + expect(parsed.completionCheck).toBeUndefined(); + expect(parsed.completionCheckAttempts).toBeUndefined(); + expect(parsed.diagnostics).toEqual([]); + }); + + it("defaults to two additional prompts and preserves user messages", () => { + const parsed = parseArgs(["--completion-check", "git-committed", "-p", "first", "second"]); + expect(parsed.completionCheck).toBe("git-committed"); + expect(parsed.completionCheckAttempts).toBe(2); + expect(parsed.messages).toEqual(["first", "second"]); + expect(parsed.unknownFlags.size).toBe(0); + expect(parsed.diagnostics).toEqual([]); + }); + + it.each([1, 2, 3])("accepts a bound of %i in both CLI syntaxes", (attempts) => { + for (const flags of [ + ["--completion-check", "git-committed", "--completion-check-attempts", String(attempts)], + [`--completion-check-attempts=${attempts}`, "--completion-check=git-committed"], + ]) { + const parsed = parseArgs(["--mode", "json", ...flags, "task"]); + expect(parsed.completionCheckAttempts).toBe(attempts); + expect(parsed.messages).toEqual(["task"]); + expect(parsed.unknownFlags.size).toBe(0); + expect(parsed.diagnostics).toEqual([]); + } + }); + + it.each(["", "0", "4", "100", "-1", "1.5", "02", "2x", "2e0", "NaN", "Infinity"])( + "rejects an invalid bound %j", + (value) => { + const parsed = parseArgs([ + "-p", + "task", + "--completion-check=git-committed", + `--completion-check-attempts=${value}`, + ]); + expect(parsed.diagnostics).toContainEqual({ + type: "error", + message: "--completion-check-attempts must be an integer from 1 to 3", + }); + expect(parsed.messages).toEqual(["task"]); + }, + ); + + it.each(["--completion-check", "--completion-check-attempts"])("requires a value for %s", (flag) => { + for (const tail of [[], ["--verbose"]]) { + const parsed = parseArgs([flag, ...tail]); + expect(parsed.diagnostics).toContainEqual({ type: "error", message: `${flag} requires a value` }); + if (tail.length) expect(parsed.verbose).toBe(true); + } + }); + + it.each(["", "off", "git", "git status; echo unsafe"])("rejects an unsupported check %j", (value) => { + const parsed = parseArgs(["--completion-check", value]); + expect(parsed.diagnostics).toContainEqual({ type: "error", message: "--completion-check must be git-committed" }); + expect(parsed.messages).toEqual([]); + }); + + it("requires the check when configuring attempts", () => { + expect(parseArgs(["--completion-check-attempts", "2"]).diagnostics).toEqual([ + { type: "error", message: "--completion-check-attempts requires --completion-check git-committed" }, + ]); + }); + + it.each([["--mode", "rpc"], ["--sdk-stdio"]])("rejects incompatible mode %j", (...flags) => { + const parsed = parseArgs(["--completion-check", "git-committed", ...flags]); + expect(parsed.diagnostics).toContainEqual({ + type: "error", + message: "--completion-check is only supported in print or JSON mode", + }); + }); + + it("leaves arguments after -- as literal user messages", () => { + const parsed = parseArgs(["-p", "--", "--completion-check", "git-committed"]); + expect(parsed.completionCheck).toBeUndefined(); + expect(parsed.messages).toEqual(["--completion-check", "git-committed"]); + }); + + it("documents opt-in behavior and the follow-up bound in help", () => { + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + try { + printHelp(); + const help = String(log.mock.calls[0]?.[0]); + expect(help).toContain("--completion-check "); + expect(help).toContain("git-committed"); + expect(help).toContain("--completion-check-attempts "); + expect(help).toContain("1..3 (default: 2)"); + } 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 new file mode 100644 index 00000000..13acc6d7 --- /dev/null +++ b/packages/coding-agent/test/completion-check-cli.test.ts @@ -0,0 +1,148 @@ +import { execFileSync, spawnSync } from "node:child_process"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, describe, expect, it } from "vitest"; + +const roots: string[] = []; +const cli = fileURLToPath(new URL("../../../apps/cli/src/main.ts", import.meta.url)); +const fixture = fileURLToPath(new URL("./fixtures/completion-check-provider.ts", import.meta.url)); +const tsconfig = fileURLToPath(new URL("../../../tsconfig.json", import.meta.url)); + +function runCli(flags: string[], repository: boolean) { + const root = mkdtempSync(join(tmpdir(), "completion-cli-")); + roots.push(root); + const cwd = join(root, "worktree"); + mkdirSync(cwd); + if (repository) { + const git = (...args: string[]) => + execFileSync( + "git", + [ + "-c", + "user.name=Completion Test", + "-c", + "user.email=completion@example.invalid", + "-c", + "commit.gpgsign=false", + ...args, + ], + { cwd, encoding: "utf8", timeout: 5_000, stdio: ["ignore", "pipe", "pipe"] }, + ); + git("init", "--quiet", "--template="); + writeFileSync(join(cwd, "source.txt"), "base\n"); + git("add", "source.txt"); + git("commit", "--quiet", "-m", "base"); + } + const callLog = join(root, "model-calls"); + // A fresh child environment prevents credentials, preloads, user config and + // network provider settings from turning these tests into a paid model run. + const env: NodeJS.ProcessEnv = { + HOME: root, + USERPROFILE: root, + XDG_CONFIG_HOME: join(root, ".config"), + XDG_CACHE_HOME: join(root, ".cache"), + STEP_CODING_AGENT_DIR: join(root, "config", "agent"), + STEPCODE_STORAGE_ROOT_DIR: join(root, "storage"), + STEP_NO_LOCAL_LLM: "1", + AWS_EC2_METADATA_DISABLED: "true", + NODE_ENV: "test", + FORCE_COLOR: "0", + COMPLETION_CHECK_CALL_LOG: callLog, + }; + for (const name of ["PATH", "SystemRoot", "SYSTEMROOT", "WINDIR", "COMSPEC", "PATHEXT"]) { + if (process.env[name] !== undefined) env[name] = process.env[name]; + } + const result = spawnSync( + process.execPath, + [ + fileURLToPath(import.meta.resolve("tsx/cli")), + "--tsconfig", + tsconfig, + cli, + "--provider", + "completion-offline", + "--model", + "faux-1", + "--api-key", + "offline-test-key", + "--no-tools", + "--no-extensions", + "--no-skills", + "--no-prompt-templates", + "--no-context-files", + "--no-session", + "--no-update-check", + "-e", + fixture, + ...flags, + "task", + ], + { cwd, env, encoding: "utf8", timeout: 20_000, maxBuffer: 1024 * 1024 }, + ); + const calls = existsSync(callLog) ? readFileSync(callLog, "utf8").trim().split("\n").length : 0; + return { ...result, calls }; +} + +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)."); + }); + + 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); + expect(checks.at(-1)).toMatchObject({ + hasNewCommit: false, + hasCommittedChanges: false, + status: "exhausted", + willFollowUp: false, + }); + }); + + 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("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-check-attempts", "4"], ["--mode", "rpc"], ["--sdk-stdio"]])( + "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-check"); + }, + ); +}); diff --git a/packages/coding-agent/test/completion-check-git-command.test.ts b/packages/coding-agent/test/completion-check-git-command.test.ts new file mode 100644 index 00000000..e0ff2609 --- /dev/null +++ b/packages/coding-agent/test/completion-check-git-command.test.ts @@ -0,0 +1,108 @@ +import type { ExecFileException, ExecFileOptions } from "node:child_process"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createGitCompletionCheck } from "../src/modes/completion-check.ts"; + +const { execFileMock } = vi.hoisted(() => ({ + execFileMock: + vi.fn< + ( + command: string, + args: string[], + options: ExecFileOptions, + callback: (error: ExecFileException | null, stdout: string, stderr: string) => void, + ) => void + >(), +})); +vi.mock("node:child_process", () => ({ execFile: execFileMock })); + +const head = "a".repeat(40); +let diffError: ExecFileException | null; +let diffStdout: string; + +beforeEach(() => { + execFileMock.mockReset(); + diffError = null; + diffStdout = ""; + execFileMock.mockImplementation((_command, args, _options, callback) => { + if (args.includes("diff")) callback(diffError, diffStdout, "private driver/config error"); + else if (args.includes("--is-inside-work-tree")) callback(null, "true\n", ""); + else if (args.includes("rev-parse")) callback(null, `${head}\n`, ""); + else callback(null, "", ""); + }); +}); + +describe("completion-check fixed Git command boundary", () => { + it("disables shell, fsmonitor, external diffs, textconv and lazy fetching with fixed limits", async () => { + const signal = new AbortController().signal; + await createGitCompletionCheck("/worktree with spaces", signal); + for (const [command, args, options] of execFileMock.mock.calls) { + expect(command).toBe("git"); + expect(args.slice(0, 6)).toEqual([ + "--no-pager", + "--no-optional-locks", + "-c", + "core.fsmonitor=false", + "-c", + "core.untrackedCache=false", + ]); + expect(options).toMatchObject({ + cwd: "/worktree with spaces", + shell: false, + encoding: "utf8", + timeout: 5_000, + maxBuffer: 64 * 1024, + killSignal: "SIGKILL", + signal, + windowsHide: true, + env: { GIT_TERMINAL_PROMPT: "0", GIT_NO_REPLACE_OBJECTS: "1", GIT_NO_LAZY_FETCH: "1" }, + }); + } + const diffArgs = execFileMock.mock.calls.find(([, args]) => args.includes("diff"))?.[1]; + expect(diffArgs?.slice(6)).toEqual([ + "diff", + "--quiet", + "--no-ext-diff", + "--no-textconv", + "--no-renames", + "--ignore-submodules=none", + head, + "HEAD", + "--", + ]); + }); + + it("uses exit status, not decoded stdout, to distinguish unchanged and changed trees", async () => { + diffStdout = "nonempty stdout is not evidence of a tree diff"; + const check = await createGitCompletionCheck("/worktree", new AbortController().signal); + expect((await check()).hasCommittedChanges).toBe(false); + diffStdout = ""; + diffError = Object.assign(new Error("quiet diff found changes"), { code: 1, killed: false }); + expect((await check()).hasCommittedChanges).toBe(true); + }); + + it.each([2, 128, "ERR_CHILD_PROCESS_STDIO_MAXBUFFER", "ABORT_ERR"])( + "treats diff exit/error %s as unavailable and redacts its output", + async (code) => { + const check = await createGitCompletionCheck("/worktree", new AbortController().signal); + diffError = Object.assign(new Error("private configuration value"), { code }); + await expect(check()).rejects.toThrow("Completion check: git diff failed (limit: 5s / 64 KiB)."); + }, + ); + + it.each([{ killed: true }, { signal: "SIGKILL" as const }])( + "does not accept an interrupted diff even if its reported exit code is 1: %j", + async (interrupted) => { + const check = await createGitCompletionCheck("/worktree", new AbortController().signal); + diffError = Object.assign(new Error("timeout"), { code: 1, ...interrupted }); + await expect(check()).rejects.toThrow("git diff failed"); + }, + ); + + it("does not accept a diff result after cancellation", async () => { + const abort = new AbortController(); + const check = await createGitCompletionCheck("/worktree", abort.signal); + diffError = Object.assign(new Error("cancelled"), { code: 1 }); + abort.abort(); + await expect(check()).rejects.toThrow("git diff failed"); + }); +}); diff --git a/packages/coding-agent/test/completion-check.test.ts b/packages/coding-agent/test/completion-check.test.ts new file mode 100644 index 00000000..33e178b0 --- /dev/null +++ b/packages/coding-agent/test/completion-check.test.ts @@ -0,0 +1,193 @@ +import { execFileSync } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + completionCheckFeedback, + createGitCompletionCheck, + getCompletionCheckAttempts, +} from "../src/modes/completion-check.ts"; + +const roots: string[] = []; + +function git(cwd: string, ...args: string[]): string { + return execFileSync( + "git", + [ + "-c", + "user.name=Completion Test", + "-c", + "user.email=completion@example.invalid", + "-c", + "commit.gpgsign=false", + ...args, + ], + { cwd, encoding: "utf8", timeout: 5_000, stdio: ["ignore", "pipe", "pipe"] }, + ); +} + +function makeRepo(): string { + const cwd = mkdtempSync(join(tmpdir(), "completion-check-")); + roots.push(cwd); + git(cwd, "init", "--quiet", "--template="); + writeFileSync(join(cwd, "source.txt"), "base\n"); + writeFileSync(join(cwd, ".gitignore"), "ignored.txt\n"); + git(cwd, "add", "source.txt", ".gitignore"); + git(cwd, "commit", "--quiet", "-m", "base"); + return cwd; +} + +afterEach(() => { + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); +}); + +describe("read-only git-committed check", () => { + it("requires a nonempty starting-HEAD..HEAD commit range", async () => { + const cwd = makeRepo(); + const base = git(cwd, "rev-parse", "HEAD").trim(); + const check = await createGitCompletionCheck(cwd, new AbortController().signal); + expect(await check()).toEqual({ + hasNewCommit: false, + hasCommittedChanges: false, + trackedDirty: false, + untrackedFiles: false, + }); + writeFileSync(join(cwd, "source.txt"), "changed\n"); + git(cwd, "add", "source.txt"); + git(cwd, "commit", "--quiet", "-m", "task change"); + expect(await check()).toEqual({ + hasNewCommit: true, + hasCommittedChanges: true, + trackedDirty: false, + untrackedFiles: false, + }); + const afterCommit = await createGitCompletionCheck(cwd, new AbortController().signal); + git(cwd, "checkout", "--quiet", "--detach", base); + expect(await afterCommit()).toEqual({ + hasNewCommit: false, + hasCommittedChanges: true, + trackedDirty: false, + untrackedFiles: false, + }); + }); + + it("rejects empty commits and a change fully reverted in later commits", async () => { + const cwd = makeRepo(); + const check = await createGitCompletionCheck(cwd, new AbortController().signal); + git(cwd, "commit", "--quiet", "--allow-empty", "-m", "empty task commit"); + expect(await check()).toEqual({ + hasNewCommit: true, + hasCommittedChanges: false, + trackedDirty: false, + untrackedFiles: false, + }); + writeFileSync(join(cwd, "source.txt"), "changed then reverted\n"); + git(cwd, "add", "source.txt"); + git(cwd, "commit", "--quiet", "-m", "temporary change"); + expect((await check()).hasCommittedChanges).toBe(true); + git(cwd, "revert", "--no-edit", "HEAD"); + expect(await check()).toEqual({ + hasNewCommit: true, + hasCommittedChanges: false, + trackedDirty: false, + untrackedFiles: false, + }); + }); + + it("recognizes binary tree differences using the quiet diff exit status", async () => { + const cwd = makeRepo(); + const check = await createGitCompletionCheck(cwd, new AbortController().signal); + writeFileSync(join(cwd, "binary"), Buffer.from([0, 255, 1, 0, 254])); + git(cwd, "add", "binary"); + git(cwd, "commit", "--quiet", "-m", "binary task change"); + expect((await check()).hasCommittedChanges).toBe(true); + }); + + it("detects staged, unstaged, and unignored files without exposing paths or contents", async () => { + const cwd = makeRepo(); + const check = await createGitCompletionCheck(cwd, new AbortController().signal); + writeFileSync(join(cwd, "source.txt"), "private file contents\n"); + writeFileSync(join(cwd, "ignored.txt"), "ignored contents\n"); + expect(await check()).toEqual({ + hasNewCommit: false, + hasCommittedChanges: false, + trackedDirty: true, + untrackedFiles: false, + }); + git(cwd, "add", "source.txt"); + expect((await check()).trackedDirty).toBe(true); + git(cwd, "commit", "--quiet", "-m", "task change"); + const privateName = "private\n?? file $(ignored).txt"; + writeFileSync(join(cwd, privateName), "more private contents\n"); + const state = await check(); + expect(state).toEqual({ + hasNewCommit: true, + hasCommittedChanges: true, + trackedDirty: false, + untrackedFiles: true, + }); + const feedback = completionCheckFeedback(state, false); + expect(feedback).toContain("unignored untracked files remain"); + expect(feedback).toContain("final answer text is missing"); + expect(feedback).not.toContain("private"); + expect(feedback.length).toBeLessThan(500); + }); + + it("disables fsmonitor commands and leaves the index untouched", async () => { + const cwd = makeRepo(); + const marker = join(cwd, "fsmonitor-ran"); + git(cwd, "config", "core.fsmonitor", `touch '${marker}'`); + const index = join(cwd, ".git", "index"); + const before = readFileSync(index); + const beforeStat = statSync(index); + const check = await createGitCompletionCheck(cwd, new AbortController().signal); + await check(); + expect(existsSync(marker)).toBe(false); + expect(readFileSync(index)).toEqual(before); + expect(statSync(index).mtimeMs).toBe(beforeStat.mtimeMs); + }); + + it("fails preflight for non-repositories and unborn HEADs", async () => { + const cwd = mkdtempSync(join(tmpdir(), "completion-no-git-")); + roots.push(cwd); + await expect(createGitCompletionCheck(cwd, new AbortController().signal)).rejects.toThrow("existing HEAD commit"); + git(cwd, "init", "--quiet", "--template="); + await expect(createGitCompletionCheck(cwd, new AbortController().signal)).rejects.toThrow("existing HEAD commit"); + }); + + it("bounds Git output and redacts the failure", async () => { + const cwd = makeRepo(); + for (let index = 0; index < 400; index++) { + writeFileSync(join(cwd, `sensitive-${index}-${"x".repeat(180)}`), "private contents"); + } + await expect(createGitCompletionCheck(cwd, new AbortController().signal)).rejects.toThrow( + "Completion check: git status failed (limit: 5s / 64 KiB).", + ); + }); + + it("obeys cancellation", async () => { + const cwd = makeRepo(); + const abort = new AbortController(); + const check = await createGitCompletionCheck(cwd, abort.signal); + abort.abort(); + await expect(check()).rejects.toThrow("Completion check: git rev-list failed"); + }); +}); + +describe("completion-check option validation for direct print-mode callers", () => { + it("defaults to off or two follow-ups when explicitly enabled", () => { + expect(getCompletionCheckAttempts({})).toBeUndefined(); + expect(getCompletionCheckAttempts({ completionCheck: "git-committed" })).toBe(2); + }); + + it.each([0, 4, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY])("rejects invalid bound %s", (attempts) => { + expect(() => + getCompletionCheckAttempts({ completionCheck: "git-committed", completionCheckAttempts: attempts }), + ).toThrow("integer from 1 to 3"); + }); + + it("rejects attempts without an enabled check", () => { + expect(() => getCompletionCheckAttempts({ completionCheckAttempts: 2 })).toThrow("requires --completion-check"); + }); +}); diff --git a/packages/coding-agent/test/fixtures/completion-check-provider.ts b/packages/coding-agent/test/fixtures/completion-check-provider.ts new file mode 100644 index 00000000..23ba76a1 --- /dev/null +++ b/packages/coding-agent/test/fixtures/completion-check-provider.ts @@ -0,0 +1,17 @@ +import { appendFileSync } from "node:fs"; +import { fauxAssistantMessage, fauxProvider } from "@step-harness/providers"; +import type { ExtensionAPI } from "../../src/core/extensions/types.ts"; + +/** Offline provider for the real CLI completion-check tests. */ +export default function completionCheckProvider(pi: ExtensionAPI): void { + const faux = fauxProvider({ provider: "completion-offline" }); + faux.setResponses( + Array.from({ length: 5 }, () => () => { + const callLog = process.env.COMPLETION_CHECK_CALL_LOG; + if (!callLog) throw new Error("completion-check fixture requires a call log"); + appendFileSync(callLog, "model call\n"); + return fauxAssistantMessage("CLI offline final"); + }), + ); + pi.registerProvider(faux.provider); +} diff --git a/packages/coding-agent/test/suite/completion-check.test.ts b/packages/coding-agent/test/suite/completion-check.test.ts new file mode 100644 index 00000000..04bf9b05 --- /dev/null +++ b/packages/coding-agent/test/suite/completion-check.test.ts @@ -0,0 +1,422 @@ +import { execFileSync } from "node:child_process"; +import { renameSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import type { AgentTool } from "@step-harness/agent-core"; +import { fauxAssistantMessage, fauxThinking, fauxToolCall, type ImageContent } from "@step-harness/providers"; +import { Type } from "typebox"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { AgentSessionRuntimeHost } from "../../src/core/agent-session-runtime.ts"; +import * as output from "../../src/core/output-guard.ts"; +import { type PrintModeOptions, runPrintMode } from "../../src/modes/print-mode.ts"; +import { createHarness, getUserTexts, type Harness, type HarnessOptions } from "./harness.ts"; + +const harnesses: Harness[] = []; +let stdout = ""; + +function git(cwd: string, ...args: string[]): string { + return execFileSync( + "git", + [ + "-c", + "user.name=Completion Test", + "-c", + "user.email=completion@example.invalid", + "-c", + "commit.gpgsign=false", + ...args, + ], + { cwd, encoding: "utf8", timeout: 5_000, stdio: ["ignore", "pipe", "pipe"] }, + ); +} + +function commitTask(harness: Harness): void { + writeFileSync(join(harness.tempDir, "source.txt"), "completed task\n"); + git(harness.tempDir, "add", "source.txt"); + git(harness.tempDir, "commit", "--quiet", "-m", "task change"); +} + +async function setup(options: HarnessOptions = {}, repository = true) { + const harness = await createHarness({ + ...options, + settings: { compaction: { enabled: false }, retry: { enabled: false }, ...options.settings }, + }); + harnesses.push(harness); + if (repository) { + git(harness.tempDir, "init", "--quiet", "--template="); + writeFileSync(join(harness.tempDir, "source.txt"), "base\n"); + git(harness.tempDir, "add", "source.txt"); + git(harness.tempDir, "commit", "--quiet", "-m", "base"); + } + // The session, agent loop, provider, extension runner, and Git reads are real. + // Only host replacement/disposal and process output are test doubles. + const host = { + session: harness.session, + cwd: harness.tempDir, + setRebindSession: vi.fn(), + newSession: vi.fn(async () => ({ cancelled: false })), + fork: vi.fn(async () => ({ cancelled: false })), + switchSession: vi.fn(async () => ({ cancelled: false })), + dispose: vi.fn(async () => { + await host.session.abort(); + await host.session.extensionRunner.emit({ type: "session_shutdown", reason: "quit" }); + host.session.dispose(); + }), + }; + const run = (options: Partial = {}) => + runPrintMode(host as unknown as AgentSessionRuntimeHost, { + mode: "text", + initialMessage: "Complete the task and commit the changes.", + completionCheck: "git-committed", + ...options, + }); + return { harness, host, run }; +} + +beforeEach(() => { + stdout = ""; + vi.spyOn(output, "writeRawStdout").mockImplementation((chunk) => { + stdout += chunk; + }); + vi.spyOn(output, "waitForRawStdoutBackpressure").mockResolvedValue(); + vi.spyOn(output, "flushRawStdout").mockResolvedValue(); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(() => { + for (const harness of harnesses.splice(0)) harness.cleanup(); + vi.restoreAllMocks(); +}); + +describe("runPrintMode same-session completion check", () => { + it("continues dirty work in the same session and emits only the final answer", async () => { + const { harness, host, run } = await setup(); + const sessionId = harness.session.sessionId; + const prompt = vi.spyOn(harness.session, "prompt"); + harness.setResponses([ + () => { + writeFileSync(join(harness.tempDir, "source.txt"), "unfinished task\n"); + return fauxAssistantMessage("premature answer"); + }, + (context) => { + expect(context.messages.some((message) => message.role === "assistant")).toBe(true); + commitTask(harness); + return fauxAssistantMessage("committed and verified"); + }, + ]); + expect(await run()).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(harness.session.sessionId).toBe(sessionId); + expect(getUserTexts(harness)[1]).toContain("tracked changes remain"); + expect(prompt.mock.calls[1]?.[1]).toEqual({ expandPromptTemplates: false }); + expect(host.newSession).not.toHaveBeenCalled(); + expect(host.fork).not.toHaveBeenCalled(); + expect(host.switchSession).not.toHaveBeenCalled(); + expect(host.dispose).toHaveBeenCalledTimes(1); + expect(stdout).toBe("committed and verified\n"); + expect(git(harness.tempDir, "status", "--porcelain")).toBe(""); + }); + + it.each(["text", "json"] as const)("adds no model calls for a clean committed result in %s mode", async (mode) => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage("done"); + }, + ]); + expect(await run({ mode })).toBe(0); + expect(harness.faux.state.callCount).toBe(1); + expect(getUserTexts(harness)).toHaveLength(1); + if (mode === "json") { + const events = stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line)); + expect(events.filter((event) => event.type === "completion_check")).toEqual([ + { + type: "completion_check", + check: "git-committed", + attempt: 0, + maxAttempts: 2, + hasNewCommit: true, + hasCommittedChanges: true, + trackedDirty: false, + untrackedFiles: false, + hasFinalText: true, + status: "passed", + willFollowUp: false, + }, + ]); + } + }); + + it("requires a new commit even when the worktree is already clean", async () => { + const { harness, run } = await setup(); + harness.setResponses([ + fauxAssistantMessage("nothing committed yet"), + () => { + commitTask(harness); + return fauxAssistantMessage("done"); + }, + ]); + expect(await run()).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(getUserTexts(harness)[1]).toContain("no new commit since the starting HEAD"); + }); + + it.each(["empty commit", "full revert"])("does not accept a clean tree after %s", async (kind) => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + if (kind === "empty commit") git(harness.tempDir, "commit", "--quiet", "--allow-empty", "-m", "empty"); + else { + commitTask(harness); + git(harness.tempDir, "revert", "--no-edit", "HEAD"); + } + return fauxAssistantMessage("task is still incomplete"); + }, + fauxAssistantMessage("still incomplete"), + fauxAssistantMessage("unable to finish"), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json" })).toBe(0); + expect(harness.faux.state.callCount).toBe(3); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness)[1]).toContain("no committed tree changes from the starting HEAD"); + const checks = 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).toMatchObject({ hasNewCommit: true, hasCommittedChanges: false, trackedDirty: false }); + expect(checks[2].status).toBe("exhausted"); + }); + + it("preserves extra user messages and initial images before checking completion", async () => { + const { harness, run } = await setup(); + const prompt = vi.spyOn(harness.session, "prompt"); + const messages = ["Also check the edge case.", "Include the test result in the final answer."]; + const images: ImageContent[] = [{ type: "image", mimeType: "image/png", data: "abc" }]; + harness.setResponses([ + fauxAssistantMessage("first"), + fauxAssistantMessage("second"), + fauxAssistantMessage("third"), + () => { + commitTask(harness); + return fauxAssistantMessage("final"); + }, + ]); + expect(await run({ initialMessage: "initial task", initialImages: images, messages })).toBe(0); + expect(getUserTexts(harness).slice(0, 3)).toEqual(["initial task", ...messages]); + expect(prompt.mock.calls[0]).toEqual(["initial task", { images }]); + expect(prompt.mock.calls[1]).toEqual([messages[0]]); + expect(prompt.mock.calls[2]).toEqual([messages[1]]); + expect(messages).toEqual(["Also check the edge case.", "Include the test result in the final answer."]); + expect(harness.faux.state.callCount).toBe(4); + expect(stdout).toBe("final\n"); + }); + + it.each([1, 2, 3])("ends a valid failed task normally after %i follow-ups without resampling", async (attempts) => { + const { harness, host, run } = await setup(); + harness.setResponses(Array.from({ length: attempts + 2 }, () => fauxAssistantMessage("Unable to finish."))); + expect(await run({ completionCheckAttempts: attempts })).toBe(0); + expect(harness.faux.state.callCount).toBe(attempts + 1); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness)).toHaveLength(attempts + 1); + expect(host.newSession).not.toHaveBeenCalled(); + expect(stdout).toBe("Unable to finish.\n"); + expect(console.error).toHaveBeenCalledWith(`Completion check incomplete after ${attempts} follow-up(s).`); + }); + + it.each([[], [fauxThinking("reasoning only")], [{ type: "text" as const, text: " \n " }]])( + "bounds empty or thinking-only final responses and returns incomplete status 2: %j", + async (...content) => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage(content); + }, + fauxAssistantMessage(content), + fauxAssistantMessage(content), + fauxAssistantMessage("must not be used"), + ]); + expect(await run()).toBe(2); + expect(harness.faux.state.callCount).toBe(3); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness)[1]).toContain("final answer text is missing"); + expect(stdout.trim()).toBe(""); + expect(console.error).toHaveBeenCalledWith( + "Completion check incomplete: no final answer text after bounded follow-up.", + ); + }, + ); + + it("recovers thinking-only output with one bounded final-answer prompt", async () => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage(fauxThinking("done thinking")); + }, + fauxAssistantMessage("final answer"), + ]); + expect(await run()).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(stdout).toBe("final answer\n"); + }); + + it.each(["error", "aborted"] as const)("never prompts again after assistant %s", async (stopReason) => { + const { harness, run } = await setup(); + harness.setResponses([ + fauxAssistantMessage("", { stopReason, errorMessage: "terminal failure" }), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ messages: ["pending user prompt"] })).toBe(1); + expect(harness.faux.state.callCount).toBe(1); + expect(getUserTexts(harness)).toHaveLength(1); + expect(console.error).toHaveBeenCalledWith("terminal failure"); + }); + + it("does not add completion prompts after an error recovered by native retry", async () => { + const { harness, run } = await setup({ settings: { retry: { enabled: true, maxRetries: 1, baseDelayMs: 1 } } }); + harness.setResponses([ + fauxAssistantMessage("", { stopReason: "error", errorMessage: "overloaded_error" }), + fauxAssistantMessage("native retry recovered"), + fauxAssistantMessage("must not be used"), + ]); + expect(await run()).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(getUserTexts(harness)).toHaveLength(1); + expect(harness.getPendingResponseCount()).toBe(1); + }); + + it("never follows a terminating permission denial", async () => { + 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", 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); + harness.setResponses([fauxAssistantMessage(fauxThinking("thinking only")), fauxAssistantMessage("unused")]); + expect(await run({ completionCheck: undefined })).toBe(0); + expect(harness.faux.state.callCount).toBe(1); + expect(console.error).not.toHaveBeenCalled(); + }); + + it("fails non-Git preflight before binding extensions or calling the model and cleans up", async () => { + const { harness, host, run } = await setup({}, false); + const bind = vi.spyOn(harness.session, "bindExtensions"); + const signals = ["SIGINT", "SIGTERM", "SIGHUP"] as const; + const before = signals.map((signal) => process.listenerCount(signal)); + expect(await run()).toBe(1); + expect(bind).not.toHaveBeenCalled(); + expect(harness.faux.state.callCount).toBe(0); + expect(host.dispose).toHaveBeenCalledTimes(1); + expect(output.flushRawStdout).toHaveBeenCalledTimes(1); + expect(signals.map((signal) => process.listenerCount(signal))).toEqual(before); + }); + + it("rejects invalid options before any model or extension call", async () => { + const { harness, host, run } = await setup(); + const bind = vi.spyOn(harness.session, "bindExtensions"); + expect(await run({ completionCheckAttempts: 4 })).toBe(1); + expect(bind).not.toHaveBeenCalled(); + expect(harness.faux.state.callCount).toBe(0); + expect(host.dispose).toHaveBeenCalledTimes(1); + }); + + it("does not classify a post-model Git failure with final text as infrastructure error", async () => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + renameSync(join(harness.tempDir, ".git"), join(harness.tempDir, ".git-unavailable")); + return fauxAssistantMessage("task failed, here is the result"); + }, + ]); + expect(await run({ mode: "json" })).toBe(0); + expect(harness.faux.state.callCount).toBe(1); + expect(stdout).toContain('"status":"unavailable"'); + }); + + it("keeps JSON history and waits for stdout backpressure before a follow-up", async () => { + const { harness, run } = await setup(); + harness.setResponses([ + fauxAssistantMessage("first"), + () => { + commitTask(harness); + return fauxAssistantMessage("last"); + }, + ]); + let release: () => void = () => {}; + const blocked = new Promise((resolve) => { + release = resolve; + }); + vi.mocked(output.waitForRawStdoutBackpressure).mockImplementation(async () => { + if (stdout.includes('"type":"completion_check"') && harness.faux.state.callCount === 1) await blocked; + }); + const running = run({ mode: "json" }); + try { + await vi.waitFor(() => expect(stdout).toContain('"status":"follow_up"')); + expect(harness.faux.state.callCount).toBe(1); + } finally { + release(); + } + expect(await running).toBe(0); + const events = stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line)); + expect(events.filter((event) => event.type === "completion_check").map((event) => event.status)).toEqual([ + "follow_up", + "passed", + ]); + expect(events.filter((event) => event.type === "message_end" && event.message.role === "user")).toHaveLength(2); + expect(stdout).toContain('"text":"first"'); + expect(stdout).toContain('"text":"last"'); + }); + + it("preserves runtime rebinding and user messages without automatic continuation into another session", async () => { + const first = await setup(); + const second = await setup(); + first.harness.setResponses([fauxAssistantMessage("before replacement")]); + second.harness.setResponses([fauxAssistantMessage("after replacement")]); + const originalPrompt = first.harness.session.prompt.bind(first.harness.session); + vi.spyOn(first.harness.session, "prompt").mockImplementationOnce(async (text, options) => { + await originalPrompt(text, options); + first.host.session = second.harness.session; + first.host.cwd = second.harness.tempDir; + await first.host.setRebindSession.mock.calls[0]?.[0]?.(second.harness.session); + }); + expect(await first.run({ mode: "json", 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"'); + expect(first.host.dispose).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/coding-agent/test/suite/regressions/compaction-integrity.test.ts b/packages/coding-agent/test/suite/regressions/compaction-integrity.test.ts new file mode 100644 index 00000000..190031b2 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/compaction-integrity.test.ts @@ -0,0 +1,324 @@ +import type { AssistantMessage, Context } from "@step-harness/providers"; +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { prepareCompaction } from "../../../src/core/compaction/compaction.ts"; +import { createHarness, type Harness } from "../harness.ts"; + +const HISTORY_MARKER = "ORIGINAL_GOAL_VERIFY_AND_SUBMIT_9F13"; +const PREVIOUS_SUMMARY = `## User Goal\n${HISTORY_MARKER}\n\nKeep the verified results and submission constraints.\n`; +const HISTORY_SUMMARY = `## User Goal\n${HISTORY_MARKER}\n\nThe earlier investigation is complete.`; +const PREFIX_SUMMARY = "## Next Actions\nInspect the retained output and run the regression test."; +const RETAINED_TEXT = "Retained investigation output. ".repeat(20); + +type Layout = "history" | "prefix" | "history-and-prefix"; + +const emptyContents: { name: string; content: AssistantMessage["content"] }[] = [ + { name: "empty array", content: [] }, + { name: "empty text", content: [{ type: "text", text: "" }] }, + { name: "thinking only", content: [{ type: "thinking", thinking: "I should write the handoff now." }] }, + { + name: "whitespace text blocks", + content: [ + { type: "text", text: " \t\r\n" }, + { type: "text", text: "\u00a0\u2003" }, + ], + }, + { + name: "thinking and whitespace", + content: [ + { type: "thinking", thinking: "Preserve the original goal." }, + { type: "text", text: "\n \t" }, + ], + }, +]; + +const failureScenarios: { + name: string; + layout: Layout; + previousSummary: boolean; + validHistoryFirst: boolean; + label: string; +}[] = [ + { name: "history", layout: "history", previousSummary: true, validHistoryFirst: false, label: "Summarization" }, + { + name: "history before a split turn", + layout: "history-and-prefix", + previousSummary: true, + validHistoryFirst: false, + label: "Summarization", + }, + { + name: "prefix after valid history", + layout: "history-and-prefix", + previousSummary: true, + validHistoryFirst: true, + label: "Turn prefix summarization", + }, + { + name: "prefix with previous summary", + layout: "prefix", + previousSummary: true, + validHistoryFirst: false, + label: "Turn prefix summarization", + }, + { + name: "prefix with no prior history", + layout: "prefix", + previousSummary: false, + validHistoryFirst: false, + label: "Turn prefix summarization", + }, +]; + +describe("compaction integrity", () => { + const harnesses: Harness[] = []; + + afterEach(() => { + vi.restoreAllMocks(); + while (harnesses.length > 0) harnesses.pop()?.cleanup(); + }); + + async function seedSession(layout: Layout, previousSummary = true): Promise { + const harness = await createHarness({ + settings: { + compaction: { keepRecentTokens: 20 }, + retry: { enabled: true, maxRetries: 2, baseDelayMs: 0 }, + }, + }); + harnesses.push(harness); + const firstKeptEntryId = harness.sessionManager.appendMessage({ + role: "user", + content: previousSummary ? "Continue the investigation." : HISTORY_MARKER, + timestamp: 1, + }); + harness.sessionManager.appendMessage( + fauxAssistantMessage(fauxToolCall("read", { path: "src/retained.ts" }, { id: "read-1" }), { + stopReason: "toolUse", + timestamp: 2, + }), + ); + harness.sessionManager.appendMessage({ + role: "toolResult", + toolCallId: "read-1", + toolName: "read", + content: [{ type: "text", text: "Previously inspected source." }], + isError: false, + timestamp: 3, + }); + if (layout === "history-and-prefix") { + harness.sessionManager.appendMessage({ role: "user", content: "Check the current turn.", timestamp: 4 }); + } + if (previousSummary) { + harness.sessionManager.appendCompaction(PREVIOUS_SUMMARY, firstKeptEntryId, 112000, { + readFiles: ["src/old.ts"], + modifiedFiles: ["src/fix.ts"], + }); + } + harness.sessionManager.appendMessage( + layout === "history" + ? { role: "user", content: RETAINED_TEXT, timestamp: 5 } + : fauxAssistantMessage(RETAINED_TEXT, { timestamp: 5 }), + ); + harness.session.agent.state.messages = harness.sessionManager.buildSessionContext().messages; + + const preparation = prepareCompaction( + harness.sessionManager.getBranch(), + harness.settingsManager.getCompactionSettings(), + ); + expect(preparation).toBeDefined(); + expect(preparation!.isSplitTurn).toBe(layout !== "history"); + expect(preparation!.messagesToSummarize.length > 0).toBe(layout !== "prefix"); + expect(preparation!.turnPrefixMessages.length > 0).toBe(layout !== "history"); + expect(preparation!.fileOps.read.has("src/retained.ts")).toBe(true); + return harness; + } + + function captureSession(harness: Harness) { + return { + entries: structuredClone(harness.sessionManager.getEntries()), + context: structuredClone(harness.sessionManager.buildSessionContext()), + messages: structuredClone(harness.session.messages), + leafId: harness.sessionManager.getLeafId(), + }; + } + + // CP-01: the marker exists only in the previous checkpoint, outside the retained messages. + it("preserves previous history through prepare, split-turn compaction, and context reload", async () => { + const harness = await seedSession("prefix"); + const requests: Context[] = []; + harness.setResponses([ + (context) => { + requests.push(context); + return fauxAssistantMessage(PREFIX_SUMMARY); + }, + ]); + const firstKeptEntryId = harness.sessionManager.getLeafId(); + expect(JSON.stringify(harness.session.messages)).toContain(HISTORY_MARKER); + + const result = await harness.session.compact(); + + expect(requests).toHaveLength(1); + expect(JSON.stringify(requests)).not.toContain(HISTORY_MARKER); + expect(result.summary).toContain( + `${PREVIOUS_SUMMARY}\n\n---\n\n**Turn Context (split turn):**\n\n${PREFIX_SUMMARY}`, + ); + expect(result.firstKeptEntryId).toBe(firstKeptEntryId); + expect(result.details).toEqual({ readFiles: ["src/old.ts", "src/retained.ts"], modifiedFiles: ["src/fix.ts"] }); + expect(harness.session.messages[0]).toMatchObject({ role: "compactionSummary", summary: result.summary }); + expect(JSON.stringify(harness.session.messages)).toContain(HISTORY_MARKER); + expect(harness.session.messages).toEqual(harness.sessionManager.buildSessionContext().messages); + expect(harness.sessionManager.getEntries().filter((entry) => entry.type === "compaction")).toHaveLength(2); + }); + + // CP-02: validate each generated part before history, split-turn scaffolding, or file metadata can mask it. + describe.each(failureScenarios)("$name", (scenario) => { + it.each(emptyContents)( + "rejects $name without replacing context or appending a checkpoint", + async ({ content }) => { + const harness = await seedSession(scenario.layout, scenario.previousSummary); + const before = captureSession(harness); + const messages = harness.session.messages; + const appendCompaction = vi.spyOn(harness.sessionManager, "appendCompaction"); + harness.setResponses([ + ...(scenario.validHistoryFirst ? [fauxAssistantMessage(HISTORY_SUMMARY)] : []), + fauxAssistantMessage(content), + ]); + + await expect(harness.session.compact()).rejects.toThrow(`${scenario.label} failed: empty summary`); + + expect(appendCompaction).not.toHaveBeenCalled(); + expect(captureSession(harness)).toEqual(before); + expect(harness.session.messages).toBe(messages); + expect(JSON.stringify(harness.session.messages)).toContain(HISTORY_MARKER); + expect(harness.faux.state.callCount).toBe(scenario.validHistoryFirst ? 2 : 1); + expect(harness.eventsOfType("summarization_retry_scheduled")).toHaveLength(0); + expect(harness.eventsOfType("compaction_end").at(-1)).toMatchObject({ + result: undefined, + aborted: false, + willRetry: false, + errorMessage: expect.stringContaining("empty summary"), + }); + }, + ); + }); + + // Without file metadata, these cases previously produced "" or only the fixed split-turn boilerplate. + it.each(["history", "prefix"])("keeps context after an empty automatic %s compaction", async (layout) => { + const harness = await createHarness({ settings: { compaction: { keepRecentTokens: 20 } } }); + harnesses.push(harness); + harness.sessionManager.appendMessage({ role: "user", content: HISTORY_MARKER, timestamp: 1 }); + if (layout === "history") { + harness.sessionManager.appendMessage( + fauxAssistantMessage("Investigated the original goal.", { timestamp: 2 }), + ); + } + harness.sessionManager.appendMessage( + layout === "history" + ? { role: "user", content: RETAINED_TEXT, timestamp: 3 } + : fauxAssistantMessage(RETAINED_TEXT, { timestamp: 3 }), + ); + harness.session.agent.state.messages = harness.sessionManager.buildSessionContext().messages; + const before = captureSession(harness); + const messages = harness.session.messages; + const appendCompaction = vi.spyOn(harness.sessionManager, "appendCompaction"); + harness.setResponses([fauxAssistantMessage([])]); + const session = harness.session as unknown as { + _runAutoCompaction(reason: "threshold", willRetry: boolean): Promise; + }; + + await expect(session._runAutoCompaction("threshold", false)).resolves.toBe(false); + + expect(appendCompaction).not.toHaveBeenCalled(); + expect(captureSession(harness)).toEqual(before); + expect(harness.session.messages).toBe(messages); + expect(harness.faux.state.callCount).toBe(1); + expect(harness.eventsOfType("compaction_end").at(-1)).toMatchObject({ + reason: "threshold", + result: undefined, + aborted: false, + willRetry: false, + errorMessage: expect.stringContaining("empty summary"), + }); + }); + + it.each(["history", "prefix", "history-and-prefix"])( + "persists valid %s summaries with file metadata", + async (layout) => { + const harness = await seedSession(layout); + const text = ` \n${layout === "prefix" ? PREFIX_SUMMARY : HISTORY_SUMMARY}\n `; + harness.setResponses([ + fauxAssistantMessage([ + { type: "thinking", thinking: "This reasoning is not part of the summary." }, + { type: "text", text }, + ]), + ...(layout === "history-and-prefix" ? [fauxAssistantMessage(PREFIX_SUMMARY)] : []), + ]); + + const result = await harness.session.compact(); + + expect(result.summary).toContain(text); + expect(result.summary).not.toContain("This reasoning is not part of the summary."); + expect(result.summary).toContain("\nsrc/old.ts\nsrc/retained.ts\n"); + expect(result.summary).toContain("\nsrc/fix.ts\n"); + expect(result.usage!.totalTokens).toBeGreaterThan(0); + expect(harness.faux.state.callCount).toBe(layout === "history-and-prefix" ? 2 : 1); + expect(harness.session.messages[0]).toMatchObject({ role: "compactionSummary", summary: result.summary }); + expect(JSON.stringify(harness.session.messages)).toContain(HISTORY_MARKER); + expect(harness.sessionManager.getEntries().filter((entry) => entry.type === "compaction")).toHaveLength(2); + }, + ); + + describe.each(["history", "prefix"])("%s failure diagnostics", (layout) => { + it.each([ + { + name: "partial length stop", + response: fauxAssistantMessage("partial", { stopReason: "length" }), + error: "generation hit the token cap", + }, + { + name: "empty length stop", + response: fauxAssistantMessage([], { stopReason: "length" }), + error: "generation hit the token cap", + }, + { + name: "provider error", + response: fauxAssistantMessage([], { stopReason: "error", errorMessage: "insufficient_quota" }), + error: "insufficient_quota", + }, + ])("preserves $name and the original context", async ({ response, error }) => { + const harness = await seedSession(layout); + const before = captureSession(harness); + harness.setResponses([response]); + + await expect(harness.session.compact()).rejects.toThrow(error); + + expect(captureSession(harness)).toEqual(before); + expect(harness.faux.state.callCount).toBe(1); + expect(harness.eventsOfType("compaction_end").at(-1)).toMatchObject({ + result: undefined, + aborted: false, + errorMessage: expect.stringContaining(error), + }); + }); + + it("preserves cancellation when the aborted response has no text", async () => { + const harness = await seedSession(layout); + const before = captureSession(harness); + harness.setResponses([ + () => { + harness.session.abortCompaction(); + return fauxAssistantMessage([], { stopReason: "aborted" }); + }, + ]); + + await expect(harness.session.compact()).rejects.toThrow(); + + expect(captureSession(harness)).toEqual(before); + expect(harness.eventsOfType("compaction_end").at(-1)).toMatchObject({ + result: undefined, + aborted: true, + errorMessage: undefined, + }); + }); + }); +}); From 602deb9be8e3599d262b39f1b0c12f311772facc Mon Sep 17 00:00:00 2001 From: MelodyVAR <61931019+MelodyVAR@users.noreply.github.com> Date: Sun, 27 Sep 2026 16:28:27 +0000 Subject: [PATCH 2/4] fix(coding-agent): recover command failures and bound compaction --- apps/cli/src/main.ts | 9 + docs/compaction-integrity.md | 90 ++++ docs/completion-check.md | 50 ++- docs/step-configuration.md | 32 ++ .../src/harness/compaction/compaction.ts | 12 +- .../harness/compaction-summary-budget.test.ts | 18 + packages/coding-agent/docs/compaction.md | 31 +- .../docs/environment-variables.md | 4 + .../coding-agent/docs/step-integration.md | 54 ++- packages/coding-agent/src/cli/args.ts | 13 + .../src/core/compaction/compaction.ts | 12 +- packages/coding-agent/src/core/tools/bash.ts | 51 ++- .../src/core/tools/output-accumulator.ts | 4 +- .../src/features/plan-mode-tools.ts | 10 +- .../src/modes/completion-check.ts | 17 +- packages/coding-agent/src/modes/print-mode.ts | 62 ++- .../coding-agent/src/step/tool-profile.ts | 82 +++- packages/coding-agent/src/utils/shell.ts | 58 ++- packages/coding-agent/test/compaction.test.ts | 21 +- .../test/completion-check-args.test.ts | 47 ++- .../test/completion-check-cli.test.ts | 65 ++- .../test/completion-check.test.ts | 42 ++ .../fixtures/command-runtime-recovery.mjs | 40 ++ .../fixtures/compaction-adaptive-models.json | 17 + .../test/shell-process-tree.test.ts | 77 ++++ .../test/step-command-output-spool.test.ts | 113 +++++ .../test/step-pi-storage-wrapper.test.ts | 6 +- .../test/step-plan-extension.test.ts | 81 +++- .../test/step-run-command-timeout.test.ts | 227 ++++++++++ .../test/step-session-wrapper.test.ts | 6 +- .../test/suite/completion-check.test.ts | 390 +++++++++++++++--- .../test/suite/plan-storage.test.ts | 200 +++++++++ .../compaction-adaptive-wire.test.ts | 214 ++++++++++ .../step-command-runtime-recovery.test.ts | 279 +++++++++++++ .../step-run-command-timeout.test.ts | 203 +++++++++ 35 files changed, 2501 insertions(+), 136 deletions(-) create mode 100644 packages/agent-core/test/harness/compaction-summary-budget.test.ts create mode 100644 packages/coding-agent/test/fixtures/command-runtime-recovery.mjs create mode 100644 packages/coding-agent/test/fixtures/compaction-adaptive-models.json create mode 100644 packages/coding-agent/test/shell-process-tree.test.ts create mode 100644 packages/coding-agent/test/step-command-output-spool.test.ts create mode 100644 packages/coding-agent/test/step-run-command-timeout.test.ts create mode 100644 packages/coding-agent/test/suite/plan-storage.test.ts create mode 100644 packages/coding-agent/test/suite/regressions/compaction-adaptive-wire.test.ts create mode 100644 packages/coding-agent/test/suite/regressions/step-command-runtime-recovery.test.ts create mode 100644 packages/coding-agent/test/suite/regressions/step-run-command-timeout.test.ts diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 55b43bc2..e4e5c2c4 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -554,6 +554,14 @@ try { syncStepLoginProfileEndpoint(getStepAuthPath()); 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.help && @@ -731,6 +739,7 @@ async function dispatchStepAppMode(prep: Extract contextWindow - reserveTokens`. Raising it must not raise a +summary request above the model's output limit or the existing 32000-token +summary ceiling. Both compaction implementations clamp the final candidate +budget after considering the reserve fraction and model budget. History and +history updates use fraction 0.8; split-turn prefixes use 0.5. + +For a model declaring `contextWindow=1048576`, `maxTokens=65536`, a 196608-token +trigger uses `reserveTokens=851968`. History and prefix requests both have output +cap 32000. Previously their caps were 681574 and 425984. A positive lower model +limit is respected; when `maxTokens <= 0` denotes an unknown limit, the reserve +fraction remains the fallback, bounded by 32000. + +Settings defaults and valid budgets are unchanged. In particular, a 65536-output +model keeps its 32000 summary cap with either the session's 16384 reserve or the +low-level helper's 24576 reserve. With an unknown model limit and reserve 24576, +history remains 19660 and prefix remains 12288. Ordinary model requests still use +their existing output budget; this change only bounds generated compaction +summaries. No new setting is introduced. + +## Recognize compaction requests without relaxing the normal cap + +Both implementations currently use the same system prompt: 581 UTF-8 bytes, +SHA-256 `7f4677db342c3991df3ed0ba729c514db1772ef7af4c39155d2a0d87d08b12bb`. +Hash decoded prompt text exactly: retain whitespace/newlines and do not add a +trailing newline. The text is `SUMMARIZATION_SYSTEM_PROMPT` in each compaction +`utils.ts`. The complete generated static instruction suffixes are pinned as: + +| Kind | UTF-8 bytes | SHA-256 | +| --- | ---: | --- | +| History | 2702 | `4379f7f63f9fbd36f3f273e94b9566d78967e2e938e4b483957bf67266490572` | +| Prefix | 2948 | `0a355395dcd867cb08c3be3229d31475b3a468c51021489fcca4fab6c967cdb6` | +| Update | 3457 | `90546e9b55c76b8ac2570b98b0d2849cb3808f102097f52f643b00253edbb333` | + +For the OpenAI Chat transport, positively identify a built-in compaction only +when all of these hold: + +1. The top-level system message matches the pinned system prompt exactly. +2. The request has exactly a system message and one user message, both text-only, + with no `tools` or `tool_choice` field. +3. The entire user text matches the generated framing: `\n...\n\n\n` + followed by the exact pinned history or prefix instructions. An update also + has `\n...\n\n\n` before its exact update + instructions. History/update may append `\n\nAdditional focus: ...`; prefix + does not append it. The match must be unambiguous and anchored, not a search + for a phrase anywhere in the transcript. + +The static suffix starts with `The messages above are a conversation to summarize.`, +`The messages above are the PREFIX of a single turn`, or +`The messages above are NEW conversation messages`, respectively. These starts +are labels for inspection; they are not sufficient classifiers on their own. +The complete suffix includes the eight-section format and detail rules. + +Branch summaries share the system prompt but have different instructions and a +2048-token output budget. The system hash alone must not grant a compaction cap +allowance. Unknown, drifted, or ambiguous summary-shaped requests require review. +A normal request containing quoted summary instructions remains normal; neither +an observed 32000 cap nor a session ID establishes that a request is compaction. +The coding-agent main system prompt varies with runtime resources and configuration +and therefore has no single compaction-style static hash. + +For the Harbor adaptive custom-model profile, the generated model overlay omits +`reasoning`, `thinkingLevelMap`, and `compat`, and the adapter emits no `--thinking` +flag. The model loader defaults `reasoning` to false. The default OpenAI Chat +serializer emits `max_completion_tokens`: 65536 for normal requests and 32000 for +recognized compaction requests with this model. Neither kind includes +`reasoning_effort` or `thinking`. Do not add `--thinking off` merely to test this +profile. Other integrity checks, including model identity and no-effort fields, +still apply to every request. + ## Offline regression tests The tests import the actual compaction and context modules and use the faux @@ -73,3 +145,21 @@ pnpm exec vitest --run test/suite/regressions/compaction-integrity.test.ts # From packages/agent-core pnpm exec vitest --run test/harness/compaction-integrity.test.ts ``` + +Budget and adaptive wire regressions are also offline: + +```sh +# From packages/agent-core +pnpm exec vitest --run test/harness/compaction-summary-budget.test.ts + +# From packages/coding-agent +pnpm exec vitest --run test/compaction.test.ts -t 'summary output budget' +pnpm exec vitest --run test/suite/regressions/compaction-adaptive-wire.test.ts +``` + +The adaptive fixture was generated with the Harbor adapter's actual overlay +builder using a synthetic model ID and endpoint. The native test loads it through +the model registry, injects an offline HTTP fetch backed by the suite faux +provider, forces automatic history/prefix compaction, and verifies an explicit +history update. It pins the system and static instruction bytes, checks both +normal and summary wire caps, and supplies no thinking-level override. diff --git a/docs/completion-check.md b/docs/completion-check.md index 1e738006..d77af815 100644 --- a/docs/completion-check.md +++ b/docs/completion-check.md @@ -5,6 +5,7 @@ The Step CLI can perform a bounded completion check in the existing session: ```sh 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 @@ -12,7 +13,9 @@ attempts option counts **additional prompts**, defaults to 2, and accepts intege 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. +`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`. 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 @@ -28,14 +31,35 @@ order. Once those prompts finish, completion requires all of these conditions: - No unignored untracked files remain. Ignored files do not block completion. - The final assistant message has non-whitespace text and no pending tool calls. +Headless clients can set `STEP_CODING_AGENT_PLAN_DIR` to place generated +Markdown plans outside the worktree; see [plan file storage](step-configuration.md#plan-file-storage). +The conditions above still apply to all files remaining in the worktree. + If any condition is missing, a short status-only prompt asks the same session to finish the task's required verification and commit work and give a final answer. -The original conversation and session ID remain in use. Already complete output -costs no extra model calls. The follow-up budget applies to the whole invocation, +The original conversation and session ID remain in use. With review off, already +complete output costs no extra model calls. The follow-up budget applies to the whole invocation, 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 +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. + +Any missing Git or final-text conditions are included in that same review prompt. +Review consumes **one of the existing follow-up slots**, with no extra budget or +new session. Later iterations use ordinary completion checks and feedback without +repeating the self-review. For example, a two-slot budget permits one combined +review/repair prompt and at most one further completion prompt. The review is an +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. + 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 @@ -43,6 +67,9 @@ 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 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 +review is not sent. Budget exhaustion and final-output exit codes are unchanged. After the follow-up budget is exhausted, a valid final answer still returns exit code **0** even if Git conditions remain unsatisfied. The canonical task verifier @@ -63,6 +90,23 @@ prompts, and adds `completion_check` events. Each successful inspection includes emits `status: "unavailable"` and `willFollowUp: false`. No filenames, file contents, diffs, commit messages, or Git stderr appear in check feedback/events. +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 +original review text is emitted as a user message by that session; it does not +assert that the model completed a review or that the result is correct. A prompt +that fails preflight or is intercepted without delivery remains unsent. +Events scheduling feedback include `followUpKind: "review"` or `"completion"`. + +When the review user message is delivered, JSON emits an additional +`completion_check` receipt with the same attempt and inspection fields and +`review.sent: true`. This preserves delivery evidence even if a later assistant +error or runtime replacement prevents another inspection. The receipt does not +run Git or consume another follow-up slot; count prompts by attempts and user +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`, `rev-list --max-count=1`, `diff --quiet HEAD --`, and NUL-delimited diff --git a/docs/step-configuration.md b/docs/step-configuration.md index 69a75765..f44f8148 100644 --- a/docs/step-configuration.md +++ b/docs/step-configuration.md @@ -59,6 +59,38 @@ Step screens pass the Step agent directory; generic TUI callers default to the system temporary directory. Rendering equivalence tests select the uncached renderer through a test-process argument. +### Plan file storage + +Native Markdown plans default to +`/.stepcode/plans/session-.md`. Headless clients can keep +these runtime files outside the project Git worktree by setting +`STEP_CODING_AGENT_PLAN_DIR` before starting Step, for example: + +```sh +export STEP_CODING_AGENT_PLAN_DIR="$HOME/.cache/step/plans" +``` + +The selected directory directly contains `session-.md`. Absolute +paths and `~/` paths are accepted; relative paths resolve from the session's +project working directory. Surrounding whitespace is trimmed, and unset or +blank values preserve the project-local default in every mode. The override +is explicit and also applies to interactive launches when set. It does not +create a directory or file until the proposal is written through the normal +file tools, with their existing permissions. An unwritable location produces +the normal file-tool error; it does not fall back to writing in the project. + +The session saves its selected plan path. Resume, branch navigation, and +re-entering plan mode keep that saved path even if the environment changes; +a fresh session selects the current setting. Existing plans are not moved, +deleted, or rewritten by this option. Keep external storage available for as +long as the session needs its plan; retention and cleanup belong to the host. + +This setting affects only generated plan paths. Agent/session storage options +do not select it, and project `.stepcode/config.toml`, skills, task checklists, +and user-created or tracked files keep their existing behavior. Git ignore +files and [completion checks](completion-check.md) are unchanged. Choose a +directory outside the worktree when the goal is to avoid untracked plan files. + ## First-run theme prompt The first interactive launch asks which theme reads best in the terminal, after diff --git a/packages/agent-core/src/harness/compaction/compaction.ts b/packages/agent-core/src/harness/compaction/compaction.ts index ab5508c4..bc921e18 100644 --- a/packages/agent-core/src/harness/compaction/compaction.ts +++ b/packages/agent-core/src/harness/compaction/compaction.ts @@ -170,10 +170,10 @@ export const DEFAULT_COMPACTION_SETTINGS: CompactionSettings = { }; /** - * Pick the summary output cap for a compaction request. Uses whichever is - * larger of the reserve-token budget and the model's own output cap (clamped to - * {@link SUMMARY_OUTPUT_TOKENS_CEILING}), so large-output models are not - * throttled by the conservative 0.8 * reserveTokens heuristic on rich sessions. + * Pick the summary output cap without letting the trigger reserve bypass output limits. + * Valid existing budgets are preserved. A positive model limit and the summary + * ceiling bound the final budget; an unknown model limit uses the reserve fraction + * bounded by {@link SUMMARY_OUTPUT_TOKENS_CEILING}. */ export function pickSummaryMaxTokens( model: { readonly maxTokens: number }, @@ -182,7 +182,9 @@ export function pickSummaryMaxTokens( ): number { const reserveBudget = Math.floor(reserveFraction * reserveTokens); const modelBudget = model.maxTokens > 0 ? Math.min(model.maxTokens, SUMMARY_OUTPUT_TOKENS_CEILING) : 0; - return Math.max(reserveBudget, modelBudget) || reserveBudget; + // reserveTokens also controls the trigger threshold; an early trigger must not expand the output cap. + const requestedBudget = Math.max(reserveBudget, modelBudget) || reserveBudget; + return Math.min(requestedBudget, modelBudget || SUMMARY_OUTPUT_TOKENS_CEILING); } /** Calculate total context tokens from provider usage. */ diff --git a/packages/agent-core/test/harness/compaction-summary-budget.test.ts b/packages/agent-core/test/harness/compaction-summary-budget.test.ts new file mode 100644 index 00000000..ac3c67e8 --- /dev/null +++ b/packages/agent-core/test/harness/compaction-summary-budget.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; +import { pickSummaryMaxTokens } from "../../src/harness/compaction/compaction.ts"; + +describe("summary output budget", () => { + it.each([ + { name: "default model budget", modelMax: 65536, reserve: 24576, history: 32000, prefix: 32000 }, + { name: "session default reserve", modelMax: 65536, reserve: 16384, history: 32000, prefix: 32000 }, + { name: "early trigger reserve", modelMax: 65536, reserve: 851968, history: 32000, prefix: 32000 }, + { name: "lower model cap", modelMax: 8192, reserve: 851968, history: 8192, prefix: 8192 }, + { name: "unknown zero budget", modelMax: 0, reserve: 851968, history: 32000, prefix: 32000 }, + { name: "unknown negative budget", modelMax: -1, reserve: 851968, history: 32000, prefix: 32000 }, + { name: "default unknown budget", modelMax: 0, reserve: 24576, history: 19660, prefix: 12288 }, + { name: "small unknown budget", modelMax: -1, reserve: 2000, history: 1600, prefix: 1000 }, + ])("bounds history and prefix output with $name", ({ modelMax, reserve, history, prefix }) => { + expect(pickSummaryMaxTokens({ maxTokens: modelMax }, reserve, 0.8)).toBe(history); + expect(pickSummaryMaxTokens({ maxTokens: modelMax }, reserve, 0.5)).toBe(prefix); + }); +}); diff --git a/packages/coding-agent/docs/compaction.md b/packages/coding-agent/docs/compaction.md index 1acc9596..6e61a8a2 100644 --- a/packages/coding-agent/docs/compaction.md +++ b/packages/coding-agent/docs/compaction.md @@ -32,7 +32,7 @@ Auto-compaction triggers when: contextTokens > contextWindow - reserveTokens ``` -By default, `reserveTokens` is 16384 tokens (configurable in `~/.stepcode/agent/settings.json` or `/.stepcode/settings.json`). This leaves room for the LLM's response. +By default, `reserveTokens` is 16384 tokens (configurable in `~/.stepcode/config.toml` or trusted `/.stepcode/config.toml`). This leaves room for the LLM's response. During a multi-turn agent run, Step checks this threshold after tools finish and their results are appended, before starting the next assistant response. If the threshold is crossed, Step compacts inside the same agent run and resumes with the summary and retained messages. It skips this between-turn check when the completed tool batch terminates the run and no queued message requires another response. Step also checks the threshold before a new user prompt and after a low-level agent run ends. @@ -40,7 +40,7 @@ You can also trigger manually with `/compact [instructions]`, where optional ins ### How It Works -1. **Find cut point**: Walk backwards from newest message, accumulating token estimates until `keepRecentTokens` (default 20k, configurable in `~/.stepcode/agent/settings.json` or `/.stepcode/settings.json`) is reached +1. **Find cut point**: Walk backwards from newest message, accumulating token estimates until `keepRecentTokens` (default 20k, configurable in `~/.stepcode/config.toml` or trusted `/.stepcode/config.toml`) is reached 2. **Extract messages**: Collect messages from the previous kept boundary (or session start) up to the cut point 3. **Generate summary**: Call LLM to summarize with structured format, passing the previous summary as iterative context when present 4. **Append entry**: Save `CompactionEntry` with summary and `firstKeptEntryId` @@ -403,16 +403,13 @@ See `SessionBeforeTreeEvent` and `TreePreparation` in the types file. ## Settings -Configure compaction in `~/.stepcode/agent/settings.json` or `/.stepcode/settings.json`: +Configure compaction in `~/.stepcode/config.toml` or trusted `/.stepcode/config.toml`: -```json -{ - "compaction": { - "enabled": true, - "reserveTokens": 16384, - "keepRecentTokens": 20000 - } -} +```toml +[compaction] +enabled = true +reserveTokens = 16384 +keepRecentTokens = 20000 ``` | Setting | Default | Description | @@ -421,4 +418,14 @@ Configure compaction in `~/.stepcode/agent/settings.json` or `/.ste | `reserveTokens` | `16384` | Tokens to reserve for LLM response | | `keepRecentTokens` | `20000` | Recent tokens to keep (not summarized) | -Disable auto-compaction with `"enabled": false`. You can still compact manually with `/compact`. +Disable auto-compaction with `enabled = false`. You can still compact manually with `/compact`. + +The reserve also influences generated summary budgets, but each history/update +or split-turn prefix request is capped at 32000 tokens and at the model's positive +`maxTokens` limit. Unknown/nonpositive model limits retain the reserve-fraction +fallback up to 32000. For example, with a 1048576-token window, setting +`reserveTokens = 851968` triggers compaction above 196608 tokens without inflating +summary output beyond the limit. `keepRecentTokens` remains approximate; the +ordinary model context/output declarations are unchanged. See the +[compaction integrity contract](../../../docs/compaction-integrity.md) for exact +request recognition and no-effort wire behavior. diff --git a/packages/coding-agent/docs/environment-variables.md b/packages/coding-agent/docs/environment-variables.md index e73d5366..9817e723 100644 --- a/packages/coding-agent/docs/environment-variables.md +++ b/packages/coding-agent/docs/environment-variables.md @@ -7,12 +7,16 @@ Step reads the variables below. Configuration shared across launches belongs in |----------|-------------| | `STEP_CODING_AGENT_DIR` | Override the agent directory; default is `~/.stepcode/agent` | | `STEP_CODING_AGENT_SESSION_DIR` | Override session storage; `--session-dir` takes precedence | +| `STEP_CODING_AGENT_PLAN_DIR` | Directory for newly selected native Markdown plan paths; defaults to `/.stepcode/plans` | | `STEP_API_KEY` | StepFun API credential | | `STEP_BASE_URL` | Override the StepFun API endpoint | | `STEP_PROVIDER`, `STEP_MODEL` | Default provider and model selection | | `VISUAL`, `EDITOR` | External editor fallback when `externalEditor` is unset | | `HTTP_PROXY`, `HTTPS_PROXY` | Proxy outbound HTTP requests | +For headless plan storage outside a Git worktree, path resolution, and saved +session behavior, see [plan file storage](../../../docs/step-configuration.md#plan-file-storage). + The CLI sets `AI_AGENT=step`. Child processes inherit this process marker and the ordinary shell environment. Shell tools do not inject session IDs, transcript paths, model IDs, or reasoning levels into command environments. diff --git a/packages/coding-agent/docs/step-integration.md b/packages/coding-agent/docs/step-integration.md index 99a396c7..67436cdf 100644 --- a/packages/coding-agent/docs/step-integration.md +++ b/packages/coding-agent/docs/step-integration.md @@ -185,6 +185,50 @@ normal user-message path. The model inspects the project and uses pi's native write and approval flow; the prompt explicitly preserves an existing `AGENTS.md` instead of replacing it. +## Shell command execution + +Foreground `run_command` calls use a 120-second timeout when `timeout_ms` is +omitted. A timed-out command and its child processes are stopped through the +native shell backend, and the agent receives an error tool result so it can +inspect the failure and continue. The timeout measures command execution after +permission handling; it does not approve or dismiss an approval request. + +Set `timeout_ms` explicitly to choose a foreground timeout between 1,000 and +600,000 milliseconds. An explicit value takes precedence over an embedded +host's existing `commandTimeoutMs` context override, which takes precedence +over the 120-second default. For a server, watcher, or longer job, use +`run_in_background: true`, inspect the returned log path, and stop the process +with the returned command when it is no longer needed. Background execution +keeps its session-bound lifecycle and ignores the foreground timeout fields. +Pi's separate native `bash` tool retains its optional timeout contract. + +On Linux, timeout and abort cleanup snapshots the command's current descendants +before killing its process group. Descendants in separate process groups are +also signaled, after checking their process start times to avoid targeting a +reused PID. This snapshot cannot contain descendants already reparented outside +the command tree or new forks racing the snapshot. It does not adopt grandchildren +for reaping; the native backend waits for its direct child and drains captured +output. Normal post-exit output draining remains unchanged. + +A foreground process terminated by a signal returns a failed tool result with +its captured output and signal name. Caller cancellation takes precedence over a +timeout, and both take precedence over the signal used for cleanup. These failed +tool results remain available to later model turns; caller cancellation itself +still stops the active turn. + +The Step character cap applies to successful and failed command output. When it +is smaller than the native 50KiB/2,000-line limit, the existing output accumulator +saves the complete raw stream before Step removes any diagnostics. Returned +fragments preserve the command status and a readable full-output log path. Those +recovery fields can exceed a very small cap rather than truncate the path. If +this additional spool fails, Step retains the full in-memory result instead of +discarding its only diagnostic copy. A command printing a log-looking notice +cannot substitute for the accumulator's actual saved-log metadata. + +Background commands keep their separate detached lifecycle and streaming log; +foreground capture limits do not apply to them. Permission decisions and explicit +denial termination still occur before command execution. + ## Interactive tool rendering Step's projected tool titles and collapsed summaries are single physical @@ -243,6 +287,14 @@ pending, in progress, or completed. The task tools track work; they do not execute, delegate, or schedule it. Approving a plan does not generate tasks from Markdown. +Markdown proposals normally live at +`/.stepcode/plans/session-.md`. Hosts can set +`STEP_CODING_AGENT_PLAN_DIR` to keep newly selected plan files outside the +project, including in headless runs. Saved session paths take precedence on +restore; this option does not migrate existing plans or project settings. +See [plan file storage](../../../docs/step-configuration.md#plan-file-storage) +for path rules and retention responsibilities. + - `enter_plan_mode`, `/plan`, and `--plan` share the same setup. Entry takes effect immediately; it does not request approval. An explicit `--plan` applies at process startup, including a continued session whose mode was off; @@ -325,7 +377,7 @@ mark so deleted or sibling-branch IDs are not reused. Plan IDs derive from their first task ID. Restoring a session restores its selected plan, not every plan into the active checklist. Existing snapshots without plan identities restore as one plan; already-mixed historical tasks are not split by guessing intent. -The Markdown plan remains a workspace file; restoring mode does not version or +The Markdown plan remains a file at its saved path; restoring mode does not version or rewind its contents. On resume or after compaction, the model should call `task_list` before continuing multi-step work instead of recreating tasks. The whole task list is not injected into every turn. diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index 577dcda5..16baae83 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -44,6 +44,8 @@ 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. */ + completionReview?: boolean; export?: string; noSkills?: boolean; skills?: string[]; @@ -206,6 +208,10 @@ export function parseArgs(args: string[]): Args { result.mode = taken.value; } } + } else if (arg === "--completion-review") { + result.completionReview = true; + } else if (arg.startsWith("--completion-review=")) { + result.diagnostics.push({ type: "error", message: "--completion-review does not take a value" }); } else if (arg === "--completion-check" || arg.startsWith("--completion-check=")) { const taken = arg.startsWith("--completion-check=") ? { value: arg.slice("--completion-check=".length), nextIndex: i } @@ -544,6 +550,12 @@ export function parseArgs(args: string[]): Args { message: "--completion-check-attempts requires --completion-check git-committed", }); } + if (result.completionReview && !result.completionCheck) { + result.diagnostics.push({ + type: "error", + message: "--completion-review requires --completion-check git-committed", + }); + } return result; } @@ -607,6 +619,7 @@ ${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 --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/core/compaction/compaction.ts b/packages/coding-agent/src/core/compaction/compaction.ts index 2092195c..d348b2ad 100644 --- a/packages/coding-agent/src/core/compaction/compaction.ts +++ b/packages/coding-agent/src/core/compaction/compaction.ts @@ -156,10 +156,10 @@ export const DEFAULT_COMPACTION_SETTINGS: CompactionSettings = { }; /** - * Pick the summary output cap for a compaction request. Uses whichever is - * larger of the reserve-token budget and the model's own output cap (clamped to - * {@link SUMMARY_OUTPUT_TOKENS_CEILING}), so large-output models are not - * throttled by the conservative 0.8 * reserveTokens heuristic on rich sessions. + * Pick the summary output cap without letting the trigger reserve bypass output limits. + * Valid existing budgets are preserved. A positive model limit and the summary + * ceiling bound the final budget; an unknown model limit uses the reserve fraction + * bounded by {@link SUMMARY_OUTPUT_TOKENS_CEILING}. */ export function pickSummaryMaxTokens( model: { readonly maxTokens: number }, @@ -168,7 +168,9 @@ export function pickSummaryMaxTokens( ): number { const reserveBudget = Math.floor(reserveFraction * reserveTokens); const modelBudget = model.maxTokens > 0 ? Math.min(model.maxTokens, SUMMARY_OUTPUT_TOKENS_CEILING) : 0; - return Math.max(reserveBudget, modelBudget) || reserveBudget; + // reserveTokens also controls the trigger threshold; an early trigger must not expand the output cap. + const requestedBudget = Math.max(reserveBudget, modelBudget) || reserveBudget; + return Math.min(requestedBudget, modelBudget || SUMMARY_OUTPUT_TOKENS_CEILING); } // ============================================================================ diff --git a/packages/coding-agent/src/core/tools/bash.ts b/packages/coding-agent/src/core/tools/bash.ts index 7724b951..46bceab7 100644 --- a/packages/coding-agent/src/core/tools/bash.ts +++ b/packages/coding-agent/src/core/tools/bash.ts @@ -55,6 +55,16 @@ export interface BashToolDetails { fullOutputPath?: string; } +/** Captured command failure with a log produced by the native accumulator. */ +export class ShellToolError extends Error { + readonly fullOutputPath: string | undefined; + + constructor(message: string, fullOutputPath?: string) { + super(message); + this.fullOutputPath = fullOutputPath; + } +} + /** * Pluggable operations for the bash tool. * Override these to delegate command execution to remote systems (for example SSH). @@ -136,6 +146,9 @@ export function createLocalShellOperations( if (timedOut) { throw new Error(`timeout:${timeout}`); } + if (child.signalCode) { + throw new Error(`signal:${child.signalCode}`); + } return { exitCode }; } finally { if (child.pid) untrackDetachedChildPid(child.pid); @@ -175,6 +188,8 @@ export interface BashToolOptions { spawnHook?: BashSpawnHook; /** Agent directory used for the managed binary PATH entry. Defaults to Pi's agent directory. */ agentDir?: string; + /** Preserve raw output before a caller applies a smaller character cap. Does not change native truncation. */ + persistOutputAboveChars?: number; } const BASH_PREVIEW_LINES = 5; @@ -387,6 +402,21 @@ export function createShellToolDefinition( clearUpdateTimer(); emitOutputUpdate(); const snapshot = output.snapshot({ persistIfTruncated: true }); + if ( + !snapshot.fullOutputPath && + options?.persistOutputAboveChars !== undefined && + snapshot.content.length > options.persistOutputAboveChars + ) { + try { + const preserved = output.snapshot({ persistFullOutput: true }); + await output.closeTempFile(); + return preserved; + } catch { + // Keep the complete in-memory output if this additional spool fails. + // The downstream cap must not discard text without a usable log. + return snapshot; + } + } await output.closeTempFile(); return snapshot; }; @@ -407,6 +437,9 @@ export function createShellToolDefinition( } else { text += `\n\n[Showing lines ${startLine}-${endLine} of ${truncation.totalLines} (${formatSize(DEFAULT_MAX_BYTES)} limit). Full output: ${snapshot.fullOutputPath}]`; } + } else if (snapshot.fullOutputPath) { + details = { fullOutputPath: snapshot.fullOutputPath }; + text += `\n\n[Full output: ${snapshot.fullOutputPath}]`; } return { text, details }; }; @@ -427,11 +460,20 @@ export function createShellToolDefinition( const snapshot = await finishOutput(); const { text } = formatOutput(snapshot, ""); if (err instanceof Error && err.message === "aborted") { - throw new Error(appendStatus(text, "Command aborted")); + throw new ShellToolError(appendStatus(text, "Command aborted"), snapshot.fullOutputPath); } if (err instanceof Error && err.message.startsWith("timeout:")) { const timeoutSecs = err.message.split(":")[1]; - throw new Error(appendStatus(text, `Command timed out after ${timeoutSecs} seconds`)); + throw new ShellToolError( + appendStatus(text, `Command timed out after ${timeoutSecs} seconds`), + snapshot.fullOutputPath, + ); + } + if (err instanceof Error && err.message.startsWith("signal:")) { + throw new ShellToolError( + appendStatus(text, `Command terminated by signal ${err.message.slice(7)}`), + snapshot.fullOutputPath, + ); } throw err; } @@ -439,7 +481,10 @@ export function createShellToolDefinition( const snapshot = await finishOutput(); const { text: outputText, details } = formatOutput(snapshot); if (exitCode !== 0 && exitCode !== null) { - throw new Error(appendStatus(outputText, `Command exited with code ${exitCode}`)); + throw new ShellToolError( + appendStatus(outputText, `Command exited with code ${exitCode}`), + snapshot.fullOutputPath, + ); } return { content: [{ type: "text", text: outputText }], details }; } finally { diff --git a/packages/coding-agent/src/core/tools/output-accumulator.ts b/packages/coding-agent/src/core/tools/output-accumulator.ts index 8d2c2f21..3d22adea 100644 --- a/packages/coding-agent/src/core/tools/output-accumulator.ts +++ b/packages/coding-agent/src/core/tools/output-accumulator.ts @@ -88,7 +88,7 @@ export class OutputAccumulator { } } - snapshot(options: { persistIfTruncated?: boolean } = {}): OutputSnapshot { + snapshot(options: { persistIfTruncated?: boolean; persistFullOutput?: boolean } = {}): OutputSnapshot { const tailTruncation = truncateTail(this.getSnapshotText(), { maxLines: this.maxLines, maxBytes: this.maxBytes, @@ -107,7 +107,7 @@ export class OutputAccumulator { maxBytes: this.maxBytes, }; - if (options.persistIfTruncated && truncation.truncated) { + if (options.persistFullOutput || (options.persistIfTruncated && truncation.truncated)) { this.ensureTempFile(); } diff --git a/packages/coding-agent/src/features/plan-mode-tools.ts b/packages/coding-agent/src/features/plan-mode-tools.ts index b4693527..54c7da07 100644 --- a/packages/coding-agent/src/features/plan-mode-tools.ts +++ b/packages/coding-agent/src/features/plan-mode-tools.ts @@ -18,6 +18,7 @@ import { type PlanReviewResult, renderPlanReviewResult, } from "../render/plan-review.ts"; +import { resolvePath } from "../utils/paths.ts"; /** Exit outcome dimension for plan_mode_exited telemetry. */ export type StepPlanExitOutcome = "approved" | "toggled_off" | "auto_headless" | "auto_rpc"; @@ -36,9 +37,14 @@ export interface StepPlanModeController { resolvePlanFilePath(extensionContext: ExtensionContext): string; } -/** Absolute path of the per-session plan file under the project workspace. */ +/** Select a new per-session plan path; the extension retains saved paths on restore. */ export function getPlanFilePath(sessionId: string, projectCwd?: string): string { - return path.join(projectCwd ?? process.cwd(), ".stepcode", "plans", `session-${sessionId}.md`); + const cwd = projectCwd ?? process.cwd(); + const planDir = process.env.STEP_CODING_AGENT_PLAN_DIR?.trim(); + return path.join( + planDir ? resolvePath(planDir, cwd) : path.join(cwd, ".stepcode", "plans"), + `session-${sessionId}.md`, + ); } /** diff --git a/packages/coding-agent/src/modes/completion-check.ts b/packages/coding-agent/src/modes/completion-check.ts index c5284a64..375a7cf2 100644 --- a/packages/coding-agent/src/modes/completion-check.ts +++ b/packages/coding-agent/src/modes/completion-check.ts @@ -5,6 +5,8 @@ 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). */ + completionReview?: boolean; } export interface GitCompletionState { @@ -16,6 +18,9 @@ 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"); } @@ -131,13 +136,23 @@ export async function createGitCompletionCheck( return check; } -export function completionCheckFeedback(git: GitCompletionState, hasFinalText: boolean): string { +export function completionCheckFeedback(git: GitCompletionState, 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 (!hasFinalText) missing.push("final answer text is missing"); + if (review) { + return ( + (missing.length > 0 ? `Completion check: ${missing.join("; ")}. ` : "") + + "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. " + + "Preserve unrelated user changes and respect permission denials." + ); + } return ( `Completion check: ${missing.join("; ")}. ` + "Complete the task's required verification and commit any remaining task changes, then provide a brief final answer. " + diff --git a/packages/coding-agent/src/modes/print-mode.ts b/packages/coding-agent/src/modes/print-mode.ts index e87668ef..a5f337e1 100644 --- a/packages/coding-agent/src/modes/print-mode.ts +++ b/packages/coding-agent/src/modes/print-mode.ts @@ -108,6 +108,8 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options let assistantFailure: AssistantMessage | undefined; const completionAbort = new AbortController(); let completionAttempts: number | undefined; + let reviewRequested = false; + let reviewSent = false; const disposeRuntime = async (): Promise => { if (disposed) return; @@ -260,7 +262,7 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options console.error(error instanceof Error ? error.message : "Completion check: Git state unavailable."); if (mode === "json") { writeRawStdout( - `${JSON.stringify({ type: "completion_check", check: "git-committed", attempt, status: "unavailable", willFollowUp: false })}\n`, + `${JSON.stringify({ type: "completion_check", check: "git-committed", attempt, status: "unavailable", willFollowUp: false, ...(options.completionReview ? { review: { requested: reviewRequested, sent: reviewSent } } : {}) })}\n`, ); } // Do not turn a task failure with valid final text into a retryable @@ -269,13 +271,34 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options } if (terminalOutcome() || 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 && hasFinalText; + 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", + attempt, + maxAttempts: completionAttempts, + ...git, + hasFinalText, + status: passed ? "passed" : willFollowUp ? "follow_up" : "exhausted", + willFollowUp, + ...(options.completionReview + ? { + review: { requested: reviewRequested, sent: reviewSent }, + ...(willFollowUp ? { followUpKind: requestReview ? "review" : "completion" } : {}), + } + : {}), + }; if (mode === "json") { - writeRawStdout( - `${JSON.stringify({ type: "completion_check", check: "git-committed", attempt, maxAttempts: completionAttempts, ...git, hasFinalText, status: passed ? "passed" : willFollowUp ? "follow_up" : "exhausted", willFollowUp })}\n`, - ); + writeRawStdout(`${JSON.stringify(completionEvent)}\n`); await waitForRawStdoutBackpressure(); } if (!willFollowUp) { @@ -284,7 +307,34 @@ export async function runPrintMode(runtimeHost: AgentSessionRuntimeHost, options } // Backpressure may yield to a signal or a runtime replacement. if (terminalOutcome() || session !== completionSession || runtimeHost.cwd !== completionCwd) break; - await session.prompt(completionCheckFeedback(git, hasFinalText), { expandPromptTemplates: false }); + 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, + // even if a later assistant error prevents another completion inspection. + const unsubscribeReview = requestReview + ? session.subscribe((event) => { + if (reviewSent || event.type !== "message_end" || event.message.role !== "user") return; + const content = event.message.content; + if ( + typeof content === "string" + ? content !== feedback + : !content.some((part) => isTextPart(part) && part.text === feedback) + ) { + return; + } + reviewSent = true; + if (mode === "json") { + writeRawStdout( + `${JSON.stringify({ ...completionEvent, review: { requested: true, sent: true } })}\n`, + ); + } + }) + : undefined; + try { + await session.prompt(feedback, { expandPromptTemplates: false }); + } finally { + unsubscribeReview?.(); + } assistantFailure ??= getAssistantFailure(session.state.messages.at(-1)); } } diff --git a/packages/coding-agent/src/step/tool-profile.ts b/packages/coding-agent/src/step/tool-profile.ts index c28e01ea..37d4ce7e 100644 --- a/packages/coding-agent/src/step/tool-profile.ts +++ b/packages/coding-agent/src/step/tool-profile.ts @@ -23,6 +23,7 @@ import type { ToolRenderContext, ToolRenderResultOptions, } from "../core/extensions/types.ts"; +import { ShellToolError } from "../core/tools/bash.ts"; import type { EditToolOptions } from "../core/tools/edit.ts"; import { generateDiffString, generateUnifiedPatch, normalizeToLF } from "../core/tools/edit-diff.ts"; import { withFileMutationQueue } from "../core/tools/file-mutation-queue.ts"; @@ -79,6 +80,7 @@ const STEP_NATIVE_TOOL_NAMES = new Set(["read", "bash", "edit", "write", "grep", const PATH_DESCRIPTION = "Relative paths resolve from the initial working directory; absolute paths and ~/ home paths are accepted."; const RUN_COMMAND_CWD_DESCRIPTION = `Working directory. ${PATH_DESCRIPTION}`; +const DEFAULT_COMMAND_TIMEOUT_MS = 120_000; const READ_FILE_DESCRIPTION = "Read a text file with optional line range; image files (PNG/JPEG/GIF/WebP) are returned as attached images. Prefer this over shell cat for token efficiency."; const WRITE_FILE_DESCRIPTION = @@ -130,7 +132,13 @@ const editFileSchema = Type.Object({ const runCommandSchema = Type.Object({ command: Type.String({ description: "Shell command string" }), cwd: Type.Optional(Type.String({ description: RUN_COMMAND_CWD_DESCRIPTION })), - timeout_ms: Type.Optional(Type.Integer({ minimum: 1000, maximum: 600000 })), + timeout_ms: Type.Optional( + Type.Integer({ + minimum: 1000, + maximum: 600000, + description: `Foreground timeout in milliseconds. Defaults to ${DEFAULT_COMMAND_TIMEOUT_MS}.`, + }), + ), max_output_chars: Type.Optional( Type.Integer({ minimum: 200, maximum: 120000, description: "Output character cap for stdout+stderr" }), ), @@ -257,6 +265,44 @@ function withStepTextLimit( return { text: `${prefix}${body}${compatibilitySuffix}`, truncated: true }; } +/** Keep recovery metadata intact when limiting either successful or failed commands. */ +function limitRunCommandText( + text: string, + maxChars: number, + fullOutputPath: string | undefined, +): { text: string; truncated: boolean } { + if (!fullOutputPath || text.length <= maxChars) return { text, truncated: false }; + let body = text; + let suffix = ""; + const status = + /(?:^|\n\n)(Command (?:aborted|timed out after [^\n]+|exited with code [^\n]+|terminated by signal [^\n]+))$/u.exec( + body, + ); + if (status) { + suffix = `\n\n${status[1]}`; + body = body.slice(0, status.index); + } + const notice = /\n\n\[(?:Showing [^\n]*\. )?Full output: ([^\n]+)\]$/u.exec(body); + // This also preserves all diagnostics when additional log persistence failed. + if (!notice || notice[1] !== fullOutputPath) return { text, truncated: false }; + suffix = `\n\n[Full output: ${notice[1]}]${suffix}`; + body = body.slice(0, notice.index); + const prefix = maxChars < 512 ? "[Output truncated]\n" : `${STEP_TRUNCATION_HINTS.run_command.banner}\n\n`; + // A path/status longer than the cap must remain usable, even without inline output. + const remaining = Math.max(0, maxChars - prefix.length - suffix.length); + let limitedBody = ""; + if (body.length <= remaining) { + limitedBody = body; + } else if (remaining >= 5) { + const head = Math.ceil((remaining - 5) * 0.7); + const tail = remaining - 5 - head; + limitedBody = `${body.slice(0, head)}\n...\n${tail > 0 ? body.slice(-tail) : ""}`; + } else if (remaining > 0) { + limitedBody = body.slice(-remaining); + } + return { text: `${prefix}${limitedBody}${suffix}`, truncated: true }; +} + /** Preserve the historical read_file suffix used by clients that display caps verbatim. */ function withReadTextLimit(text: string, maxChars: number): { text: string; truncated: boolean } { if (text.length <= maxChars) return { text, truncated: false }; @@ -268,7 +314,14 @@ function withReadTextLimit(text: string, maxChars: number): { text: string; trun function applyStepTextLimit(result: AnyResult, toolName: StepTruncationToolName, maxChars: number): AnyResult { const text = textFromResult(result); - const limited = withStepTextLimit(toolName, text, maxChars); + const limited = + toolName === "run_command" + ? limitRunCommandText( + text, + maxChars, + typeof result.details?.fullOutputPath === "string" ? result.details.fullOutputPath : undefined, + ) + : withStepTextLimit(toolName, text, maxChars); if (!limited.truncated) return result; return { ...result, @@ -1012,10 +1065,11 @@ function mapRunCommandArgs(args: RunCommandInput, ctx?: ExtensionContext): { com throw new Error(`max_output_chars must be between ${MIN_COMMAND_OUTPUT_CHARS} and ${MAX_COMMAND_OUTPUT_CHARS}`); } const configuredTimeout = getContextValue(ctx, "commandTimeoutMs"); - const timeoutMs = args.timeout_ms ?? (typeof configuredTimeout === "number" ? configuredTimeout : undefined); + const timeoutMs = + args.timeout_ms ?? (typeof configuredTimeout === "number" ? configuredTimeout : DEFAULT_COMMAND_TIMEOUT_MS); return { command: args.command, - timeout: timeoutMs === undefined ? undefined : timeoutMs / 1000, + timeout: args.run_in_background === true ? undefined : timeoutMs / 1000, }; } @@ -1408,10 +1462,22 @@ export function createStepToolProfile(cwd: string, options: StepToolProfileOptio agentDir, }); } - const nativeForCwd = - args.cwd === undefined ? nativeBash : createBashToolDefinition(commandCwd, nativeBashOptions); - const result = await nativeForCwd.execute(toolCallId, mapRunCommandArgs(args, ctx), signal, onUpdate, ctx); - return applyStepTextLimit(result, "run_command", resolveStepMaxChars(args.max_output_chars, ctx)); + const nativeArgs = mapRunCommandArgs(args, ctx); + const maxChars = resolveStepMaxChars(args.max_output_chars, ctx); + const nativeForCwd = createBashToolDefinition(args.cwd === undefined ? cwd : commandCwd, { + ...nativeBashOptions, + persistOutputAboveChars: maxChars, + }); + try { + const result = await nativeForCwd.execute(toolCallId, nativeArgs, signal, onUpdate, ctx); + 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 error; + } }, renderCall: nativeBash.renderCall ? (args, theme, context) => diff --git a/packages/coding-agent/src/utils/shell.ts b/packages/coding-agent/src/utils/shell.ts index bf72fac4..7a27800a 100644 --- a/packages/coding-agent/src/utils/shell.ts +++ b/packages/coding-agent/src/utils/shell.ts @@ -1,4 +1,4 @@ -import { existsSync } from "node:fs"; +import { existsSync, readdirSync, readFileSync } from "node:fs"; import { delimiter, join } from "node:path"; import { type ChildProcess, spawn, spawnSync } from "child_process"; import { getBinDir } from "../config.ts"; @@ -238,6 +238,51 @@ export function killTrackedDetachedChildren(): void { trackedDetachedChildPids.clear(); } +type LinuxProcessIdentity = { pid: number; ppid: number; startTime: string }; + +function readLinuxProcessIdentity(pid: number): LinuxProcessIdentity | undefined { + try { + const stat = readFileSync(`/proc/${pid}/stat`, "utf8"); + const fields = stat.slice(stat.lastIndexOf(")") + 2).split(" "); + const startTime = fields[19]; + return startTime ? { pid, ppid: Number(fields[1]), startTime } : undefined; + } catch { + return undefined; + } +} + +/** Snapshot descendants before killing their parent makes them untraceable. */ +function getLinuxDescendants(pid: number): LinuxProcessIdentity[] { + const children = new Map(); + try { + // /proc//task//children is absent on some Linux kernels. + // The stat parent field is available without that optional interface. + for (const entry of readdirSync("/proc")) { + const childPid = Number(entry); + if (!Number.isSafeInteger(childPid) || childPid <= 0) continue; + const child = readLinuxProcessIdentity(childPid); + if (!child) continue; + const siblings = children.get(child.ppid) ?? []; + siblings.push(child); + children.set(child.ppid, siblings); + } + } catch { + return []; + } + const descendants: LinuxProcessIdentity[] = []; + const pending = [pid]; + const visited = new Set(pending); + for (let index = 0; index < pending.length; index++) { + for (const child of children.get(pending[index]) ?? []) { + if (visited.has(child.pid)) continue; + visited.add(child.pid); + descendants.push(child); + pending.push(child.pid); + } + } + return descendants; +} + /** * Kill a process and all its children (cross-platform) */ @@ -260,6 +305,7 @@ export function killProcessTree(pid: number): void { // Ignore errors if taskkill fails. } } else { + const descendants = process.platform === "linux" ? getLinuxDescendants(pid) : []; // Use SIGKILL on Unix/Linux/Mac try { process.kill(-pid, "SIGKILL"); @@ -271,5 +317,15 @@ export function killProcessTree(pid: number): void { // Process already dead } } + // A detached child can have a different group/session. Target only the + // identities observed in this command's subtree, never all host processes. + for (const descendant of descendants.reverse()) { + if (readLinuxProcessIdentity(descendant.pid)?.startTime !== descendant.startTime) continue; + try { + process.kill(descendant.pid, "SIGKILL"); + } catch { + // The descendant already exited or was killed with the parent group. + } + } } } diff --git a/packages/coding-agent/test/compaction.test.ts b/packages/coding-agent/test/compaction.test.ts index 3fba904c..0e78feee 100644 --- a/packages/coding-agent/test/compaction.test.ts +++ b/packages/coding-agent/test/compaction.test.ts @@ -298,6 +298,20 @@ describe("shouldCompact", () => { }); describe("summary output budget", () => { + it.each([ + { name: "default model budget", modelMax: 65536, reserve: 24576, history: 32000, prefix: 32000 }, + { name: "session default reserve", modelMax: 65536, reserve: 16384, history: 32000, prefix: 32000 }, + { name: "early trigger reserve", modelMax: 65536, reserve: 851968, history: 32000, prefix: 32000 }, + { name: "lower model cap", modelMax: 8192, reserve: 851968, history: 8192, prefix: 8192 }, + { name: "unknown zero budget", modelMax: 0, reserve: 851968, history: 32000, prefix: 32000 }, + { name: "unknown negative budget", modelMax: -1, reserve: 851968, history: 32000, prefix: 32000 }, + { name: "default unknown budget", modelMax: 0, reserve: 24576, history: 19660, prefix: 12288 }, + { name: "small unknown budget", modelMax: -1, reserve: 2000, history: 1600, prefix: 1000 }, + ])("bounds history and prefix output with $name", ({ modelMax, reserve, history, prefix }) => { + expect(pickSummaryMaxTokens({ maxTokens: modelMax }, reserve, 0.8)).toBe(history); + expect(pickSummaryMaxTokens({ maxTokens: modelMax }, reserve, 0.5)).toBe(prefix); + }); + it("DEFAULT_COMPACTION_SETTINGS.reserveTokens is 24576 (bumped from 16384 for rich sessions)", () => { expect(DEFAULT_COMPACTION_SETTINGS.reserveTokens).toBe(24576); }); @@ -324,10 +338,9 @@ describe("summary output budget", () => { expect(pickSummaryMaxTokens({ maxTokens: -1 }, 24576, 0.8)).toBe(19660); }); - it("pickSummaryMaxTokens keeps small-model output caps as the ceiling when they beat the reserve fraction", () => { - // Small model with 8000 output; reserveBudget at 0.5 = floor(0.5 * 24576) = 12288 - // modelBudget = min(8000, 32000) = 8000; smaller than reserveBudget so reserveBudget wins. - expect(pickSummaryMaxTokens({ maxTokens: 8000 }, 24576, 0.5)).toBe(12288); + it("pickSummaryMaxTokens respects a small model cap below the reserve fraction", () => { + // The 12288-token reserve-derived budget must not exceed the model's 8000-token limit. + expect(pickSummaryMaxTokens({ maxTokens: 8000 }, 24576, 0.5)).toBe(8000); }); it("pickSummaryMaxTokens uses the smaller 0.5 fraction for turn-prefix summaries", () => { diff --git a/packages/coding-agent/test/completion-check-args.test.ts b/packages/coding-agent/test/completion-check-args.test.ts index 96d1518e..65f0884e 100644 --- a/packages/coding-agent/test/completion-check-args.test.ts +++ b/packages/coding-agent/test/completion-check-args.test.ts @@ -6,6 +6,7 @@ describe("completion-check CLI options", () => { const parsed = parseArgs(["-p", "task"]); expect(parsed.completionCheck).toBeUndefined(); expect(parsed.completionCheckAttempts).toBeUndefined(); + expect(parsed.completionReview).toBeUndefined(); expect(parsed.diagnostics).toEqual([]); }); @@ -13,6 +14,7 @@ describe("completion-check CLI options", () => { const parsed = parseArgs(["--completion-check", "git-committed", "-p", "first", "second"]); expect(parsed.completionCheck).toBe("git-committed"); expect(parsed.completionCheckAttempts).toBe(2); + expect(parsed.completionReview).toBeUndefined(); expect(parsed.messages).toEqual(["first", "second"]); expect(parsed.unknownFlags.size).toBe(0); expect(parsed.diagnostics).toEqual([]); @@ -68,18 +70,51 @@ describe("completion-check CLI options", () => { ]); }); - it.each([["--mode", "rpc"], ["--sdk-stdio"]])("rejects incompatible mode %j", (...flags) => { - const parsed = parseArgs(["--completion-check", "git-committed", ...flags]); + 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([1, 2, 3])("parses review as a boolean sharing the %i follow-up budget", (attempts) => { + const parsed = parseArgs([ + "--completion-review", + "--completion-check=git-committed", + `--completion-check-attempts=${attempts}`, + "-p", + "first", + "second", + ]); + expect(parsed.completionReview).toBe(true); + expect(parsed.completionCheckAttempts).toBe(attempts); + expect(parsed.messages).toEqual(["first", "second"]); + expect(parsed.unknownFlags.size).toBe(0); + expect(parsed.diagnostics).toEqual([]); + }); + + it.each(["true", "false", "2"])("rejects a value supplied to the review flag: %s", (value) => { + const parsed = parseArgs(["--completion-check=git-committed", `--completion-review=${value}`]); expect(parsed.diagnostics).toContainEqual({ type: "error", - message: "--completion-check is only supported in print or JSON mode", + message: "--completion-review does not take a value", }); }); + it.each([["--mode", "rpc"], ["--sdk-stdio"]])("rejects incompatible mode %j", (...flags) => { + for (const review of [[], ["--completion-review"]]) { + const parsed = parseArgs(["--completion-check", "git-committed", ...review, ...flags]); + expect(parsed.diagnostics).toContainEqual({ + type: "error", + message: "--completion-check is only supported in print or JSON mode", + }); + } + }); + it("leaves arguments after -- as literal user messages", () => { - const parsed = parseArgs(["-p", "--", "--completion-check", "git-committed"]); + const parsed = parseArgs(["-p", "--", "--completion-check", "git-committed", "--completion-review"]); expect(parsed.completionCheck).toBeUndefined(); - expect(parsed.messages).toEqual(["--completion-check", "git-committed"]); + expect(parsed.completionReview).toBeUndefined(); + expect(parsed.messages).toEqual(["--completion-check", "git-committed", "--completion-review"]); }); it("documents opt-in behavior and the follow-up bound in help", () => { @@ -91,6 +126,8 @@ describe("completion-check CLI options", () => { expect(help).toContain("git-committed"); expect(help).toContain("--completion-check-attempts "); expect(help).toContain("1..3 (default: 2)"); + expect(help).toContain("--completion-review"); + expect(help).toContain("shares its follow-up budget"); } 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 13acc6d7..80a006f5 100644 --- a/packages/coding-agent/test/completion-check-cli.test.ts +++ b/packages/coding-agent/test/completion-check-cli.test.ts @@ -50,6 +50,10 @@ function runCli(flags: string[], repository: boolean) { NODE_ENV: "test", FORCE_COLOR: "0", COMPLETION_CHECK_CALL_LOG: callLog, + // 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. + GIT_CEILING_DIRECTORIES: root, }; for (const name of ["PATH", "SystemRoot", "SYSTEMROOT", "WINDIR", "COMSPEC", "PATHEXT"]) { if (process.env[name] !== undefined) env[name] = process.env[name]; @@ -110,6 +114,10 @@ describe("real CLI completion-check dispatch with an offline provider", () => { .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, @@ -118,6 +126,38 @@ describe("real CLI completion-check dispatch with an offline provider", () => { }); }); + 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("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("fails a non-repository before the provider is invoked", () => { const result = runCli(["-p", "--completion-check", "git-committed"], false); expect(result.error).toBeUndefined(); @@ -135,14 +175,19 @@ describe("real CLI completion-check dispatch with an offline provider", () => { expect(result.stdout).toBe("CLI offline final\n"); }); - it.each([["--completion-check-attempts", "4"], ["--mode", "rpc"], ["--sdk-stdio"]])( - "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-check"); - }, - ); + 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-"); + }); }); diff --git a/packages/coding-agent/test/completion-check.test.ts b/packages/coding-agent/test/completion-check.test.ts index 33e178b0..2194e70f 100644 --- a/packages/coding-agent/test/completion-check.test.ts +++ b/packages/coding-agent/test/completion-check.test.ts @@ -178,7 +178,9 @@ describe("read-only git-committed check", () => { describe("completion-check option validation for direct print-mode callers", () => { it("defaults to off or two follow-ups when explicitly enabled", () => { expect(getCompletionCheckAttempts({})).toBeUndefined(); + expect(getCompletionCheckAttempts({ completionReview: false })).toBeUndefined(); expect(getCompletionCheckAttempts({ completionCheck: "git-committed" })).toBe(2); + expect(getCompletionCheckAttempts({ completionCheck: "git-committed", completionReview: true })).toBe(2); }); it.each([0, 4, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY])("rejects invalid bound %s", (attempts) => { @@ -190,4 +192,44 @@ describe("completion-check option validation for direct print-mode callers", () it("rejects attempts without an enabled check", () => { 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", + ); + }); +}); + +describe("completion self-review feedback", () => { + it("reviews visible requirements without inventing a missing Git condition", () => { + const feedback = completionCheckFeedback( + { hasNewCommit: true, hasCommittedChanges: true, trackedDirty: false, untrackedFiles: false }, + true, + true, + ); + expect(feedback).toContain("original visible task"); + expect(feedback).toContain("public interfaces and types"); + expect(feedback).toContain("boundary cases"); + expect(feedback).toContain("final diff"); + expect(feedback).toContain("after the last edit"); + expect(feedback).toContain("Preserve unrelated user changes and respect permission denials."); + expect(feedback).not.toContain("Completion check:"); + }); + + it("combines missing delivery conditions and review in one prompt", () => { + const git = { hasNewCommit: false, hasCommittedChanges: false, trackedDirty: true, untrackedFiles: true }; + const feedback = completionCheckFeedback(git, false, true); + for (const condition of [ + "no new commit since the starting HEAD", + "no committed tree changes from the starting HEAD", + "tracked changes remain", + "unignored untracked files remain", + "final answer text is missing", + ]) { + expect(feedback).toContain(condition); + } + expect(feedback).toContain("Final self-review:"); + expect(completionCheckFeedback(git, false, false)).toBe(completionCheckFeedback(git, false)); + expect(completionCheckFeedback(git, false)).not.toContain("Final self-review:"); + }); }); diff --git a/packages/coding-agent/test/fixtures/command-runtime-recovery.mjs b/packages/coding-agent/test/fixtures/command-runtime-recovery.mjs new file mode 100644 index 00000000..4f1abb0c --- /dev/null +++ b/packages/coding-agent/test/fixtures/command-runtime-recovery.mjs @@ -0,0 +1,40 @@ +import { spawn } from "node:child_process"; +import { appendFileSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const [mode, directory, outputMode] = process.argv.slice(2); + +function identity(pid) { + const stat = readFileSync(`/proc/${pid}/stat`, "utf8"); + const fields = stat.slice(stat.lastIndexOf(")") + 2).split(" "); + return { pid, startTime: fields[19] }; +} + +if (mode === "signal") { + process.stdout.write("test-runner-started\n", () => process.kill(process.pid, outputMode)); +} else if (mode === "child") { + process.stdout.on("error", () => {}); + const interval = setInterval(() => { + appendFileSync(join(directory, "heartbeat"), "tick\n"); + if (outputMode === "stream") process.stdout.write("descendant-diagnostic\n"); + }, 20); + // Independent self-exit bound survives an interrupted or failing test runner. + setTimeout(() => { + clearInterval(interval); + process.exit(0); + }, 8_000); +} else { + const child = spawn(process.execPath, [fileURLToPath(import.meta.url), "child", directory, outputMode], { + detached: true, + stdio: outputMode === "stream" ? ["ignore", "inherit", "ignore"] : "ignore", + }); + child.once("spawn", () => { + writeFileSync(join(directory, "owned-pids.json"), JSON.stringify({ parent: identity(process.pid), child: identity(child.pid) })); + process.stdout.write("owned-child-ready\n"); + }); + setTimeout(() => { + child.kill("SIGKILL"); + process.exit(0); + }, 8_500); +} diff --git a/packages/coding-agent/test/fixtures/compaction-adaptive-models.json b/packages/coding-agent/test/fixtures/compaction-adaptive-models.json new file mode 100644 index 00000000..790da5d7 --- /dev/null +++ b/packages/coding-agent/test/fixtures/compaction-adaptive-models.json @@ -0,0 +1,17 @@ +{ + "providers": { + "openai": { + "baseUrl": "https://compact-study.invalid/v1", + "api": "openai-completions", + "models": [ + { + "id": "harbor-adaptive-fixture", + "name": "harbor-adaptive-fixture", + "api": "openai-completions", + "maxTokens": 65536, + "contextWindow": 1048576 + } + ] + } + } +} diff --git a/packages/coding-agent/test/shell-process-tree.test.ts b/packages/coding-agent/test/shell-process-tree.test.ts new file mode 100644 index 00000000..c06ae891 --- /dev/null +++ b/packages/coding-agent/test/shell-process-tree.test.ts @@ -0,0 +1,77 @@ +import * as fs from "node:fs"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { killProcessTree } from "../src/utils/shell.ts"; + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, readFileSync: vi.fn(actual.readFileSync), readdirSync: vi.fn(actual.readdirSync) }; +}); + +function processStat(pid: number, parent: number, started: string): string { + const fields = Array(22).fill("0"); + fields[0] = "S"; + fields[1] = String(parent); + fields[2] = String(pid); // Every fixture process has a different group. + fields[19] = started; + return `${pid} (fixture (with parentheses)) ${fields.join(" ")}`; +} + +describe.skipIf(process.platform !== "linux")("Linux command descendant cleanup", () => { + beforeEach(() => { + vi.spyOn(process, "kill").mockReturnValue(true); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.mocked(fs.readFileSync).mockReset(); + vi.mocked(fs.readdirSync).mockReset(); + }); + + function installProcessTable(reusedPid?: number): void { + const table = new Map([ + [101, processStat(101, 1, "1001")], + [102, processStat(102, 101, "1002")], + [103, processStat(103, 102, "1003")], + [199, processStat(199, 1, "1099")], + ]); + const reads = new Map(); + // The cast selects readdirSync's string[] overload in this filesystem mock. + vi.mocked(fs.readdirSync).mockReturnValue(["self", ...table.keys()].map(String) as never); + vi.mocked(fs.readFileSync).mockImplementation((path) => { + const pid = Number(/^\/proc\/(\d+)\/stat$/u.exec(String(path))?.[1]); + const count = (reads.get(pid) ?? 0) + 1; + reads.set(pid, count); + if (pid === reusedPid && count > 1) return processStat(pid, 1, "9000"); + const stat = table.get(pid); + if (!stat) throw Object.assign(new Error("process exited"), { code: "ENOENT" }); + return stat; + }); + } + + it("kills nested descendants in separate groups without targeting an unrelated process", () => { + installProcessTable(); + killProcessTree(101); + expect(vi.mocked(process.kill).mock.calls).toEqual([ + [-101, "SIGKILL"], + [103, "SIGKILL"], + [102, "SIGKILL"], + ]); + }); + + it("does not signal a descendant PID whose start time changed after the snapshot", () => { + installProcessTable(102); + killProcessTree(101); + expect(vi.mocked(process.kill).mock.calls).toEqual([ + [-101, "SIGKILL"], + [103, "SIGKILL"], + ]); + }); + + it("retains process-group cleanup when proc discovery is unavailable", () => { + vi.mocked(fs.readdirSync).mockImplementation(() => { + throw Object.assign(new Error("proc unavailable"), { code: "EACCES" }); + }); + killProcessTree(101); + expect(vi.mocked(process.kill).mock.calls).toEqual([[-101, "SIGKILL"]]); + }); +}); diff --git a/packages/coding-agent/test/step-command-output-spool.test.ts b/packages/coding-agent/test/step-command-output-spool.test.ts new file mode 100644 index 00000000..7d71af01 --- /dev/null +++ b/packages/coding-agent/test/step-command-output-spool.test.ts @@ -0,0 +1,113 @@ +import * as fs from "node:fs"; +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Writable } from "node:stream"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createStepToolProfile } from "../src/step/tool-profile.ts"; + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, createWriteStream: vi.fn(actual.createWriteStream) }; +}); + +function textOf(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content.map((block) => (block.type === "text" ? (block.text ?? "") : "")).join("\n"); +} + +describe("Step command output spool recovery", () => { + let directory: string; + const logs = new Set(); + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "step-output-spool-")); + vi.mocked(fs.createWriteStream).mockClear(); + }); + + afterEach(async () => { + for (const path of logs) await rm(path, { force: true }); + logs.clear(); + await rm(directory, { recursive: true, force: true }); + }); + + it.each([false, true])("returns all diagnostics if spooling fails (printed log notice: %s)", async (printNotice) => { + const raw = `${"head".repeat(2_000)}\nSOLE_COPY_DIAGNOSTIC\n${"tail".repeat(2_000)}${printNotice ? "\n\n[Full output: /not-a-real-spool.log]" : ""}`; + vi.mocked(fs.createWriteStream).mockImplementationOnce(() => { + return new Writable({ + write(_chunk, _encoding, callback) { + callback(Object.assign(new Error("disk full"), { code: "ENOSPC" })); + }, + }) as fs.WriteStream; + }); + const tool = createStepToolProfile(directory, { + agentDir: join(directory, "agent"), + bash: { + operations: { + exec: async (_command, _cwd, { onData }) => { + onData(Buffer.from(raw)); + return { exitCode: 7 }; + }, + }, + }, + }).find((candidate) => candidate.name === "run_command")!; + await expect( + tool.execute( + "spool-failed", + { command: "fixture", max_output_chars: 1_000 }, + undefined, + undefined, + undefined as never, + ), + ).rejects.toThrow(`${raw}\n\nCommand exited with code 7`); + expect(fs.createWriteStream).toHaveBeenCalledTimes(1); + }); + + it.each([ + { nativeCap: false, limit: 200, exitCode: 7 }, + { nativeCap: true, limit: 200, exitCode: 7 }, + { nativeCap: true, limit: 1_000, exitCode: 0 }, + ])( + "keeps the full log and status with nativeCap=$nativeCap, limit=$limit, exitCode=$exitCode", + async ({ nativeCap, limit, exitCode }) => { + const bytes = Buffer.from( + `${"head".repeat(nativeCap ? 8_000 : 2_000)}\nMIDDLE_DIAGNOSTIC\n${"tail".repeat(nativeCap ? 8_000 : 2_000)}\nEOF_DIAGNOSTIC\n`, + ); + const tool = createStepToolProfile(directory, { + agentDir: join(directory, "agent"), + bash: { + operations: { + exec: async (_command, _cwd, { onData }) => { + onData(bytes.subarray(0, 8_001)); + onData(bytes.subarray(8_001)); + return { exitCode }; + }, + }, + }, + }).find((candidate) => candidate.name === "run_command")!; + let text: string; + try { + const result = await tool.execute( + "capped", + { command: "fixture", max_output_chars: limit }, + undefined, + undefined, + undefined as never, + ); + expect(exitCode).toBe(0); + text = textOf(result); + } catch (error) { + expect(exitCode).toBe(7); + text = (error as Error).message; + expect(text).toContain("Command exited with code 7"); + } + const fullPath = /Full output: ([^\]\n]+)/u.exec(text)?.[1]; + expect(fullPath).toBeDefined(); + logs.add(fullPath!); + expect(await readFile(fullPath!)).toEqual(bytes); + expect(text).toContain("truncated"); + expect(text.length).toBeLessThanOrEqual(Math.max(limit, fullPath!.length + 100)); + if (limit >= 1_000) expect(text).toContain("EOF_DIAGNOSTIC"); + expect(fs.createWriteStream).toHaveBeenCalledTimes(1); + }, + ); +}); diff --git a/packages/coding-agent/test/step-pi-storage-wrapper.test.ts b/packages/coding-agent/test/step-pi-storage-wrapper.test.ts index 4fc437b6..019a01a3 100644 --- a/packages/coding-agent/test/step-pi-storage-wrapper.test.ts +++ b/packages/coding-agent/test/step-pi-storage-wrapper.test.ts @@ -1,5 +1,5 @@ import { existsSync } from "node:fs"; -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { copyFile, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, test, vi } from "vitest"; @@ -52,7 +52,9 @@ describe("Step Pi storage wrapper", () => { const agentDir = join(root, ".stepcode", "agent"); const binaryPath = join(agentDir, "bin", process.platform === "win32" ? "rg.exe" : "rg"); await mkdir(join(agentDir, "bin"), { recursive: true }); - await writeFile(binaryPath, "placeholder"); + // Managed tools must pass --version before lookup can select them. + // Node provides a runnable cross-platform fixture for this path-only test. + await copyFile(process.execPath, binaryPath); vi.stubEnv("STEP_CODING_AGENT_DIR", agentDir); vi.stubEnv("AI_AGENT", "step"); diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index 1fab41a9..56cc67e7 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -1,13 +1,14 @@ import * as fs from "node:fs"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { homedir, tmpdir } from "node:os"; import path from "node:path"; import { type Component, setKeybindings } from "@step-harness/pi-tui"; -import { afterEach, beforeAll, expect, test, vi } from "vitest"; +import { afterEach, beforeAll, beforeEach, expect, test, vi } from "vitest"; import { createEventBus } from "../src/core/event-bus.ts"; import type { ExtensionAPI, ExtensionContext, ToolDefinition } from "../src/core/extensions/types.ts"; import { KeybindingsManager } from "../src/core/keybindings.ts"; 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 { StepTelemetryReporter } from "../src/step/telemetry.ts"; @@ -59,6 +60,7 @@ const cleanups: Array<() => void> = []; // The review component resolves keys through the global keybindings registry. beforeAll(() => setKeybindings(new KeybindingsManager())); +beforeEach(() => vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", undefined)); afterEach(() => { vi.restoreAllMocks(); @@ -607,10 +609,16 @@ test("plan extension registers only the plan command and its two model tools", ( expect([...harness.tools.keys()]).toEqual(["enter_plan_mode", "exit_plan_mode"]); }); -test.each(["/plan", "--plan", "enter_plan_mode"])( - "%s preserves tools and announces a writable plan path", - async (entry) => { +const planEntryCases = ["/plan", "--plan", "enter_plan_mode"].flatMap((entry) => + [false, true].map((external) => [entry, external] as const), +); + +test.each(planEntryCases)( + "%s preserves tools and announces a writable plan path (external storage: %s)", + async (entry, external) => { const cwd = makeWorkspace(); + const planDir = external ? path.join(makeWorkspace(), "runtime plans") : path.join(cwd, ".stepcode", "plans"); + if (external) vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", planDir); const { telemetry, events } = createTelemetryRecorder(); const harness = createHarness({ cwd, flags: { plan: entry === "--plan" } }); const originalTools = ["read_file", "run_command", "write", "edit", "custom_tool"]; @@ -621,7 +629,8 @@ test.each(["/plan", "--plan", "enter_plan_mode"])( else if (entry === "--plan") await harness.emit({ type: "session_start", reason: "startup" }, context); else await runTool(harness, "enter_plan_mode", {}, context); - const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + const planPath = path.join(planDir, "session-sess-1.md"); + expect(fs.existsSync(planDir)).toBe(false); const source = entry === "enter_plan_mode" ? "agent" : "user"; expect(lastPlanEntry(harness)).toMatchObject({ enabled: true, @@ -642,9 +651,11 @@ test.each(["/plan", "--plan", "enter_plan_mode"])( await expect( harness.emit({ type: "tool_call", toolName, input: { path: path.relative(cwd, planPath) } }, context), ).resolves.toBeUndefined(); - await expect( - harness.emit({ type: "tool_call", toolName, input: { path: "src/app.ts" } }, context), - ).resolves.toMatchObject({ block: true }); + for (const target of ["src/app.ts", ".stepcode/config.toml", path.join(planDir, "other-plan.md")]) { + await expect( + harness.emit({ type: "tool_call", toolName, input: { path: target } }, context), + ).resolves.toMatchObject({ block: true }); + } } await expect( harness.emit({ type: "tool_call", toolName: "run_command", input: { command: "git status" } }, context), @@ -685,10 +696,11 @@ test.each(["session_start", "session_tree"] as const)( "%s restores the active sibling's source, path and original tools without telemetry", async (type) => { const cwd = makeWorkspace(); + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", path.join(makeWorkspace(), "new-plans")); const { telemetry, events } = createTelemetryRecorder(); const harness = createHarness({ cwd }); const root = harness.session.appendCustomEntry("branch-root"); - const pathA = path.join(cwd, "proposal-a.md"); + const pathA = path.join(cwd, ".stepcode", "plans", "proposal-a.md"); const pathB = path.join(cwd, "proposal-b.md"); const toolsA = ["read_file", "write", "edit", "branch_a_tool"]; const toolsB = ["read_file", "write_file", "branch_b_tool"]; @@ -973,6 +985,55 @@ test("switching to a fresh session clears the preceding plan path and source", a }); }); +test.each([undefined, "", " \t "])("plan storage keeps the legacy default for %j", (value) => { + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", value); + const cwd = makeWorkspace(); + expect(getPlanFilePath("sess-1", cwd)).toBe(path.join(cwd, ".stepcode", "plans", "session-sess-1.md")); + expect(getPlanFilePath("sess-1")).toBe(path.join(process.cwd(), ".stepcode", "plans", "session-sess-1.md")); +}); + +test("plan storage resolves absolute, session-relative and home-relative overrides", () => { + const cwd = path.join(makeWorkspace(), "project"); + const external = path.join(makeWorkspace(), "plans with spaces"); + for (const [value, expected] of [ + [` ${external} `, external], + ["../runtime plans", path.resolve(cwd, "../runtime plans")], + ["~/runtime-plans", path.join(homedir(), "runtime-plans")], + ]) { + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", value); + expect(getPlanFilePath("sess-1", cwd)).toBe(path.join(expected, "session-sess-1.md")); + } +}); + +test("the selected external plan path stays pinned until a fresh session", async () => { + const cwd = makeWorkspace(); + const firstRoot = makeWorkspace(); + const nextRoot = makeWorkspace(); + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", firstRoot); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + await runTool(harness, "enter_plan_mode", {}, harness.ctx()); + const firstPath = path.join(firstRoot, "session-sess-1.md"); + expect(lastPlanEntry(harness).planFilePath).toBe(firstPath); + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", nextRoot); + for (const type of ["session_start", "session_tree"] as const) { + await restoreLifecycle(harness, type); + expect(await runTool(harness, "enter_plan_mode", {}, harness.ctx())).toMatchObject({ + content: [{ text: expect.stringContaining(firstPath) }], + }); + } + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", undefined); + await harness.commands.get("plan")!.handler("", harness.ctx()); + await restoreLifecycle(harness, "session_start"); + await runTool(harness, "enter_plan_mode", {}, harness.ctx()); + expect(lastPlanEntry(harness).planFilePath).toBe(firstPath); + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", nextRoot); + harness.session.newSession({ id: "sess-2" }); + await harness.emit({ type: "session_start", reason: "new" }, harness.ctx()); + await runTool(harness, "enter_plan_mode", {}, harness.ctx()); + expect(lastPlanEntry(harness).planFilePath).toBe(path.join(nextRoot, "session-sess-2.md")); +}); + test.each(["session_start", "session_tree"] as const)( "%s migrates only active-branch legacy todos without duplicate imports", async (type) => { diff --git a/packages/coding-agent/test/step-run-command-timeout.test.ts b/packages/coding-agent/test/step-run-command-timeout.test.ts new file mode 100644 index 00000000..3a403185 --- /dev/null +++ b/packages/coding-agent/test/step-run-command-timeout.test.ts @@ -0,0 +1,227 @@ +import { mkdir, mkdtemp, readFile, rm } 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, ToolRenderContext } from "../src/core/extensions/types.ts"; +import { type BashOperations, createBashTool } from "../src/core/tools/bash.ts"; +import { createStepToolProfile } from "../src/step/tool-profile.ts"; +import { initTheme, theme } from "../src/theme/theme.ts"; +import { killProcessTree } from "../src/utils/shell.ts"; + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +describe("Step foreground run_command timeout", () => { + let directory: string; + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "step-command-timeout-")); + }); + + afterEach(async () => { + await rm(directory, { recursive: true, force: true }); + }); + + function commandTool() { + const exec = vi.fn(async (_command, _cwd, { onData }) => { + onData(Buffer.from("command completed\n")); + return { exitCode: 0 }; + }); + const tool = createStepToolProfile(directory, { + agentDir: join(directory, "agent"), + bash: { operations: { exec } }, + }).find((candidate) => candidate.name === "run_command"); + if (!tool) throw new Error("Step run_command tool is missing"); + return { tool, exec }; + } + + it("defaults an omitted foreground timeout to 120 seconds, including a cwd override", async () => { + // Regression: without the Step fallback a foreground command can consume + // the whole outer run budget without returning a recoverable tool error. + // Inspect the native operation arguments; never wait for the 120s timer. + const { tool, exec } = commandTool(); + await mkdir(join(directory, "nested")); + await tool.execute("default", { command: "fixture" }, undefined, undefined, undefined as never); + await tool.execute( + "default-with-cwd", + { command: "fixture", cwd: "nested" }, + undefined, + undefined, + undefined as never, + ); + + expect(exec.mock.calls.map((call) => call[2].timeout)).toEqual([120, 120]); + expect(exec.mock.calls.map((call) => call[1])).toEqual([directory, join(directory, "nested")]); + }); + + it.each([1_000, 1_250, 600_000])("preserves explicit timeout_ms=%i in native seconds", async (timeoutMs) => { + const { tool, exec } = commandTool(); + await tool.execute( + "explicit", + { command: "fixture", timeout_ms: timeoutMs }, + undefined, + undefined, + undefined as never, + ); + + expect(exec).toHaveBeenCalledTimes(1); + expect(exec.mock.calls[0]?.[2].timeout).toBe(timeoutMs / 1000); + }); + + it("uses an existing numeric host context timeout before the Step default", async () => { + const { tool, exec } = commandTool(); + const context = { cwd: directory, commandTimeoutMs: 180_500 } as unknown as ExtensionContext; + await tool.execute("context", { command: "fixture" }, undefined, undefined, context); + + expect(exec.mock.calls[0]?.[2].timeout).toBe(180.5); + }); + + it("lets an explicit timeout override the host context", async () => { + const { tool, exec } = commandTool(); + const context = { cwd: directory, commandTimeoutMs: 2_000 } as unknown as ExtensionContext; + await tool.execute( + "explicit-over-context", + { command: "fixture", timeout_ms: 240_000 }, + undefined, + undefined, + context, + ); + + expect(exec.mock.calls[0]?.[2].timeout).toBe(240); + }); + + it("resolves a relative cwd against the host context without losing its timeout", async () => { + const { tool, exec } = commandTool(); + const hostCwd = join(directory, "host"); + await mkdir(join(hostCwd, "nested"), { recursive: true }); + const context = { cwd: hostCwd, commandTimeoutMs: 45_000 } as unknown as ExtensionContext; + await tool.execute("cwd-context", { command: "fixture", cwd: "nested" }, undefined, undefined, context); + + expect(exec.mock.calls[0]?.[1]).toBe(join(hostCwd, "nested")); + expect(exec.mock.calls[0]?.[2].timeout).toBe(45); + }); + + it("keeps an absolute cwd and explicit timeout independent of host defaults", async () => { + const { tool, exec } = commandTool(); + const absoluteCwd = join(directory, "absolute"); + await mkdir(absoluteCwd); + const context = { cwd: join(directory, "host"), commandTimeoutMs: 5_000 } as unknown as ExtensionContext; + await tool.execute( + "cwd-explicit", + { command: "fixture", cwd: absoluteCwd, timeout_ms: 90_000 }, + undefined, + undefined, + context, + ); + + expect(exec.mock.calls[0]?.[1]).toBe(absoluteCwd); + expect(exec.mock.calls[0]?.[2].timeout).toBe(90); + }); + + it.each([0, -1, 999, 600_001, 1_000.5, Number.NaN, Number.POSITIVE_INFINITY])( + "rejects invalid explicit timeout_ms=%s before executing, even with a valid host default", + async (timeoutMs) => { + const { tool, exec } = commandTool(); + const context = { cwd: directory, commandTimeoutMs: 120_000 } as unknown as ExtensionContext; + await expect( + tool.execute( + "invalid", + { command: "must not execute", timeout_ms: timeoutMs }, + undefined, + undefined, + context, + ), + ).rejects.toThrow("timeout_ms must be between 1000 and 600000"); + expect(exec).not.toHaveBeenCalled(); + }, + ); + + it("retains native Bash's omitted timeout and fractional seconds contract", async () => { + const exec = vi.fn(async () => ({ exitCode: 0 })); + const bash = createBashTool(directory, { operations: { exec }, agentDir: join(directory, "agent") }); + await bash.execute("native-omitted", { command: "fixture" }); + await bash.execute("native-explicit", { command: "fixture", timeout: 0.125 }); + + expect(exec.mock.calls.map((call) => call[2].timeout)).toEqual([undefined, 0.125]); + }); + + it("does not render a foreground deadline for an omitted-timeout background call or result", () => { + const { tool } = commandTool(); + initTheme("dark", false); + const args = { command: "fixture-command", run_in_background: true }; + const context: ToolRenderContext, typeof args> = { + args, + toolCallId: "render-background", + invalidate: () => {}, + lastComponent: undefined, + state: {}, + cwd: directory, + executionStarted: false, + argsComplete: true, + isPartial: false, + expanded: false, + showImages: false, + isError: false, + }; + const call = tool.renderCall!(args, theme, context).render(200).join("\n"); + const result = tool.renderResult!( + { content: [{ type: "text", text: "Started background command." }], details: { background: true } }, + { expanded: false, isPartial: false }, + theme, + context, + ) + .render(200) + .join("\n"); + expect(call).toContain("fixture-command"); + expect(call).not.toContain("timeout"); + expect(result).toContain("Started background command."); + expect(result).not.toContain("timeout"); + + // A positive control verifies that this renderer exposes foreground timeouts. + const foreground = { command: "fixture-command", timeout_ms: 5_000 }; + expect( + tool.renderCall!(foreground, theme, { ...context, args: foreground }) + .render(200) + .join("\n"), + ).toContain("timeout 5s"); + }); + + it.skipIf(process.platform === "win32")( + "keeps background commands on their detached lifecycle and ignores foreground timeout_ms", + async () => { + const { tool, exec } = commandTool(); + // The owned process stays alive beyond the explicit 1s foreground limit. + // A self-exit bounds cleanup even if the test runner is interrupted. + const program = [ + 'setTimeout(() => process.stdout.write("background-after-timeout\\n"), 1500);', + "setTimeout(() => process.exit(0), 8000);", + ].join(" "); + const result = await tool.execute( + "background", + { + command: `exec ${shellQuote(process.execPath)} -e ${shellQuote(program)}`, + run_in_background: true, + timeout_ms: 1_000, + }, + undefined, + undefined, + { cwd: directory, commandTimeoutMs: 1_000 } as unknown as ExtensionContext, + ); + const details = result.details as { background: boolean; pid: number; logPath: string }; + try { + expect(details.background).toBe(true); + expect(Number.isSafeInteger(details.pid) && details.pid > 0).toBe(true); + expect(exec).not.toHaveBeenCalled(); + expect(() => process.kill(details.pid, 0)).not.toThrow(); + await expect + .poll(async () => readFile(details.logPath, "utf8"), { timeout: 5_000 }) + .toContain("background-after-timeout"); + } finally { + // Only this test's returned process group is targeted, never a global sweep. + if (Number.isSafeInteger(details.pid) && details.pid > 0) killProcessTree(details.pid); + await rm(details.logPath, { force: true }); + } + }, + ); +}); diff --git a/packages/coding-agent/test/step-session-wrapper.test.ts b/packages/coding-agent/test/step-session-wrapper.test.ts index d6a1c7f3..9cb385a3 100644 --- a/packages/coding-agent/test/step-session-wrapper.test.ts +++ b/packages/coding-agent/test/step-session-wrapper.test.ts @@ -1,5 +1,5 @@ import { chmodSync, existsSync } from "node:fs"; -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { copyFile, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, test, vi } from "vitest"; @@ -112,7 +112,9 @@ describe("Step Pi storage wrappers", () => { const agentDir = join(root, ".stepcode", "agent"); const rgPath = join(agentDir, "bin", process.platform === "win32" ? "rg.exe" : "rg"); await mkdir(join(agentDir, "bin"), { recursive: true }); - await writeFile(rgPath, "placeholder"); + // Managed tools must pass --version before lookup can select them. + // Node provides a runnable cross-platform fixture for this path-only test. + await copyFile(process.execPath, rgPath); expect(getToolPath("rg", agentDir)).toBe(rgPath); expect(getToolPath("rg", agentDir)).not.toContain(`${join(root, ".pi")}/`); diff --git a/packages/coding-agent/test/suite/completion-check.test.ts b/packages/coding-agent/test/suite/completion-check.test.ts index 04bf9b05..898eec29 100644 --- a/packages/coding-agent/test/suite/completion-check.test.ts +++ b/packages/coding-agent/test/suite/completion-check.test.ts @@ -13,6 +13,15 @@ import { createHarness, getUserTexts, type Harness, type HarnessOptions } from " const harnesses: Harness[] = []; let stdout = ""; +function getCompletionEvents(): Record[] { + return stdout + .trim() + .split("\n") + .filter(Boolean) + .map((line) => JSON.parse(line) as Record) + .filter((event) => event.type === "completion_check"); +} + function git(cwd: string, ...args: string[]): string { return execFileSync( "git", @@ -268,32 +277,47 @@ describe("runPrintMode same-session completion check", () => { expect(stdout).toBe("final answer\n"); }); - it.each(["error", "aborted"] as const)("never prompts again after assistant %s", async (stopReason) => { - const { harness, run } = await setup(); - harness.setResponses([ - fauxAssistantMessage("", { stopReason, errorMessage: "terminal failure" }), - fauxAssistantMessage("must not be used"), - ]); - expect(await run({ messages: ["pending user prompt"] })).toBe(1); - expect(harness.faux.state.callCount).toBe(1); - expect(getUserTexts(harness)).toHaveLength(1); - expect(console.error).toHaveBeenCalledWith("terminal failure"); - }); + it.each([ + { stopReason: "error", completionReview: false }, + { stopReason: "error", completionReview: true }, + { stopReason: "aborted", completionReview: false }, + { stopReason: "aborted", completionReview: true }, + ] as const)( + "never prompts after assistant $stopReason with review=$completionReview", + async ({ stopReason, completionReview }) => { + const { harness, run } = await setup(); + 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(harness.faux.state.callCount).toBe(1); + expect(getUserTexts(harness)).toHaveLength(1); + expect(console.error).toHaveBeenCalledWith("terminal failure"); + expect(getCompletionEvents()).toEqual([]); + }, + ); - it("does not add completion prompts after an error recovered by native retry", async () => { - const { harness, run } = await setup({ settings: { retry: { enabled: true, maxRetries: 1, baseDelayMs: 1 } } }); - harness.setResponses([ - fauxAssistantMessage("", { stopReason: "error", errorMessage: "overloaded_error" }), - fauxAssistantMessage("native retry recovered"), - fauxAssistantMessage("must not be used"), - ]); - expect(await run()).toBe(0); - expect(harness.faux.state.callCount).toBe(2); - expect(getUserTexts(harness)).toHaveLength(1); - expect(harness.getPendingResponseCount()).toBe(1); - }); + it.each([false, true])( + "does not add prompts after native retry recovery with review=%s", + async (completionReview) => { + const { harness, run } = await setup({ + settings: { retry: { enabled: true, maxRetries: 1, baseDelayMs: 1 } }, + }); + harness.setResponses([ + fauxAssistantMessage("", { stopReason: "error", errorMessage: "overloaded_error" }), + fauxAssistantMessage("native retry recovered"), + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", completionReview })).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(getUserTexts(harness)).toHaveLength(1); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getCompletionEvents()).toEqual([]); + }, + ); - it("never follows a terminating permission denial", async () => { + 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", @@ -314,7 +338,7 @@ describe("runPrintMode same-session completion check", () => { fauxAssistantMessage(fauxToolCall("blocked_tool", {}), { stopReason: "toolUse" }), fauxAssistantMessage("must not be used"), ]); - expect(await run({ mode: "json", messages: ["pending user prompt"] })).toBe(1); + 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"); @@ -351,18 +375,22 @@ describe("runPrintMode same-session completion check", () => { expect(host.dispose).toHaveBeenCalledTimes(1); }); - it("does not classify a post-model Git failure with final text as infrastructure error", async () => { - const { harness, run } = await setup(); - harness.setResponses([ - () => { - renameSync(join(harness.tempDir, ".git"), join(harness.tempDir, ".git-unavailable")); - return fauxAssistantMessage("task failed, here is the result"); - }, - ]); - expect(await run({ mode: "json" })).toBe(0); - expect(harness.faux.state.callCount).toBe(1); - expect(stdout).toContain('"status":"unavailable"'); - }); + it.each([false, true])( + "retains final-text exit 0 after Git becomes unavailable with review=%s", + async (completionReview) => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + renameSync(join(harness.tempDir, ".git"), join(harness.tempDir, ".git-unavailable")); + return fauxAssistantMessage("task failed, here is the result"); + }, + ]); + expect(await run({ mode: "json", completionReview })).toBe(0); + expect(harness.faux.state.callCount).toBe(1); + expect(stdout).toContain('"status":"unavailable"'); + if (completionReview) expect(getCompletionEvents()[0].review).toEqual({ requested: false, sent: false }); + }, + ); it("keeps JSON history and waits for stdout backpressure before a follow-up", async () => { const { harness, run } = await setup(); @@ -401,22 +429,276 @@ describe("runPrintMode same-session completion check", () => { expect(stdout).toContain('"text":"last"'); }); - it("preserves runtime rebinding and user messages without automatic continuation into another session", async () => { - const first = await setup(); - const second = await setup(); - first.harness.setResponses([fauxAssistantMessage("before replacement")]); - second.harness.setResponses([fauxAssistantMessage("after replacement")]); - const originalPrompt = first.harness.session.prompt.bind(first.harness.session); - vi.spyOn(first.harness.session, "prompt").mockImplementationOnce(async (text, options) => { - await originalPrompt(text, options); - first.host.session = second.harness.session; - first.host.cwd = second.harness.tempDir; - await first.host.setRebindSession.mock.calls[0]?.[0]?.(second.harness.session); - }); - expect(await first.run({ mode: "json", 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"'); - expect(first.host.dispose).toHaveBeenCalledTimes(1); + it.each([false, true])( + "preserves explicit rebinding without automatic continuation with review=%s", + async (completionReview) => { + const first = await setup(); + const second = await setup(); + first.harness.setResponses([fauxAssistantMessage("before replacement")]); + second.harness.setResponses([fauxAssistantMessage("after replacement")]); + const originalPrompt = first.harness.session.prompt.bind(first.harness.session); + vi.spyOn(first.harness.session, "prompt").mockImplementationOnce(async (text, options) => { + await originalPrompt(text, options); + first.host.session = second.harness.session; + 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(getUserTexts(second.harness)).toEqual(["explicit next message"]); + expect(stdout).toContain('"text":"after replacement"'); + expect(stdout).not.toContain('"type":"completion_check"'); + expect(first.host.dispose).toHaveBeenCalledTimes(1); + }, + ); +}); + +describe("opt-in completion self-review", () => { + it.each(["text", "json"] as const)( + "reviews a clean committed result once in the same session/model in %s mode", + async (mode) => { + const { harness, host, run } = await setup(); + const sessionId = harness.session.sessionId; + const model = harness.session.model; + const prompt = vi.spyOn(harness.session, "prompt"); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage("initial final"); + }, + (context) => { + expect(context.messages.some((message) => message.role === "assistant")).toBe(true); + expect(getUserTexts(harness)[1]).toContain("original visible task"); + expect(getUserTexts(harness)[1]).toContain("public interfaces and types"); + expect(getUserTexts(harness)[1]).toContain("after the last edit"); + return fauxAssistantMessage("reviewed final"); + }, + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode, completionReview: true })).toBe(0); + expect(harness.faux.state.callCount).toBe(2); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness)).toHaveLength(2); + expect(prompt.mock.calls[1]?.[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(); + if (mode === "text") { + expect(stdout).toBe("reviewed final\n"); + } else { + const checks = getCompletionEvents(); + 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, "passed", undefined, { requested: true, sent: true }], + ]); + expect(checks[0]).toMatchObject({ + hasNewCommit: true, + hasCommittedChanges: true, + trackedDirty: false, + hasFinalText: true, + }); + } + }, + ); + + it("combines review with dirty repair and uses only the shared two-follow-up budget", async () => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + writeFileSync(join(harness.tempDir, "source.txt"), "unfinished task\n"); + return fauxAssistantMessage("initial final"); + }, + fauxAssistantMessage("reviewed, but work remains"), + () => { + commitTask(harness); + return fauxAssistantMessage("fixed and verified"); + }, + fauxAssistantMessage("must not be used"), + ]); + expect(await run({ mode: "json", completionReview: true, completionCheckAttempts: 2 })).toBe(0); + expect(harness.faux.state.callCount).toBe(3); + expect(harness.getPendingResponseCount()).toBe(1); + const users = getUserTexts(harness); + expect(users).toHaveLength(3); + expect(users[1]).toContain("Final self-review:"); + expect(users[1]).toContain("tracked changes remain"); + expect(users[1]).toContain("no new commit since the starting HEAD"); + expect(users[2]).toContain("tracked changes remain"); + expect(users[2]).not.toContain("Final self-review:"); + expect(getCompletionEvents().map((event) => [event.attempt, event.status, event.followUpKind])).toEqual([ + [0, "follow_up", "review"], + [0, "follow_up", "review"], + [1, "follow_up", "completion"], + [2, "passed", undefined], + ]); + expect(git(harness.tempDir, "status", "--porcelain")).toBe(""); }); + + it.each([1, 2, 3])( + "does not extend a budget of %i or resample an incomplete result with final text", + async (attempts) => { + const { harness, host, run } = await setup(); + const head = git(harness.tempDir, "rev-parse", "HEAD"); + harness.setResponses(Array.from({ length: attempts + 2 }, () => fauxAssistantMessage("Unable to finish."))); + expect(await run({ mode: "json", completionReview: true, completionCheckAttempts: attempts })).toBe(0); + expect(harness.faux.state.callCount).toBe(attempts + 1); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness).filter((text) => text.includes("Final self-review:"))).toHaveLength(1); + expect(getCompletionEvents().at(-1)).toMatchObject({ + attempt: attempts, + status: "exhausted", + willFollowUp: false, + }); + expect(git(harness.tempDir, "rev-parse", "HEAD")).toBe(head); + expect(host.newSession).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith(`Completion check incomplete after ${attempts} follow-up(s).`); + }, + ); + + it.each([[], [fauxThinking("reasoning only")], [{ type: "text" as const, text: " \n " }]])( + "keeps missing final-text handling within the shared budget: %j", + async (...content) => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage(content); + }, + fauxAssistantMessage(content), + fauxAssistantMessage(content), + fauxAssistantMessage("unused"), + ]); + expect(await run({ completionReview: true })).toBe(2); + expect(harness.faux.state.callCount).toBe(3); + expect(harness.getPendingResponseCount()).toBe(1); + expect(getUserTexts(harness)[1]).toContain("final answer text is missing"); + expect(getUserTexts(harness)[1]).toContain("Final self-review:"); + expect(getUserTexts(harness)[2]).not.toContain("Final self-review:"); + expect(stdout.trim()).toBe(""); + }, + ); + + it("keeps explicit user prompts before the single review", async () => { + const { harness, run } = await setup(); + harness.setResponses([ + fauxAssistantMessage("first"), + () => { + commitTask(harness); + return fauxAssistantMessage("second"); + }, + fauxAssistantMessage("reviewed"), + ]); + expect(await run({ completionReview: true, messages: ["Explicit extra requirement."] })).toBe(0); + expect(getUserTexts(harness).slice(0, 2)).toEqual([ + "Complete the task and commit the changes.", + "Explicit extra requirement.", + ]); + expect(getUserTexts(harness)[2]).toContain("Final 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 () => { + const { harness, run } = await setup(); + harness.setResponses([ + () => { + commitTask(harness); + return fauxAssistantMessage("initial final"); + }, + ]); + const originalPrompt = harness.session.prompt.bind(harness.session); + vi.spyOn(harness.session, "prompt") + .mockImplementationOnce(originalPrompt) + .mockRejectedValueOnce(new Error("review preflight failed")); + expect(await run({ mode: "json", completionReview: true })).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 } }, + }); + 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", completionReview: true })).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(["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"); + }, + 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", 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 }); + }, + ); }); diff --git a/packages/coding-agent/test/suite/plan-storage.test.ts b/packages/coding-agent/test/suite/plan-storage.test.ts new file mode 100644 index 00000000..4e3d9b38 --- /dev/null +++ b/packages/coding-agent/test/suite/plan-storage.test.ts @@ -0,0 +1,200 @@ +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { afterEach, beforeEach, expect, test, vi } from "vitest"; +import type { AgentSessionRuntimeHost } from "../../src/core/agent-session-runtime.ts"; +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 { readStepConfig, resolveStepConfigPath } from "../../src/step/config-toml.ts"; +import { createStepToolProfile } from "../../src/step/tool-profile.ts"; +import { initTheme } from "../../src/theme/theme.ts"; +import { createHarness, getMessageText, getUserTexts, type Harness } from "./harness.ts"; + +const harnesses: Harness[] = []; +const roots: string[] = []; +let stdout = ""; + +function git(cwd: string, ...args: string[]): string { + return execFileSync("git", ["--no-optional-locks", ...args], { + cwd, + encoding: "utf8", + timeout: 10_000, + stdio: ["ignore", "pipe", "pipe"], + }); +} + +async function setup(external: boolean) { + const storage = mkdtempSync(join(tmpdir(), "step-plan-storage-")); + roots.push(storage); + const harness = await createHarness({ + settings: { compaction: { enabled: false }, retry: { enabled: false } }, + extensionFactories: [ + (pi) => { + // Step file tools take their working directory from the real session context. + for (const tool of createStepToolProfile(process.cwd(), { agentDir: join(storage, "agent") })) { + if (["read_file", "write_file", "edit_file"].includes(tool.name)) pi.registerTool(tool); + } + }, + createStepPlanExtension(), + ], + }); + harnesses.push(harness); + const cwd = harness.tempDir; + // Reuse existing history without creating commits; sparse checkout keeps the + // fixture small. Git still reads .gitignore from its index, so the project + // override below uses a directory that this repository does not ignore. + const source = fileURLToPath(new URL("../../../../", import.meta.url)); + git(cwd, "clone", "--quiet", "--shared", "--no-checkout", "--template=", source, "."); + git(cwd, "config", "core.excludesFile", join(cwd, ".git", "info", "exclude")); + git(cwd, "sparse-checkout", "set", "--no-cone", "/package.json"); + git(cwd, "checkout", "--quiet", "--detach", "HEAD"); + expect(git(cwd, "status", "--porcelain")).toBe(""); + expect(existsSync(join(cwd, ".gitignore"))).toBe(false); + + const planDir = external ? join(storage, "runtime plans") : join(cwd, "runtime-plans"); + vi.stubEnv("STEP_CODING_AGENT_PLAN_DIR", planDir); + const planPath = join(planDir, `session-${harness.session.sessionId}.md`); + harness.setResponses([ + fauxAssistantMessage([fauxToolCall("enter_plan_mode", {})]), + fauxAssistantMessage([ + fauxToolCall("write_file", { path: planPath, content: "# Proposal\nCheck the parser.\n" }), + ]), + fauxAssistantMessage([ + fauxToolCall("edit_file", { + path: planPath, + search: "Check the parser.", + replace: "Check the parser and tests.", + }), + ]), + fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]), + fauxAssistantMessage("Done."), + fauxAssistantMessage("Done."), + ]); + const host = { + session: harness.session, + cwd, + setRebindSession: vi.fn(), + newSession: vi.fn(async () => ({ cancelled: false })), + fork: vi.fn(async () => ({ cancelled: false })), + switchSession: vi.fn(async () => ({ cancelled: false })), + dispose: async () => { + await harness.session.abort(); + await harness.session.extensionRunner.emit({ type: "session_shutdown", reason: "quit" }); + harness.session.dispose(); + }, + }; + const run = () => + runPrintMode(host as unknown as AgentSessionRuntimeHost, { + mode: "json", + initialMessage: "Plan the change, implement it, and finish with committed work.", + completionCheck: "git-committed", + completionCheckAttempts: 1, + }); + return { harness, cwd, planPath, run }; +} + +beforeEach(() => { + stdout = ""; + initTheme("dark"); + vi.spyOn(output, "writeRawStdout").mockImplementation((chunk) => { + stdout += chunk; + }); + vi.spyOn(output, "waitForRawStdoutBackpressure").mockResolvedValue(); + vi.spyOn(output, "flushRawStdout").mockResolvedValue(); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(() => { + for (const harness of harnesses.splice(0)) harness.cleanup(); + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); + vi.restoreAllMocks(); +}); + +test.each([false, true])( + "native planning keeps Git completion sensitive to plan location (external: %s)", + async (external) => { + const { harness, cwd, planPath, run } = await setup(external); + expect(await run()).toBe(0); + const tools = harness.eventsOfType("tool_execution_end"); + expect(tools.map((event) => [event.toolName, event.isError])).toEqual([ + ["enter_plan_mode", false], + ["write_file", false], + ["edit_file", false], + ["exit_plan_mode", false], + ]); + 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(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. + expect(existsSync(join(cwd, ".stepcode"))).toBe(false); + const checks = stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line) as Record) + .filter((event) => event.type === "completion_check"); + // The checkout already has committed work; this fixture makes no new commit. + // The check must still require one, including when external plans keep it clean. + expect(checks[0]).toMatchObject({ + hasNewCommit: false, + hasCommittedChanges: false, + trackedDirty: false, + untrackedFiles: !external, + hasFinalText: true, + status: "follow_up", + willFollowUp: true, + }); + expect(checks).toHaveLength(2); + expect(checks[1]).toMatchObject({ status: "exhausted", untrackedFiles: !external }); + expect(getUserTexts(harness)[1]).toContain("no new commit since the starting HEAD"); + if (external) { + expect(git(cwd, "status", "--porcelain")).toBe(""); + expect(getUserTexts(harness)[1]).not.toContain("unignored untracked files remain"); + } else { + expect(git(cwd, "status", "--porcelain", "--untracked-files=all")).toBe( + `?? runtime-plans/session-${harness.session.sessionId}.md\n`, + ); + expect(getUserTexts(harness)[1]).toContain("unignored untracked files remain"); + } + }, +); + +test("external planning preserves project settings, tracked plans and untracked user files", async () => { + const { harness, cwd, planPath, run } = await setup(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"); + const userPath = join(cwd, "user-notes.md"); + mkdirSync(join(cwd, ".stepcode", "plans"), { recursive: true }); + writeFileSync(configPath, '# Keep project settings\ntheme = "step-blue"\n'); + writeFileSync(oldPlanPath, "A user-maintained plan.\n"); + writeFileSync(userPath, "Untracked user notes.\n"); + git( + cwd, + "add", + "--sparse", + "--force", + ".stepcode/config.toml", + `.stepcode/plans/session-${harness.session.sessionId}.md`, + ); + const before = git(cwd, "status", "--porcelain", "--untracked-files=all"); + const trackedBefore = git(cwd, "ls-files", "--stage", "--", ".stepcode"); + + expect(await run()).toBe(0); + expect(readFileSync(planPath, "utf8")).toContain("Check the parser and tests."); + expect(readFileSync(configPath, "utf8")).toBe('# Keep project settings\ntheme = "step-blue"\n'); + expect(readFileSync(oldPlanPath, "utf8")).toBe("A user-maintained plan.\n"); + expect(readFileSync(userPath, "utf8")).toBe("Untracked user notes.\n"); + expect(resolveStepConfigPath(process.env, cwd)).toBe(configPath); + expect(readStepConfig(configPath)).toEqual({ theme: "step-blue" }); + expect(git(cwd, "status", "--porcelain", "--untracked-files=all")).toBe(before); + expect(git(cwd, "ls-files", "--stage", "--", ".stepcode")).toBe(trackedBefore); + expect(getUserTexts(harness)[1]).toContain("tracked changes remain"); + expect(getUserTexts(harness)[1]).toContain("unignored untracked files remain"); + expect(existsSync(join(cwd, ".gitignore"))).toBe(false); +}); diff --git a/packages/coding-agent/test/suite/regressions/compaction-adaptive-wire.test.ts b/packages/coding-agent/test/suite/regressions/compaction-adaptive-wire.test.ts new file mode 100644 index 00000000..5aa7ed79 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/compaction-adaptive-wire.test.ts @@ -0,0 +1,214 @@ +import { createHash } from "node:crypto"; +import { readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import type { AssistantMessage, Context, SimpleStreamOptions } from "@step-harness/providers"; +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { streamSimple } from "@step-harness/providers/compat"; +import { afterEach, expect, it, vi } from "vitest"; +import { estimateContextTokens } from "../../../src/core/compaction/compaction.ts"; +import { SUMMARIZATION_SYSTEM_PROMPT } from "../../../src/core/compaction/utils.ts"; +import { createHarness, getMessageText, type Harness } from "../harness.ts"; + +const SYSTEM_SHA256 = "7f4677db342c3991df3ed0ba729c514db1772ef7af4c39155d2a0d87d08b12bb"; +const TEMPLATE_SHA256 = { + history: "4379f7f63f9fbd36f3f273e94b9566d78967e2e938e4b483957bf67266490572", + prefix: "0a355395dcd867cb08c3be3229d31475b3a468c51021489fcca4fab6c967cdb6", + update: "90546e9b55c76b8ac2570b98b0d2849cb3808f102097f52f643b00253edbb333", +}; +const GOAL = "SYNTHETIC_GOAL: preserve the parser API and finish the existing plan."; +const ZERO_USAGE = { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, +}; + +type RequestKind = "main" | keyof typeof TEMPLATE_SHA256; +interface CapturedRequest { + kind: RequestKind; + body: Record; + options: Pick; +} + +let harness: Harness | undefined; +afterEach(() => { + harness?.cleanup(); + harness = undefined; + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +}); + +it("keeps Harbor adaptive main and compaction wire budgets separate without a thinking override", async () => { + // Generated by Harbor StepHarness._build_model_config() with adaptive and + // openai-chat. In particular, reasoning and compat are absent, not overridden. + const modelsJson = JSON.parse( + readFileSync(fileURLToPath(new URL("../../fixtures/compaction-adaptive-models.json", import.meta.url)), "utf8"), + ) as Record; + const h = await createHarness({ + modelsJson, + settings: { + compaction: { enabled: true, reserveTokens: 851968, keepRecentTokens: 20000 }, + retry: { enabled: false }, + }, + }); + harness = h; + await h.authStorage.modify("openai", async () => ({ type: "api_key", key: "offline-fixture-key" })); + const model = h.session.modelRuntime.getModel("openai", "harbor-adaptive-fixture"); + expect(model).toMatchObject({ + api: "openai-completions", + reasoning: false, + contextWindow: 1048576, + maxTokens: 65536, + }); + if (!model) throw new Error("Harbor adaptive fixture model was not loaded"); + expect(model.compat).toBeUndefined(); + expect(h.settingsManager.getDefaultThinkingLevel()).toBeUndefined(); + // The normal model-switch path clamps the CLI default to model capabilities. + // There is no explicit setThinkingLevel("off") or defaultThinkingLevel setting. + await h.session.setModel(model); + expect(h.session.thinkingLevel).toBe("off"); + + vi.stubGlobal("fetch", () => { + throw new Error("All HTTP must use the offline serialization interceptor"); + }); + const requests: CapturedRequest[] = []; + const productionStream = h.session.agent.streamFunction; + h.session.agent.streamFunction = (requestModel, context, options) => { + let kind: RequestKind = "main"; + if (context.systemPrompt === SUMMARIZATION_SYSTEM_PROMPT) { + const text = getMessageText(context.messages[0]); + // Controlled synthetic input: extract the generated static instruction + // suffix to pin it. External auditing must also check the system hash, + // message shape, exact suffix and framing (docs/compaction-integrity.md). + let suffix = text.slice(text.lastIndexOf("\n\n\n") + "\n\n\n".length); + if (suffix.startsWith("\n")) { + kind = "update"; + suffix = suffix.slice(suffix.lastIndexOf("\n\n\n") + "\n\n\n".length); + } else { + kind = suffix.startsWith("The messages above are the PREFIX") ? "prefix" : "history"; + } + expect(createHash("sha256").update(suffix, "utf8").digest("hex")).toBe(TEMPLATE_SHA256[kind]); + expect(Object.hasOwn(options ?? {}, "reasoning")).toBe(false); + expect(context.tools).toBeUndefined(); + } + const fetch: typeof globalThis.fetch = async (input, init) => { + const url = input instanceof Request ? input.url : String(input); + expect(url).toBe("https://compact-study.invalid/v1/chat/completions"); + const raw = input instanceof Request ? await input.clone().text() : String(init?.body); + const body = JSON.parse(raw) as Record; + requests.push({ + kind, + body, + options: { + reasoning: options?.reasoning, + maxTokens: options?.maxTokens, + cacheRetention: options?.cacheRetention, + sessionId: options?.sessionId, + }, + }); + const response = await streamSimple(h.getModel(), context, { ...options, cacheRetention: "none" }).result(); + if (response.stopReason !== "stop") throw new Error(response.errorMessage ?? "Unexpected faux stop reason"); + const content = getMessageText(response); + const common = { + id: `offline-${requests.length}`, + object: "chat.completion.chunk", + created: 1, + model: model.id, + }; + const chunks = [ + { ...common, choices: [{ index: 0, delta: { role: "assistant", content }, finish_reason: null }] }, + { + ...common, + choices: [{ index: 0, delta: {}, finish_reason: "stop" }], + usage: { + prompt_tokens: Math.ceil(JSON.stringify(body.messages).length / 4), + completion_tokens: Math.ceil(content.length / 4), + }, + }, + ]; + return new Response( + `${chunks.map((chunk) => `data: ${JSON.stringify(chunk)}\n\n`).join("")}data: [DONE]\n\n`, + { + status: 200, + headers: { "content-type": "text/event-stream" }, + }, + ); + }; + // Keep the resolved model and native API dispatch intact; inject only fetch. + return productionStream(requestModel, context, { ...options, fetch }); + }; + + h.setResponses([fauxAssistantMessage("Ready for synthetic inspection.")]); + await h.session.prompt(GOAL); + const sessionId = h.session.sessionId; + const syntheticAssistant = (content: AssistantMessage["content"]): AssistantMessage => ({ + ...fauxAssistantMessage(content, { timestamp: 1 }), + api: model.api, + provider: model.provider, + model: model.id, + usage: ZERO_USAGE, + }); + h.sessionManager.appendMessage({ role: "user", content: "Inspect the current synthetic turn.", timestamp: 1 }); + h.sessionManager.appendMessage( + syntheticAssistant([fauxToolCall("read", { path: "synthetic.log" }, { id: "old-read" })]), + ); + h.sessionManager.appendMessage({ + role: "toolResult", + toolCallId: "old-read", + toolName: "read", + isError: false, + content: [{ type: "text", text: "synthetic log row\n".repeat(45000) }], + timestamp: 1, + }); + h.sessionManager.appendMessage(syntheticAssistant([{ type: "text", text: "retained evidence\n".repeat(6000) }])); + h.session.agent.state.messages = h.sessionManager.buildSessionContext().messages; + expect(estimateContextTokens(h.session.messages).tokens).toBeGreaterThan(196608); + const historySummary = (context: Context) => { + expect(JSON.stringify(context.messages)).toContain(GOAL); + return fauxAssistantMessage(`## User Goal\n${GOAL}\n## Next Actions\nContinue the existing plan.`); + }; + h.setResponses([ + historySummary, + fauxAssistantMessage("## Next Actions\nInspect retained evidence and continue."), + fauxAssistantMessage("Continued after automatic compaction."), + ]); + await h.session.prompt("Continue the existing task."); + expect(h.eventsOfType("compaction_end")).toMatchObject([{ reason: "threshold", aborted: false, willRetry: false }]); + expect(JSON.stringify(h.session.messages)).toContain(GOAL); + expect(h.session.sessionId).toBe(sessionId); + + h.sessionManager.appendMessage({ + role: "user", + content: "new retained evidence\n".repeat(5000), + timestamp: Date.now(), + }); + h.session.agent.state.messages = h.sessionManager.buildSessionContext().messages; + h.setResponses([historySummary]); + await h.session.compact(); + h.setResponses([fauxAssistantMessage("Continued after history update.")]); + await h.session.prompt("Continue the existing plan."); + expect(h.getPendingResponseCount()).toBe(0); + expect(requests.map((request) => request.kind)).toEqual(["main", "history", "prefix", "main", "update", "main"]); + expect(createHash("sha256").update(SUMMARIZATION_SYSTEM_PROMPT, "utf8").digest("hex")).toBe(SYSTEM_SHA256); + + for (const { kind, body, options } of requests) { + expect(body.max_completion_tokens).toBe(kind === "main" ? 65536 : 32000); + expect(Object.hasOwn(body, "max_tokens")).toBe(false); + for (const field of ["reasoning_effort", "thinking", "reasoning", "enable_thinking", "thinking_token_budget"]) { + expect(Object.hasOwn(body, field), `${kind}: ${field}`).toBe(false); + } + if (kind !== "main") { + const messages = body.messages as Array<{ role: string; content: unknown }>; + expect(messages.map((message) => message.role)).toEqual(["system", "user"]); + expect(createHash("sha256").update(getMessageText(messages[0]), "utf8").digest("hex")).toBe(SYSTEM_SHA256); + expect(options.cacheRetention).toBe("none"); + expect(Object.hasOwn(body, "tools")).toBe(false); + } + } + // Optional audit output is test-only; normal suite runs write no evidence files. + const evidenceDir = process.env.COMPACT_FIX_EVIDENCE; + if (evidenceDir) writeFileSync(join(evidenceDir, "adaptive-wire-requests.json"), JSON.stringify(requests, null, 2)); +}); diff --git a/packages/coding-agent/test/suite/regressions/step-command-runtime-recovery.test.ts b/packages/coding-agent/test/suite/regressions/step-command-runtime-recovery.test.ts new file mode 100644 index 00000000..713cd009 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/step-command-runtime-recovery.test.ts @@ -0,0 +1,279 @@ +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { fauxAssistantMessage, fauxToolCall, type ToolResultMessage } from "@step-harness/providers"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import type { BashOperations } from "../../../src/core/tools/bash.ts"; +import { createStepToolProfile } from "../../../src/step/tool-profile.ts"; +import { createHarness, getAssistantTexts, getMessageText, type Harness } from "../harness.ts"; + +const fixturePath = fileURLToPath(new URL("../../fixtures/command-runtime-recovery.mjs", import.meta.url)); + +type ProcessIdentity = { pid: number; startTime: string }; +type OwnedProcesses = { parent: ProcessIdentity; child: ProcessIdentity }; + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +async function ownedProcessRunning(identity: ProcessIdentity): Promise { + try { + const stat = await readFile(`/proc/${identity.pid}/stat`, "utf8"); + const fields = stat.slice(stat.lastIndexOf(")") + 2).split(" "); + return fields[19] === identity.startTime && !["Z", "X"].includes(fields[0]); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return false; + throw error; + } +} + +async function stopOwnedProcesses(directory: string): Promise { + let owned: OwnedProcesses; + try { + owned = JSON.parse(await readFile(join(directory, "owned-pids.json"), "utf8")) as OwnedProcesses; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return; + throw error; + } + for (const identity of [owned.child, owned.parent]) { + if (!Number.isSafeInteger(identity.pid) || identity.pid <= 0 || !(await ownedProcessRunning(identity))) continue; + try { + process.kill(identity.pid, "SIGKILL"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + } + } +} + +describe("Step command runtime recovery", () => { + let directory: string; + let harness: Harness | undefined; + const outputPaths = new Set(); + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "step-runtime-recovery-")); + }); + + afterEach(async () => { + if (process.platform === "linux") await stopOwnedProcesses(directory); + await harness?.session.abort(); + harness?.cleanup(); + harness = undefined; + for (const outputPath of outputPaths) await rm(outputPath, { force: true }); + outputPaths.clear(); + await rm(directory, { recursive: true, force: true }); + }); + + async function setup(operations?: BashOperations): Promise { + harness = await createHarness({ + models: [{ id: "runtime-recovery-faux", contextWindow: 1_000_000, maxTokens: 64_000 }], + tools: [], + initialActiveToolNames: ["run_command", "read_file"], + extensionFactories: [ + (pi) => { + const profile = createStepToolProfile(directory, { + agentDir: join(directory, "agent"), + ...(operations ? { bash: { operations } } : {}), + }); + for (const tool of profile) { + if (tool.name === "run_command" || tool.name === "read_file") pi.registerTool(tool); + } + }, + ], + }); + await harness.session.bindExtensions({ mode: "print" }); + return harness; + } + + describe.skipIf(process.platform !== "linux")("separate-group descendants", () => { + it.each(["idle", "stream"])("stops a %s descendant and returns a timeout to the next faux turn", async (mode) => { + const current = await setup(); + const command = `exec ${shellQuote(process.execPath)} ${shellQuote(fixturePath)} parent ${shellQuote(directory)} ${mode}`; + let observed: ToolResultMessage | undefined; + current.setResponses([ + fauxAssistantMessage( + fauxToolCall("run_command", { command, cwd: directory, timeout_ms: 1_000 }, { id: "timed-command" }), + { stopReason: "toolUse" }, + ), + (context) => { + observed = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "timed-command", + ) as ToolResultMessage | undefined; + return fauxAssistantMessage("The timeout was returned."); + }, + ]); + let watchdogFired = false; + const watchdog = setTimeout(() => { + watchdogFired = true; + void current.session.abort(); + }, 4_000); + const startedAt = performance.now(); + try { + await current.session.prompt("Run the bounded local fixture."); + expect(watchdogFired).toBe(false); + expect(performance.now() - startedAt).toBeLessThan(3_000); + expect(current.faux.state.callCount).toBe(2); + expect(observed).toMatchObject({ isError: true, toolName: "run_command" }); + expect(getMessageText(observed)).toContain("owned-child-ready"); + expect(getMessageText(observed)).toContain("Command timed out after 1 seconds"); + expect(getMessageText(observed)).not.toContain("terminated by signal"); + const owned = JSON.parse(await readFile(join(directory, "owned-pids.json"), "utf8")) as OwnedProcesses; + for (const identity of [owned.parent, owned.child]) { + await expect.poll(() => ownedProcessRunning(identity), { timeout: 1_000 }).toBe(false); + } + expect(current.eventsOfType("tool_execution_end")[0]?.result.terminate).not.toBe(true); + } finally { + clearTimeout(watchdog); + await stopOwnedProcesses(directory); + } + }); + }); + + it.skipIf(process.platform !== "linux")( + "keeps caller abort ahead of the kill signal and retains its error for a resumed faux turn", + async () => { + const current = await setup(); + const command = `exec ${shellQuote(process.execPath)} ${shellQuote(fixturePath)} parent ${shellQuote(directory)} stream`; + current.setResponses([ + fauxAssistantMessage( + fauxToolCall("run_command", { command, cwd: directory, timeout_ms: 5_000 }, { id: "aborted-command" }), + { stopReason: "toolUse" }, + ), + ]); + const pending = current.session.prompt("Start the local command."); + try { + await expect + .poll( + () => + current + .eventsOfType("tool_execution_update") + .some((event) => getMessageText(event.partialResult).includes("owned-child-ready")), + { timeout: 2_000 }, + ) + .toBe(true); + await current.session.abort(); + await pending; + let observed: ToolResultMessage | undefined; + current.setResponses([ + (context) => { + observed = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "aborted-command", + ) as ToolResultMessage | undefined; + return fauxAssistantMessage("Resumed after caller cancellation."); + }, + ]); + await current.session.prompt("Inspect the previous command result."); + expect(observed).toMatchObject({ isError: true }); + expect(getMessageText(observed)).toContain("owned-child-ready"); + expect(getMessageText(observed)).toContain("Command aborted"); + expect(getMessageText(observed)).not.toMatch(/timed out|terminated by signal/u); + const owned = JSON.parse(await readFile(join(directory, "owned-pids.json"), "utf8")) as OwnedProcesses; + for (const identity of [owned.parent, owned.child]) { + await expect.poll(() => ownedProcessRunning(identity), { timeout: 1_000 }).toBe(false); + } + } finally { + await stopOwnedProcesses(directory); + await current.session.abort(); + await pending; + } + }, + ); + + describe.skipIf(process.platform === "win32")("signal termination", () => { + it.each(["SIGTERM", "SIGKILL"])( + "reports %s and captured output as an error in the next faux request", + async (signal) => { + const current = await setup(); + const command = `exec ${shellQuote(process.execPath)} ${shellQuote(fixturePath)} signal ${shellQuote(directory)} ${signal}`; + let observed: ToolResultMessage | undefined; + current.setResponses([ + fauxAssistantMessage( + fauxToolCall("run_command", { command, timeout_ms: 1_000 }, { id: "signal-command" }), + { stopReason: "toolUse" }, + ), + (context) => { + observed = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "signal-command", + ) as ToolResultMessage | undefined; + return fauxAssistantMessage("The interrupted test needs attention."); + }, + ]); + await current.session.prompt("Run the local signal fixture."); + expect(current.faux.state.callCount).toBe(2); + expect(observed).toMatchObject({ isError: true }); + expect(getMessageText(observed)).toContain("test-runner-started"); + expect(getMessageText(observed)).toContain(`Command terminated by signal ${signal}`); + expect(current.eventsOfType("tool_execution_end")[0]?.result.terminate).not.toBe(true); + }, + ); + }); + + it.each(["success", "exit", "timeout", "abort"] as const)( + "preserves the sole diagnostic copy below 50KiB on %s before applying the Step cap", + async (outcome) => { + const marker = "SOLE_COPY_DIAGNOSTIC_caf\u00e9"; + const bytes = Buffer.from( + `${"head".repeat(3_000)}\n${marker}\n${"tail".repeat(3_000)}\nEOF_DIAGNOSTIC\n`, + "utf8", + ); + expect(bytes.length).toBeLessThan(50 * 1024); + const operations: BashOperations = { + exec: async (_command, _cwd, { onData }) => { + const split = bytes.indexOf(Buffer.from("\u00e9")) + 1; + onData(bytes.subarray(0, split)); + onData(bytes.subarray(split)); + if (outcome === "timeout") throw new Error("timeout:1"); + if (outcome === "abort") throw new Error("aborted"); + return { exitCode: outcome === "exit" ? 7 : 0 }; + }, + }; + const current = await setup(operations); + let observed: ToolResultMessage | undefined; + let recovered: ToolResultMessage | undefined; + let fullOutputPath: string | undefined; + current.setResponses([ + fauxAssistantMessage( + fauxToolCall("run_command", { command: "fake-test", max_output_chars: 1_000 }, { id: "capped-command" }), + { stopReason: "toolUse" }, + ), + (context) => { + observed = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "capped-command", + ) as ToolResultMessage | undefined; + fullOutputPath = /Full output: ([^\]\n]+)/u.exec(getMessageText(observed))?.[1]; + if (!fullOutputPath) return fauxAssistantMessage("Missing recoverable log."); + outputPaths.add(fullOutputPath); + return fauxAssistantMessage( + fauxToolCall( + "read_file", + { path: fullOutputPath, start_line: 2, end_line: 2 }, + { id: "read-diagnostic" }, + ), + { stopReason: "toolUse" }, + ); + }, + (context) => { + recovered = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "read-diagnostic", + ) as ToolResultMessage | undefined; + return fauxAssistantMessage("Recovered the diagnostic from the saved log."); + }, + ]); + await current.session.prompt("Run the test and inspect the complete log when truncated."); + expect(fullOutputPath).toBeDefined(); + expect(observed?.isError).toBe(outcome !== "success"); + expect(getMessageText(observed).length).toBeLessThanOrEqual(1_000); + expect(getMessageText(observed)).not.toContain(marker); + expect(getMessageText(observed)).toContain("EOF_DIAGNOSTIC"); + if (outcome === "exit") expect(getMessageText(observed)).toContain("Command exited with code 7"); + if (outcome === "timeout") expect(getMessageText(observed)).toContain("Command timed out after 1 seconds"); + if (outcome === "abort") expect(getMessageText(observed)).toContain("Command aborted"); + expect(await readFile(fullOutputPath!)).toEqual(bytes); + expect(getMessageText(recovered)).toContain(marker); + expect(recovered?.isError).toBe(false); + expect(current.faux.state.callCount).toBe(3); + expect(getAssistantTexts(current)).toContain("Recovered the diagnostic from the saved log."); + }, + ); +}); diff --git a/packages/coding-agent/test/suite/regressions/step-run-command-timeout.test.ts b/packages/coding-agent/test/suite/regressions/step-run-command-timeout.test.ts new file mode 100644 index 00000000..c9f18217 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/step-run-command-timeout.test.ts @@ -0,0 +1,203 @@ +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { BashOperations } from "../../../src/core/tools/bash.ts"; +import { createStepExtension } from "../../../src/features/step.ts"; +import { createStepToolProfile } from "../../../src/step/tool-profile.ts"; +import { killProcessTree } from "../../../src/utils/shell.ts"; +import { createHarness, getAssistantTexts, getMessageText, type Harness } from "../harness.ts"; + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +function killRecordedChild(pid: number): void { + try { + process.kill(pid, "SIGKILL"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + } +} + +async function isRunning(pid: number): Promise { + try { + if (process.platform === "linux") { + // A killed descendant may briefly remain a zombie under the host's PID 1. + // That is no longer executing; only examine PIDs recorded by our fixture. + const status = await readFile(`/proc/${pid}/stat`, "utf8"); + return !["Z", "X"].includes(status.slice(status.lastIndexOf(")") + 2).split(" ")[0]); + } + process.kill(pid, 0); + return true; + } catch (error) { + if (["ESRCH", "ENOENT"].includes((error as NodeJS.ErrnoException).code ?? "")) return false; + throw error; + } +} + +describe("Step run_command timeout recovery through the agent loop", () => { + let directory: string; + let harness: Harness | undefined; + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "step-timeout-recovery-")); + }); + + afterEach(async () => { + await harness?.session.abort(); + harness?.cleanup(); + harness = undefined; + await rm(directory, { recursive: true, force: true }); + }); + + it.skipIf(process.platform === "win32")( + "returns a 1s foreground timeout as a failed tool result and completes the next command", + async () => { + harness = await createHarness({ + tools: [], + initialActiveToolNames: ["run_command"], + extensionFactories: [ + (pi) => { + const tool = createStepToolProfile(directory, { agentDir: join(directory, "agent") }).find( + (candidate) => candidate.name === "run_command", + ); + if (!tool) throw new Error("Step run_command tool is missing"); + pi.registerTool(tool); + }, + ], + }); + await harness.session.bindExtensions({ mode: "print" }); + const program = [ + 'const { spawn } = require("node:child_process");', + 'const { writeFileSync } = require("node:fs");', + 'const child = spawn(process.execPath, ["-e", "setTimeout(() => process.exit(0), 8000)"], { stdio: "ignore" });', + 'child.once("spawn", () => {', + 'writeFileSync("owned-pids.json", JSON.stringify({ parent: process.pid, child: child.pid }));', + 'process.stdout.write("owned-child-ready\\n");', + "});", + 'setTimeout(() => { child.kill("SIGKILL"); process.exit(0); }, 8000);', + ].join(" "); + const command = `exec ${shellQuote(process.execPath)} -e ${shellQuote(program)}`; + harness.setResponses([ + fauxAssistantMessage( + fauxToolCall("run_command", { command, cwd: directory, timeout_ms: 1_000 }, { id: "owned-timeout" }), + { stopReason: "toolUse" }, + ), + (context) => { + const failed = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "owned-timeout", + ); + expect(failed).toMatchObject({ toolName: "run_command", isError: true }); + expect(getMessageText(failed)).toContain("owned-child-ready"); + expect(getMessageText(failed)).toContain("Command timed out after 1 seconds"); + return fauxAssistantMessage( + fauxToolCall( + "run_command", + { command: "printf 'next-command-ok\\n'", cwd: directory }, + { id: "after-timeout" }, + ), + { stopReason: "toolUse" }, + ); + }, + (context) => { + const succeeded = context.messages.find( + (message) => message.role === "toolResult" && message.toolCallId === "after-timeout", + ); + expect(succeeded).toMatchObject({ isError: false }); + expect(getMessageText(succeeded)).toContain("next-command-ok"); + return fauxAssistantMessage("continued after the timed-out command"); + }, + ]); + + // A separate safety bound cannot masquerade as the native 1s timeout. + let watchdogFired = false; + const watchdog = setTimeout(() => { + watchdogFired = true; + void harness?.session.abort(); + }, 6_000); + let owned: { parent: number; child: number } | undefined; + try { + await harness.session.prompt("Run the local command and recover from a timeout if necessary."); + expect(watchdogFired).toBe(false); + expect(harness.faux.state.callCount).toBe(3); + expect(harness.getPendingResponseCount()).toBe(0); + expect(getAssistantTexts(harness)).toContain("continued after the timed-out command"); + const ends = harness.eventsOfType("tool_execution_end"); + expect(ends.map((event) => [event.toolCallId, event.isError])).toEqual([ + ["owned-timeout", true], + ["after-timeout", false], + ]); + expect(ends[0]?.result.terminate).not.toBe(true); + owned = JSON.parse(await readFile(join(directory, "owned-pids.json"), "utf8")) as { + parent: number; + child: number; + }; + for (const pid of [owned.parent, owned.child]) { + expect(Number.isSafeInteger(pid) && pid > 0).toBe(true); + await expect.poll(() => isRunning(pid), { timeout: 3_000 }).toBe(false); + } + } finally { + clearTimeout(watchdog); + await harness.session.abort(); + // Recover our own PID receipt even if an assertion failed before it was read. + if (!owned) { + const receipt = await readFile(join(directory, "owned-pids.json"), "utf8").catch(() => ""); + if (receipt) owned = JSON.parse(receipt) as { parent: number; child: number }; + } + if (owned && Number.isSafeInteger(owned.parent) && owned.parent > 0 && (await isRunning(owned.parent))) { + killProcessTree(owned.parent); + } + if (owned && Number.isSafeInteger(owned.child) && owned.child > 0 && (await isRunning(owned.child))) { + // This descendant shares the parent's group, so target its own PID only. + killRecordedChild(owned.child); + } + } + }, + ); + + it("retains a terminating explicit tool deny for bounded commands", async () => { + const exec = vi.fn(async () => ({ exitCode: 0 })); + harness = await createHarness({ + tools: [], + initialActiveToolNames: ["run_command"], + extensionFactories: [ + createStepExtension({ + permission: { + env: {}, + initialPreset: "bypass", + toolOverrides: { run_command: "deny" }, + }, + }), + (pi) => { + const tool = createStepToolProfile(directory, { + agentDir: join(directory, "agent"), + bash: { operations: { exec } }, + }).find((candidate) => candidate.name === "run_command"); + if (!tool) throw new Error("Step run_command tool is missing"); + pi.registerTool(tool); + }, + ], + }); + await harness.session.bindExtensions({ mode: "print" }); + harness.setResponses([ + fauxAssistantMessage( + fauxToolCall("run_command", { command: "printf 'must-not-run\\n'" }, { id: "explicit-deny" }), + { stopReason: "toolUse" }, + ), + fauxAssistantMessage("must not continue after an explicit deny"), + ]); + await harness.session.prompt("Exercise the existing explicit tool deny."); + + expect(exec).not.toHaveBeenCalled(); + expect(harness.faux.state.callCount).toBe(1); + expect(harness.eventsOfType("tool_execution_end")).toEqual([ + expect.objectContaining({ + toolCallId: "explicit-deny", + isError: true, + result: expect.objectContaining({ terminate: true }), + }), + ]); + }); +}); From 544ea303610b224074ba8cb26184a1aa8528a1ca Mon Sep 17 00:00:00 2001 From: MelodyVAR <61931019+MelodyVAR@users.noreply.github.com> Date: Sun, 27 Sep 2026 17:01:32 +0000 Subject: [PATCH 3/4] fix(coding-agent): exercise built CLI in completion tests --- packages/coding-agent/test/completion-check-cli.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/coding-agent/test/completion-check-cli.test.ts b/packages/coding-agent/test/completion-check-cli.test.ts index 80a006f5..17f84f6c 100644 --- a/packages/coding-agent/test/completion-check-cli.test.ts +++ b/packages/coding-agent/test/completion-check-cli.test.ts @@ -6,9 +6,10 @@ import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; const roots: string[] = []; -const cli = fileURLToPath(new URL("../../../apps/cli/src/main.ts", import.meta.url)); +// 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)); const fixture = fileURLToPath(new URL("./fixtures/completion-check-provider.ts", import.meta.url)); -const tsconfig = fileURLToPath(new URL("../../../tsconfig.json", import.meta.url)); function runCli(flags: string[], repository: boolean) { const root = mkdtempSync(join(tmpdir(), "completion-cli-")); @@ -61,9 +62,6 @@ function runCli(flags: string[], repository: boolean) { const result = spawnSync( process.execPath, [ - fileURLToPath(import.meta.resolve("tsx/cli")), - "--tsconfig", - tsconfig, cli, "--provider", "completion-offline", From d8805284e80201750516ee8e4f11fd5ae570c85e Mon Sep 17 00:00:00 2001 From: MelodyVAR <61931019+MelodyVAR@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:04:27 +0000 Subject: [PATCH 4/4] fix(coding-agent): recognize Step file exploration tools in prompts --- CHANGELOG.md | 7 ++++ packages/coding-agent/CHANGELOG.md | 7 ++++ .../coding-agent/src/core/system-prompt.ts | 6 +-- .../coding-agent/test/system-prompt.test.ts | 41 +++++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 packages/coding-agent/CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..577600b5 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,7 @@ +# Changelog + +## [Unreleased] + +### Fixed + +- Recognize `search_files`, `find_files`, and `list_directory` when selecting default system-prompt file-exploration guidance. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md new file mode 100644 index 00000000..577600b5 --- /dev/null +++ b/packages/coding-agent/CHANGELOG.md @@ -0,0 +1,7 @@ +# Changelog + +## [Unreleased] + +### Fixed + +- Recognize `search_files`, `find_files`, and `list_directory` when selecting default system-prompt file-exploration guidance. diff --git a/packages/coding-agent/src/core/system-prompt.ts b/packages/coding-agent/src/core/system-prompt.ts index e131c59d..bd5f0018 100644 --- a/packages/coding-agent/src/core/system-prompt.ts +++ b/packages/coding-agent/src/core/system-prompt.ts @@ -150,9 +150,9 @@ export function buildSystemPrompt(options: BuildSystemPromptOptions): string { const hasBash = tools.includes("bash") || tools.includes("run_command"); const hasPowerShell = tools.includes("powershell"); - const hasGrep = tools.includes("grep"); - const hasFind = tools.includes("find"); - const hasLs = tools.includes("ls"); + const hasGrep = tools.includes("grep") || tools.includes("search_files"); + const hasFind = tools.includes("find") || tools.includes("find_files"); + const hasLs = tools.includes("ls") || tools.includes("list_directory"); const hasRead = tools.includes("read") || tools.includes("read_file"); // File exploration guidelines diff --git a/packages/coding-agent/test/system-prompt.test.ts b/packages/coding-agent/test/system-prompt.test.ts index 8e4c4f20..ac4c1f4f 100644 --- a/packages/coding-agent/test/system-prompt.test.ts +++ b/packages/coding-agent/test/system-prompt.test.ts @@ -48,8 +48,11 @@ describe("buildSystemPrompt", () => { }); test.each([ + [["bash"], "Use bash for file operations"], + [["run_command"], "Use bash for file operations"], [["powershell"], "Use PowerShell for file operations"], [["bash", "powershell"], "Use bash or PowerShell for file operations"], + [["run_command", "powershell"], "Use bash or PowerShell for file operations"], ] as const)("uses shell-specific guidance for %j", (selectedTools, expected) => { const prompt = buildSystemPrompt({ selectedTools: [...selectedTools], @@ -75,6 +78,44 @@ describe("buildSystemPrompt", () => { }); }); + describe.each(["run_command", "powershell"])("file exploration guidance with %s", (shell) => { + test.each(["search_files", "find_files", "list_directory"])( + "does not add shell fallback when the Step %s alias is available", + (tool) => { + const prompt = buildSystemPrompt({ + selectedTools: [shell, tool], + contextFiles: [], + skills: [], + cwd: process.cwd(), + }); + + expect(prompt).not.toContain("for file operations"); + }, + ); + + test.each(["grep", "find", "ls"])("keeps the legacy %s capability recognized", (tool) => { + const prompt = buildSystemPrompt({ + selectedTools: [shell, tool], + contextFiles: [], + skills: [], + cwd: process.cwd(), + }); + + expect(prompt).not.toContain("for file operations"); + }); + }); + + test("recognizes mixed legacy and Step file exploration tools", () => { + const prompt = buildSystemPrompt({ + selectedTools: ["run_command", "grep", "find_files", "list_directory"], + contextFiles: [], + skills: [], + cwd: process.cwd(), + }); + + expect(prompt).not.toContain("for file operations"); + }); + describe("custom tool snippets", () => { test("includes custom tools in available tools section when promptSnippet is provided", () => { const prompt = buildSystemPrompt({