feat: add Codex-style tool mode#26
Merged
Merged
Conversation
This was referenced Jun 23, 2026
📝 WalkthroughWalkthroughAdds ChangesCodex Tool Mode
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review it |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 27-29: The PTY tests in src/process-sessions.test.ts (lines
137-158) that call manager.start({tty: true}) will fail when node-pty is not
installed, even though it is marked as optional in package.json. Modify the test
file to conditionally handle the absence of node-pty by either wrapping the
affected test cases with a check to skip them when node-pty is unavailable, or
by providing a mock implementation of node-pty for when the optional dependency
is not present. Ensure that startPty() either skips execution gracefully or has
a mocked fallback so the test suite can pass whether or not node-pty is
installed.
In `@src/apply-patch.ts`:
- Line 2: The issue is that symlinks within the workspace are not being rejected
before applying updates, which can cause `replaceFile()` to replace the symlink
path with a regular file instead of the actual target file. Add a check using
`lstat()` (which does not follow symlinks) at the beginning of the file
processing logic to detect if the final resolved path is a symlink, and reject
it before any file operations like `stat()`, `readFile()`, or `rename()` are
performed. This ensures symlinks are caught and handled appropriately rather
than being overwritten as regular files.
- Around line 1-2: The temporary file names used in staged writes are
predictable and vulnerable to being overwritten. Generate random suffixes for
temporary file names to make them unpredictable, and when calling writeFile for
temporary files, add the flag option set to "wx" to ensure exclusive creation
that will fail if the file already exists. Apply this fix to all temporary file
write operations in the file, including the ones referenced at lines 343-344.
In `@src/process-sessions.ts`:
- Around line 231-235: The resize property in the session.process object
currently provides a no-op function when input.tty is true, which incorrectly
advertises resize support for pipe-backed sessions. This causes the system to
accept resize requests and report success without actually resizing. Remove the
ternary logic that sets resize to a no-op function when input.tty is true, and
instead leave the resize property undefined unconditionally so that existing
guards can properly reject resize requests for non-PTY sessions.
- Around line 219-229: The startPipe method is passing the raw input.command
string to spawn instead of using the fully resolved shell command from
resolveShellCommand. Modify the spawn call to use shell.executable as the
command and combine shell.args with the command string as arguments passed to
spawn, similar to how the PTY backend implementation handles it. This ensures
critical shell flags (like /d /s /c for Windows cmd.exe or -lc for login shells)
are preserved when executing the pipe-backed command.
In `@src/server.ts`:
- Around line 1103-1177: The show_changes tool registration is not gated by
Codex mode, so it still registers when config.toolMode === "codex", adding a 6th
tool when the Codex surface should only have 5 tools. Modify the condition for
the show_changes registration (the if block checking config.widgets ===
"changes") to exclude Codex mode by adding an additional condition that prevents
its registration when config.toolMode === "codex".
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 469c0360-89ce-42a5-b8dc-f5bfbf610495
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
docs/chatgpt-coding-workflow.mddocs/codex-tool-mode-qa.mddocs/configuration.mdpackage.jsonscripts/fix-node-pty-permissions.mjssrc/apply-patch.test.tssrc/apply-patch.tssrc/config.test.tssrc/config.tssrc/process-platform.test.tssrc/process-platform.tssrc/process-sessions.test.tssrc/process-sessions.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/workspace-app.tsx
Waishnav
force-pushed
the
feat/codex-tool-mode
branch
from
7fa956c to
17fa037
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/codex-tool-mode-qa.md`:
- Line 31: The workspace probe currently relies on `pwd`, which is Unix-specific
and can fail in the `cmd.exe` path used by `resolveShellCommand()`. Update the
QA step to use a platform-aware check in the same docs section, either by
branching on OS/shell or replacing `pwd` with a cross-platform equivalent so the
workspace confirmation works on Windows and non-Windows shells.
- Around line 49-54: Scope the PTY checklist in the docs to non-Windows hosts
only, since ProcessSessionManager.start() uses the PTY path only when
process.platform is not win32. Update the expectations around tty=true, SIGINT
via \u0003, and PTY resize to apply only on POSIX systems, and note that Windows
currently falls back to pipe mode rather than surfacing the node-pty behavior.
In `@src/process-sessions.ts`:
- Around line 123-124: Reject the unsupported tty=true path on Windows in
process-sessions.ts instead of falling through to startPipe(session, input). In
the session startup branch that checks input.tty and process.platform, detect
the Windows case up front and return an explicit error from the session creation
flow rather than silently downgrading; keep startPty(session, input) for
supported platforms and leave startPipe(session, input) for non-TTY sessions
only.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 511d1293-bcf8-45df-a2bf-334edc4ab1c9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
docs/chatgpt-coding-workflow.mddocs/codex-tool-mode-qa.mddocs/configuration.mdpackage.jsonscripts/fix-node-pty-permissions.mjssrc/apply-patch.test.tssrc/apply-patch.tssrc/config.test.tssrc/config.tssrc/process-platform.test.tssrc/process-platform.tssrc/process-sessions.test.tssrc/process-sessions.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/workspace-app.tsx
✅ Files skipped from review due to trivial changes (2)
- scripts/fix-node-pty-permissions.mjs
- docs/chatgpt-coding-workflow.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src/ui/card-types.test.ts
- package.json
- src/config.test.ts
- docs/configuration.md
- src/config.ts
- src/ui/card-types.ts
- src/ui/workspace-app.tsx
- src/apply-patch.ts
- src/process-platform.test.ts
- src/apply-patch.test.ts
- src/server.ts
- src/process-sessions.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/apply-patch.test.ts`:
- Around line 136-160: The current tests in applyPatch lock in overwrite
behavior for add/move collisions, but the patch application contract should
reject existing destinations unless using update semantics. Update the
expectations around applyPatch so Add File and Move to cases against an existing
path assert a failure instead of successful replacement, using the existing
applyPatch helper and the overwriteRoot scenario to cover both duplicate.txt and
destination.txt. Keep the explicit Update File path as the only allowed
replacement behavior.
- Line 127: The patch application flow is leaving partial writes behind when a
later action fails, so the workspace is not restored to its original state.
Update the apply logic in apply-patch.ts so patch actions are applied
atomically: stage changes first or track successful writes and roll them back on
error, especially for Add File and other mutating actions. Adjust the
apply-patch.test.ts failure case to assert the workspace remains unchanged after
a rejected patch, using the applyPatch flow and its file mutation helpers as the
key points to fix.
In `@src/process-sessions.ts`:
- Around line 231-232: The TTY startup path in process-sessions.ts is using the
non-interactive yield default, which can delay returning sessionId for
interactive commands. Update the yieldTimeMs selection in the session startup
flow so that when input.tty is true it uses DEFAULT_INTERACTIVE_YIELD_MS, and
keep DEFAULT_EXEC_YIELD_MS for non-TTY sessions before calling
this.waitForExit(session, yieldTimeMs).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8586309-9c23-4033-b0d6-58904af6e41a
📒 Files selected for processing (5)
src/apply-patch.test.tssrc/apply-patch.tssrc/process-sessions.test.tssrc/process-sessions.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/apply-patch.ts
- src/server.ts
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.
Summary
Adds an opt-in Codex-style tool mode that exposes a smaller coding-oriented tool surface while preserving the existing tool modes as the default.
What changed
Compatibility
Codex mode is opt-in. Existing minimal and full tool modes retain their current behavior. The legacy
DEVSPACE_MINIMAL_TOOLSconfiguration remains supported.Summary by CodeRabbit
minimal,full, andcodex.