fix: set spawn cwd for workingDirectory to work around codex -C bug#138
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHandlers now always pass a unified options object (cwd and envOverride) into command utilities; command utilities accept a typed CommandOptions (cwd/envOverride) and apply cwd/env to spawn options. Tests were updated to expect the new three-argument executeCommand signature and a new working-directory test was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Handler
participant CmdUtil as CommandUtil
participant Child as ChildProc
Client->>Handler: execute(request { workingDirectory?, envOverride?, stream? })
Handler->>Handler: build cmdArgs and resolve cwd -> resolvedWorkDir
Handler->>CmdUtil: executeCommand('codex', cmdArgs, cmdOptions { cwd: resolvedWorkDir, envOverride })
CmdUtil->>Child: spawn(process, args, spawnOptions { cwd: path.resolve(cmdOptions.cwd), env: mergedEnv })
Child-->>CmdUtil: stdout / stderr / exit
CmdUtil-->>Handler: return result / stream callbacks
Handler-->>Client: respond with output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6d0cac0 to
ce52945
Compare
The workingDirectory parameter was only forwarded as a -C CLI flag, but Codex CLI review ignores -C and resolves the git repo from process.cwd() instead (openai/codex#9084). Setting cwd on spawn() options makes the child process start in the intended directory. - Add CommandOptions interface with optional cwd to executeCommand and executeCommandStreaming - Use SpawnOptionsWithoutStdio for proper typing (no more as-never) - Pass workingDirectory as spawn cwd from review and codex handlers - Keep -C flag in args for forward compat when upstream fixes the bug - Add 5 dedicated tests for cwd propagation Signed-off-by: TiM <[email protected]>
ce52945 to
daf2f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/handlers.ts`:
- Around line 156-168: Normalize the workingDirectory to an absolute path before
building cmdArgs and cmdOptions: in both CodexToolHandler and ReviewToolHandler,
run path.resolve(workingDirectory) (or equivalent) and use that resolved path
for the "-C" entry in cmdArgs and for cwd in cmdOptions passed to executeCommand
and executeCommandStreaming; ensure you update any places that construct
cmdArgs/cmdOptions (references: workingDirectory, cmdArgs, cmdOptions,
executeCommand, executeCommandStreaming) so the CLI sees the same absolute path
the child process uses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac4984e5-7fde-41aa-b6aa-1fe0ed4981a3
📒 Files selected for processing (7)
src/__tests__/default-model.test.tssrc/__tests__/error-scenarios.test.tssrc/__tests__/model-selection.test.tssrc/__tests__/resume-functionality.test.tssrc/__tests__/working-directory.test.tssrc/tools/handlers.tssrc/utils/command.ts
✅ Files skipped from review due to trivial changes (2)
- src/tests/resume-functionality.test.ts
- src/tests/working-directory.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tests/model-selection.test.ts
- src/tests/error-scenarios.test.ts
- src/tests/default-model.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/handlers.ts`:
- Around line 162-174: The current cmdOptions unconditionally includes cwd:
resolvedWorkDir which violates the resume path; update the cmdOptions
construction in handlers.ts to only include cwd when not useResume (apply the
same guard used for sandbox and fullAuto), so that when useResume is true the
spawn options omit cwd; ensure both executeCommandStreaming('codex', ...) and
executeCommand('codex', ...) use this conditional cmdOptions, and add a
unit/integration test that runs the resume path with a workingDirectory set to
assert that cwd is not applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c964809-1ed4-4b97-a17d-f04d2ffa3e4c
📒 Files selected for processing (1)
src/tools/handlers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/handlers.ts`:
- Around line 59-62: Validate the incoming workingDirectory string before
resolving and passing it to the child spawn: after computing resolvedWorkDir
from workingDirectory, check filesystem existence and that it refers to a
directory (use fs.existsSync/stat/isDirectory or async equivalents) and if the
check fails throw a ValidationError with a clear message (e.g.,
"workingDirectory does not exist" or "workingDirectory is not a directory").
Update the code around resolvedWorkDir in src/tools/handlers.ts (where
workingDirectory is processed) to perform this validation and throw
ValidationError instead of letting spawn surface ENOENT/ENOTDIR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1f88233-3957-4fa4-923b-1640dca9aee0
📒 Files selected for processing (2)
src/__tests__/working-directory.test.tssrc/tools/handlers.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/working-directory.test.ts
Summary
cwdto Node'sspawn()options whenworkingDirectoryis provided, so the child Codex CLI process starts in the correct directory-Cflag is ignored bycodex review-Cin CLI args for forward compatibility when the upstream CLI bug is fixedSpawnOptionsWithoutStdiofor proper typing (replacesRecord<string, unknown>+as nevercast)CommandOptionsinterface with optionalcwdandenvOverrideProblem
The
workingDirectoryparameter on thereviewandcodextools was only forwarded as a-CCLI flag. However, Codex CLI'sreviewsubcommand ignores-Cand resolves the git repo fromprocess.cwd()instead. This makes it impossible to review code in git worktrees via MCP.Root cause: Two compounding issues:
spawn()did not setcwd, so the child process inherited the MCP server's working directoryreviewsubcommand usesprocess.cwd()instead of the-Cpath (openai/codex#9084)Solution
By setting
cwdonspawn()options,process.cwd()in the child Codex process returns the intended directory, bypassing the CLI bug entirely.Verified with manual testing:
-Cflagworkdirreported-ConlycwdonlyTest plan
working-directory.test.ts)tsc)cwdonspawn()makes Codex resolve the correct git repoSummary by CodeRabbit
New Features
Tests