Skip to content

feat: add Codex-style tool mode#26

Open
Waishnav wants to merge 43 commits into
mainfrom
feat/codex-tool-mode
Open

feat: add Codex-style tool mode#26
Waishnav wants to merge 43 commits into
mainfrom
feat/codex-tool-mode

Conversation

@Waishnav

@Waishnav Waishnav commented Jun 22, 2026

Copy link
Copy Markdown
Owner

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

  • adds DEVSPACE_TOOL_MODE=codex alongside the existing minimal and full modes
  • exposes stable short tool names in Codex mode
  • limits the Codex-mode tool surface to open_workspace, read, apply_patch, exec_command, and write_stdin
  • adds a workspace-confined apply_patch implementation with add, update, delete, and move operations
  • preserves line endings and executable file modes while applying patches
  • returns unified diff payloads and change statistics for patch tool cards
  • adds resumable command sessions for foreground and background processes
  • supports incremental output polling, stdin interaction, process interruption, output limits, and process-group cleanup
  • adds optional PTY support through node-pty, including terminal resizing
  • renders apply_patch through the existing diff UI and process tools through the existing shell UI
  • adds configuration, workflow, rollout, and manual QA documentation for the new mode

Compatibility

Codex mode is opt-in. Existing minimal and full tool modes retain their current behavior. The legacy DEVSPACE_MINIMAL_TOOLS configuration remains supported.

Summary by CodeRabbit

  • New Features
    • Added experimental "codex" tool mode with an alternative set of tools for patching and process management
    • Introduced patch-based file editing via apply_patch tool and process execution/management via exec_command and write_stdin tools
    • Extended tool mode configuration options: set DEVSPACE_TOOL_MODE to "minimal", "full", or "codex"

Waishnav added 30 commits June 22, 2026 04:13
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds DEVSPACE_TOOL_MODE=codex as a third tool mode. It replaces the minimalTools boolean config with a ToolMode string type, implements a applyPatch engine and a ProcessSessionManager (pipe and PTY-backed), registers new MCP tools (apply_patch, exec_command, write_stdin) in the server, updates UI tool classification, adds node-pty as an optional dependency with a macOS permission-fix script, and adds documentation plus a manual QA checklist.

Changes

Codex Tool Mode

Layer / File(s) Summary
ToolMode config contract and parser
src/config.ts, src/config.test.ts
Replaces minimalTools: boolean with toolMode: ToolMode ("minimal" | "full" | "codex") in ServerConfig, adds parseToolMode with legacy DEVSPACE_MINIMAL_TOOLS mapping, and updates tests to assert string mode values.
Platform shell resolution and process tree termination
src/process-platform.ts, src/process-platform.test.ts
Adds resolveShellCommand (selects cmd.exe, login shell, or POSIX shell by platform/SHELL) and terminateProcessTree (Windows taskkill, POSIX group kill, or direct child kill), with multi-platform test coverage.
Patch parsing and application engine
src/apply-patch.ts, src/apply-patch.test.ts
Implements parsePatch and applyPatch: parses *** Begin Patch format into add/update/delete/move actions, confines paths to workspace root (rejecting ../ escapes and symlink traversal), applies hunks with normalization fallbacks, generates unified diff, and performs transactional writes with Windows-safe rename. Tests cover multi-action patches, move semantics, path-escape and symlink rejection, and hunk mismatch errors.
ProcessSessionManager: pipe and PTY-backed sessions
src/process-sessions.ts, src/process-sessions.test.ts
Adds ProcessSessionManager managing long-running shell sessions via stdio pipes or PTY (node-pty). Buffers output with max-size cap, returns ProcessSnapshot with truncation/running/exit metadata, enforces workspace ownership on writes, and schedules TTL cleanup. Tests cover foreground/background lifecycle, stdin writes, Ctrl-C interrupt, output truncation, and PTY resize.
Server: Codex tool registration and session wiring
src/server.ts
Updates toolNamesFor/serverInstructions for Codex mode, registers exec_command and write_stdin via registerCodexProcessTools, adds Codex-only apply_patch tool, gates non-Codex tools on toolMode !== "codex", switches search tools to toolMode === "full", and wires processSessions lifecycle into createServer/close.
UI tool-name recognition and display
src/ui/card-types.ts, src/ui/card-types.test.ts, src/ui/workspace-app.tsx
Extends ToolName union and isEditTool/isShellTool guards for apply_patch, exec_command, write_stdin. Adds display cases in getToolDisplay, shows running/ran state in shell badges, and prefers sessionId in getToolLabel.
node-pty dependency, permission script, and docs
package.json, scripts/fix-node-pty-permissions.mjs, docs/configuration.md, docs/chatgpt-coding-workflow.md, docs/codex-tool-mode-qa.md
Adds node-pty ^1.0.0 as optional dependency with a macOS spawn-helper permission-fix postinstall script and expanded test scripts. Documentation adds the codex mode table and session-polling details; a new QA checklist covers tool presence, patch behavior, foreground/background sessions, PTY, cleanup, and mode regression.

Sequence Diagram(s)

sequenceDiagram
    participant Client as MCP Client (Codex)
    participant Server as MCP Server
    participant ProcessSessionManager
    participant ChildProcess

    rect rgba(70, 130, 180, 0.5)
        Note over Client,ChildProcess: exec_command (foreground - completes within yield window)
        Client->>Server: exec_command(command, cwd)
        Server->>ProcessSessionManager: start(input)
        ProcessSessionManager->>ChildProcess: spawn via pipe/PTY
        ChildProcess-->>ProcessSessionManager: output + exit
        ProcessSessionManager-->>Server: ProcessSnapshot(running=false, output)
        Server-->>Client: result(output, exitCode)
    end

    rect rgba(70, 180, 130, 0.5)
        Note over Client,ChildProcess: exec_command (background - yields with sessionId)
        Client->>Server: exec_command(command, cwd)
        Server->>ProcessSessionManager: start(input)
        ProcessSessionManager->>ChildProcess: spawn via pipe/PTY
        ProcessSessionManager-->>Server: ProcessSnapshot(running=true, sessionId)
        Server-->>Client: result(sessionId, partial output)
        Client->>Server: write_stdin(sessionId, chars)
        Server->>ProcessSessionManager: write(input)
        ProcessSessionManager->>ChildProcess: stdin write / PTY resize / Ctrl-C
        ProcessSessionManager-->>Server: ProcessSnapshot(running, output)
        Server-->>Client: result(updated output, exitCode)
    end
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐇 Hop, hop, a new codex mode appears,
With patches and sessions to calm all your fears!
exec_command runs, the PTY glows bright,
apply_patch scurries with transactional might.
The rabbit grins wide at each tool in its place —
write_stdin delivers at cottontail pace! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add Codex-style tool mode' directly summarizes the main change: introducing a new Codex-style tool mode configuration option, which is the primary feature described across all modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/codex-tool-mode
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feat/codex-tool-mode

Comment @coderabbitai help to get the list of available commands.

@Waishnav

Copy link
Copy Markdown
Owner Author

@coderabbitai review it

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

@Waishnav Sure! I'll kick off a full review of this PR right away.

✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4795509 and 7fa956c.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (17)
  • docs/chatgpt-coding-workflow.md
  • docs/codex-tool-mode-qa.md
  • docs/configuration.md
  • package.json
  • scripts/fix-node-pty-permissions.mjs
  • src/apply-patch.test.ts
  • src/apply-patch.ts
  • src/config.test.ts
  • src/config.ts
  • src/process-platform.test.ts
  • src/process-platform.ts
  • src/process-sessions.test.ts
  • src/process-sessions.ts
  • src/server.ts
  • src/ui/card-types.test.ts
  • src/ui/card-types.ts
  • src/ui/workspace-app.tsx

Comment thread package.json
Comment on lines +27 to +29
"postinstall": "node scripts/fix-node-pty-permissions.mjs",
"start": "node dist/cli.js serve",
"test": "tsx src/config.test.ts && tsx src/roots.test.ts && tsx src/skills.test.ts && tsx src/workspaces.test.ts && tsx src/review-checkpoints.test.ts && tsx src/oauth-store.test.ts",
"test": "tsx src/config.test.ts && tsx src/ui/card-types.test.ts && tsx src/apply-patch.test.ts && tsx src/process-platform.test.ts && tsx src/process-sessions.test.ts && tsx src/roots.test.ts && tsx src/skills.test.ts && tsx src/workspaces.test.ts && tsx src/review-checkpoints.test.ts && tsx src/oauth-store.test.ts",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n 'node-pty|describe\.skip|it\.skip|test\.skip|skip' src/process-sessions.test.ts

Repository: Waishnav/devspace

Length of output: 155


🏁 Script executed:

#!/bin/bash
# First, check if the test file exists
ls -la src/process-sessions.test.ts 2>&1 || echo "File not found"

Repository: Waishnav/devspace

Length of output: 233


🏁 Script executed:

#!/bin/bash
# Check the structure of src directory to find test files
find src -name "*.test.ts" -type f | head -20

Repository: Waishnav/devspace

Length of output: 397


🏁 Script executed:

#!/bin/bash
# Check package.json for optionalDependencies
cat -n package.json | grep -A 5 "optionalDependencies"

Repository: Waishnav/devspace

Length of output: 242


🏁 Script executed:

#!/bin/bash
# Read the full test file
cat -n src/process-sessions.test.ts

Repository: Waishnav/devspace

Length of output: 6066


🏁 Script executed:

#!/bin/bash
# Find and read the ProcessSessionManager implementation
find src -name "process-sessions.ts" -o -name "process-sessions.js" | head -5

Repository: Waishnav/devspace

Length of output: 179


🏁 Script executed:

#!/bin/bash
# Check how node-pty is imported/required in the codebase
rg -n "require.*node-pty|import.*node-pty|from.*node-pty" src/ --type ts

Repository: Waishnav/devspace

Length of output: 472


🏁 Script executed:

#!/bin/bash
# Read process-sessions.ts around the node-pty handling
sed -n '235,260p' src/process-sessions.ts

Repository: Waishnav/devspace

Length of output: 1165


🏁 Script executed:

#!/bin/bash
# Check if there's try/catch or fallback logic after the error throw in startPty
sed -n '240,280p' src/process-sessions.ts

Repository: Waishnav/devspace

Length of output: 1421


🏁 Script executed:

#!/bin/bash
# Check the start() method to see if it handles PTY errors gracefully
rg -n "async start" src/process-sessions.ts -A 10

Repository: Waishnav/devspace

Length of output: 1029


The test will fail on Unix platforms when node-pty is omitted from optional dependencies.

npm test runs src/process-sessions.test.ts, which includes unconditional PTY tests for Unix systems (lines 137-158). These tests call manager.start({tty: true}), which internally calls startPty(). When node-pty is missing, startPty() throws an error that is not caught by the test. The test must either skip these tests conditionally or mock node-pty when the optional dependency is unavailable, since node-pty is marked as optional in package.json.

🤖 Prompt for 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.

In `@package.json` around lines 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.

Comment thread src/apply-patch.ts
Comment on lines +1 to +2
import { constants } from "node:fs";
import { access, mkdir, readFile, realpath, rename, rm, stat, writeFile } from "node:fs/promises";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Use an exclusive random temporary file for staged writes.

The temporary name is predictable and writeFile() truncates existing files. A real workspace file matching the generated temp name can be overwritten before the patch commit/rollback path runs. Use a random suffix plus flag: "wx".

🛡️ Proposed fix
+import { randomUUID } from "node:crypto";
 import { constants } from "node:fs";
-    const temporary = `${destination}.devspace-patch-${process.pid}-${pendingWrites.length}`;
-    await writeFile(temporary, file.content, file.mode === undefined ? undefined : { mode: file.mode });
+    const temporary = `${destination}.devspace-patch-${process.pid}-${randomUUID()}`;
+    await writeFile(temporary, file.content, {
+      ...(file.mode === undefined ? {} : { mode: file.mode }),
+      flag: "wx",
+    });

Also applies to: 343-344

🤖 Prompt for 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.

In `@src/apply-patch.ts` around lines 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.

Comment thread src/apply-patch.ts
@@ -0,0 +1,442 @@
import { constants } from "node:fs";
import { access, mkdir, readFile, realpath, rename, rm, stat, writeFile } from "node:fs/promises";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject symlink file paths before applying updates.

resolveConfinedPath() only rejects symlinks that resolve outside the workspace. For an inside-root symlink, stat()/readFile() follow the link, but replaceFile() later renames over the symlink path, replacing the link with a regular file. Reject the final path with lstat() or write consistently to the resolved target.

🛡️ Proposed fix
-import { access, mkdir, readFile, realpath, rename, rm, stat, writeFile } from "node:fs/promises";
+import { access, lstat, mkdir, readFile, realpath, rename, rm, stat, writeFile } from "node:fs/promises";
-    const metadata = await stat(absolute);
+    const linkMetadata = await lstat(absolute);
+    if (linkMetadata.isSymbolicLink()) {
+      throw patchError(`path is a symbolic link: ${displayPath}`);
+    }
+    const metadata = await stat(absolute);
     if (!metadata.isFile()) throw patchError(`path is not a regular file: ${displayPath}`);

Also applies to: 275-277

🤖 Prompt for 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.

In `@src/apply-patch.ts` at 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.

Comment thread src/process-sessions.ts
Comment on lines +219 to +229
private startPipe(session: ProcessSession, input: StartCommandInput): void {
const shell = resolveShellCommand(input.command);
const detached = process.platform !== "win32";
const child = spawn(input.command, {
cwd: input.cwd,
env: process.env,
stdio: "pipe",
windowsHide: true,
detached,
shell: shell.executable,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify pipe-backed command execution no longer discards resolveShellCommand() args.
# Expected after the fix: no nearby spawn(input.command) call using shell.executable as the shell option.
rg -n -C3 'spawn\(input\.command|shell:\s*shell\.executable' src/process-sessions.ts

Repository: Waishnav/devspace

Length of output: 604


🏁 Script executed:

# Find resolveShellCommand function definition
fd -t f "\.ts$" src/ | xargs rg -l "resolveShellCommand" | head -5

Repository: Waishnav/devspace

Length of output: 232


🏁 Script executed:

# Search for resolveShellCommand definition
rg -n "function resolveShellCommand|const resolveShellCommand|export.*resolveShellCommand" src/

Repository: Waishnav/devspace

Length of output: 219


🏁 Script executed:

# Check PTY spawn usage to see how shell args are used
rg -n -B5 -A10 "nodePty\.spawn.*shell\." src/ | head -80

Repository: Waishnav/devspace

Length of output: 978


🏁 Script executed:

# Get the full definition of resolveShellCommand
sed -n '35,100p' src/process-platform.ts

Repository: Waishnav/devspace

Length of output: 1444


🏁 Script executed:

# Also check the return type structure
rg -n "shell\.args" src/process-sessions.ts | head -20

Repository: Waishnav/devspace

Length of output: 217


Spawn the resolved shell command, not the raw command string.

resolveShellCommand() returns both executable and args, with args containing critical shell flags (/d /s /c for Windows cmd.exe, -lc for login shells, -c for others), but startPipe() only uses shell.executable. This causes pipe-backed exec_command to drop the shell args and pass the raw command string to spawn, which treats it as a program name instead of a shell command. The PTY backend correctly uses both shell.executable and shell.args.

🐛 Proposed fix
     const shell = resolveShellCommand(input.command);
     const detached = process.platform !== "win32";
-    const child = spawn(input.command, {
+    const child = spawn(shell.executable, shell.args, {
       cwd: input.cwd,
       env: process.env,
       stdio: "pipe",
       windowsHide: true,
       detached,
-      shell: shell.executable,
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private startPipe(session: ProcessSession, input: StartCommandInput): void {
const shell = resolveShellCommand(input.command);
const detached = process.platform !== "win32";
const child = spawn(input.command, {
cwd: input.cwd,
env: process.env,
stdio: "pipe",
windowsHide: true,
detached,
shell: shell.executable,
});
private startPipe(session: ProcessSession, input: StartCommandInput): void {
const shell = resolveShellCommand(input.command);
const detached = process.platform !== "win32";
const child = spawn(shell.executable, shell.args, {
cwd: input.cwd,
env: process.env,
stdio: "pipe",
windowsHide: true,
detached,
});
🤖 Prompt for 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.

In `@src/process-sessions.ts` around lines 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.

Comment thread src/process-sessions.ts
Comment on lines +231 to +235
session.process = {
write: (data) => child.stdin.write(data),
kill: (signal = "SIGTERM") => terminateProcessTree(child, signal, detached),
resize: input.tty ? () => undefined : undefined,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not advertise resize support for pipe-backed sessions.

When tty: true falls back to startPipe() currently on Windows, this no-op resize method makes write_stdin accept columns/rows and report success without resizing. Leave resize undefined so the existing guard rejects non-PTY resize requests.

🐛 Proposed fix
     session.process = {
       write: (data) => child.stdin.write(data),
       kill: (signal = "SIGTERM") => terminateProcessTree(child, signal, detached),
-      resize: input.tty ? () => undefined : undefined,
     };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
session.process = {
write: (data) => child.stdin.write(data),
kill: (signal = "SIGTERM") => terminateProcessTree(child, signal, detached),
resize: input.tty ? () => undefined : undefined,
};
session.process = {
write: (data) => child.stdin.write(data),
kill: (signal = "SIGTERM") => terminateProcessTree(child, signal, detached),
};
🤖 Prompt for 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.

In `@src/process-sessions.ts` around lines 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.

Comment thread src/server.ts
Comment on lines +1103 to 1177
if (config.toolMode === "codex") {
registerAppTool(
server,
"apply_patch",
{
title: "Apply patch",
description:
"Apply one Codex-style patch inside an open workspace. Supports adding, updating, deleting, and moving files. Use this for all file modifications. Paths must be relative to the workspace. Call open_workspace first and pass workspaceId.",
inputSchema: {
workspaceId: z
.string()
.describe("Workspace identifier returned by open_workspace."),
patch: z
.string()
.describe("Patch text enclosed by *** Begin Patch and *** End Patch markers."),
},
outputSchema: resultOutputSchema({
additions: z.number(),
removals: z.number(),
files: z.array(
z.object({
path: z.string(),
previousPath: z.string().optional(),
operation: z.enum(["add", "update", "delete", "move"]),
}),
),
}),
...toolWidgetDescriptorMeta(config, "edit"),
annotations: EDIT_TOOL_ANNOTATIONS,
},
async ({ workspaceId, patch }) => {
const startedAt = performance.now();
const workspace = workspaces.getWorkspace(workspaceId);
const applied = await applyPatch(workspace.root, patch);
const paths = applied.files.map((file) => file.path).join(", ");
const result = `Applied patch to ${applied.files.length} file(s): ${paths}`;
const content = [textBlock(result)];
const displayPath = applied.files.length === 1
? applied.files[0]?.path
: `${applied.files.length} files`;

logToolCall(config, {
tool: "apply_patch",
workspaceId,
success: true,
durationMs: Math.round(performance.now() - startedAt),
});

return {
content,
_meta: {
tool: "apply_patch",
card: {
workspaceId,
path: displayPath,
summary: {
files: applied.files.length,
additions: applied.additions,
removals: applied.removals,
},
payload: { patch: applied.patch },
},
},
structuredContent: {
result,
additions: applied.additions,
removals: applied.removals,
files: applied.files,
},
};
},
);
}

if (config.widgets === "changes") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep Codex mode to the five-tool surface.

The Codex branch registers apply_patch, but the show_changes registration below still adds a sixth tool when widgets === "changes". Gate show_changes out of Codex mode so discovery matches the documented Codex tool contract.

🐛 Proposed fix
-  if (config.widgets === "changes") {
+  if (config.widgets === "changes" && config.toolMode !== "codex") {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (config.toolMode === "codex") {
registerAppTool(
server,
"apply_patch",
{
title: "Apply patch",
description:
"Apply one Codex-style patch inside an open workspace. Supports adding, updating, deleting, and moving files. Use this for all file modifications. Paths must be relative to the workspace. Call open_workspace first and pass workspaceId.",
inputSchema: {
workspaceId: z
.string()
.describe("Workspace identifier returned by open_workspace."),
patch: z
.string()
.describe("Patch text enclosed by *** Begin Patch and *** End Patch markers."),
},
outputSchema: resultOutputSchema({
additions: z.number(),
removals: z.number(),
files: z.array(
z.object({
path: z.string(),
previousPath: z.string().optional(),
operation: z.enum(["add", "update", "delete", "move"]),
}),
),
}),
...toolWidgetDescriptorMeta(config, "edit"),
annotations: EDIT_TOOL_ANNOTATIONS,
},
async ({ workspaceId, patch }) => {
const startedAt = performance.now();
const workspace = workspaces.getWorkspace(workspaceId);
const applied = await applyPatch(workspace.root, patch);
const paths = applied.files.map((file) => file.path).join(", ");
const result = `Applied patch to ${applied.files.length} file(s): ${paths}`;
const content = [textBlock(result)];
const displayPath = applied.files.length === 1
? applied.files[0]?.path
: `${applied.files.length} files`;
logToolCall(config, {
tool: "apply_patch",
workspaceId,
success: true,
durationMs: Math.round(performance.now() - startedAt),
});
return {
content,
_meta: {
tool: "apply_patch",
card: {
workspaceId,
path: displayPath,
summary: {
files: applied.files.length,
additions: applied.additions,
removals: applied.removals,
},
payload: { patch: applied.patch },
},
},
structuredContent: {
result,
additions: applied.additions,
removals: applied.removals,
files: applied.files,
},
};
},
);
}
if (config.widgets === "changes") {
if (config.toolMode === "codex") {
registerAppTool(
server,
"apply_patch",
{
title: "Apply patch",
description:
"Apply one Codex-style patch inside an open workspace. Supports adding, updating, deleting, and moving files. Use this for all file modifications. Paths must be relative to the workspace. Call open_workspace first and pass workspaceId.",
inputSchema: {
workspaceId: z
.string()
.describe("Workspace identifier returned by open_workspace."),
patch: z
.string()
.describe("Patch text enclosed by *** Begin Patch and *** End Patch markers."),
},
outputSchema: resultOutputSchema({
additions: z.number(),
removals: z.number(),
files: z.array(
z.object({
path: z.string(),
previousPath: z.string().optional(),
operation: z.enum(["add", "update", "delete", "move"]),
}),
),
}),
...toolWidgetDescriptorMeta(config, "edit"),
annotations: EDIT_TOOL_ANNOTATIONS,
},
async ({ workspaceId, patch }) => {
const startedAt = performance.now();
const workspace = workspaces.getWorkspace(workspaceId);
const applied = await applyPatch(workspace.root, patch);
const paths = applied.files.map((file) => file.path).join(", ");
const result = `Applied patch to ${applied.files.length} file(s): ${paths}`;
const content = [textBlock(result)];
const displayPath = applied.files.length === 1
? applied.files[0]?.path
: `${applied.files.length} files`;
logToolCall(config, {
tool: "apply_patch",
workspaceId,
success: true,
durationMs: Math.round(performance.now() - startedAt),
});
return {
content,
_meta: {
tool: "apply_patch",
card: {
workspaceId,
path: displayPath,
summary: {
files: applied.files.length,
additions: applied.additions,
removals: applied.removals,
},
payload: { patch: applied.patch },
},
},
structuredContent: {
result,
additions: applied.additions,
removals: applied.removals,
files: applied.files,
},
};
},
);
}
if (config.widgets === "changes" && config.toolMode !== "codex") {
🤖 Prompt for 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.

In `@src/server.ts` around lines 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant