fix(agent): stop blocking CLI startup on an unbounded git status - #89
Merged
Conversation
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>
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
new ToolExecutor(cwd)unconditionally constructed aGitCommitGuard, and the guard's constructor ran two synchronousspawnSyncgit calls:Those ran on every
aether agentand everyaether chatstart — before the first turn, on the main thread, whether or not the run ever calledgit_commit. Two properties make that expensive rather than merely wasteful:git statusreports 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=allenumerates 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:git status --porcelain=v1 -z --untracked-files=all(as shipped)timeoutreturned 124git --no-optional-locks -c core.literalPathspecs=true status --porcelain=v1 -z --untracked-files=all -- .(this PR)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 statusreturns 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 testhangs on this machine" hazard in this repo. EverymkdtempSync-based test that built aToolExecutorwalked the entire home tree. On the pre-fix tree the suite was killed at 300s withToolExecutor 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.
ToolExecutorno longer constructsGitCommitGuardin 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, andplanGitCommitcomputes candidates ascurrentDirty − baseline. A baseline captured at commit time is by definition equal to the current state, so the difference is empty for every commit andgit_commitbecomes 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
fsdirectly — 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 intool_executor.test.tswrote their fixture file withwriteFileSyncand then asked for a commit; they now write throughexec.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 fromSpawnGitRunnernow carries--no-optional-locks, so a plaingit statusno 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.tsexportstmpWorkspace(prefix)andTEMP_ROOT, and pinsGIT_CEILING_DIRECTORIESat the canonical temp root at import time. Every test that builds aToolExecutornow allocates through it:bridge,process_tree,release_canaries,tool_executor,tool_registry,web.Decision I was asked to make explicitly:
GIT_CEILING_DIRECTORIESrather thangit 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 throughprocess.env, so it coversspawnSynccalls the executor makes without any change to the executor.Test evidence
Environment: worktree
~/agent-w4core-wt(a clean worktree offorigin/main@ c165be0), Windows 11, Git Bash, defaultTEMP=C:\Users\lilbe\AppData\Local\Temp— not redirected. Nogit stashwas used at any point in producing this branch.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_msis thenode --testphase; the wall clock is the fullnpm test, which isnpm run build(atsccompile) followed by the test phase. Both numbers move with machine load; the first of the two runs reportedduration_ms 137027.5677with a5m23.252swall 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: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:
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, aread_filedoes not arm it, awrite_filedoes; 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-locksin 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.tsalso usesSpawnGitRunnerand 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_commitsemantics change only for edits made outside the executor before the agent's first mutating action, as described above.GIT_CEILING_DIRECTORIESpin is test-only, set from a test module, and additive: it appends to any existing value rather than overwriting it..githubworkflows,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 teston such a machine hung with no diagnostic — the suite simply stopped making progress inside aToolExecutortest — and the only workaround was to know to pointTEMPsomewhere else. After this branch the defaultTEMPworks, and the mechanism that made it fail is documented at the top oftest/tmp_workspace.tsso the next person does not have to rediscover it.Found but not fixed
process_tree.test.tsis 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.GitCommitGuardis wrong when the workspace is a subdirectory of the repository.git status --porcelainemits paths relative to the repository root, but the guard feeds them back togit add -- <path>/git reset -- <path>withcwdset 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.