fix(shell): verify working directory exists before launching process#1299
Merged
Conversation
ShellTool and BackgroundJobExecutionActor set ProcessStartInfo.WorkingDirectory without checking that the path exists. Only the session scratch dir was auto-created; any other resolved cwd (explicit arg, project dir, inherited cwd, or a background job's working directory) was passed straight to Process.Start, which throws an opaque, platform-specific error when the directory is missing. Agents could not tell why the command failed and retry-looped instead of creating the directory. Both paths now fail loudly with an actionable message naming the path and the mkdir remedy (platform-aware: no -p on Windows cmd.exe), and distinguish a missing directory from a path that is actually a file. No silent auto-creation: the cwd is a security-relevant, approval-gated input. Approvals are existence-agnostic, so an approval granted before the directory exists still matches once the agent creates it, avoiding a re-approval loop. Closes #1286
| "auto-ack-slack-gateway-missing-cwd"); | ||
| ActorRegistry.For(Sys).Register<SlackGatewayActorKey>(autoAckRef); | ||
|
|
||
| var missingDir = Path.Combine(_dir.Path, "does", "not", "exist"); |
| [Fact] | ||
| public async Task Missing_explicit_working_directory_returns_helpful_error() | ||
| { | ||
| var missingDir = Path.GetFullPath(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); |
| [Fact] | ||
| public async Task Working_directory_that_is_a_file_returns_not_a_directory_error() | ||
| { | ||
| var filePath = Path.GetFullPath(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); |
| { | ||
| // No explicit arg, so the resolution chain falls back to ProjectDirectory — | ||
| // proving the existence guard covers the fallback paths, not just explicit args. | ||
| var sessionDir = Path.GetFullPath(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); |
| // No explicit arg, so the resolution chain falls back to ProjectDirectory — | ||
| // proving the existence guard covers the fallback paths, not just explicit args. | ||
| var sessionDir = Path.GetFullPath(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); | ||
| var missingProjectDir = Path.GetFullPath(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); |
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.
Problem
Closes #1286.
ShellToolandBackgroundJobExecutionActorsetProcessStartInfo.WorkingDirectorywithout verifying the path exists. Only the session scratch dir was auto-created; any other resolved cwd (explicit arg, project dir, inherited cwd, or a background job's working directory) was passed straight toProcess.Start, which throws an opaque, platform-specific error when the directory is missing. Agents couldn't tell why the command failed and retry-looped instead of creating the directory.Fix
Both process-launch paths now fail loudly before
Process.Startwith an actionable message:Working directory '<path>' does not exist. Create it first, e.g.: mkdir -p "<path>"(platform-aware — no-pon Windowscmd.exe)Working directory '<path>' is a file, not a directory.No silent auto-creation: the cwd is a security-relevant, approval-gated input, so we fail loud per the constitution's "no silent fallbacks" rule. The session scratch dir keeps its existing auto-create behavior.
Why this is loop-free
Approvals are existence-agnostic — saved and matched as normalized path strings (
Path.GetFullPath, noDirectory.Exists, no TTL). So an approval granted for a not-yet-existent cwd still matches after the agent runsmkdir. The cryptic error was the only thing keeping the agent stuck.Changes
src/Netclaw.Actors/Tools/ShellTool.cssrc/Netclaw.Actors/Jobs/BackgroundJobExecutionActor.csProcess.Start, reported via the existingReportCompletion(Failed, …)idiom.src/Netclaw.Actors.Tests/Tools/ShellToolTests.cssrc/Netclaw.Actors.Tests/Jobs/BackgroundJobIntegrationTests.csFailed.Verification
ShellToolTests+BackgroundJobIntegrationTests— all green (4 new)dotnet slopwatch analyze— 0 issuesAdd-FileHeaders.ps1 -Verify— all headers presentReview notes (follow-ups, not blocking)
A multi-angle review found no functional bug in the diff. Minor items left as potential follow-ups:
Directory.Existsreturns false for ACL failures.WorkingDirectoryinherit the daemon cwd, whichShellTooldeliberately never does.PathUtilityhelper); themkdirhint isn't shell-escaped (cosmetic).