From 27c6381916794577a536dd7e83427ae687e423e6 Mon Sep 17 00:00:00 2001 From: DBarr3 <143002219+DBarr3@users.noreply.github.com> Date: Thu, 20 Aug 2026 06:10:31 -0400 Subject: [PATCH] fix(agent): stop blocking CLI startup on an unbounded git status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constructing a ToolExecutor built a GitCommitGuard unconditionally, and the guard's constructor ran two synchronous git calls — `git status --porcelain=v1 -z --untracked-files=all` and `git diff --cached`. Both ran on every `aether agent` and `aether chat` start, before the first turn, whether or not the run ever called git_commit. `git status` reports the whole repository regardless of where it runs, and `-uall` enumerates every untracked path individually, so the cost was O(entire repository) with the event loop frozen for the duration. Three changes: 1. The guard is now built by ToolExecutor on the first MUTATING tool call (write_file, run_shell, run_tests, git_commit) instead of in the constructor. Startup runs no git at all, and a read-only session never runs any. It is not deferred as far as the first git_commit: constructing the guard is what takes the "already dirty" baseline, and a baseline taken at commit time equals the current state, which would make git_commit an unconditional no-op. 2. Both probes are bounded to the workspace subtree with a `-- .` pathspec, and every git invocation now carries `--no-optional-locks` so inspecting a repository no longer rewrites the user's index as a side effect. 3. Test workspaces route through test/tmp_workspace.ts, which pins GIT_CEILING_DIRECTORIES at the temp root. On a machine whose temp directory sits inside a repository, repository discovery previously walked out of the temp workspace into that ambient repository and every probe became O(entire home directory); the suite hung there. CI never saw it because hosted runners' temp roots are not inside a repository. Measured on Windows from a temp workspace inside a version-controlled home directory: the unbounded probe did not finish inside a 120s cap (exit 124); the same probe with the pathspec returns in 0.51s. Full suite with the default TEMP now completes: 1119 tests, 137.0s of test time. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/git_commit_guard.ts | 71 ++++++++++++++++++++++++++++++++--- src/core/tool_executor.ts | 50 ++++++++++++++++++++++-- test/bridge.test.ts | 26 ++++++------- test/git_commit_guard.test.ts | 19 ++++++++++ test/process_tree.test.ts | 12 +++--- test/release_canaries.test.ts | 6 +-- test/tmp_workspace.ts | 44 ++++++++++++++++++++++ test/tool_executor.test.ts | 50 ++++++++++++++++++++---- test/tool_registry.test.ts | 6 +-- test/web.test.ts | 10 ++--- 10 files changed, 247 insertions(+), 47 deletions(-) create mode 100644 test/tmp_workspace.ts diff --git a/src/core/git_commit_guard.ts b/src/core/git_commit_guard.ts index b284551..438bb36 100644 --- a/src/core/git_commit_guard.ts +++ b/src/core/git_commit_guard.ts @@ -11,11 +11,57 @@ export interface GitRunner { run(args: string[]): GitRunResult; } +/** + * Options prefixed to EVERY git invocation this runner makes. + * + * `--no-optional-locks` is not cosmetic. Without it a plain `git status` + * rewrites the user's index as a side effect of being read, so merely starting + * the agent mutates the repository and can lose a race with the user's own git + * process in the same worktree. Inspection must be inspection. + * `core.literalPathspecs` keeps a path that happens to begin with `:` from + * being reinterpreted as pathspec magic. + */ +export const GIT_GLOBAL_ARGS: readonly string[] = [ + "--no-optional-locks", + "-c", + "core.literalPathspecs=true", +]; + +/** + * The pathspec appended to the two repository-state probes. + * + * `git status` reports the WHOLE repository, not the directory it runs in. + * Unbounded, the probe is O(entire repository) even when the agent's workspace + * is one small directory inside it, and `--untracked-files=all` turns that into + * an enumeration of every untracked path individually. The guard only ever + * stages paths inside the workspace, so bounding the probe to the workspace + * subtree removes work that could never have produced a candidate. + */ +export const WORKSPACE_PATHSPEC: readonly string[] = ["--", "."]; + +/** `git status` probe: dirty paths in the workspace subtree, NUL-delimited. */ +export const STATUS_PROBE: readonly string[] = [ + "status", + "--porcelain=v1", + "-z", + "--untracked-files=all", + ...WORKSPACE_PATHSPEC, +]; + +/** `git diff --cached` probe: staged paths in the workspace subtree. */ +export const STAGED_PROBE: readonly string[] = [ + "diff", + "--cached", + "--name-only", + "-z", + ...WORKSPACE_PATHSPEC, +]; + export class SpawnGitRunner implements GitRunner { constructor(private readonly cwd: string) {} run(args: string[]): GitRunResult { - const result = spawnSync("git", ["-c", "core.literalPathspecs=true", ...args], { + const result = spawnSync("git", [...GIT_GLOBAL_ARGS, ...args], { cwd: this.cwd, shell: false, encoding: "utf8", @@ -98,14 +144,27 @@ export interface GitCommitResult { exitCode: number; } +/** + * Staging guard for the `git_commit` tool. + * + * Constructing the guard takes the "already dirty before the agent touched + * anything" baseline, so construction must happen BEFORE the run's first + * workspace mutation — it cannot be deferred all the way to the first + * `git_commit` call. At that point the baseline would equal the current state, + * the set difference in `planGitCommit` would be empty for every commit, and + * the tool would become an unconditional no-op. The owner (`ToolExecutor`) + * therefore constructs the guard lazily on the first MUTATING tool call rather + * than at CLI start: still before anything can have changed, but off the + * startup path and never paid at all by a run that only reads. + */ export class GitCommitGuard { private readonly initialDirty: string[]; private readonly initialStaged: string[]; private readonly initError: string | null; constructor(private readonly runner: GitRunner) { - const dirty = runner.run(["status", "--porcelain=v1", "-z", "--untracked-files=all"]); - const staged = runner.run(["diff", "--cached", "--name-only", "-z"]); + const dirty = runner.run([...STATUS_PROBE]); + const staged = runner.run([...STAGED_PROBE]); this.initialDirty = dirty.ok ? parsePorcelainPaths(dirty.stdout) : []; this.initialStaged = staged.ok ? parseNulPaths(staged.stdout) : []; this.initError = dirty.ok && staged.ok ? null : "not a usable git repository"; @@ -114,8 +173,8 @@ export class GitCommitGuard { commit(message: string): GitCommitResult { if (this.initError) return { output: "[git_commit refused: " + this.initError + "]", exitCode: 1 }; - const dirty = this.runner.run(["status", "--porcelain=v1", "-z", "--untracked-files=all"]); - const staged = this.runner.run(["diff", "--cached", "--name-only", "-z"]); + const dirty = this.runner.run([...STATUS_PROBE]); + const staged = this.runner.run([...STAGED_PROBE]); if (!dirty.ok || !staged.ok) { return { output: "[git_commit refused: unable to inspect repository state]", exitCode: 1 }; } @@ -131,7 +190,7 @@ export class GitCommitGuard { const added = this.runner.run(["add", "-A", "--", ...plan.candidates]); if (!added.ok) return { output: "[git_commit add failed: " + added.stderr.trim() + "]", exitCode: added.exitCode }; - const after = this.runner.run(["diff", "--cached", "--name-only", "-z"]); + const after = this.runner.run([...STAGED_PROBE]); const stagedAfter = after.ok ? parseNulPaths(after.stdout) : []; if (!after.ok || !samePaths(stagedAfter, plan.candidates)) { this.runner.run(["reset", "-q", "HEAD", "--", ...plan.candidates]); diff --git a/src/core/tool_executor.ts b/src/core/tool_executor.ts index 6c4a670..b2a097c 100644 --- a/src/core/tool_executor.ts +++ b/src/core/tool_executor.ts @@ -25,6 +25,17 @@ const DEFAULT_TEST_CMD = ""; const SNAPSHOT_MAX_BYTES = 1024 * 1024; const SEARCH_MAX_HITS = 40; const SEARCH_SKIP_DIRS = new Set([".git", "node_modules", "dist"]); +/** + * Tools that can change the workspace. The git commit guard's baseline must be + * taken before the first of these runs, and there is no reason to take it + * before that: a session that only reads never touches git at all. + */ +const MUTATING_TOOLS: ReadonlySet = new Set([ + "write_file", + "run_shell", + "run_tests", + "git_commit", +]); /** Per-call execution controls for the shell-backed tools. */ export interface RunOptions { @@ -51,7 +62,12 @@ export interface FileSnapshot { export class ToolExecutor { private readonly root: string; - private readonly committer: GitCommitGuard; + /** + * Built on first use by armCommitGuard(), never in the constructor. See the + * comment there — constructing it runs synchronous git probes, and doing that + * eagerly froze the CLI before its first turn. + */ + private committer: GitCommitGuard | null = null; constructor( cwd: string, @@ -60,7 +76,29 @@ export class ToolExecutor { // Canonicalize the root once (resolve any symlinks in the workspace path). const r = resolve(cwd); this.root = existsSync(r) ? realpathSync(r) : r; - this.committer = new GitCommitGuard(new SpawnGitRunner(this.root)); + } + + /** + * Build the git commit guard if this run has not built one yet. + * + * Constructing the guard is what takes its "dirty before the agent started" + * baseline, and that costs two synchronous `git` calls. Doing it in the + * ToolExecutor constructor meant every `aether agent` / `aether chat` paid + * for it at startup, on the main thread, before the first turn rendered — + * whether or not the run ever committed anything. + * + * The baseline still has to be taken before the agent's first mutation, so + * it cannot be deferred to the first `git_commit`: at that point the baseline + * would equal the current state and every commit would find nothing new. + * The correct moment is the first MUTATING tool call — the workspace is + * provably untouched by this run, the CLI is already interactive, and a + * session that only reads never pays the cost at all. + */ + private armCommitGuard(): GitCommitGuard { + if (!this.committer) { + this.committer = new GitCommitGuard(new SpawnGitRunner(this.root)); + } + return this.committer; } /** @@ -219,6 +257,9 @@ export class ToolExecutor { return { output: `[tool ${name} rejected: ${validation.error}]`, exitCode: 1 }; } const args = validation.args; + // Take the git baseline before the first tool that could change the + // workspace — never at construction, and never as late as git_commit. + if (MUTATING_TOOLS.has(name)) this.armCommitGuard(); try { switch (name as ToolName) { case "read_file": @@ -259,6 +300,9 @@ export class ToolExecutor { return { output: `[tool ${name} rejected: ${validation.error}]`, exitCode: 1 }; } const args = validation.args; + // run_shell / run_tests never reach execute() — arm here too, before the + // command that may edit the workspace actually starts. + if (MUTATING_TOOLS.has(name)) this.armCommitGuard(); if (name === "web_search") { const limit = Number(args["limit"]); const text = await webSearch( @@ -367,7 +411,7 @@ export class ToolExecutor { } private gitCommit(message: string): ToolResult { - return this.committer.commit(message); + return this.armCommitGuard().commit(message); } } diff --git a/test/bridge.test.ts b/test/bridge.test.ts index 869cb33..d5c4614 100644 --- a/test/bridge.test.ts +++ b/test/bridge.test.ts @@ -1,8 +1,8 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, readFileSync, rmSync, symlinkSync, mkdirSync, existsSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { readFileSync, rmSync, symlinkSync, mkdirSync, existsSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; +import { TEMP_ROOT, tmpWorkspace } from "./tmp_workspace.js"; import { fileURLToPath } from "node:url"; import { @@ -146,7 +146,7 @@ test("LineBuffer yields multiple complete lines in one chunk", () => { // --- tool executor (one path-guard, [exit N] shape) ------------------------ test("ToolExecutor writes then reads a file in the workspace", () => { - const dir = mkdtempSync(join(tmpdir(), "aether-tool-")); + const dir = tmpWorkspace("aether-tool-"); try { const ex = new ToolExecutor(dir); const w = ex.execute("write_file", { path: "a.txt", content: "hello" }); @@ -161,7 +161,7 @@ test("ToolExecutor writes then reads a file in the workspace", () => { }); test("ToolExecutor refuses a path escaping the workspace", () => { - const dir = mkdtempSync(join(tmpdir(), "aether-tool-")); + const dir = tmpWorkspace("aether-tool-"); try { const ex = new ToolExecutor(dir); const r = ex.execute("read_file", { path: "../../etc/passwd" }); @@ -173,7 +173,7 @@ test("ToolExecutor refuses a path escaping the workspace", () => { }); test("repo_search finds literal matches recursively without external grep", () => { - const dir = mkdtempSync(join(tmpdir(), "aether-search-")); + const dir = tmpWorkspace("aether-search-"); try { mkdirSync(join(dir, "sub")); mkdirSync(join(dir, "node_modules")); @@ -197,7 +197,7 @@ test("repo_search finds literal matches recursively without external grep", () = }); test("repo_search caps matches at 40 hits", () => { - const dir = mkdtempSync(join(tmpdir(), "aether-search-cap-")); + const dir = tmpWorkspace("aether-search-cap-"); try { writeFileSync(join(dir, "many.txt"), Array.from({ length: 50 }, (_, i) => `needle ${i}`).join("\n"), "utf8"); const r = new ToolExecutor(dir).execute("repo_search", { query: "needle" }); @@ -222,7 +222,7 @@ test("capHeadTail keeps the END (pytest summary) when output is over the cap", ( }); test("unknown tool returns a clear error, never throws", () => { - const ex = new ToolExecutor(tmpdir()); + const ex = new ToolExecutor(TEMP_ROOT); const r = ex.execute("frobnicate", {}); assert.equal(r.exitCode, 1); assert.match(r.output, /unknown tool/); @@ -254,7 +254,7 @@ class FakeBrain implements Brain { } test("hostLoop executes a tool_call and feeds the result back", async () => { - const dir = mkdtempSync(join(tmpdir(), "aether-host-")); + const dir = tmpWorkspace("aether-host-"); try { const brain = new FakeBrain(); const exec = new ToolExecutor(dir); @@ -279,7 +279,7 @@ test("hostLoop executes a tool_call and feeds the result back", async () => { }); test("hostLoop gate denies a tool_call: not executed, brain gets a refusal result", async () => { - const dir = mkdtempSync(join(tmpdir(), "aether-gate-")); + const dir = tmpWorkspace("aether-gate-"); try { const brain = new FakeBrain(); // emits write_file id=c1 then done on result const exec = new ToolExecutor(dir); @@ -327,7 +327,7 @@ class TwoCallBrain implements Brain { } test("two sequential tool calls: each result pairs to its own id, in order", async () => { - const dir = mkdtempSync(join(tmpdir(), "aether-corr-")); + const dir = tmpWorkspace("aether-corr-"); try { const exec = new ToolExecutor(dir); exec.execute("write_file", { path: "a.txt", content: "AAA" }); @@ -347,8 +347,8 @@ test("two sequential tool calls: each result pairs to its own id, in order", asy // --- probe 2: path-guard canonicalization (the security boundary) ---------- test("path-guard rejects traversal, absolute, and symlink escapes", () => { - const dir = mkdtempSync(join(tmpdir(), "aether-guard-")); - const outside = mkdtempSync(join(tmpdir(), "aether-outside-")); + const dir = tmpWorkspace("aether-guard-"); + const outside = tmpWorkspace("aether-outside-"); try { const ex = new ToolExecutor(dir); // .. traversal @@ -378,7 +378,7 @@ test("run_shell surfaces a non-zero exit code and captures stderr", async (t) => // `node -e` (not a platform-specific shell one-liner) so this passes // identically on Windows and POSIX without a win32/posix branch, and runs // in an isolated tmpdir so it can't leave stray output under process.cwd(). - const ex = new ToolExecutor(mkdtempSync(join(tmpdir(), "aether-sh-"))); + const ex = new ToolExecutor(tmpWorkspace("aether-sh-")); const r = await ex.executeAsync("run_shell", { command: `node -e "process.stderr.write('boom'); process.exit(3)"`, }); diff --git a/test/git_commit_guard.test.ts b/test/git_commit_guard.test.ts index b1cc5b7..9f81f37 100644 --- a/test/git_commit_guard.test.ts +++ b/test/git_commit_guard.test.ts @@ -1,7 +1,10 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { + GIT_GLOBAL_ARGS, GitCommitGuard, + STAGED_PROBE, + STATUS_PROBE, parseNulPaths, parsePorcelainPaths, planGitCommit, @@ -27,6 +30,22 @@ class FakeRunner implements GitRunner { } } +test("repository probes are bounded to the workspace and take no optional locks", () => { + // `git status` reports the whole repository regardless of where it runs, and + // `--untracked-files=all` enumerates every untracked path individually. A + // workspace that is a small directory inside a large repository therefore + // paid O(entire repository) per probe — measured at over 120s on a machine + // whose temp directory sits inside a version-controlled home directory. The + // pathspec bounds it to the subtree the guard can actually stage from. + assert.ok(STATUS_PROBE.includes("--"), "status probe must carry a pathspec"); + assert.deepEqual(STATUS_PROBE.slice(-2), ["--", "."]); + assert.deepEqual(STAGED_PROBE.slice(-2), ["--", "."]); + // Reading a repository must not write to it: without this, merely starting + // the agent rewrites the user's index. + assert.ok(GIT_GLOBAL_ARGS.includes("--no-optional-locks")); + assert.ok(GIT_GLOBAL_ARGS.includes("core.literalPathspecs=true")); +}); + test("porcelain parsers handle rename records, spaces, and deduplication", () => { assert.deepEqual( parsePorcelainPaths(" M dirty file.txt\0R new.ts\0old.ts\0?? added.ts\0"), diff --git a/test/process_tree.test.ts b/test/process_tree.test.ts index 772b12a..4f8e678 100644 --- a/test/process_tree.test.ts +++ b/test/process_tree.test.ts @@ -11,9 +11,9 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { writeFileSync } from "node:fs"; import { join } from "node:path"; +import { tmpWorkspace } from "./tmp_workspace.js"; import { ToolExecutor } from "../src/core/tool_executor.js"; /** True while a pid exists. Signal 0 tests for existence without delivering. */ @@ -59,7 +59,7 @@ function pidsFrom(output: string): { child: number | null; grandchild: number | } test("a timed-out command kills its whole tree, not just the shell", async () => { - const dir = mkdtempSync(join(tmpdir(), "aether-tree-")); + const dir = tmpWorkspace("aether-tree-"); const script = treeScript(dir); const exec = new ToolExecutor(dir); @@ -78,7 +78,7 @@ test("a timed-out command kills its whole tree, not just the shell", async () => }); test("an aborted command kills its whole tree and reports aborted, not timed out", async () => { - const dir = mkdtempSync(join(tmpdir(), "aether-abort-")); + const dir = tmpWorkspace("aether-abort-"); const script = treeScript(dir); const exec = new ToolExecutor(dir); const controller = new AbortController(); @@ -104,7 +104,7 @@ test("an aborted command kills its whole tree and reports aborted, not timed out }); test("a normal command still returns its real exit code and output", async () => { - const dir = mkdtempSync(join(tmpdir(), "aether-ok-")); + const dir = tmpWorkspace("aether-ok-"); const exec = new ToolExecutor(dir); const ok = await exec.executeAsync("run_shell", { command: `"${process.execPath}" -e "console.log('hi')"` }); @@ -119,7 +119,7 @@ test("the event loop keeps running while a command is in flight", async () => { // spawnSync blocked the loop outright, freezing heartbeats, renderers and any // AbortController for the duration. Proving a timer fires during the call is // what distinguishes async execution from a merely faster synchronous one. - const dir = mkdtempSync(join(tmpdir(), "aether-loop-")); + const dir = tmpWorkspace("aether-loop-"); const exec = new ToolExecutor(dir); let ticks = 0; const timer = setInterval(() => { diff --git a/test/release_canaries.test.ts b/test/release_canaries.test.ts index b68b213..52a50d1 100644 --- a/test/release_canaries.test.ts +++ b/test/release_canaries.test.ts @@ -19,9 +19,9 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +import { tmpWorkspace } from "./tmp_workspace.js"; import { ToolExecutor } from "../src/core/tool_executor.js"; import { ContextRegistry } from "../src/core/context_registry.js"; @@ -31,7 +31,7 @@ test("canary 1: a refused write never lands, and the write path is real", async // bridge.test.ts already proves the brain receives a refusal. What it does // not check is the disk. A gate that refuses in the transcript while the // write still lands is the failure that matters, and it would pass there. - const dir = mkdtempSync(join(tmpdir(), "aether-canary1-")); + const dir = tmpWorkspace("aether-canary1-"); const target = join(dir, "guarded.txt"); writeFileSync(target, "ORIGINAL\n"); const exec = new ToolExecutor(dir); diff --git a/test/tmp_workspace.ts b/test/tmp_workspace.ts new file mode 100644 index 0000000..f06750d --- /dev/null +++ b/test/tmp_workspace.ts @@ -0,0 +1,44 @@ +// Temp workspaces for tests that build a ToolExecutor. +// +// Every workspace here lives under os.tmpdir(). On a hosted CI runner that is +// harmless: the temp root is not inside a git repository, so any `git` command +// an executor runs from a temp workspace fails discovery immediately. On a +// developer machine it is not harmless. If the temp root happens to sit inside +// a repository — a Windows box whose %TEMP% is C:\Users\\AppData\Local\Temp +// and whose home directory is version-controlled is the common case — repository +// discovery walks up out of the temp directory, finds that ambient repository, +// and every probe becomes O(entire home directory). Measured on such a machine: +// a single `git status --porcelain=v1 -z --untracked-files=all` from a temp +// workspace did not finish inside a 120s cap, and the full suite hung. +// +// Pinning GIT_CEILING_DIRECTORIES at the temp root stops discovery there, so a +// temp workspace is "not a git repository" on every machine — the same thing CI +// has always seen. It is preferred over `git init`-ing each workspace because +// it changes nothing about what the tests exercise: a `git init` would silently +// convert the non-repository paths these tests cover into live-repository paths +// and spawn an extra git process per test, whereas the ceiling only removes an +// accident of where the temp directory happens to live. +import { mkdtempSync, realpathSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { delimiter, join } from "node:path"; + +/** Canonical temp root; also the ceiling for git repository discovery. */ +export const TEMP_ROOT = realpathSync(tmpdir()); + +function pinGitCeiling(): void { + const current = process.env["GIT_CEILING_DIRECTORIES"]; + const parts = current ? current.split(delimiter).filter(Boolean) : []; + if (parts.includes(TEMP_ROOT)) return; + parts.push(TEMP_ROOT); + process.env["GIT_CEILING_DIRECTORIES"] = parts.join(delimiter); +} + +// Applied at import time so it is in place before any test body runs. Child +// processes inherit process.env, which is how the executor's `git` calls see it. +pinGitCeiling(); + +/** Create a temp workspace directory under the ceiling-pinned temp root. */ +export function tmpWorkspace(prefix: string): string { + pinGitCeiling(); + return mkdtempSync(join(TEMP_ROOT, prefix)); +} diff --git a/test/tool_executor.test.ts b/test/tool_executor.test.ts index adabae8..5a00d25 100644 --- a/test/tool_executor.test.ts +++ b/test/tool_executor.test.ts @@ -1,15 +1,15 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { mkdirSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +import { tmpWorkspace } from "./tmp_workspace.js"; import { spawnSync } from "node:child_process"; import { ToolExecutor } from "../src/core/tool_executor.js"; const canSpawnGit = !spawnSync("git", ["--version"], { encoding: "utf8" }).error; function initRepo(): string { - const dir = mkdtempSync(join(tmpdir(), "aether-gitcommit-")); + const dir = tmpWorkspace("aether-gitcommit-"); spawnSync("git", ["init", "-q"], { cwd: dir }); spawnSync("git", ["config", "user.email", "test@example.com"], { cwd: dir }); spawnSync("git", ["config", "user.name", "Test"], { cwd: dir }); @@ -24,7 +24,11 @@ test("git_commit passes a message with shell metacharacters through unexecuted", const dir = initRepo(); try { const exec = new ToolExecutor(dir); - writeFileSync(join(dir, "a.txt"), "changed\n"); + // Write through the tool, the way the agent does. The commit guard's + // baseline is armed by the first mutating tool call, so a change made + // behind the executor's back is (correctly) part of the baseline and would + // not be a commit candidate. + exec.execute("write_file", { path: "a.txt", content: "changed\n" }); const r = exec.execute("git_commit", { message: 'fix "cap & retry" `whoami` $(id)' }); assert.equal(r.exitCode, 0); const log = spawnSync("git", ["log", "-1", "--pretty=%s"], { cwd: dir, encoding: "utf8" }); @@ -52,7 +56,7 @@ test("run_tests with no explicit command and no configured testCmd does not defa // silent fallback to a real test runner. brain_protocol.ts's wire-encoding // default was fixed there; this covers the sibling default in ToolExecutor // itself, which is what actually executes brain-initiated run_tests calls. - const dir = mkdtempSync(join(tmpdir(), "aether-runtests-")); + const dir = tmpWorkspace("aether-runtests-"); try { const exec = new ToolExecutor(dir); // no testCmd passed — must NOT become "pytest -q" const r = await exec.executeAsync("run_tests", {}); @@ -64,7 +68,7 @@ test("run_tests with no explicit command and no configured testCmd does not defa }); test("run_tests still honors an explicit command even with no configured testCmd", async (t) => { - const dir = mkdtempSync(join(tmpdir(), "aether-runtests-explicit-")); + const dir = tmpWorkspace("aether-runtests-explicit-"); try { const exec = new ToolExecutor(dir); const r = await exec.executeAsync("run_tests", { command: process.platform === "win32" ? "exit 0" : "true" }); @@ -76,7 +80,7 @@ test("run_tests still honors an explicit command even with no configured testCmd }); test("run_tests honors a configured testCmd when the call omits an explicit command", async (t) => { - const dir = mkdtempSync(join(tmpdir(), "aether-runtests-cfg-")); + const dir = tmpWorkspace("aether-runtests-cfg-"); try { const exec = new ToolExecutor(dir, process.platform === "win32" ? "exit 0" : "true"); const r = await exec.executeAsync("run_tests", {}); @@ -87,12 +91,42 @@ test("run_tests honors a configured testCmd when the call omits an explicit comm } }); +test("the commit guard is armed by the first mutating tool, not by construction", async (t) => { + // Constructing a ToolExecutor used to run two synchronous `git` calls, which + // froze `aether agent` / `aether chat` before the first turn even in runs + // that never committed. The baseline now comes from the first MUTATING tool + // call. This asserts the whole ordering at once: construction does not arm + // it, a read does not arm it, and the write does — so a change made behind + // the executor's back before that point is baseline, not a commit candidate. + if (!canSpawnGit) { t.skip("sandbox blocks child process spawning"); return; } + const dir = initRepo(); + try { + const exec = new ToolExecutor(dir); + writeFileSync(join(dir, "a.txt"), "edited outside the agent\n"); + assert.equal(exec.execute("read_file", { path: "a.txt" }).exitCode, 0); + + const wrote = exec.execute("write_file", { path: "b.txt", content: "agent wrote this\n" }); + assert.equal(wrote.exitCode, 0); + + const r = exec.execute("git_commit", { message: "feat: only what the agent touched" }); + assert.equal(r.exitCode, 0, r.output); + + const named = spawnSync("git", ["show", "--name-only", "--pretty=format:", "HEAD"], { + cwd: dir, + encoding: "utf8", + }).stdout.split("\n").map((line) => line.trim()).filter(Boolean); + assert.deepEqual(named, ["b.txt"], "must commit the agent's write and not the out-of-band edit"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test("git_commit surfaces a real failure instead of reporting the old HEAD as success", async (t) => { if (!canSpawnGit) { t.skip("sandbox blocks child process spawning"); return; } const dir = initRepo(); try { const exec = new ToolExecutor(dir); - writeFileSync(join(dir, "a.txt"), "changed again\n"); + exec.execute("write_file", { path: "a.txt", content: "changed again\n" }); // Break the repo's ability to commit: a pre-commit hook that always fails. const hookPath = join(dir, ".git", "hooks", "pre-commit"); mkdirSync(join(dir, ".git", "hooks"), { recursive: true }); diff --git a/test/tool_registry.test.ts b/test/tool_registry.test.ts index ec90f72..3c7a011 100644 --- a/test/tool_registry.test.ts +++ b/test/tool_registry.test.ts @@ -1,8 +1,8 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { existsSync, mkdtempSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { existsSync } from "node:fs"; import { join } from "node:path"; +import { tmpWorkspace } from "./tmp_workspace.js"; import { TOOLS } from "../src/core/brain_protocol.js"; import { ToolExecutor } from "../src/core/tool_executor.js"; import { @@ -70,7 +70,7 @@ test("validators reject unknown, malformed, oversized, and extra arguments", () }); test("executor rejects malformed calls before filesystem side effects", () => { - const dir = mkdtempSync(join(tmpdir(), "aether-tool-schema-")); + const dir = tmpWorkspace("aether-tool-schema-"); const executor = new ToolExecutor(dir); const result = executor.execute("write_file", { path: "created.txt", content: 42 }); assert.equal(result.exitCode, 1); diff --git a/test/web.test.ts b/test/web.test.ts index cac7a59..491ea6f 100644 --- a/test/web.test.ts +++ b/test/web.test.ts @@ -10,7 +10,7 @@ import { webSearch, } from "../src/core/web.js"; import { ToolExecutor } from "../src/core/tool_executor.js"; -import { tmpdir } from "node:os"; +import { TEMP_ROOT } from "./tmp_workspace.js"; // --- isSafeUrl: the SSRF guard (refuse loopback/private/link-local + non-http) --- test("isSafeUrl refuses non-http(s) schemes", () => { @@ -206,27 +206,27 @@ test("webSearch happy path: parses DDG-shaped results via injected transport", a // --- ToolExecutor.executeAsync: web tools route through the executor -------- test("executeAsync('web_fetch') refuses a loopback url via the production guard", async () => { - const ex = new ToolExecutor(tmpdir()); + const ex = new ToolExecutor(TEMP_ROOT); const r = await ex.executeAsync("web_fetch", { url: "http://127.0.0.1:1/" }); assert.equal(r.exitCode, 0, "web tools are advisory output, exit 0"); assert.match(r.output, /\[web_fetch refused:/); }); test("executeAsync('web_search') refuses a non-http url and returns a string", async () => { - const ex = new ToolExecutor(tmpdir()); + const ex = new ToolExecutor(TEMP_ROOT); const r = await ex.executeAsync("web_search", { query: "" }); assert.equal(typeof r.output, "string"); }); test("sync execute() on a web tool points at the async path (no silent no-op)", () => { - const ex = new ToolExecutor(tmpdir()); + const ex = new ToolExecutor(TEMP_ROOT); const r = ex.execute("web_fetch", { url: "https://example.com" }); assert.equal(r.exitCode, 1); assert.match(r.output, /async/); }); test("executeAsync delegates the 6 sync tools to execute() unchanged", async () => { - const ex = new ToolExecutor(tmpdir()); + const ex = new ToolExecutor(TEMP_ROOT); const r = await ex.executeAsync("frobnicate", {}); assert.equal(r.exitCode, 1); assert.match(r.output, /unknown tool/);