Repository navigation
Replace ambient process state with values resolved once - #948
Conversation
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
PR Summary by QodoResolve process context once and inject repository state
AI Description
Diagram
High-Level Assessment
Files changed (161)
|
A vanished TempDir surfaces as a DirectoryNotFoundException wherever the victim next touches its tree, arbitrarily far from whatever removed it; dispose is the one place that can still name the owner. The orchestrator tests tripped it at once: cleanup of a standalone worktree deletes the path it is handed, and they handed it the whole fixture rather than a directory under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remembering a host probe only pays off if one router outlives a call, and the watcher re-detects every 60s for as long as it runs, so the router is a singleton that call sites take rather than construct. A static memo was shared across concurrent tests that each cleared it, so one test's write could answer another's probe and its budget assertion saw no round trip.
The two git children behind ResolveForRepo set no working directory of their own, so they inherited the process's and a test could steer them only by moving it; handing the value down without also handing it to them would have changed nothing. The ban covers the setter too, and one test still moves the process, because a path with no directory component resolves against nothing else.
RepoRootOf shells out to git and blocks on the child's output before its timeout applies, so a global selection that saves nothing must not ask for it. The same guard holds a saving in-repo selection to one probe rather than two.
56ac712 to
059fcaf
Compare
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
Static review only; I did not build or run tests. I found one remaining process-wide test-isolation issue.
| // silently turn the write into Change.Failed without the guard. | ||
| using var tmp = new TempDir(); | ||
| using var tmp = new TempDir(); | ||
| #pragma warning disable RS0030 // a path with no directory component resolves nowhere else |
There was a problem hiding this comment.
[P2] Fully isolate the remaining cwd mutation
[NotInParallel("CwdMutation")] only serializes tests that share that constraint key; this is the only occurrence in the repository, so this test still runs alongside every unconstrained Core test while the block below redirects the process-wide current directory. Any peer doing relative I/O or starting a child without an explicit WorkingDirectory can therefore observe (or act on) tmp.Path, recreating the ambient-state race this PR is intended to remove. Please use bare [NotInParallel] so this test runs completely alone, or move this case behind a subprocess/explicit base-path seam.
There was a problem hiding this comment.
You're right that the key was doing nothing — "CwdMutation" appears exactly once in the repo, so the cohort was this test alone and every unconstrained Core test ran alongside it.
Digging into it, though, the test could not reach the branch it was named for. Update canonicalises the path at CodexConfigToml.cs:461 (configPath = CanonicalConfigPath(configPath), i.e. Path.GetFullPath) about forty lines before the guard at :498 reads Path.GetDirectoryName(configPath). By then the path is always absolute, so GetDirectoryName is never empty. I checked by deleting the guard outright and making the create unconditional: all 43 tests in the class stayed green, including this one.
So moving the process working directory was only giving Path.GetFullPath a base — it exercised nothing. I've deleted the test and the two comment lines above the guard that claimed a directory-less path could arrive there. That drops the last Environment.CurrentDirectory = in src/ or test/, so nothing in the suite moves the process any more, which seemed a better answer than isolating it with a bare constraint.
One thing I looked at and deliberately did not change: a relative CODEX_HOME is honoured today and resolved against the process directory. Rejecting it would make kcap write to ~/.codex while Codex still reads the relative path, so I left that alone. It is not unique to Codex either — nine env vars become path roots and only HOME is rooted-checked. Separate concern from this PR.
Update canonicalises the config path before the directory guard runs, so GetDirectoryName never sees a directory-less one — removing that guard outright leaves the whole class green. Moving the process working directory was only giving GetFullPath a base, and it was the last such move in the suite.
|
NO FINDINGS |
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
Static review complete; no actionable findings.
2 participants
Refs #781 — AI-2528
What & why
A fixture deleted from outside its owner surfaces as a
DirectoryNotFoundExceptionwherever the victim next touches its tree, arbitrarily far from whatever removed it.
TempDirnow reports that at dispose, the one place that can still name the owner —which immediately caught orchestrator tests handing production cleanup their whole
fixture root instead of a directory under it.
Two ambient reads turned up the same way. The provider router's memo was a mutable
static that concurrent tests cleared under each other, so one test's write could
answer another's probe. The working directory was read from the process wherever it
was wanted. Both are resolved once now and injected; ambient cwd joins
BannedSymbols.txt, so a new read is a build error rather than a convention.Where to look
The two git children behind
ResolveForReposet no working directory of their own,so they inherited the process's. Handing the value down without also handing it to
them would have changed nothing.
One test still moves the process: a path with no directory component resolves against
nothing else.
GitProviderRouteris a required constructor argument on the import sources, sokcap-server's
PiAi892ImportE2ETestsstops compiling when it advances this submodule.A matching change there is expected rather than a compatibility overload here.
Verification
dotnet test --solution Capacitor.slnx— 13012 tests, 12943 passed, 68 skipped, 1failed. That failure,
Installed_codex_schema_matches_the_vendored_pin, reproducesunchanged on
main(installed codex 0.154.0 against a pin taken from 0.147.0) and isSkip.When'd on CI, which has no codex.Rewritten assertions checked by mutation rather than by going green — pointing the
injected directory away from the repo fails 2/3
ResolveForRepoTests, 2/2 Uninstall--projecttests and 3/4 Setup acceptance tests; dropping the Cursor workspace-rootguard fails its test. The survivors are the rows whose docs already state they are
directory-independent.
dotnet publish -c Release: no IL2026/IL3050. Assembly-exclusive tests 517 → 499.