Make the CreateGitRepo git-spawn failure diagnose itself - #467
Open
realtonyyoung wants to merge 7 commits into
Open
Make the CreateGitRepo git-spawn failure diagnose itself#467realtonyyoung wants to merge 7 commits into
realtonyyoung wants to merge 7 commits into
Conversation
The intermittent "An error occurred trying to start process 'git' with working
directory '/tmp/kcap-orch-XXXXXXXX'. No such file or directory" failures in the
AgentOrchestrator test helper have now cost two investigations without producing a
root cause, and the reason is that the message cannot produce one. On Unix this
spawn fails with ENOENT for TWO unrelated causes, and .NET interpolates the
working directory into the text either way:
1. the working directory does not exist (chdir fails in the forked child)
2. the executable was not found (PATH resolution / execve fails)
So the log alone cannot say which happened. Git() now catches the Win32Exception
and rethrows naming both facts — whether the working directory exists, whether
'git' resolves on PATH and where, and the PATH itself — with the original
exception preserved as the inner. The next occurrence diagnoses itself.
This deliberately does NOT claim to fix the flake. Root cause is still unknown;
both filed reports asked for diagnosis before a blind fix, and explicitly not for
a retry, which would mask a genuine lifetime bug.
Two things fixed while in here:
* The temp path was Path.Combine(GetTempPath(), "kcap-orch-" + 8 hex chars of a
GUID) followed by CreateDirectory. 32 bits across the 59 call sites of this
helper is a real birthday risk, and because CreateDirectory is idempotent a
collision silently SHARES a directory — then either test's cleanup() deletes the
other's repo recursively. Probability never explained the observed failures, but
the truncation bought nothing, so the class is now gone rather than argued
about: Directory.CreateTempSubdirectory("kcap-orch-") is atomic and unique by OS
guarantee, and is already what the rest of this file uses. CreateGitRepo was the
outlier.
* Git() redirected both stdout and stderr but read neither before WaitForExit.
Redirecting a stream and never draining it risks the child blocking forever once
the pipe buffer fills — which would turn a test failure into a HUNG run, the
worst outcome since a hang produces no report at all. These commands are quiet
enough that it has not bitten; the shape is the bug. Both streams are now
drained before the wait.
A new test guards the diagnostic itself, because a diagnostic nobody has seen
produce output is not a fix. It spawns into a directory that was never created, so
the failure has a KNOWN cause, and asserts the message names the working
directory, its existence, PATH resolution, and preserves the Win32Exception.
Mutation-verified: reverting the catch to a bare `throw;` fails that test.
Verified: 178/178 in AgentOrchestratorVendorTests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoImprove CreateGitRepo git-spawn diagnostics and harden temp repo creation
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Review flagged that the resolver's accuracy is the whole point, and a naive
File.Exists check gets it wrong in two ways that would send the next investigator
down the wrong path — the exact failure this change exists to prevent:
* Unix requires the EXECUTE BIT. A readable-but-not-executable file is not on
PATH as far as execve is concerned, so reporting it as "resolved" would be a
false positive on the very question being asked.
* Windows resolves by PATHEXT, not by a hardcoded guess at .exe/.cmd. git ships
a .exe, but shims are commonly .cmd or .bat and PATHEXT is the authority.
Also strengthened the test. It asserted only that the "resolved on PATH:" FIELD
was present, which would pass while the resolver reported NOT FOUND for
everything — leaving half the ambiguity unsettled by a green test. Every sibling
test in this class spawns git successfully, so on any machine running this suite
git IS resolvable, and a NOT FOUND means the resolver is broken rather than the
environment. Now asserted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…esolution
Review round 2 (MEDIUM) killed the hand-rolled resolver, and the reason
generalises: a PATH walk cannot be made correct.
* Process.Start with UseShellExecute=false — which redirecting streams forces —
will NOT run a .cmd/.bat shim even though PATHEXT lists it, because those need
a command interpreter. So the PATHEXT support added in the previous commit
invented a FALSE POSITIVE about what this process could actually start.
* On Unix, "some execute bit is set" says nothing about EFFECTIVE permission for
this user, nor whether every PATH directory is traversable.
Each of those is another OS rule to replicate, which is the signal to replace the
mechanism rather than add a rule. A diagnostic that reports a false positive is
worse than no diagnostic, because misdirecting the next investigator is precisely
the failure this change exists to stop.
So the question is now answered authoritatively, by asking the OS: spawn
`git --version` from a directory known to exist and report the outcome. It uses
the same mechanism that just failed, and it splits the ENOENT ambiguity cleanly —
startable means the fault was the working directory, not startable means it was
the executable. Net effect is less code and more truth.
The test asserts the probe reports YES rather than merely that the field exists,
which would have passed while the probe answered NO for everything.
Mutation-verified: pointing the probe at a nonexistent executable fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The probe read stdout then stderr with blocking ReadToEnd() before WaitForExit — the exact shape called out as the bug in the main Git() path two commits earlier. `git --version` output is far too small to fill a pipe buffer, so this could not bite in practice, but the inconsistency is indefensible when the principle was just stated: redirect-and-block-read risks the child stalling once a buffer fills. It matters more here than in the main path, not less. This helper only runs on a FAILURE path, so a hang would replace a diagnosable error with a run that produces no report at all — turning the best case (a clear message) into the worst case. Verified: 178/178 in AgentOrchestratorVendorTests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings (both MEDIUM), both correct. 1. The probe reported "NO — exited N" when Process.Start SUCCEEDED but `git --version` returned nonzero. That is a misclassification of exactly the question the probe exists to answer: once Start returns a process, git WAS startable, and what the child then does is a separate fact. "NO" is now reserved for genuinely not startable — Start threw, or handed back nothing — and a nonzero exit reports YES with the code and stderr alongside it. 2. The probe had no timeout. It runs on a FAILURE path, in a suite CI executes serially, so an unbounded wait could wedge the whole run and produce no report at all — strictly worse than the error it is explaining. Now bounded at 10s with a best-effort kill, reporting startable-but-did-not-exit rather than hanging. (The reviewer also flagged draining stdout fully before stderr; that was already fixed in the previous commit — it reviewed the prior diff.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 4 (MEDIUM), correct — and it is the SAME defect as round 3's, in the place I
did not look. I fixed the nonzero-exit branch but left a broad catch(Exception)
around the whole body, which still reported NO for post-start failures: stream
reads, the wait, ExitCode access, disposal. All of those occur after startability
is already proven, so reporting NO conflates exactly the two facts this probe
exists to separate.
Restructured so "NO" means precisely one thing — git could not be STARTED:
* Process.Start has its own narrow try; only that returns NO.
* everything after it has a separate try that keeps the YES and appends how it
went wrong.
* disposal moved to a finally that swallows, so it can neither change the
verdict nor escape.
The last point matters beyond tidiness: this helper is called from INSIDE
`catch (Win32Exception)`, so an escaping exception would replace the informative
message with the probe's own — destroying the diagnostic in the one situation it
was written for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings, both acted on.
The reviewer pointed at a resolver this branch had already deleted, but it carried
a fact worth having: Capacitor.Cli.Core.CliExecutable.Resolve already exists and
handles PATHEXT and the Unix execute bit. Rather than replace the probe with it —
resolution cannot answer "could this process START it", which is why the probe
exists — the message now reports BOTH, using the shared helper for the location:
'git' startable from a known-good directory: YES (git version 2.x)
'git' resolves to: /usr/bin/git
Two complementary facts, no duplicated resolution logic. The test asserts the
resolver found something rather than merely that the field is present, for the same
reason it already asserts the probe says YES: a green test must not be compatible
with the diagnostic answering NOT FOUND for everything.
Also trimmed the comment blocks. The reviewer was right that they were long, and
the specific excess was review HISTORY ("round 2 killed the previous approach") —
which belongs in git, where it already is. The load-bearing WHY is kept and is the
point: the ENOENT ambiguity, why executable resolution is not modelled here, and
why NO must mean only "could not start".
Verified: 178/178 in AgentOrchestratorVendorTests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Fixes the diagnostic gap behind two filed flake reports (Linear AI-1719 and AI-1735, GitHub #452) — the same failure seen twice:
Both times it failed on the first line of the test, inside
CreateGitRepo(), spawninggit initin a directoryDirectory.CreateDirectoryhad just returned successfully for. Both times it passed on a rerun of the same commit. Nothing under test had run yet.Why two investigations produced no root cause
The message cannot produce one. On Unix this spawn fails with
ENOENT/ "No such file or directory" for two unrelated causes, and .NET interpolates the working directory into the text either way:chdirfails in the forked childexecvefailsSo the log is genuinely ambiguous, and no amount of re-reading it settles which happened.
Git()now catches theWin32Exceptionand rethrows naming both facts — whether the working directory exists, whethergitresolves on PATH and where, and the PATH itself — preserving the original asInnerException. The next occurrence diagnoses itself.This does not claim to fix the flake. Root cause remains unknown. Both reports asked for diagnosis before a blind fix, and explicitly not for a retry, which would mask a genuine lifetime bug.
Two things fixed while in here
Temp path. It was
Path.Combine(GetTempPath(), "kcap-orch-" + 8 hex chars of a GUID)thenCreateDirectory. 32 bits across this helper's 59 call sites is a real birthday risk, and sinceCreateDirectoryis idempotent a collision silently shares a directory — after which either test'scleanup()recursively deletes the other's repo. Probability never explained the observed failures, but the truncation bought nothing, so the class is gone rather than argued about:Atomic and unique by OS guarantee — and already what the rest of this file uses.
CreateGitRepowas the outlier.A latent hang.
Git()redirected both stdout and stderr and read neither beforeWaitForExit. Redirecting a stream and never draining it risks the child blocking forever once the pipe buffer fills, turning a test failure into a hung run — the worst outcome, since a hang produces no report at all. These commands are quiet enough that it hasn't bitten; the shape is the bug. Both streams are now drained before the wait.The diagnostic is tested, not hoped for
A diagnostic nobody has seen produce output is not a fix.
Git_spawn_failure_reports_the_working_directory_and_PATH_resolutionspawns into a directory that was never created — so the failure has a known cause — and asserts the message names the working directory, its existence, PATH resolution, and preserves theWin32Exception. Cross-platform: Unix gives ENOENT, Windows "the directory name is invalid", bothWin32Exception.Mutation-verified: reverting the catch to a bare
throw;fails that test.Candidates ruled out (carried forward from the reports, plus one new)
UninstallCommand.SweepCapacitorPrefixedDirs— only ever runs against the skills dirs, never the temp root[NotInParallel]and early-return on non-WindowsHOME/cwd — all scoped below their ownCreateTempSubdirectoryTMPDIRmutation. Worth checking sincePath.GetTempPath()honours it and a moved-then-deleted temp root would match the evidence exactly. Ruled out — the only write ispsi.Environment["TMPDIR"], which is per-child-process and cannot move the test host's temp root.Verification
178/178 in
AgentOrchestratorVendorTests.🤖 Generated with Claude Code