diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index a5a87af2..2ff093a2 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -60,6 +60,7 @@ body: - opencode - copilot - pi + - grok - other / not sure validations: required: true diff --git a/AGENTS.md b/AGENTS.md index b6493a42..6400f9cb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,11 +29,11 @@ Entry point is `src/cli.ts`. It parses flags with commander, resolves config, ha Each agent implements the `Agent` interface in `types.ts` (`name`, async `run(prompt, cwd, options)` returning `{ output, usage }`, optional `close()`). They share two responsibilities: stream stdout, extract a structured `AgentOutput` (`success`, `summary`, `key_changes_made`, `key_learnings`, commit-message fields when configured, plus `should_fully_stop` only when `--stop-when` is active) that matches the schema built by `buildAgentOutputSchema(...)`, and accumulate `TokenUsage`. `factory.ts` picks one based on config. -- `claude.ts` / `codex.ts` / `copilot.ts` / `pi.ts`: spawn the CLI per iteration in non-interactive mode. Codex uses `--output-schema` pointing at the run's schema file; Claude uses `--json-schema`, treats the last successful structured result as terminal, raises `PermanentAgentError` for low credit balance exits, and after a short grace period shuts down a lingering Claude process tree if it stays alive. Copilot uses JSONL output plus prompt-level schema instructions, then parses the final `assistant.message` content. Pi runs in JSON mode, appends the final output schema to the prompt, and parses the assistant JSON reply from Pi's streamed events. +- `claude.ts` / `codex.ts` / `copilot.ts` / `pi.ts` / `grok.ts`: spawn the CLI per iteration in non-interactive mode. Codex uses `--output-schema` pointing at the run's schema file; Claude uses `--json-schema`, treats the last successful structured result as terminal, raises `PermanentAgentError` for low credit balance exits, and after a short grace period shuts down a lingering Claude process tree if it stays alive. Copilot uses JSONL output plus prompt-level schema instructions, then parses the final `assistant.message` content. Pi runs in JSON mode, appends the final output schema to the prompt, and parses the assistant JSON reply from Pi's streamed events. Grok uses `-p` with `--output-format streaming-json` and `--json-schema`, defaults to `--always-approve`, and reads `structuredOutput` plus usage from the final `end` event. - `rovodev.ts` / `opencode.ts`: long-running local HTTP servers managed via `managed-process.ts` (start once, reuse across iterations, close on shutdown). OpenCode creates a per-run session and applies a blanket allow rule to avoid prompt blocking. - `acp.ts`: handles `acp:` specs through the bundled `acpx` runtime and registry. It keeps a persistent per-run session keyed by run ID under `.gnhf/runs//acp-sessions`, embeds the output schema in the prompt, parses only output text deltas as final JSON, records ACP lifecycle events in `gnhf.log`, and reports per-iteration token usage from ACP `used` deltas when available with prompt-length plus tool-call estimates as a fallback. Estimated ACP usage is marked for the renderer so totals are prefixed with `~`. Path and arg overrides are native-agent-only; ACP targets are customized via `acpRegistryOverrides` in config (a target-name -> spawn-command map fed into acpx's agent registry) or by passing a raw ACP server command directly after `acp:`. Raw command specs are redacted to `acp:custom`/`custom` in debug logs, errors, and telemetry. The e2e suite exercises the full wire path against the `acp-mock` package registered through that same override mechanism. - `json-extract.ts`: shared recovery for final agent JSON that may be fenced or prose-wrapped; use it before adding ad-hoc parsing to integrations that must validate output against the agent schema. -- `stream-utils.ts`: shared JSONL parsing, `AbortSignal` wiring, and child-process lifecycle helpers. When touching agent streaming, start here. +- `stream-utils.ts`: shared JSONL parsing, `AbortSignal` wiring, and child-process spawn/lifecycle helpers. Per-iteration CLI agents must spawn through `spawnAgentProcess`, the only place that keeps argv intact across a Windows `.cmd`/`.bat` shim. When touching agent streaming, start here. Reserved args managed by gnhf are rejected in `config.ts` via `isReservedAgentArg` - if you add a new flag that gnhf controls, add it to that list so user overrides can't shadow it. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f7d99ffc..12aa6ddb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,7 +40,7 @@ pnpm run test:coverage # vitest with coverage, excludes e2e/ ``` Run a single test file with `pnpm exec vitest run src/core/orchestrator.test.ts`, or filter by name with `pnpm exec vitest run -t "name substring"`. -E2E tests shell out to the built `dist/cli.mjs` against a mock `opencode` server in `e2e/fixtures/`, so they require a prior build (`pnpm test` and `pnpm run test:e2e` do this automatically). +E2E tests shell out to the built `dist/cli.mjs` against the mock agent CLIs and servers in `e2e/fixtures/`, so they require a prior build (`pnpm test` and `pnpm run test:e2e` do this automatically). Add new e2e tests as `e2e/*.test.ts` so the directory glob in both scripts picks them up. CI (`.github/workflows/ci.yml`) runs lint, format:check, typecheck, and test on Ubuntu, macOS, and Windows with Node 24; all four must stay green. diff --git a/README.md b/README.md index 7b061e82..74d07218 100644 --- a/README.md +++ b/README.md @@ -221,7 +221,7 @@ If the file does not exist yet, `gnhf` creates it on first run with its defaults A supplied `--agent` is written as the default agent. With the default configuration, it has this exact content: - + ```yaml # Agent to use by default: native agent name or acp: @@ -236,6 +236,7 @@ agent: claude # codex: /path/to/custom-codex # copilot: /path/to/custom-copilot # pi: /path/to/custom-pi +# grok: /path/to/custom-grok # Native agent CLI arg overrides (optional) # ACP targets do not support path or arg overrides. @@ -256,6 +257,9 @@ agent: claude # - gpt-5.5 # - --thinking # - high +# grok: +# - -m +# - grok-4.5-build # Custom ACP target commands (optional) # Maps acp: names to spawn commands. Useful for naming a @@ -286,7 +290,7 @@ Use `acpRegistryOverrides` to map `acp:` names to custom spawn commands You can also pass a raw custom ACP server command directly as a quoted `acp:` spec, for example `gnhf --agent 'acp:./bin/dev-acp --profile ci' "fix the tests"`. - Use it for agent-specific options like models, profiles, or reasoning settings without adding a dedicated `gnhf` config field for each one. -- For `codex`, `claude`, and `copilot`, `gnhf` adds its usual non-interactive permission default only when you do not provide your own permission or execution-mode flag. If you set one explicitly, `gnhf` treats that as user-managed and does not add its default on top. +- For `codex`, `claude`, `copilot`, and `grok`, `gnhf` adds its usual non-interactive permission default only when you do not provide your own permission or execution-mode flag. If you set one explicitly, `gnhf` treats that as user-managed and does not add its default on top. - Flags that `gnhf` manages itself for a given agent, such as output-shaping or local-server startup flags, are rejected during config loading so you get a clear error instead of duplicate-argument ambiguity. For `pi` specifically, `--api-key` is also blocked; configure the Pi API key via Pi's own config or the environment variable it reads, not via `agentArgsOverride`. `commitMessage` controls the subject line that gnhf uses for each successful iteration commit. @@ -305,6 +309,7 @@ agentPathOverride: codex: /usr/local/bin/my-codex-wrapper copilot: ~/bin/copilot-wrapper pi: ~/bin/pi-wrapper + grok: ~/bin/grok-wrapper ``` Paths may be absolute, bare executable names already on your `PATH`, `~`-prefixed, or relative to the config directory (`~/.gnhf/`). The override replaces only the binary name; all standard arguments are preserved, so the replacement must be CLI-compatible with the original agent. On Windows, `.cmd` and `.bat` wrappers are supported, including bare names resolved from `PATH`. For `rovodev`, the override must point to an `acli`-compatible binary since gnhf invokes it as ` rovodev serve ...`. @@ -325,7 +330,7 @@ Set `GNHF_TELEMETRY=0` to turn it off. ## Agents -`gnhf` supports six native agents plus ACP targets. ACP support is powered by [`acpx`](https://github.com/openclaw/acpx), which is bundled with `gnhf` and provides the runtime and agent registry for `acp:` specs. +`gnhf` supports seven native agents plus ACP targets. ACP support is powered by [`acpx`](https://github.com/openclaw/acpx), which is bundled with `gnhf` and provides the runtime and agent registry for `acp:` specs. | Agent | Flag | Requirements | Notes | | ------------------ | --------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | @@ -333,6 +338,7 @@ Set `GNHF_TELEMETRY=0` to turn it off. | Codex | `--agent codex` | Install OpenAI's `codex` CLI and sign in first. | `gnhf` invokes `codex exec` directly in non-interactive mode. | | GitHub Copilot CLI | `--agent copilot` | Install GitHub Copilot CLI and sign in first. | `gnhf` invokes `copilot` directly in non-interactive JSONL mode. Copilot currently exposes assistant output tokens, but not full input/cache token totals; see https://github.com/github/copilot-cli/issues/1152. | | Pi | `--agent pi` | Install the `pi` CLI and configure a usable provider/model first. | `gnhf` invokes `pi` directly in JSON mode, appends the final output schema to the prompt, and disables Pi session persistence with `--no-session`. | +| Grok | `--agent grok` | Install xAI's `grok` CLI and sign in first. | `gnhf` invokes `grok` directly in non-interactive mode with `--output-format streaming-json` and `--json-schema`, defaulting to `--always-approve` so tool calls do not block on prompts. Token usage arrives only when the turn ends, so `--max-tokens` stops after an iteration, not mid-iteration. | | Rovo Dev | `--agent rovodev` | Install Atlassian's `acli` and authenticate it with Rovo Dev first. | `gnhf` starts a local `acli rovodev serve --disable-session-token ` process automatically in the repo workspace. | | OpenCode | `--agent opencode` | Install `opencode` and configure at least one usable model provider first. | `gnhf` starts a local `opencode serve --hostname 127.0.0.1 --port --print-logs` process automatically, creates a per-run session, and applies a blanket allow rule so tool calls do not block on prompts. | | ACP target | `--agent acp:` | Install and authenticate the target supported by the bundled [`acpx`](https://github.com/openclaw/acpx) registry, such as `acp:gemini`, or pass a quoted custom ACP server command. | `gnhf` runs the target through ACP with a persistent per-run session under `.gnhf/runs//acp-sessions`; token usage and `--max-tokens` use ACP `used` deltas when available, with prompt-length plus tool-call estimates as a fallback, and `agentPathOverride` and `agentArgsOverride` do not apply. | diff --git a/e2e/e2e-grok.test.ts b/e2e/e2e-grok.test.ts new file mode 100644 index 00000000..920c6e0a --- /dev/null +++ b/e2e/e2e-grok.test.ts @@ -0,0 +1,307 @@ +import { execFileSync, spawn } from "node:child_process"; +import { + existsSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, describe, expect, it } from "vitest"; + +const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); +const distCliPath = join(repoRoot, "dist", "cli.mjs"); +const fixtureBinDir = join(repoRoot, "e2e", "fixtures"); + +// Empty gitconfig so the developer's real ~/.gitconfig (commit.gpgsign, +// core.hooksPath, credential helpers) cannot affect these runs. +const emptyGitConfigDir = mkdtempSync( + join(tmpdir(), "gnhf-e2e-grok-gitconfig-"), +); +const emptyGitConfigPath = join(emptyGitConfigDir, "gitconfig"); +writeFileSync(emptyGitConfigPath, "", "utf-8"); + +const sanitizedGitEnv: NodeJS.ProcessEnv = { + GIT_CONFIG_GLOBAL: emptyGitConfigPath, + GIT_CONFIG_SYSTEM: emptyGitConfigPath, + GIT_TERMINAL_PROMPT: "0", +}; + +interface RunResult { + code: number | null; + signal: NodeJS.Signals | null; + stdout: string; + stderr: string; +} + +function git(args: string[], cwd: string): string { + return execFileSync("git", args, { + cwd, + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + env: { ...process.env, ...sanitizedGitEnv }, + }).trim(); +} + +function createRepo(): string { + const cwd = mkdtempSync(join(tmpdir(), "gnhf-e2e-grok-")); + git(["init", "-b", "main"], cwd); + git(["config", "user.name", "gnhf tests"], cwd); + git(["config", "user.email", "tests@example.com"], cwd); + writeFileSync(join(cwd, "README.md"), "# fixture\n", "utf-8"); + git(["add", "README.md"], cwd); + git(["commit", "-m", "init"], cwd); + return cwd; +} + +function readJsonLines(filePath: string): Record[] { + if (!existsSync(filePath)) return []; + return readFileSync(filePath, "utf-8") + .trim() + .split("\n") + .filter(Boolean) + .map((line) => JSON.parse(line) as Record); +} + +function findRunLogPath(cwd: string): string { + const runsDir = join(cwd, ".gnhf", "runs"); + const runs = readdirSync(runsDir); + if (runs.length !== 1) { + throw new Error( + `Expected exactly one run in ${runsDir}, found ${runs.length}`, + ); + } + return join(runsDir, runs[0]!, "gnhf.log"); +} + +function runCli( + cwd: string, + args: string[], + options: { env?: NodeJS.ProcessEnv } = {}, +): Promise { + return new Promise((resolveResult, reject) => { + const child = spawn(process.execPath, [distCliPath, ...args], { + cwd, + env: options.env, + stdio: ["pipe", "pipe", "pipe"], + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => { + stdout += chunk.toString(); + }); + child.stderr.on("data", (chunk) => { + stderr += chunk.toString(); + }); + child.on("error", reject); + child.on("close", (code, signal) => { + resolveResult({ code, signal, stdout, stderr }); + }); + child.stdin.end(); + }); +} + +function createTestEnv( + mockLogPath: string, + tempDirs: string[], +): NodeJS.ProcessEnv { + const home = mkdtempSync(join(tmpdir(), "gnhf-e2e-grok-home-")); + tempDirs.push(home); + + return { + ...process.env, + ...sanitizedGitEnv, + HOME: home, + USERPROFILE: home, + PATH: `${fixtureBinDir}${process.platform === "win32" ? ";" : ":"}${process.env.PATH ?? ""}`, + GNHF_MOCK_GROK_LOG_PATH: mockLogPath, + }; +} + +function readMockInvocations( + mockLogPath: string, +): { argv: string[]; prompt: string; schema: string; cwd: string }[] { + return readJsonLines(mockLogPath) + .filter((entry) => entry.event === "cli:invoked") + .map((entry) => ({ + argv: entry.argv as string[], + prompt: String(entry.prompt), + schema: String(entry.schema), + cwd: String(entry.cwd), + })); +} + +describe("gnhf grok e2e", () => { + const tempDirs: string[] = []; + + afterEach(() => { + for (const dir of tempDirs.splice(0)) { + try { + rmSync(dir, { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 200, + }); + } catch { + // Windows: child processes may briefly hold file locks after exit + } + } + }); + + // Skipped on Windows: the fixture `grok` on PATH is a `.cmd` shim, so these + // runs go through `cmd.exe`, which cannot carry the multi-line iteration + // prompt as a single argv token no matter how it is quoted. + it.skipIf(process.platform === "win32")( + "runs an iteration through the grok CLI wire format and commits the result", + async () => { + const cwd = createRepo(); + tempDirs.push(cwd); + const logDir = mkdtempSync(join(tmpdir(), "gnhf-e2e-grok-logs-")); + tempDirs.push(logDir); + const mockLogPath = join(logDir, "mock-grok.jsonl"); + + const result = await runCli( + cwd, + [ + "add a line to the readme", + "--agent", + "grok", + "--max-iterations", + "1", + ], + { env: createTestEnv(mockLogPath, tempDirs) }, + ); + + expect(result.code).toBe(0); + expect(result.stdout).toContain("gnhf stopped"); + expect(result.stdout).toContain("grok ran"); + expect(result.stdout).toContain("max iterations reached (1)"); + // Usage from the terminal `end` event: 1200 fresh + 300 cache-read input, + // and 150 reasoning tokens that `total_tokens` shows sit outside + // `output_tokens` (450 + 150 = 600). Authoritative, so no "~" prefix. + expect(result.stdout).toContain("2K in"); + expect(result.stdout).toContain("600 out"); + expect(result.stdout).not.toContain("~2K in"); + + expect(git(["rev-list", "--count", "HEAD"], cwd)).toBe("2"); + expect(git(["log", "-1", "--format=%s"], cwd)).toContain("gnhf 1:"); + expect(git(["rev-parse", "--abbrev-ref", "HEAD"], cwd)).toContain( + "gnhf/", + ); + expect(readFileSync(join(cwd, "README.md"), "utf-8")).toContain( + "- grok change", + ); + + const invocations = readMockInvocations(mockLogPath); + expect(invocations).toHaveLength(1); + const invocation = invocations[0]!; + expect(invocation.prompt).toContain("add a line to the readme"); + expect(invocation.argv).toContain("-p"); + expect(invocation.argv).toContain("--always-approve"); + expect(invocation.argv.join(" ")).toContain( + "--output-format streaming-json", + ); + expect(JSON.parse(invocation.schema)).toMatchObject({ + type: "object", + required: ["success", "summary", "key_changes_made", "key_learnings"], + }); + + const debugEntries = readJsonLines(findRunLogPath(cwd)); + const debugEvents = debugEntries.map((entry) => entry.event); + expect(debugEvents).toContain("agent:run:start"); + expect(debugEvents).toContain("agent:run:end"); + expect(debugEvents).toContain("run:complete"); + expect( + debugEntries.find((entry) => entry.event === "iteration:end")?.success, + ).toBe(true); + }, + 30_000, + ); + + it.skipIf(process.platform === "win32")( + "keeps the finished iteration's commit when its reported usage trips --max-tokens", + async () => { + const cwd = createRepo(); + tempDirs.push(cwd); + const logDir = mkdtempSync(join(tmpdir(), "gnhf-e2e-grok-logs-")); + tempDirs.push(logDir); + const mockLogPath = join(logDir, "mock-grok.jsonl"); + + // grok only reports usage in the terminal `end` event, so a low + // --max-tokens budget must not roll back the iteration that just + // finished: the work is committed, then the run stops. + const result = await runCli( + cwd, + [ + "add a line to the readme", + "--agent", + "grok", + "--max-iterations", + "5", + "--max-tokens", + "100", + ], + { env: createTestEnv(mockLogPath, tempDirs) }, + ); + + expect(result.code).toBe(0); + expect(result.stdout).toContain("max tokens reached (2100/100)"); + expect(git(["rev-list", "--count", "HEAD"], cwd)).toBe("2"); + expect(readFileSync(join(cwd, "README.md"), "utf-8")).toContain( + "- grok change", + ); + expect(readMockInvocations(mockLogPath)).toHaveLength(1); + + const debugEntries = readJsonLines(findRunLogPath(cwd)); + expect(debugEntries.map((entry) => entry.event)).not.toContain( + "agent:run:aborted", + ); + expect( + debugEntries.find((entry) => entry.event === "iteration:end")?.success, + ).toBe(true); + }, + 30_000, + ); + + it.skipIf(process.platform === "win32")( + "recovers the iteration output when grok only prints JSON as text", + async () => { + const cwd = createRepo(); + tempDirs.push(cwd); + const logDir = mkdtempSync(join(tmpdir(), "gnhf-e2e-grok-logs-")); + tempDirs.push(logDir); + const mockLogPath = join(logDir, "mock-grok.jsonl"); + + const result = await runCli( + cwd, + [ + "add a line to the readme", + "--agent", + "grok", + "--max-iterations", + "1", + ], + { + env: { + ...createTestEnv(mockLogPath, tempDirs), + GNHF_MOCK_GROK_TEXT_ONLY: "1", + }, + }, + ); + + expect(result.code).toBe(0); + expect(git(["rev-list", "--count", "HEAD"], cwd)).toBe("2"); + expect(git(["log", "-1", "--format=%s"], cwd)).toContain("gnhf 1:"); + + const notesPath = join(dirname(findRunLogPath(cwd)), "notes.md"); + expect(readFileSync(notesPath, "utf-8")).toContain( + "appended a mock grok change to README.md", + ); + }, + 30_000, + ); +}); diff --git a/e2e/fixtures/grok b/e2e/fixtures/grok new file mode 100755 index 00000000..3645e8b0 --- /dev/null +++ b/e2e/fixtures/grok @@ -0,0 +1,4 @@ +#!/usr/bin/env sh + +SCRIPT_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +exec node "$SCRIPT_DIR/mock-grok-cli.mjs" "$@" diff --git a/e2e/fixtures/grok.cmd b/e2e/fixtures/grok.cmd new file mode 100644 index 00000000..aa3d6231 --- /dev/null +++ b/e2e/fixtures/grok.cmd @@ -0,0 +1,2 @@ +@echo off +node "%~dp0\mock-grok-cli.mjs" %* diff --git a/e2e/fixtures/mock-grok-cli.mjs b/e2e/fixtures/mock-grok-cli.mjs new file mode 100755 index 00000000..a1bb3ca2 --- /dev/null +++ b/e2e/fixtures/mock-grok-cli.mjs @@ -0,0 +1,78 @@ +#!/usr/bin/env node + +// Stands in for xAI's `grok` CLI in non-interactive mode: it records the argv +// gnhf built, edits the workspace like a real agent turn would, and replays +// grok's `--output-format streaming-json` event stream on stdout. + +import { appendFileSync } from "node:fs"; +import { join } from "node:path"; +import process from "node:process"; + +function appendLog(event, details = {}) { + const logPath = process.env.GNHF_MOCK_GROK_LOG_PATH; + if (!logPath) return; + appendFileSync( + logPath, + `${JSON.stringify({ pid: process.pid, event, ...details })}\n`, + "utf-8", + ); +} + +function readFlag(argv, flag) { + const index = argv.indexOf(flag); + return index === -1 ? undefined : argv[index + 1]; +} + +function emit(event) { + process.stdout.write(`${JSON.stringify(event)}\n`); +} + +const argv = process.argv.slice(2); +const prompt = readFlag(argv, "-p"); +const schema = readFlag(argv, "--json-schema"); + +appendLog("cli:invoked", { + argv, + prompt, + schema, + outputFormat: readFlag(argv, "--output-format"), + alwaysApprove: argv.includes("--always-approve"), + cwd: process.cwd(), +}); + +const marker = `- grok change ${argv.length}-${process.pid}\n`; +appendFileSync(join(process.cwd(), "README.md"), marker, "utf-8"); +appendLog("workspace:changed", { marker: marker.trim() }); + +const output = { + success: true, + summary: "appended a mock grok change to README.md", + key_changes_made: ["README.md: appended one line"], + key_learnings: ["the mock grok CLI streams events as JSONL"], +}; + +emit({ type: "text", data: "Reading README.md" }); +emit({ type: "text", data: " and appending one line...\n" }); + +const usage = { + input_tokens: 1200, + cache_read_input_tokens: 300, + output_tokens: 450, + reasoning_tokens: 150, + total_tokens: 2100, +}; + +if (process.env.GNHF_MOCK_GROK_TEXT_ONLY === "1") { + // Some grok builds print the JSON answer as prose instead of populating + // `structuredOutput`; gnhf must still recover the schema-shaped object. + emit({ + type: "text", + data: `\n\`\`\`json\n${JSON.stringify(output)}\n\`\`\`\n`, + }); + emit({ type: "end", stopReason: "stop", usage }); +} else { + emit({ type: "end", stopReason: "stop", usage, structuredOutput: output }); +} + +appendLog("cli:exit", { code: 0 }); +process.exit(0); diff --git a/skills/gnhf/SKILL.md b/skills/gnhf/SKILL.md index f2946a3f..db4ab3ab 100644 --- a/skills/gnhf/SKILL.md +++ b/skills/gnhf/SKILL.md @@ -149,7 +149,7 @@ Do not ask what to review first. Reconstruct state: git status --short git branch --show-current git log --oneline --decorate --max-count=20 -pgrep -fl 'gnhf|claude|codex|copilot|opencode|rovodev' || true +pgrep -fl 'gnhf|claude|codex|copilot|grok|opencode|rovodev' || true ``` Inspect likely GNHF branches, notes, logs, terminal sessions, and changed files. If a GNHF process is still running, report that first. diff --git a/src/core/agents/claude.test.ts b/src/core/agents/claude.test.ts index 85df836f..6a6c7d3a 100644 --- a/src/core/agents/claude.test.ts +++ b/src/core/agents/claude.test.ts @@ -35,6 +35,10 @@ describe("ClaudeAgent", () => { beforeEach(() => { vi.clearAllMocks(); + // Default the `where` shim lookup to "nothing found". Return values survive + // clearAllMocks, so a shim path left behind by one test would otherwise put + // later tests on the cmd.exe path and escape their argv on Windows hosts. + vi.mocked(execFileSync).mockReturnValue("" as never); agent = new ClaudeAgent(); }); @@ -142,14 +146,14 @@ describe("ClaudeAgent", () => { expect(mockSpawn).toHaveBeenCalledWith( "C:\\tools\\claude.cmd", [ - "-p", - "test prompt", - "--verbose", - "--output-format", - "stream-json", - "--json-schema", + '^"-p^"', + '^"test^ prompt^"', + '^"--verbose^"', + '^"--output-format^"', + '^"stream-json^"', + '^"--json-schema^"', expect.any(String), - "--dangerously-skip-permissions", + '^"--dangerously-skip-permissions^"', ], { cwd: "/work/dir", @@ -177,14 +181,14 @@ describe("ClaudeAgent", () => { expect(mockSpawn).toHaveBeenCalledWith( "claude-code-switch", [ - "-p", - "test prompt", - "--verbose", - "--output-format", - "stream-json", - "--json-schema", + '^"-p^"', + '^"test^ prompt^"', + '^"--verbose^"', + '^"--output-format^"', + '^"stream-json^"', + '^"--json-schema^"', expect.any(String), - "--dangerously-skip-permissions", + '^"--dangerously-skip-permissions^"', ], { cwd: "/work/dir", diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index 7c748977..88c940d5 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -1,4 +1,4 @@ -import { execFileSync, spawn } from "node:child_process"; +import type { ChildProcess } from "node:child_process"; import { createWriteStream } from "node:fs"; import { buildAgentOutputSchema, @@ -11,7 +11,13 @@ import { PermanentAgentError, } from "./types.js"; import { shutdownChildProcess } from "./managed-process.js"; -import { parseJSONLStream, setupAbortHandler } from "./stream-utils.js"; +import { + parseJSONLStream, + setupAbortHandler, + spawnAgentProcess, + spawnsDetached, + terminateChildProcess, +} from "./stream-utils.js"; const DEFAULT_FINAL_RESULT_EXIT_GRACE_MS = 15_000; @@ -52,70 +58,14 @@ interface ClaudeAgentDeps { schema?: AgentOutputSchema; } -function shouldUseWindowsShell( - bin: string, - platform: NodeJS.Platform, -): boolean { - if (platform !== "win32") { - return false; - } - - if (/\.(cmd|bat)$/i.test(bin)) { - return true; - } - - if (/[\\/]/.test(bin)) { - return false; - } - - try { - const resolved = execFileSync("where", [bin], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - }); - const firstMatch = resolved - .split(/\r?\n/) - .map((line) => line.trim()) - .find(Boolean); - return firstMatch ? /\.(cmd|bat)$/i.test(firstMatch) : false; - } catch { - return false; - } -} - -function terminateClaudeProcess( - child: ReturnType, - platform: NodeJS.Platform, -): void { - if (platform === "win32" && child.pid) { - try { - execFileSync("taskkill", ["/T", "/F", "/PID", String(child.pid)], { - stdio: "ignore", - }); - } catch { - // Best-effort: the process may have already exited. - } - return; - } - - if (child.pid) { - try { - process.kill(-child.pid, "SIGTERM"); - return; - } catch { - // Fall back to the direct child if it was not started as a process group. - } - } - - child.kill("SIGTERM"); -} - async function shutdownClaudeProcess( - child: ReturnType, + child: ChildProcess, platform: NodeJS.Platform, ): Promise { if (platform === "win32") { - terminateClaudeProcess(child, platform); + terminateChildProcess(child, platform, { + detached: spawnsDetached(platform), + }); return; } @@ -225,14 +175,15 @@ export class ClaudeAgent implements Agent { return new Promise((resolve, reject) => { const logStream = logPath ? createWriteStream(logPath) : null; + const detached = spawnsDetached(this.platform); - const child = spawn( + const child = spawnAgentProcess( this.bin, buildClaudeArgs(prompt, this.schema, this.extraArgs), + this.platform, { cwd, - detached: this.platform !== "win32", - shell: shouldUseWindowsShell(this.bin, this.platform), + detached, stdio: ["ignore", "pipe", "pipe"], env: process.env, }, @@ -240,7 +191,7 @@ export class ClaudeAgent implements Agent { if ( setupAbortHandler(signal, child, reject, () => - terminateClaudeProcess(child, this.platform), + terminateChildProcess(child, this.platform, { detached }), ) ) { return; diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index 86369a8d..20af8cb8 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -24,6 +24,10 @@ function createMockProcess() { describe("CodexAgent", () => { beforeEach(() => { vi.clearAllMocks(); + // Default the `where` shim lookup to "nothing found". Return values survive + // clearAllMocks, so a shim path left behind by one test would otherwise put + // later tests on the cmd.exe path and escape their argv on Windows hosts. + vi.mocked(execFileSync).mockReturnValue("" as never); }); it("does not use a shell for direct Windows launches", () => { @@ -69,14 +73,14 @@ describe("CodexAgent", () => { expect(mockSpawn).toHaveBeenCalledWith( "C:\\tools\\codex.cmd", [ - "exec", - "test prompt", - "--json", - "--output-schema", - "/tmp/schema.json", - "--dangerously-bypass-approvals-and-sandbox", - "--color", - "never", + '^"exec^"', + '^"test^ prompt^"', + '^"--json^"', + '^"--output-schema^"', + '^"/tmp/schema.json^"', + '^"--dangerously-bypass-approvals-and-sandbox^"', + '^"--color^"', + '^"never^"', ], { cwd: "/work/dir", @@ -103,14 +107,14 @@ describe("CodexAgent", () => { expect(mockSpawn).toHaveBeenCalledWith( "codex-switch", [ - "exec", - "test prompt", - "--json", - "--output-schema", - "/tmp/schema.json", - "--dangerously-bypass-approvals-and-sandbox", - "--color", - "never", + '^"exec^"', + '^"test^ prompt^"', + '^"--json^"', + '^"--output-schema^"', + '^"/tmp/schema.json^"', + '^"--dangerously-bypass-approvals-and-sandbox^"', + '^"--color^"', + '^"never^"', ], { cwd: "/work/dir", diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index f1b25a19..7c29a83d 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -1,4 +1,3 @@ -import { execFileSync, spawn } from "node:child_process"; import { createWriteStream } from "node:fs"; import type { Agent, @@ -11,6 +10,8 @@ import { parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, + spawnAgentProcess, + terminateChildProcess, } from "./stream-utils.js"; interface CodexItemCompleted { @@ -35,55 +36,6 @@ interface CodexAgentDeps { platform?: NodeJS.Platform; } -function shouldUseWindowsShell( - bin: string, - platform: NodeJS.Platform, -): boolean { - if (platform !== "win32") { - return false; - } - - if (/\.(cmd|bat)$/i.test(bin)) { - return true; - } - - if (/[\\/]/.test(bin)) { - return false; - } - - try { - const resolved = execFileSync("where", [bin], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - }); - const firstMatch = resolved - .split(/\r?\n/) - .map((line) => line.trim()) - .find(Boolean); - return firstMatch ? /\.(cmd|bat)$/i.test(firstMatch) : false; - } catch { - return false; - } -} - -function terminateCodexProcess( - child: ReturnType, - platform: NodeJS.Platform, -): void { - if (platform === "win32" && child.pid) { - try { - execFileSync("taskkill", ["/T", "/F", "/PID", String(child.pid)], { - stdio: "ignore", - }); - } catch { - // Best-effort: the process may have already exited. - } - return; - } - - child.kill("SIGTERM"); -} - function buildCodexArgs( prompt: string, schemaPath: string, @@ -143,12 +95,12 @@ export class CodexAgent implements Agent { return new Promise((resolve, reject) => { const logStream = logPath ? createWriteStream(logPath) : null; - const child = spawn( + const child = spawnAgentProcess( this.bin, buildCodexArgs(prompt, this.schemaPath, this.extraArgs), + this.platform, { cwd, - shell: shouldUseWindowsShell(this.bin, this.platform), stdio: ["ignore", "pipe", "pipe"], env: process.env, }, @@ -156,7 +108,7 @@ export class CodexAgent implements Agent { if ( setupAbortHandler(signal, child, reject, () => - terminateCodexProcess(child, this.platform), + terminateChildProcess(child, this.platform, { detached: false }), ) ) { return; diff --git a/src/core/agents/copilot.test.ts b/src/core/agents/copilot.test.ts index e5e326c0..0a935ab5 100644 --- a/src/core/agents/copilot.test.ts +++ b/src/core/agents/copilot.test.ts @@ -29,6 +29,10 @@ function emitJson(proc: ReturnType, event: unknown) { describe("CopilotAgent", () => { beforeEach(() => { vi.clearAllMocks(); + // Default the `where` shim lookup to "nothing found". Return values survive + // clearAllMocks, so a shim path left behind by one test would otherwise put + // later tests on the cmd.exe path and escape their argv on Windows hosts. + vi.mocked(execFileSync).mockReturnValue("" as never); }); it("spawns copilot in non-interactive JSONL mode with the default permission flag", () => { diff --git a/src/core/agents/copilot.ts b/src/core/agents/copilot.ts index 0807cbc2..5ed7383e 100644 --- a/src/core/agents/copilot.ts +++ b/src/core/agents/copilot.ts @@ -1,4 +1,3 @@ -import { execFileSync, spawn } from "node:child_process"; import { createWriteStream } from "node:fs"; import { buildAgentOutputSchema, @@ -10,11 +9,13 @@ import { type AgentRunOptions, type TokenUsage, } from "./types.js"; -import { parseAgentJson } from "./json-extract.js"; +import { parseAgentOutputJson } from "./json-extract.js"; import { parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, + spawnAgentProcess, + terminateChildProcess, } from "./stream-utils.js"; interface CopilotAssistantMessageEvent { @@ -40,55 +41,6 @@ interface CopilotAgentDeps { schema?: AgentOutputSchema; } -function shouldUseWindowsShell( - bin: string, - platform: NodeJS.Platform, -): boolean { - if (platform !== "win32") { - return false; - } - - if (/\.(cmd|bat)$/i.test(bin)) { - return true; - } - - if (/[\\/]/.test(bin)) { - return false; - } - - try { - const resolved = execFileSync("where", [bin], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - }); - const firstMatch = resolved - .split(/\r?\n/) - .map((line) => line.trim()) - .find(Boolean); - return firstMatch ? /\.(cmd|bat)$/i.test(firstMatch) : false; - } catch { - return false; - } -} - -function terminateCopilotProcess( - child: ReturnType, - platform: NodeJS.Platform, -): void { - if (platform === "win32" && child.pid) { - try { - execFileSync("taskkill", ["/T", "/F", "/PID", String(child.pid)], { - stdio: "ignore", - }); - } catch { - // Best-effort: the process may have already exited. - } - return; - } - - child.kill("SIGTERM"); -} - function userSpecifiedPermissionMode(userArgs: string[]): boolean { return userArgs.some( (arg) => @@ -196,25 +148,8 @@ function parseCopilotOutput( text: string, schema: AgentOutputSchema, ): AgentOutput { - const parsed = parseAgentJson(text, (value) => { - try { - validateAgentOutput(value, schema); - return true; - } catch { - return false; - } - }); - if (parsed !== null) { - return validateAgentOutput(parsed, schema); - } - - const fallbackParsed = parseAgentJson(text); - if (fallbackParsed !== null) { - return validateAgentOutput(fallbackParsed, schema); - } - - throw new SyntaxError( - "copilot output did not contain a parseable JSON object", + return parseAgentOutputJson(text, "copilot", (value) => + validateAgentOutput(value, schema), ); } @@ -245,12 +180,12 @@ export class CopilotAgent implements Agent { return new Promise((resolve, reject) => { const logStream = logPath ? createWriteStream(logPath) : null; - const child = spawn( + const child = spawnAgentProcess( this.bin, buildCopilotArgs(prompt, this.schema, this.extraArgs), + this.platform, { cwd, - shell: shouldUseWindowsShell(this.bin, this.platform), stdio: ["ignore", "pipe", "pipe"], env: process.env, }, @@ -258,7 +193,7 @@ export class CopilotAgent implements Agent { if ( setupAbortHandler(signal, child, reject, () => - terminateCopilotProcess(child, this.platform), + terminateChildProcess(child, this.platform, { detached: false }), ) ) { return; diff --git a/src/core/agents/factory.test.ts b/src/core/agents/factory.test.ts index c68ada7e..657a40e9 100644 --- a/src/core/agents/factory.test.ts +++ b/src/core/agents/factory.test.ts @@ -45,6 +45,17 @@ vi.mock("./pi.js", () => { return { PiAgent }; }); +vi.mock("./grok.js", () => { + const GrokAgent = vi.fn(function ( + this: Record, + deps?: Record, + ) { + this.name = "grok"; + this.deps = deps; + }); + return { GrokAgent }; +}); + vi.mock("./rovodev.js", () => { const RovoDevAgent = vi.fn(function ( this: Record, @@ -86,6 +97,7 @@ import { AcpAgent } from "./acp.js"; import { ClaudeAgent } from "./claude.js"; import { CopilotAgent } from "./copilot.js"; import { CodexAgent } from "./codex.js"; +import { GrokAgent } from "./grok.js"; import { OpenCodeAgent } from "./opencode.js"; import { PiAgent } from "./pi.js"; import { RovoDevAgent } from "./rovodev.js"; @@ -301,6 +313,46 @@ describe("createAgent", () => { }); }); + it("creates a GrokAgent when name is 'grok'", () => { + const agent = createAgent("grok", stubRunInfo, undefined, undefined, { + includeStopField: false, + }); + expect(GrokAgent).toHaveBeenCalledWith({ + bin: undefined, + extraArgs: undefined, + schema: noStopSchema, + }); + expect(agent.name).toBe("grok"); + }); + + it("passes path override and extra args through to the GrokAgent", () => { + const agent = createAgent( + "grok", + stubRunInfo, + "/custom/grok", + ["-m", "grok-4.5-build"], + { includeStopField: false }, + ); + + expect(GrokAgent).toHaveBeenCalledWith({ + bin: "/custom/grok", + extraArgs: ["-m", "grok-4.5-build"], + schema: noStopSchema, + }); + expect(agent.name).toBe("grok"); + }); + + it("hands GrokAgent a schema that requires should_fully_stop when includeStopField is true", () => { + createAgent("grok", stubRunInfo, undefined, undefined, { + includeStopField: true, + }); + expect(GrokAgent).toHaveBeenCalledWith({ + bin: undefined, + extraArgs: undefined, + schema: withStopSchema, + }); + }); + it("creates a RovoDevAgent when name is 'rovodev'", () => { const agent = createAgent("rovodev", stubRunInfo, undefined, undefined, { includeStopField: false, diff --git a/src/core/agents/factory.ts b/src/core/agents/factory.ts index cddb3840..e7acfe8c 100644 --- a/src/core/agents/factory.ts +++ b/src/core/agents/factory.ts @@ -10,6 +10,7 @@ import { AcpAgent } from "./acp.js"; import { ClaudeAgent } from "./claude.js"; import { CopilotAgent } from "./copilot.js"; import { CodexAgent } from "./codex.js"; +import { GrokAgent } from "./grok.js"; import { OpenCodeAgent } from "./opencode.js"; import { PiAgent } from "./pi.js"; import { RovoDevAgent } from "./rovodev.js"; @@ -73,6 +74,12 @@ export function createAgent( extraArgs: agentArgsOverride, schema, }); + case "grok": + return new GrokAgent({ + bin: pathOverride, + extraArgs: agentArgsOverride, + schema, + }); case "rovodev": return new RovoDevAgent(runInfo.schemaPath, { bin: pathOverride, diff --git a/src/core/agents/grok.test.ts b/src/core/agents/grok.test.ts new file mode 100644 index 00000000..7f332dab --- /dev/null +++ b/src/core/agents/grok.test.ts @@ -0,0 +1,636 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { EventEmitter } from "node:events"; + +vi.mock("node:child_process", () => ({ + execFileSync: vi.fn(), + spawn: vi.fn(), +})); + +import { execFileSync, spawn } from "node:child_process"; +import { GrokAgent } from "./grok.js"; +import { buildAgentOutputSchema } from "./types.js"; + +const mockSpawn = vi.mocked(spawn); + +const STOP_SCHEMA = buildAgentOutputSchema({ + includeStopField: true, +}); + +function createMockProcess() { + const proc = Object.assign(new EventEmitter(), { + stdout: new EventEmitter(), + stderr: new EventEmitter(), + stdin: null, + kill: vi.fn(), + }); + return proc as typeof proc & ReturnType; +} + +function emitLine(proc: ReturnType, obj: unknown) { + proc.stdout.emit("data", Buffer.from(JSON.stringify(obj) + "\n")); +} + +describe("GrokAgent", () => { + let agent: GrokAgent; + + beforeEach(() => { + vi.clearAllMocks(); + // Default the `where` shim lookup to "nothing found". Return values survive + // clearAllMocks, so a shim path left behind by one test would otherwise put + // later tests on the cmd.exe path and escape their argv on Windows hosts. + vi.mocked(execFileSync).mockReturnValue("" as never); + agent = new GrokAgent(); + }); + + it("has name 'grok'", () => { + expect(agent.name).toBe("grok"); + }); + + it("spawns grok with streaming-json output format and always-approve", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const unixAgent = new GrokAgent({ + platform: "darwin", + }); + + unixAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "grok", + [ + "-p", + "test prompt", + "--output-format", + "streaming-json", + "--json-schema", + expect.any(String), + "--always-approve", + ], + { + cwd: "/work/dir", + detached: true, + shell: false, + stdio: ["ignore", "pipe", "pipe"], + env: process.env, + }, + ); + }); + + it("uses the configured schema for --json-schema", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const configuredAgent = new GrokAgent({ + schema: STOP_SCHEMA, + }); + + configuredAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "grok", + [ + "-p", + "test prompt", + "--output-format", + "streaming-json", + "--json-schema", + JSON.stringify(STOP_SCHEMA), + "--always-approve", + ], + expect.any(Object), + ); + }); + + it("does not use a shell for direct Windows launches", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const windowsAgent = new GrokAgent({ + platform: "win32", + }); + + windowsAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "grok", + [ + "-p", + "test prompt", + "--output-format", + "streaming-json", + "--json-schema", + expect.any(String), + "--always-approve", + ], + { + cwd: "/work/dir", + detached: false, + shell: false, + stdio: ["ignore", "pipe", "pipe"], + env: process.env, + }, + ); + }); + + it("uses a shell on Windows for cmd wrapper paths", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const windowsAgent = new GrokAgent({ + bin: "C:\\tools\\grok.cmd", + platform: "win32", + }); + + windowsAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "C:\\tools\\grok.cmd", + [ + '^"-p^"', + '^"test^ prompt^"', + '^"--output-format^"', + '^"streaming-json^"', + '^"--json-schema^"', + expect.any(String), + '^"--always-approve^"', + ], + { + cwd: "/work/dir", + detached: false, + shell: true, + stdio: ["ignore", "pipe", "pipe"], + env: process.env, + }, + ); + }); + + it("uses a shell on Windows when a bare override resolves to a cmd wrapper", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + vi.mocked(execFileSync).mockReturnValue( + "C:\\tools\\grok-wrapper.cmd\r\n" as never, + ); + const windowsAgent = new GrokAgent({ + bin: "grok-wrapper", + platform: "win32", + }); + + windowsAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "grok-wrapper", + [ + '^"-p^"', + '^"test^ prompt^"', + '^"--output-format^"', + '^"streaming-json^"', + '^"--json-schema^"', + expect.any(String), + '^"--always-approve^"', + ], + { + cwd: "/work/dir", + detached: false, + shell: true, + stdio: ["ignore", "pipe", "pipe"], + env: process.env, + }, + ); + }); + + it("passes configured extra args through to grok", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const configuredAgent = new GrokAgent({ + extraArgs: ["-m", "grok-4.5-build", "--permission-mode", "auto"], + }); + + configuredAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "grok", + [ + "-m", + "grok-4.5-build", + "--permission-mode", + "auto", + "-p", + "test prompt", + "--output-format", + "streaming-json", + "--json-schema", + expect.any(String), + ], + expect.any(Object), + ); + }); + + it("does not add --always-approve when the user already set --always-approve", () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const configuredAgent = new GrokAgent({ + extraArgs: ["--always-approve"], + }); + + configuredAgent.run("test prompt", "/work/dir"); + + expect(mockSpawn).toHaveBeenCalledWith( + "grok", + [ + "--always-approve", + "-p", + "test prompt", + "--output-format", + "streaming-json", + "--json-schema", + expect.any(String), + ], + expect.any(Object), + ); + }); + + it("kills the full process tree on Windows when aborted", async () => { + const proc = createMockProcess(); + Object.defineProperty(proc, "pid", { value: 5678 }); + mockSpawn.mockReturnValue(proc); + const controller = new AbortController(); + const windowsAgent = new GrokAgent({ + platform: "win32", + }); + + const promise = windowsAgent.run("test prompt", "/work/dir", { + signal: controller.signal, + }); + controller.abort(); + + await expect(promise).rejects.toThrow("Agent was aborted"); + expect(vi.mocked(execFileSync)).toHaveBeenCalledWith( + "taskkill", + ["/T", "/F", "/PID", "5678"], + { stdio: "ignore" }, + ); + expect(proc.kill).not.toHaveBeenCalled(); + }); + + it("kills the whole process group on unix when aborted", async () => { + const proc = createMockProcess(); + Object.defineProperty(proc, "pid", { value: 4321 }); + mockSpawn.mockReturnValue(proc); + const killSpy = vi + .spyOn(process, "kill") + .mockImplementation(() => true as never); + const controller = new AbortController(); + const unixAgent = new GrokAgent({ platform: "darwin" }); + + const promise = unixAgent.run("test prompt", "/work/dir", { + signal: controller.signal, + }); + controller.abort(); + + await expect(promise).rejects.toThrow("Agent was aborted"); + expect(killSpy).toHaveBeenCalledWith(-4321, "SIGTERM"); + expect(proc.kill).not.toHaveBeenCalled(); + killSpy.mockRestore(); + }); + + it("falls back to signalling the child directly when it leads no group", async () => { + const proc = createMockProcess(); + Object.defineProperty(proc, "pid", { value: 4321 }); + mockSpawn.mockReturnValue(proc); + const killSpy = vi.spyOn(process, "kill").mockImplementation(() => { + throw new Error("ESRCH"); + }); + const controller = new AbortController(); + const unixAgent = new GrokAgent({ platform: "darwin" }); + + const promise = unixAgent.run("test prompt", "/work/dir", { + signal: controller.signal, + }); + controller.abort(); + + await expect(promise).rejects.toThrow("Agent was aborted"); + expect(proc.kill).toHaveBeenCalledWith("SIGTERM"); + killSpy.mockRestore(); + }); + + it("resolves structuredOutput and usage from the end event", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const onMessage = vi.fn(); + const onUsage = vi.fn(); + + const promise = agent.run("prompt", "/cwd", { onMessage, onUsage }); + + emitLine(proc, { type: "text", data: '{"success":' }); + emitLine(proc, { type: "text", data: "true}" }); + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + usage: { + input_tokens: 100, + cache_read_input_tokens: 50, + output_tokens: 20, + reasoning_tokens: 5, + total_tokens: 175, + }, + structuredOutput: { + success: true, + summary: "done", + key_changes_made: ["a"], + key_learnings: ["b"], + }, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toEqual({ + output: { + success: true, + summary: "done", + key_changes_made: ["a"], + key_learnings: ["b"], + }, + usage: { + inputTokens: 150, + outputTokens: 25, + cacheReadTokens: 50, + cacheCreationTokens: 0, + }, + }); + expect(onMessage).toHaveBeenCalledWith('{"success":true}'); + expect(onUsage).toHaveBeenCalledWith({ + inputTokens: 150, + outputTokens: 25, + cacheReadTokens: 50, + cacheCreationTokens: 0, + }); + }); + + it("accounts for reasoning tokens reported outside output_tokens", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + usage: { + input_tokens: 1000, + output_tokens: 200, + reasoning_tokens: 4000, + total_tokens: 5200, + }, + structuredOutput: { + success: true, + summary: "done", + key_changes_made: [], + key_learnings: [], + }, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + usage: { + inputTokens: 1000, + outputTokens: 4200, + cacheReadTokens: 0, + cacheCreationTokens: 0, + }, + }); + }); + + it("reports usage only after the finished iteration has settled", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const controller = new AbortController(); + // Mirrors the orchestrator aborting mid-iteration once --max-tokens trips. + const onUsage = vi.fn(() => controller.abort()); + + const promise = agent.run("prompt", "/cwd", { + onUsage, + signal: controller.signal, + }); + + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + usage: { input_tokens: 100, output_tokens: 100 }, + structuredOutput: { + success: true, + summary: "done", + key_changes_made: [], + key_learnings: [], + }, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "done" }, + }); + expect(onUsage).toHaveBeenCalledTimes(1); + }); + + it("keeps surfacing the most recent streamed text", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const onMessage = vi.fn(); + + const promise = agent.run("prompt", "/cwd", { onMessage }); + + emitLine(proc, { type: "text", data: "a".repeat(2000) }); + emitLine(proc, { type: "text", data: " newest output" }); + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + structuredOutput: { + success: true, + summary: "done", + key_changes_made: [], + key_learnings: [], + }, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true }, + }); + const lastMessage = onMessage.mock.calls.at(-1)?.[0] as string; + expect(lastMessage.endsWith("newest output")).toBe(true); + expect(lastMessage.length).toBeLessThanOrEqual(200); + }); + + it("falls back to parsing streamed text when structuredOutput is missing", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + + emitLine(proc, { + type: "text", + data: JSON.stringify({ + success: true, + summary: "from text", + key_changes_made: [], + key_learnings: [], + }), + }); + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + usage: { + input_tokens: 10, + output_tokens: 4, + }, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "from text" }, + usage: { + inputTokens: 10, + outputTokens: 4, + cacheReadTokens: 0, + cacheCreationTokens: 0, + }, + }); + }); + + it("rejects when grok exits non-zero", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + proc.stderr.emit("data", Buffer.from("auth failed")); + proc.emit("close", 1); + + await expect(promise).rejects.toThrow( + "grok exited with code 1: auth failed", + ); + }); + + it("rejects when there is no end event", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow("grok returned no end event"); + }); + + it("rejects unexpected stopReason values", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + emitLine(proc, { + type: "end", + stopReason: "Error", + structuredOutput: { + success: true, + summary: "x", + key_changes_made: [], + key_learnings: [], + }, + }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow('grok reported stopReason "Error"'); + }); + + it("accepts unrecognised terminal stopReason values", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + emitLine(proc, { + type: "end", + stopReason: "Completed", + structuredOutput: { + success: true, + summary: "x", + key_changes_made: [], + key_learnings: [], + }, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "x" }, + }); + }); + + it("prefers the streamed JSON object that matches the schema", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + emitLine(proc, { + type: "text", + data: `Here is the result:\n${JSON.stringify({ + success: true, + summary: "from text", + key_changes_made: [], + key_learnings: [], + })}\nDebug: {"tool":"bash"}`, + }); + emitLine(proc, { type: "end", stopReason: "EndTurn" }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "from text" }, + }); + }); + + it("falls back to the streamed text when structuredOutput is an explicit null", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + emitLine(proc, { + type: "text", + data: JSON.stringify({ + success: true, + summary: "from text", + key_changes_made: [], + key_learnings: [], + }), + }); + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + structuredOutput: null, + }); + proc.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "from text" }, + }); + }); + + it("reports unparseable streamed text as a parse failure", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + emitLine(proc, { type: "text", data: "I could not finish the task." }); + emitLine(proc, { type: "end", stopReason: "EndTurn" }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow( + "grok output did not contain a parseable JSON object", + ); + }); + + it("rejects invalid structuredOutput against the schema", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + emitLine(proc, { + type: "end", + stopReason: "EndTurn", + structuredOutput: { success: true }, + }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow("Invalid grok output"); + }); +}); diff --git a/src/core/agents/grok.ts b/src/core/agents/grok.ts new file mode 100644 index 00000000..2260ef62 --- /dev/null +++ b/src/core/agents/grok.ts @@ -0,0 +1,301 @@ +import { createWriteStream } from "node:fs"; +import { + buildAgentOutputSchema, + validateAgentOutput, + type Agent, + type AgentOutput, + type AgentOutputSchema, + type AgentResult, + type AgentRunOptions, + type TokenUsage, +} from "./types.js"; +import { parseAgentOutputJson } from "./json-extract.js"; +import { + parseJSONLStream, + setupAbortHandler, + setupChildProcessHandlers, + spawnAgentProcess, + spawnsDetached, + terminateChildProcess, +} from "./stream-utils.js"; + +interface GrokTextEvent { + type: "text"; + data?: string; +} + +interface GrokEndEvent { + type: "end"; + stopReason?: string; + usage?: { + input_tokens?: number; + cache_read_input_tokens?: number; + output_tokens?: number; + reasoning_tokens?: number; + total_tokens?: number; + }; + structuredOutput?: unknown; +} + +type GrokEvent = GrokTextEvent | GrokEndEvent | { type: string }; + +interface GrokAgentDeps { + bin?: string; + extraArgs?: string[]; + platform?: NodeJS.Platform; + schema?: AgentOutputSchema; +} + +/** + * How much of the streamed text to keep for the renderer's live message panel, + * which only shows a few wrapped lines. Keeping a bounded trailing window means + * the panel follows what grok is doing now instead of freezing on the first + * few lines of the turn. + */ +const LIVE_MESSAGE_CHARS = 200; + +/** + * `stopReason` values that mean the turn did not complete normally. Anything + * else is treated as terminal-and-fine, because an unrecognised spelling of + * "finished" must not stall the whole run; a turn that really did end early + * still fails schema validation of its output. + */ +const FAILED_STOP_REASONS = new Set([ + "aborted", + "cancel", + "canceled", + "cancelled", + "error", + "failed", + "interrupted", + "maxoutputtokens", + "maxtokens", + "refusal", + "refused", + "timedout", + "timeout", +]); + +function isFailedStopReason(stopReason: string | undefined): boolean { + if (!stopReason) return false; + return FAILED_STOP_REASONS.has( + stopReason.toLowerCase().replace(/[\s_-]/g, ""), + ); +} + +function userSpecifiedPermissionMode(userArgs: string[]): boolean { + return userArgs.some( + (arg) => + arg === "--always-approve" || + arg === "--permission-mode" || + arg.startsWith("--permission-mode=") || + arg === "--allow" || + arg.startsWith("--allow=") || + arg === "--deny" || + arg.startsWith("--deny=") || + arg === "--disallowed-tools" || + arg.startsWith("--disallowed-tools=") || + arg === "--tools" || + arg.startsWith("--tools="), + ); +} + +function buildGrokArgs( + prompt: string, + schema: AgentOutputSchema, + extraArgs?: string[], +): string[] { + const userArgs = extraArgs ?? []; + + return [ + ...userArgs, + "-p", + prompt, + "--output-format", + "streaming-json", + "--json-schema", + JSON.stringify(schema), + ...(userSpecifiedPermissionMode(userArgs) ? [] : ["--always-approve"]), + ]; +} + +function toTokenUsage(usage: GrokEndEvent["usage"] | undefined): TokenUsage { + if (!usage) { + return { + inputTokens: 0, + outputTokens: 0, + cacheReadTokens: 0, + cacheCreationTokens: 0, + }; + } + + const cacheReadTokens = usage.cache_read_input_tokens ?? 0; + // Match Claude-style cumulative input accounting: billed/prompt tokens + // include both fresh input and cache reads when both are reported. + const inputTokens = (usage.input_tokens ?? 0) + cacheReadTokens; + const outputTokens = usage.output_tokens ?? 0; + // grok bills reasoning in its own bucket and only `total_tokens` says whether + // that bucket is already inside `output_tokens`, so the reported total is the + // authority for anything the input/output buckets do not account for. + const totalTokens = usage.total_tokens; + return { + inputTokens, + outputTokens: + typeof totalTokens === "number" + ? Math.max(outputTokens, totalTokens - inputTokens) + : outputTokens, + cacheReadTokens, + cacheCreationTokens: 0, + }; +} + +function validateGrokOutput( + value: unknown, + schema: AgentOutputSchema, +): AgentOutput { + try { + return validateAgentOutput(value, schema); + } catch (err) { + throw new Error( + `Invalid grok output: ${err instanceof Error ? err.message : err}`, + ); + } +} + +/** + * Prefer grok's own `structuredOutput`, falling back to the streamed text for + * builds that only emit the JSON answer as prose, or that report "no structured + * answer" as an explicit null. The fallback text is the whole turn's + * transcript, so recovery goes through the shared schema-aware extractor. + */ +function parseGrokOutput( + end: GrokEndEvent, + streamedText: string, + schema: AgentOutputSchema, +): AgentOutput { + if (end.structuredOutput != null) { + return validateGrokOutput(end.structuredOutput, schema); + } + + const finalText = streamedText.trim(); + if (!finalText) { + throw new Error("grok returned no structuredOutput or text"); + } + + return parseAgentOutputJson(finalText, "grok", (value) => + validateGrokOutput(value, schema), + ); +} + +export class GrokAgent implements Agent { + name = "grok"; + + private bin: string; + private extraArgs?: string[]; + private platform: NodeJS.Platform; + private schema: AgentOutputSchema; + + constructor(deps: GrokAgentDeps = {}) { + this.bin = deps.bin ?? "grok"; + this.extraArgs = deps.extraArgs; + this.platform = deps.platform ?? process.platform; + this.schema = + deps.schema ?? buildAgentOutputSchema({ includeStopField: false }); + } + + run( + prompt: string, + cwd: string, + options?: AgentRunOptions, + ): Promise { + const { onUsage, onMessage, signal, logPath } = options ?? {}; + + return new Promise((resolve, reject) => { + const logStream = logPath ? createWriteStream(logPath) : null; + const detached = spawnsDetached(this.platform); + const child = spawnAgentProcess( + this.bin, + buildGrokArgs(prompt, this.schema, this.extraArgs), + this.platform, + { + cwd, + detached, + stdio: ["ignore", "pipe", "pipe"], + env: process.env, + }, + ); + + if ( + setupAbortHandler(signal, child, reject, () => + terminateChildProcess(child, this.platform, { detached }), + ) + ) { + return; + } + + let endEvent: GrokEndEvent | null = null; + let textBuffer = ""; + let liveMessage = ""; + let usageReported = false; + + // grok reports usage exactly once, in the terminal `end` event that + // immediately precedes process exit. Surfacing it before this run settles + // would let a `--max-tokens` abort roll back an iteration that has + // already finished, so it is always reported after the promise settles + // and left to the orchestrator's post-iteration limit check. + const reportUsage = () => { + if (usageReported || !endEvent) return; + usageReported = true; + onUsage?.(toTokenUsage(endEvent.usage)); + }; + + parseJSONLStream(child.stdout!, logStream, (event) => { + if (event.type === "text") { + const data = + "data" in event && typeof event.data === "string" ? event.data : ""; + if (!data) return; + textBuffer += data; + liveMessage = (liveMessage + data).slice(-LIVE_MESSAGE_CHARS); + const visible = liveMessage.trim(); + if (visible) onMessage?.(visible); + return; + } + + if (event.type === "end") { + endEvent = event as GrokEndEvent; + } + }); + + setupChildProcessHandlers( + child, + "grok", + logStream, + (err) => { + reject(err); + reportUsage(); + }, + () => { + try { + const end = endEvent; + if (!end) { + throw new Error("grok returned no end event"); + } + if (isFailedStopReason(end.stopReason)) { + throw new Error( + `grok reported stopReason ${JSON.stringify(end.stopReason)}`, + ); + } + resolve({ + output: parseGrokOutput(end, textBuffer, this.schema), + usage: toTokenUsage(end.usage), + }); + } catch (err) { + reject(err instanceof Error ? err : new Error(String(err))); + } finally { + reportUsage(); + } + }, + ); + }); + } +} diff --git a/src/core/agents/json-extract.test.ts b/src/core/agents/json-extract.test.ts index 715bd95c..89755f26 100644 --- a/src/core/agents/json-extract.test.ts +++ b/src/core/agents/json-extract.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { extractLastJsonObject, parseAgentJson, + parseAgentOutputJson, stripJsonFences, tryExtractBalancedObject, } from "./json-extract.js"; @@ -136,3 +137,51 @@ describe("parseAgentJson", () => { expect(parseAgentJson(" ")).toBeNull(); }); }); + +describe("parseAgentOutputJson", () => { + interface Answer { + summary: string; + } + + function validateAnswer(value: unknown): Answer { + const record = value as Record; + if (typeof record?.summary !== "string") { + throw new Error("summary is required"); + } + return { summary: record.summary }; + } + + it("prefers the object the validator accepts over other JSON in the text", () => { + const text = [ + 'Tool result: {"tool":"bash"}', + '{"summary":"the real answer"}', + 'Debug: {"elapsed":12}', + ].join("\n"); + + expect(parseAgentOutputJson(text, "grok", validateAnswer)).toEqual({ + summary: "the real answer", + }); + }); + + it("surfaces the validator's error when the only JSON does not validate", () => { + expect(() => + parseAgentOutputJson( + 'Here it is: {"success":true}', + "grok", + validateAnswer, + ), + ).toThrow("summary is required"); + }); + + it("throws a named SyntaxError when the text holds no JSON object", () => { + expect(() => + parseAgentOutputJson( + "I could not finish the task.", + "grok", + validateAnswer, + ), + ).toThrow( + new SyntaxError("grok output did not contain a parseable JSON object"), + ); + }); +}); diff --git a/src/core/agents/json-extract.ts b/src/core/agents/json-extract.ts index eb7295a5..7942abb2 100644 --- a/src/core/agents/json-extract.ts +++ b/src/core/agents/json-extract.ts @@ -85,6 +85,41 @@ export function extractLastJsonObject( return null; } +/** + * Recover an agent's structured answer from its final message text. + * + * A schema-aware pass runs first so a transcript that also contains unrelated + * JSON (tool arguments, debug dumps) cannot win over the real answer, then a + * plain pass runs so a near-miss answer surfaces its validation error instead + * of a generic "no JSON found". `validate` owns the agent's error wording for + * anything that parses; only the no-JSON-at-all case is reported here. + */ +export function parseAgentOutputJson( + text: string, + agentName: string, + validate: (value: unknown) => T, +): T { + const matched = parseAgentJson(text, (value) => { + try { + validate(value); + return true; + } catch { + return false; + } + }); + if (matched !== null) { + return validate(matched); + } + + const parsed = parseAgentJson(text); + if (parsed === null) { + throw new SyntaxError( + `${agentName} output did not contain a parseable JSON object`, + ); + } + return validate(parsed); +} + export function parseAgentJson( text: string, accepts?: (value: unknown) => boolean, diff --git a/src/core/agents/opencode.ts b/src/core/agents/opencode.ts index 3ca9fe6c..c3be2a0a 100644 --- a/src/core/agents/opencode.ts +++ b/src/core/agents/opencode.ts @@ -16,7 +16,7 @@ import { type TokenUsage, } from "./types.js"; import { appendDebugLog, serializeError } from "../debug-log.js"; -import { parseAgentJson } from "./json-extract.js"; +import { parseAgentOutputJson } from "./json-extract.js"; import { shutdownChildProcess } from "./managed-process.js"; interface OpenCodeMessagePart { @@ -217,25 +217,8 @@ function parseOpenCodeOutput( text: string, schema: AgentOutputSchema, ): AgentOutput { - const parsed = parseAgentJson(text, (value) => { - try { - validateAgentOutput(value, schema); - return true; - } catch { - return false; - } - }); - if (parsed !== null) { - return validateAgentOutput(parsed, schema); - } - - const fallbackParsed = parseAgentJson(text); - if (fallbackParsed !== null) { - return validateAgentOutput(fallbackParsed, schema); - } - - throw new SyntaxError( - "opencode output did not contain a parseable JSON object", + return parseAgentOutputJson(text, "opencode", (value) => + validateAgentOutput(value, schema), ); } diff --git a/src/core/agents/pi.ts b/src/core/agents/pi.ts index 9295c1f8..9cbcdc05 100644 --- a/src/core/agents/pi.ts +++ b/src/core/agents/pi.ts @@ -1,4 +1,3 @@ -import { execFileSync, spawn } from "node:child_process"; import { createWriteStream } from "node:fs"; import { buildAgentOutputSchema, @@ -13,6 +12,9 @@ import { parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, + spawnAgentProcess, + spawnsDetached, + terminateChildProcess, } from "./stream-utils.js"; interface PiAgentDeps { @@ -24,64 +26,6 @@ interface PiAgentDeps { type JsonRecord = Record; -function shouldUseWindowsShell( - bin: string, - platform: NodeJS.Platform, -): boolean { - if (platform !== "win32") { - return false; - } - - if (/\.(cmd|bat)$/i.test(bin)) { - return true; - } - - if (/[\\/]/.test(bin)) { - return false; - } - - try { - const resolved = execFileSync("where", [bin], { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - }); - const firstMatch = resolved - .split(/\r?\n/) - .map((line) => line.trim()) - .find(Boolean); - return firstMatch ? /\.(cmd|bat)$/i.test(firstMatch) : false; - } catch { - return false; - } -} - -function terminatePiProcess( - child: ReturnType, - platform: NodeJS.Platform, -): void { - if (platform === "win32" && child.pid) { - try { - execFileSync("taskkill", ["/T", "/F", "/PID", String(child.pid)], { - stdio: "ignore", - }); - } catch { - // Best-effort: the process may have already exited. - } - return; - } - - if (child.pid) { - try { - process.kill(-child.pid, "SIGTERM"); - return; - } catch { - // Fall back to the direct child if it was not started as a process group. - } - } - - child.kill("SIGTERM"); -} - function buildPiPrompt(prompt: string, schema: AgentOutputSchema): string { return `${prompt} @@ -221,20 +165,25 @@ export class PiAgent implements Agent { return new Promise((resolve, reject) => { const logStream = logPath ? createWriteStream(logPath) : null; - const child = spawn(this.bin, buildPiArgs(this.extraArgs), { - cwd, - detached: this.platform !== "win32", - shell: shouldUseWindowsShell(this.bin, this.platform), - stdio: ["pipe", "pipe", "pipe"], - env: process.env, - }); + const detached = spawnsDetached(this.platform); + const child = spawnAgentProcess( + this.bin, + buildPiArgs(this.extraArgs), + this.platform, + { + cwd, + detached, + stdio: ["pipe", "pipe", "pipe"], + env: process.env, + }, + ); child.stdin?.write(buildPiPrompt(prompt, this.schema)); child.stdin?.end(); if ( setupAbortHandler(signal, child, reject, () => - terminatePiProcess(child, this.platform), + terminateChildProcess(child, this.platform, { detached }), ) ) { return; diff --git a/src/core/agents/rovodev.ts b/src/core/agents/rovodev.ts index 5fc571ba..d26e6c8e 100644 --- a/src/core/agents/rovodev.ts +++ b/src/core/agents/rovodev.ts @@ -14,7 +14,7 @@ import type { } from "./types.js"; import { validateAgentOutput } from "./types.js"; import { appendDebugLog, serializeError } from "../debug-log.js"; -import { parseAgentJson } from "./json-extract.js"; +import { parseAgentOutputJson } from "./json-extract.js"; import { shutdownChildProcess } from "./managed-process.js"; interface RovoDevRequestUsageEvent { @@ -746,47 +746,27 @@ export class RovoDevAgent implements Agent { const schema = JSON.parse( readFileSync(this.schemaPath, "utf-8"), ) as AgentOutputSchema; - const parsed = parseAgentJson(finalText, (value) => { - try { - validateAgentOutput(value, schema); - return true; - } catch { - return false; - } - }); - if (parsed === null) { - const fallbackParsed = parseAgentJson(finalText); - if (fallbackParsed !== null) { - try { - validateAgentOutput(fallbackParsed, schema); - } catch (error) { - const message = - error instanceof Error ? error.message : String(error); - throw new Error(`Failed to parse rovodev output: ${message}`); - } - } - const parseError = new SyntaxError( - "rovodev output did not contain a parseable JSON object", - ); - appendDebugLog("rovodev:output:parse-error", { - sessionId, - outputTextLength: finalText.length, - outputTextSample: finalText.slice(0, 512), - error: serializeError(parseError), - }); - throw new Error(`Failed to parse rovodev output: ${parseError.message}`); - } - appendDebugLog("rovodev:output:parsed", { - sessionId, - outputTextLength: finalText.length, - }); let output; try { - output = validateAgentOutput(parsed, schema); + output = parseAgentOutputJson(finalText, "rovodev", (value) => + validateAgentOutput(value, schema), + ); } catch (error) { + if (error instanceof SyntaxError) { + appendDebugLog("rovodev:output:parse-error", { + sessionId, + outputTextLength: finalText.length, + outputTextSample: finalText.slice(0, 512), + error: serializeError(error), + }); + } const message = error instanceof Error ? error.message : String(error); throw new Error(`Failed to parse rovodev output: ${message}`); } + appendDebugLog("rovodev:output:parsed", { + sessionId, + outputTextLength: finalText.length, + }); return { output, usage, diff --git a/src/core/agents/stream-utils.test.ts b/src/core/agents/stream-utils.test.ts index 81a00491..2dbc79db 100644 --- a/src/core/agents/stream-utils.test.ts +++ b/src/core/agents/stream-utils.test.ts @@ -1,11 +1,23 @@ import { EventEmitter } from "node:events"; import { describe, it, expect, vi } from "vitest"; + +vi.mock("node:child_process", () => ({ + execFileSync: vi.fn(), + spawn: vi.fn(), +})); + +import { spawn } from "node:child_process"; import { + escapeWindowsShellArgument, + escapeWindowsShellCommand, parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, + spawnAgentProcess, } from "./stream-utils.js"; +const mockSpawn = vi.mocked(spawn); + function createMockChild() { const child = new EventEmitter() as EventEmitter & { stderr: EventEmitter; @@ -165,3 +177,78 @@ describe("setupAbortHandler", () => { expect(child.kill).toHaveBeenCalledTimes(1); }); }); + +describe("escapeWindowsShellArgument", () => { + it("quotes an argument that holds spaces so cmd.exe cannot split it", () => { + expect(escapeWindowsShellArgument("test prompt")).toBe('^"test^ prompt^"'); + }); + + it("escapes embedded quotes for the child's command-line parser", () => { + expect(escapeWindowsShellArgument('say "hi"')).toBe('^"say^ \\^"hi\\^"^"'); + }); + + it("keeps a JSON schema argument in one piece", () => { + expect(escapeWindowsShellArgument('{"type":"object"}')).toBe( + '^"{\\^"type\\^":\\^"object\\^"}^"', + ); + }); + + it("doubles a trailing backslash run so it is not read as escaping the quote", () => { + expect(escapeWindowsShellArgument("C:\\dir\\")).toBe('^"C:\\dir\\\\^"'); + }); + + it("escapes shell operators that would otherwise run as commands", () => { + expect(escapeWindowsShellArgument("a & b | c > d")).toBe( + '^"a^ ^&^ b^ ^|^ c^ ^>^ d^"', + ); + }); +}); + +describe("escapeWindowsShellCommand", () => { + it("leaves an ordinary shim path untouched", () => { + expect(escapeWindowsShellCommand("C:\\tools\\grok.cmd")).toBe( + "C:\\tools\\grok.cmd", + ); + }); + + it("escapes meta characters in a shim path", () => { + expect(escapeWindowsShellCommand("C:\\Program Files\\grok.cmd")).toBe( + "C:\\Program^ Files\\grok.cmd", + ); + }); +}); + +describe("spawnAgentProcess", () => { + it("passes argv through verbatim when no shell is involved", () => { + spawnAgentProcess("grok", ["-p", "test prompt"], "darwin", { + cwd: "/work/dir", + detached: true, + }); + + expect(mockSpawn).toHaveBeenCalledWith("grok", ["-p", "test prompt"], { + cwd: "/work/dir", + detached: true, + shell: false, + }); + }); + + it("escapes argv for a Windows shim that can only run through cmd.exe", () => { + spawnAgentProcess( + "C:\\tools\\grok.cmd", + ["-p", "test prompt", "--json-schema", '{"type":"object"}'], + "win32", + { cwd: "/work/dir" }, + ); + + expect(mockSpawn).toHaveBeenCalledWith( + "C:\\tools\\grok.cmd", + [ + '^"-p^"', + '^"test^ prompt^"', + '^"--json-schema^"', + '^"{\\^"type\\^":\\^"object\\^"}^"', + ], + { cwd: "/work/dir", shell: true }, + ); + }); +}); diff --git a/src/core/agents/stream-utils.ts b/src/core/agents/stream-utils.ts index 22899469..339a2f01 100644 --- a/src/core/agents/stream-utils.ts +++ b/src/core/agents/stream-utils.ts @@ -1,7 +1,150 @@ -import type { ChildProcess } from "node:child_process"; +import { + execFileSync, + spawn, + type ChildProcess, + type SpawnOptions, +} from "node:child_process"; import type { Readable } from "node:stream"; import type { WriteStream } from "node:fs"; +/** + * npm installs some agent CLIs as `.cmd`/`.bat` shims on Windows, which + * `spawn` can only launch through a shell. Bare names are resolved with + * `where` so a configured override that points at a shim still works. + * + * Kept module-private so every caller goes through `spawnAgentProcess`, which + * is the only place that knows how to keep argv intact once a shell is in play. + */ +function shouldUseWindowsShell( + bin: string, + platform: NodeJS.Platform, +): boolean { + if (platform !== "win32") { + return false; + } + + if (/\.(cmd|bat)$/i.test(bin)) { + return true; + } + + if (/[\\/]/.test(bin)) { + return false; + } + + try { + const resolved = execFileSync("where", [bin], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); + const firstMatch = resolved + .split(/\r?\n/) + .map((line) => line.trim()) + .find(Boolean); + return firstMatch ? /\.(cmd|bat)$/i.test(firstMatch) : false; + } catch { + return false; + } +} + +/** + * Characters `cmd.exe` interprets itself. Prefixing each with `^` makes the + * whole command line opaque to the shell so that argument splitting is left to + * the child's own command-line parser. + */ +const WINDOWS_SHELL_META_CHARS = /([()\][%!^"`<>&|;, *?])/g; + +/** + * Escape a program name for the `cmd.exe` command line. + */ +export function escapeWindowsShellCommand(command: string): string { + return command.replace(WINDOWS_SHELL_META_CHARS, "^$1"); +} + +/** + * Escape one argument for the `cmd.exe` command line. + * + * Node's `shell: true` joins argv with plain spaces and quotes nothing, so an + * argument holding spaces, quotes or JSON reaches the agent shredded into + * several tokens. The argument is first quoted the way `CommandLineToArgvW` + * expects (backslash runs before a quote are doubled, quotes are escaped), then + * every character `cmd.exe` would act on - including the quotes just added - is + * `^`-escaped so the shell passes the whole thing through untouched. + */ +export function escapeWindowsShellArgument(arg: string): string { + const quoted = `"${arg.replace(/(\\*)"/g, '$1$1\\"').replace(/(\\*)$/, "$1$1")}"`; + return quoted.replace(WINDOWS_SHELL_META_CHARS, "^$1"); +} + +/** + * Spawn an agent CLI with argv that survives the platform's spawn boundary. + * + * Everywhere but a Windows `.cmd`/`.bat` shim this is a plain `spawn`, which + * already delivers argv verbatim. Shims can only be launched through `cmd.exe`, + * so their argv is escaped first; without that, multi-word prompts and JSON + * schema arguments arrive split apart. + */ +export function spawnAgentProcess( + bin: string, + args: string[], + platform: NodeJS.Platform, + options: Omit, +): ChildProcess { + const shell = shouldUseWindowsShell(bin, platform); + if (!shell) { + return spawn(bin, args, { ...options, shell }); + } + + return spawn( + escapeWindowsShellCommand(bin), + args.map(escapeWindowsShellArgument), + { ...options, shell }, + ); +} + +/** + * Whether an agent CLI should be spawned as its own process-group leader. + * Group leadership is what lets `terminateChildProcess` reach the tools the + * agent spawned; Windows has no process groups and uses `taskkill /T` instead. + */ +export function spawnsDetached(platform: NodeJS.Platform): boolean { + return platform !== "win32"; +} + +/** + * Terminate a spawned agent CLI and, where possible, everything it started. + * + * `detached` must match the `detached` option the child was spawned with: + * signalling the negated pid only reaches the agent's own descendants when the + * child leads its own process group, and would otherwise signal gnhf's group. + */ +export function terminateChildProcess( + child: ChildProcess, + platform: NodeJS.Platform, + { detached }: { detached: boolean }, +): void { + if (platform === "win32" && child.pid) { + try { + execFileSync("taskkill", ["/T", "/F", "/PID", String(child.pid)], { + stdio: "ignore", + }); + } catch { + // Best-effort: the process may have already exited. + } + return; + } + + if (detached && child.pid) { + try { + process.kill(-child.pid, "SIGTERM"); + return; + } catch { + // Fall back to the direct child if it was not started as a process group. + } + } + + child.kill("SIGTERM"); +} + /** * Wire stderr collection, spawn-error handling, and the common close-handler * prefix (logStream.end + non-zero exit code rejection) for a child process. diff --git a/src/core/bootstrap-config.golden.yml b/src/core/bootstrap-config.golden.yml index 4117a30a..0b79b438 100644 --- a/src/core/bootstrap-config.golden.yml +++ b/src/core/bootstrap-config.golden.yml @@ -10,6 +10,7 @@ agent: claude # codex: /path/to/custom-codex # copilot: /path/to/custom-copilot # pi: /path/to/custom-pi +# grok: /path/to/custom-grok # Native agent CLI arg overrides (optional) # ACP targets do not support path or arg overrides. @@ -30,6 +31,9 @@ agent: claude # - gpt-5.5 # - --thinking # - high +# grok: +# - -m +# - grok-4.5-build # Custom ACP target commands (optional) # Maps acp: names to spawn commands. Useful for naming a diff --git a/src/core/config.test.ts b/src/core/config.test.ts index fe188bb9..beabbcd7 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -430,6 +430,38 @@ describe("loadConfig", () => { ); }); + it("allows safe agentArgsOverride.grok flags", () => { + mockReadFileSync.mockReturnValue( + "agentArgsOverride:\n grok:\n - -m\n - grok-4.5-build\n - --always-approve\n", + ); + + const config = loadConfig(); + + expect(config.agentArgsOverride).toEqual({ + grok: ["-m", "grok-4.5-build", "--always-approve"], + }); + }); + + it.each([ + "-p", + "--single", + "--prompt-file", + "--prompt-file=/tmp/prompt.md", + "--prompt-json", + "--output-format", + "--output-format=json", + "--json-schema", + '--json-schema={"type":"object"}', + ])("throws when agentArgsOverride.grok contains reserved flag %s", (flag) => { + mockReadFileSync.mockReturnValue( + `agentArgsOverride:\n grok:\n - ${flag}\n`, + ); + + expect(() => loadConfig()).toThrow( + /agentArgsOverride\.grok\[0\].*managed by gnhf/, + ); + }); + it("reads acpRegistryOverrides from config", () => { mockReadFileSync.mockReturnValue( [ diff --git a/src/core/config.ts b/src/core/config.ts index 5613639f..f2831e6e 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -13,6 +13,7 @@ export const AGENT_NAMES = [ "opencode", "copilot", "pi", + "grok", ] as const; export type AgentName = (typeof AGENT_NAMES)[number]; @@ -191,6 +192,20 @@ function isReservedAgentArg(agent: AgentName, arg: string): boolean { arg === "--api-key" || arg.startsWith("--api-key=") ); + case "grok": + return ( + arg === "-p" || + arg === "--single" || + arg.startsWith("--single=") || + arg === "--prompt-file" || + arg.startsWith("--prompt-file=") || + arg === "--prompt-json" || + arg.startsWith("--prompt-json=") || + arg === "--output-format" || + arg.startsWith("--output-format=") || + arg === "--json-schema" || + arg.startsWith("--json-schema=") + ); } } @@ -524,6 +539,7 @@ function serializeConfig(config: Config): string { "# codex: /path/to/custom-codex", "# copilot: /path/to/custom-copilot", "# pi: /path/to/custom-pi", + "# grok: /path/to/custom-grok", "", "# Native agent CLI arg overrides (optional)", "# ACP targets do not support path or arg overrides.", @@ -544,6 +560,9 @@ function serializeConfig(config: Config): string { "# - gpt-5.5", "# - --thinking", "# - high", + "# grok:", + "# - -m", + "# - grok-4.5-build", "", "# Custom ACP target commands (optional)", "# Maps acp: names to spawn commands. Useful for naming a",