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, + }); + }); + }); +});