Skip to content

fix(agent): stop blocking CLI startup on an unbounded git status - #89

Merged
AetherAI3 merged 1 commit into
mainfrom
fix/lazy-git-commit-guard
Aug 20, 2026
Merged

fix(agent): stop blocking CLI startup on an unbounded git status#89
AetherAI3 merged 1 commit into
mainfrom
fix/lazy-git-commit-guard

Conversation

@AetherAI3

@AetherAI3 AetherAI3 commented Aug 20, 2026

Copy link
Copy Markdown
Owner

The defect

new ToolExecutor(cwd) unconditionally constructed a GitCommitGuard, and the guard's constructor ran two synchronous spawnSync git calls:

git status --porcelain=v1 -z --untracked-files=all
git diff --cached --name-only -z

Those ran on every aether agent and every aether chat start — before the first turn, on the main thread, whether or not the run ever called git_commit. Two properties make that expensive rather than merely wasteful:

  • git status reports the whole repository, not the directory it is invoked from. If the workspace is a small directory inside a large repository, the probe still walks the large repository.
  • --untracked-files=all enumerates every untracked path individually rather than collapsing untracked directories.

With no pathspec, the cost is O(entire worktree) and the event loop is frozen for its duration, so the CLI shows nothing at all while it runs. The same pair of calls repeated on every git_commit.

Measured, on this machine

Windows 11, Git Bash, %TEMP% = C:\Users\lilbe\AppData\Local\Temp, which sits inside a version-controlled home directory (C:\Users\lilbe\.git). From an empty temp workspace inside that ambient repository:

probe result
git status --porcelain=v1 -z --untracked-files=all (as shipped) did not finish inside a 120s cap — timeout returned 124
git --no-optional-locks -c core.literalPathspecs=true status --porcelain=v1 -z --untracked-files=all -- . (this PR) 0.51s

The unbounded run spent its two minutes enumerating AppData/Local/Microsoft/Windows/INetCache, .codex/visualizations/..., and similar — none of which could ever have produced a commit candidate for a workspace elsewhere on disk.

Why CI never caught this

The same code path is exercised on every CI run and has always been green, because a hosted runner's temp directory is not inside a git repository. Repository discovery from a temp workspace fails immediately there, git status returns an error in milliseconds, and the guard records "not a usable git repository". The pathology only appears when the temp root — or the workspace — happens to live inside a repository, which is the normal situation for a Windows developer whose home directory is under version control. The green matrix was accurate about the runner and silent about everybody else.

This is also the root cause of the standing "npm test hangs on this machine" hazard in this repo. Every mkdtempSync-based test that built a ToolExecutor walked the entire home tree. On the pre-fix tree the suite was killed at 300s with ToolExecutor writes then reads a file in the workspace (199518ms) still running.

What changed

1. The guard is built lazily, on the first mutating tool call.

ToolExecutor no longer constructs GitCommitGuard in its constructor. It constructs it on the first call to a tool that can change the workspace — write_file, run_shell, run_tests, git_commit. Startup runs no git at all, and a session that only reads never runs any.

Decision I was asked to make explicitly: why not defer all the way to the first git_commit. Constructing the guard is what captures the "already dirty before the agent started" baseline, and planGitCommit computes candidates as currentDirty − baseline. A baseline captured at commit time is by definition equal to the current state, so the difference is empty for every commit and git_commit becomes an unconditional no-op. The first mutating tool call is the latest moment that is still provably before the run's first mutation, so that is where the baseline is taken.

Behaviour change this implies, stated plainly: a change made to the workspace behind the executor's back — by the user, another process, or a test writing with fs directly — between process start and the agent's first mutating action is now part of the baseline and will not be a commit candidate. The baseline is now "the workspace immediately before the agent first touched it" rather than "the workspace when the process started". For a guard whose entire purpose is to avoid committing work the agent did not do, that is a tightening, not a loosening. Two tests in tool_executor.test.ts wrote their fixture file with writeFileSync and then asked for a commit; they now write through exec.execute("write_file", …), which is how the agent actually mutates a workspace, and a new test pins the ordering directly.

2. The probes are bounded and no longer take locks.

Both probes carry a -- . pathspec (STATUS_PROBE / STAGED_PROBE), bounding the walk to the workspace subtree. The guard only ever stages paths inside the workspace, so this removes work that could never have produced a candidate. Every git invocation from SpawnGitRunner now carries --no-optional-locks, so a plain git status no longer rewrites the user's index as a side effect of the agent starting — a side effect the CLI should never have had, and one that can lose a race with the user's own git process in the same worktree.

3. Test workspaces can no longer be held hostage by where os.tmpdir() lives.

New test/tmp_workspace.ts exports tmpWorkspace(prefix) and TEMP_ROOT, and pins GIT_CEILING_DIRECTORIES at the canonical temp root at import time. Every test that builds a ToolExecutor now allocates through it: bridge, process_tree, release_canaries, tool_executor, tool_registry, web.

Decision I was asked to make explicitly: GIT_CEILING_DIRECTORIES rather than git init. The ceiling stops repository discovery at the temp root, so a temp workspace is "not a git repository" on every machine — exactly what CI has always seen. It changes nothing about what the tests exercise. git init-ing each workspace would do the opposite: it would silently convert the non-repository code paths these tests cover into live-repository paths, spawn an extra git process per test, and leave the underlying accident (git discovery escaping the temp directory) in place for any future code that shells out to git from a temp directory. The ceiling is inherited by child processes through process.env, so it covers spawnSync calls the executor makes without any change to the executor.

Test evidence

Environment: worktree ~/agent-w4core-wt (a clean worktree off origin/main @ c165be0), Windows 11, Git Bash, default TEMP=C:\Users\lilbe\AppData\Local\Temp — not redirected. No git stash was used at any point in producing this branch.

$ cd ~/agent-w4core-wt && npm run typecheck
> tsc -p tsconfig.json --noEmit
(clean)

$ cd ~/agent-w4core-wt && echo "TEMP=$TEMP" && time npm test        # run 2 of 2
TEMP=C:\Users\lilbe\AppData\Local\Temp
...
ℹ tests 1119
ℹ suites 0
ℹ pass 1118
ℹ fail 0
ℹ cancelled 0
ℹ skipped 1
ℹ todo 0
ℹ duration_ms 199320.7096

real    3m45.106s

The suite completes, green. That is the proof: on the pre-fix tree, with the same default TEMP, it did not — it was killed at 300s with a single test 199s in. duration_ms is the node --test phase; the wall clock is the full npm test, which is npm run build (a tsc compile) followed by the test phase. Both numbers move with machine load; the first of the two runs reported duration_ms 137027.5677 with a 5m23.252s wall clock, so read the shape (it finishes at all, in minutes rather than never) rather than the exact seconds.

That first run had one failure — a pre-existing load-dependent flake in process_tree.test.ts, which passed on the second run and is not a regression from this branch:

✖ a timed-out command kills its whole tree, not just the shell (5269.7584ms)
  AssertionError: no grandchild pid in output: [timeout after 2s]\nCHILD:37196

The test starts a child which spawns a grandchild and asserts the grandchild printed its pid before a 1500 ms timeout fires. Under full-suite CPU pressure the grandchild's Node cold start did not print in time, so the assertion had no pid to read — it is a race against a fixed 1500 ms budget, and it fails before reaching anything this branch touches. It passed in the second full-suite run, and run in isolation on this same branch it passes 3/3:

$ node --test --test-isolation=none dist/test/process_tree.test.js   # x3
✔ a timed-out command kills its whole tree, not just the shell (3002.2269ms)
✔ a timed-out command kills its whole tree, not just the shell (2880.3981ms)
✔ a timed-out command kills its whole tree, not just the shell (3260.1267ms)
ℹ pass 4 / fail 0   (each run)

Nothing in this branch touches process spawning, timeouts, or reaping. I have left it alone rather than widening a timeout in a file whose whole subject is timing; see "found but not fixed".

New tests added here:

  • test/tool_executor.test.ts — "the commit guard is armed by the first mutating tool, not by construction": asserts the full ordering in one go. Construction does not arm the guard, a read_file does not arm it, a write_file does; an edit made out of band before that point lands in the baseline, and the resulting commit contains the agent's file and only the agent's file.
  • test/git_commit_guard.test.ts — "repository probes are bounded to the workspace and take no optional locks": pins the pathspec on both probes and --no-optional-locks in the global arg prefix.

Blast radius

  • GitCommitGuard's public shape is unchanged; the constructor still takes the baseline. Only its call site moved, plus the flags on the git invocations.
  • src/commands/slash_git_tools.ts also uses SpawnGitRunner and therefore inherits --no-optional-locks. That is intended — those are read-side git helpers too — and no behaviour of theirs depends on the index being refreshed.
  • git_commit semantics change only for edits made outside the executor before the agent's first mutating action, as described above.
  • The GIT_CEILING_DIRECTORIES pin is test-only, set from a test module, and additive: it appends to any existing value rather than overwriting it.
  • .github workflows, package.json, and the CI matrix are untouched.

This also unblocks local testing for every future contributor whose temp directory happens to sit inside a repository. Until now, npm test on such a machine hung with no diagnostic — the suite simply stopped making progress inside a ToolExecutor test — and the only workaround was to know to point TEMP somewhere else. After this branch the default TEMP works, and the mechanism that made it fail is documented at the top of test/tmp_workspace.ts so the next person does not have to rediscover it.

Found but not fixed

  • process_tree.test.ts is timing-fragile under load. The 1500 ms budget in "a timed-out command kills its whole tree" has to cover two Node cold starts on Windows. Its sibling test gives itself 900 ms of settle time before asserting and did not flake. Out of lease for this PR, and worth fixing deliberately rather than by nudging a constant.
  • GitCommitGuard is wrong when the workspace is a subdirectory of the repository. git status --porcelain emits paths relative to the repository root, but the guard feeds them back to git add -- <path> / git reset -- <path> with cwd set to the workspace root. When those differ, the pathspecs do not resolve. This is pre-existing and unchanged by this PR — the -- . pathspec bounds which paths are reported, not how they are spelled. Fixing it means either resolving candidates against the repository root or emitting :(top)-anchored pathspecs, which is a separate change with its own tests.

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) <noreply@anthropic.com>
@AetherAI3
AetherAI3 merged commit ac64236 into main Aug 20, 2026
5 checks passed
@AetherAI3
AetherAI3 deleted the fix/lazy-git-commit-guard branch August 20, 2026 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant