Skip to content

fix(codex): approve MCP tools with PreToolUse hooks#2984

Open
cvolzer3 wants to merge 1 commit into
codex/sandbox-mcp-tool-permissionsfrom
codex/codex-mcp-pretool-hook
Open

fix(codex): approve MCP tools with PreToolUse hooks#2984
cvolzer3 wants to merge 1 commit into
codex/sandbox-mcp-tool-permissionsfrom
codex/codex-mcp-pretool-hook

Conversation

@cvolzer3

Copy link
Copy Markdown
Contributor

Summary

  • replace the Codex MCP proxy approval path with a Codex PreToolUse/PostToolUse hook bridge
  • install the hook into a private per-session CODEX_HOME and forward decisions through the existing MCP Store approval state
  • update the bundled codex-acp binary to a hook-capable version and make the downloader version-aware
  • keep one-shot approvals temporary by restoring backend approval state from PostToolUse

Stacked on #2954. That base PR contains the shared MCP approval plumbing; this PR is the Codex parity layer.

Testing

  • (cd packages/agent && ../../node_modules/.bin/vitest run --config vitest.config.ts src/adapters/codex/mcp-approval-hook.test.ts src/adapters/codex/spawn.test.ts src/server/agent-server.test.ts)
  • (cd packages/workspace-server && ./node_modules/.bin/vitest run --config vitest.config.ts src/services/agent/codex-home.test.ts src/services/agent/agent.test.ts src/services/agent/auth-adapter.test.ts src/services/mcp-proxy/mcp-proxy.test.ts)
  • ./node_modules/.bin/vitest run apps/code/scripts/download-binaries.test.mjs
  • (cd packages/agent && ../../node_modules/.bin/tsc --noEmit -p tsconfig.json)
  • (cd packages/workspace-server && ../../node_modules/.bin/tsc --noEmit -p tsconfig.json)
  • ./node_modules/.bin/biome lint <changed files>
  • git diff --check
  • (cd packages/agent && node ../../scripts/rimraf.mjs dist && ../../node_modules/.bin/tsup)

@github-actions

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit 5a5ec6c.

@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff (1)

  1. packages/workspace-server/src/services/agent/agent.ts, line 1110-1135 (link)

    P2 prepareCodexHome failures are now session-fatal

    The previous code wrapped prepareCodexHome in a try-catch with an explicit comment: "A skills-prep failure must not kill the session; Codex falls back to its default home." That guard is gone. Now any filesystem error while writing the hook script or the hooks.json (e.g. a full disk, a permissions problem on appDataPath) will propagate, abort the session, and require a user retry.

    The change may be intentional — starting a Codex session without a working approval hook leaves MCP tools in an uncontrolled state — but if so, the old comment and the rationale for its removal should be documented so the decision is clear to future readers.

Reviews (1): Last reviewed commit: "fix(codex): approve MCP tools with PreTo..." | Re-trigger Greptile

Comment on lines +77 to +84
function readRequestBody(req: http.IncomingMessage): Promise<string> {
return new Promise((resolve, reject) => {
const chunks: Buffer[] = [];
req.on("data", (chunk: Buffer) => chunks.push(chunk));
req.on("end", () => resolve(Buffer.concat(chunks).toString("utf8")));
req.on("error", reject);
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 No request-body size cap

readRequestBody accumulates the entire payload into memory with no upper bound. Codex passes the full tool_input in the hook payload, so a tool with a large input (file content, search results, etc.) would buffer it all before the bridge processes it. On localhost with a bearer token the risk of deliberate abuse is low, but a Codex bug producing an outsized payload could cause OOM or excessive latency. A simple MAX_BODY_BYTES guard (e.g. 4 MB) and an early rejection keeps the bridge robust without changing any semantics.

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