From 480c2594c249384e7725b14fd291d89ac84b9edf Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 14 May 2026 07:45:39 +0000 Subject: [PATCH 01/19] feat: add OpenAI Codex platform support Codex hooks use the same JSON wire format as Claude Code hooks (see https://developers.openai.com/codex/hooks), so this change introduces: - New codex/ package re-exporting the relevant claude types and helpers with Codex-specific documentation (Stop, PreToolUse, PermissionRequest, PostToolUse, UserPromptSubmit, SessionStart). - PlatformCodex constant and codex- handler registrations on every unified handler (OnStop, OnBeforeExecution, OnAfterFileEdit, OnPromptSubmit). StopContext.ShouldSkip() now also honors StopHookActive for Codex. - OnAfterFileEdit's codex-post-tool-use registration recognizes apply_patch in addition to Write and Edit, matching the upstream tool surface. - hookshot install --codex writes ~/.codex/hooks.json with appropriate matchers (Bash|apply_patch for PreToolUse, apply_patch|Edit|Write for PostToolUse). The installer reminds users that codex_hooks = true under [features] in ~/.codex/config.toml is still required. - Tests verify that the codex handlers are registered alongside the existing platforms and that PlatformCodex behaves like Claude/Droid for ShouldSkip(). - README, doc.go, docs/reference-unified.md, examples/multi-hook, and a new docs/reference-codex.md document the platform end-to-end. Co-authored-by: Ashwin Ramaswami --- README.md | 45 +++- cmd/hookshot/main.go | 93 +++++++- codex/doc.go | 66 ++++++ codex/helpers.go | 93 ++++++++ codex/helpers_test.go | 198 +++++++++++++++++ codex/types.go | 132 +++++++++++ doc.go | 5 +- docs/reference-codex.md | 433 ++++++++++++++++++++++++++++++++++++ docs/reference-unified.md | 29 ++- examples/multi-hook/main.go | 16 +- hookshot.go | 2 +- unified.go | 144 +++++++++++- unified_test.go | 23 ++ 13 files changed, 1245 insertions(+), 34 deletions(-) create mode 100644 codex/doc.go create mode 100644 codex/helpers.go create mode 100644 codex/helpers_test.go create mode 100644 codex/types.go create mode 100644 docs/reference-codex.md diff --git a/README.md b/README.md index 8c12988..1ae2993 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # hookshot -A Go library for building hooks for AI coding agents like [Cursor](https://cursor.com/docs/agent/hooks), [Claude Code](https://docs.claude.com/en/docs/claude-code/hooks), [Windsurf Cascade](https://docs.codeium.com/windsurf/memories#hooks), and [Factory Droid](https://docs.factory.ai/reference/hooks-reference). +A Go library for building hooks for AI coding agents like [Cursor](https://cursor.com/docs/agent/hooks), [Claude Code](https://docs.claude.com/en/docs/claude-code/hooks), [Windsurf Cascade](https://docs.codeium.com/windsurf/memories#hooks), [Factory Droid](https://docs.factory.ai/reference/hooks-reference), and [OpenAI Codex](https://developers.openai.com/codex/hooks). Hooks are a key component of [Agentic Coding Security Management (ACSM)](https://corridor.dev/blog/introducing-acsm/) — they let you observe, control, and secure AI agent behavior in your development environment. @@ -97,16 +97,36 @@ hookshot install --binary /path/to/my-hooks } ``` +### OpenAI Codex (`~/.codex/hooks.json`) + +Codex hooks are gated behind a feature flag. Enable it in `~/.codex/config.toml`: + +```toml +[features] +codex_hooks = true +``` + +Then configure the hooks themselves: + +```json +{ + "hooks": { + "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], + "PreToolUse": [{ "matcher": "Bash|apply_patch", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }] + } +} +``` + ## Unified Handlers -Write once, run on all four platforms: +Write once, run on all five platforms: -| Handler | Claude Code | Cursor | Windsurf Cascade | Factory Droid | -|---------|-------------|--------|------------------|---------------| -| `OnStop` | Stop | stop | post-cascade-response | Stop | -| `OnBeforeExecution` | PreToolUse | beforeShellExecution, beforeMCPExecution | pre-run-command, pre-mcp-tool-use | PreToolUse | -| `OnAfterFileEdit` | PostToolUse | afterFileEdit | post-write-code | PostToolUse | -| `OnPromptSubmit` | UserPromptSubmit | beforeSubmitPrompt | pre-user-prompt | UserPromptSubmit | +| Handler | Claude Code | Cursor | Windsurf Cascade | Factory Droid | OpenAI Codex | +|---------|-------------|--------|------------------|---------------|--------------| +| `OnStop` | Stop | stop | post-cascade-response | Stop | Stop | +| `OnBeforeExecution` | PreToolUse | beforeShellExecution, beforeMCPExecution | pre-run-command, pre-mcp-tool-use | PreToolUse | PreToolUse | +| `OnAfterFileEdit` | PostToolUse | afterFileEdit | post-write-code | PostToolUse | PostToolUse | +| `OnPromptSubmit` | UserPromptSubmit | beforeSubmitPrompt | pre-user-prompt | UserPromptSubmit | UserPromptSubmit | ## Platform-Specific Handlers @@ -140,6 +160,13 @@ hookshot.Register("droid-pre-tool-use", func() { return droid.PassThrough() }) }) + +// OpenAI Codex: Pre-tool use (matches Bash, apply_patch, and MCP tools) +hookshot.Register("codex-pre-tool-use", func() { + hookshot.Run(func(input codex.PreToolUseInput) codex.PreToolUseOutput { + return codex.PassThrough() + }) +}) ``` ## Documentation @@ -149,6 +176,7 @@ hookshot.Register("droid-pre-tool-use", func() { - [Cursor Reference](docs/reference-cursor.md) - [Windsurf Cascade Reference](docs/reference-cascade.md) - [Factory Droid Reference](docs/reference-droid.md) +- [OpenAI Codex Reference](docs/reference-codex.md) Full API documentation is available via godoc: @@ -158,6 +186,7 @@ go doc github.com/CorridorSecurity/hookshot/claude go doc github.com/CorridorSecurity/hookshot/cursor go doc github.com/CorridorSecurity/hookshot/cascade go doc github.com/CorridorSecurity/hookshot/droid +go doc github.com/CorridorSecurity/hookshot/codex ``` Or view online at [pkg.go.dev/github.com/CorridorSecurity/hookshot](https://pkg.go.dev/github.com/CorridorSecurity/hookshot). diff --git a/cmd/hookshot/main.go b/cmd/hookshot/main.go index 5696631..770a538 100644 --- a/cmd/hookshot/main.go +++ b/cmd/hookshot/main.go @@ -15,6 +15,7 @@ // // hookshot install --binary ./my-hooks // hookshot install --binary ./my-hooks --claude --cursor +// hookshot install --binary ./my-hooks --codex package main import ( @@ -56,7 +57,7 @@ Usage: Commands: build Build hooks binary for one or more platforms - install Install hooks to AI coding agent config files (Claude Code, Cursor, Droid, Cascade) + install Install hooks to AI coding agent config files (Claude Code, Cursor, Droid, Cascade, Codex) Run 'hookshot -h' for command-specific help.`) } @@ -199,11 +200,12 @@ func runInstall(args []string) { fs := flag.NewFlagSet("install", flag.ExitOnError) var ( - binaryPath string - claudeFlag bool - cursorFlag bool - droidFlag bool - cascadeFlag bool + binaryPath string + claudeFlag bool + cursorFlag bool + droidFlag bool + cascadeFlag bool + codexFlag bool ) fs.StringVar(&binaryPath, "binary", "", "Path to hooks binary (required)") @@ -211,6 +213,7 @@ func runInstall(args []string) { fs.BoolVar(&cursorFlag, "cursor", false, "Install to Cursor only") fs.BoolVar(&droidFlag, "droid", false, "Install to Factory Droid only") fs.BoolVar(&cascadeFlag, "cascade", false, "Install to Windsurf Cascade only") + fs.BoolVar(&codexFlag, "codex", false, "Install to OpenAI Codex only") fs.Usage = func() { fmt.Println(`Install hooks to AI coding agent config files. @@ -224,6 +227,7 @@ Examples: hookshot install --binary ./my-hooks --cursor # Cursor only hookshot install --binary ./my-hooks --droid # Factory Droid only hookshot install --binary ./my-hooks --cascade # Windsurf Cascade only + hookshot install --binary ./my-hooks --codex # OpenAI Codex only Flags:`) fs.PrintDefaults() @@ -251,11 +255,12 @@ Flags:`) } // If none specified, install to all - if !claudeFlag && !cursorFlag && !droidFlag && !cascadeFlag { + if !claudeFlag && !cursorFlag && !droidFlag && !cascadeFlag && !codexFlag { claudeFlag = true cursorFlag = true droidFlag = true cascadeFlag = true + codexFlag = true } if claudeFlag { @@ -286,6 +291,13 @@ Flags:`) } } + if codexFlag { + if err := installCodex(absPath); err != nil { + fmt.Fprintf(os.Stderr, "Error installing to OpenAI Codex: %v\n", err) + os.Exit(1) + } + } + fmt.Println("\nInstallation complete!") } @@ -454,6 +466,73 @@ func installDroid(binaryPath string) error { return nil } +func installCodex(binaryPath string) error { + homeDir, _ := os.UserHomeDir() + configPath := filepath.Join(homeDir, ".codex", "hooks.json") + + fmt.Printf("Installing to OpenAI Codex (%s)...\n", configPath) + + // Read existing config or create new + var config map[string]any + data, err := os.ReadFile(configPath) + if err == nil { + json.Unmarshal(data, &config) + } + if config == nil { + config = make(map[string]any) + } + + // Codex hook config follows the same shape as Claude Code's settings, but + // lives in ~/.codex/hooks.json. PostToolUse matches "apply_patch|Edit|Write" + // because Codex uses apply_patch for file edits in addition to Write/Edit + // aliases. + hooks := map[string]any{ + "Stop": []map[string]any{{ + "hooks": []map[string]any{{ + "type": "command", + "command": binaryPath + " codex-stop", + }}, + }}, + "PreToolUse": []map[string]any{{ + "matcher": "Bash|apply_patch", + "hooks": []map[string]any{{ + "type": "command", + "command": binaryPath + " codex-pre-tool-use", + }}, + }}, + "PostToolUse": []map[string]any{{ + "matcher": "apply_patch|Edit|Write", + "hooks": []map[string]any{{ + "type": "command", + "command": binaryPath + " codex-post-tool-use", + }}, + }}, + "UserPromptSubmit": []map[string]any{{ + "hooks": []map[string]any{{ + "type": "command", + "command": binaryPath + " codex-user-prompt-submit", + }}, + }}, + } + + config["hooks"] = hooks + + os.MkdirAll(filepath.Dir(configPath), 0755) + + output, err := json.MarshalIndent(config, "", " ") + if err != nil { + return err + } + + if err := os.WriteFile(configPath, output, 0644); err != nil { + return err + } + + fmt.Println(" Installed hooks: Stop, PreToolUse, PostToolUse, UserPromptSubmit") + fmt.Println(" Note: Codex hooks require codex_hooks = true under [features] in ~/.codex/config.toml") + return nil +} + func installCascade(binaryPath string) error { homeDir, _ := os.UserHomeDir() configPath := filepath.Join(homeDir, ".codeium", "windsurf", "hooks.json") diff --git a/codex/doc.go b/codex/doc.go new file mode 100644 index 0000000..d85a94b --- /dev/null +++ b/codex/doc.go @@ -0,0 +1,66 @@ +// Package codex provides types and helpers for OpenAI Codex hooks. +// +// OpenAI Codex hooks use the same JSON wire format as Claude Code hooks, so +// this package re-exports types and helpers from the claude package for +// convenience. Codex adds the apply_patch tool name to PreToolUse and +// PostToolUse events, and exposes a few Codex-specific fields (turn_id, +// model, last_assistant_message) that are not represented in the shared +// types but are still available as raw JSON when needed. +// +// See https://developers.openai.com/codex/hooks for the upstream +// specification. +// +// # Configuration +// +// Codex hooks are enabled behind a feature flag in ~/.codex/config.toml: +// +// [features] +// codex_hooks = true +// +// Configure hook commands in ~/.codex/hooks.json (or inline under [hooks] in +// ~/.codex/config.toml): +// +// { +// "hooks": { +// "PreToolUse": [ +// { +// "matcher": "Bash|apply_patch", +// "hooks": [{ "type": "command", "command": "/path/to/hook codex-pre-tool-use" }] +// } +// ], +// "PostToolUse": [ +// { +// "matcher": "apply_patch|Edit|Write", +// "hooks": [{ "type": "command", "command": "/path/to/hook codex-post-tool-use" }] +// } +// ], +// "UserPromptSubmit": [ +// { "hooks": [{ "type": "command", "command": "/path/to/hook codex-user-prompt-submit" }] } +// ], +// "Stop": [ +// { "hooks": [{ "type": "command", "command": "/path/to/hook codex-stop" }] } +// ] +// } +// } +// +// # Example Usage +// +// package main +// +// import ( +// "github.com/CorridorSecurity/hookshot" +// "github.com/CorridorSecurity/hookshot/codex" +// ) +// +// func main() { +// hookshot.Register("codex-pre-tool-use", func() { +// hookshot.Run(func(input codex.PreToolUseInput) codex.PreToolUseOutput { +// if input.ToolName == "Bash" { +// return codex.Allow("Bash command reviewed by hook") +// } +// return codex.PassThrough() +// }) +// }) +// hookshot.RunCommand() +// } +package codex diff --git a/codex/helpers.go b/codex/helpers.go new file mode 100644 index 0000000..d92c4a5 --- /dev/null +++ b/codex/helpers.go @@ -0,0 +1,93 @@ +package codex + +import "github.com/CorridorSecurity/hookshot/claude" + +// ============================================================================= +// Stop Helpers (re-exported from claude) +// ============================================================================= + +// Continue allows Codex to stop normally. +var Continue = claude.Continue + +// Block prevents Codex from stopping and asks it to continue the turn. +// The reason is used as the continuation prompt text. +var Block = claude.Block + +// StopWith creates a StopOutput that halts Codex entirely (continue=false). +var StopWith = claude.StopWith + +// ============================================================================= +// PreToolUse Helpers (re-exported from claude) +// ============================================================================= + +// Allow permits the tool to execute. Note that Codex currently parses but +// does not enforce permissionDecision: "allow" for PreToolUse, so this +// effectively falls through to the normal flow. +var Allow = claude.Allow + +// AllowSilent permits the tool to execute without showing output. +var AllowSilent = claude.AllowSilent + +// AllowWithInput permits the tool with modified input parameters. Note that +// Codex currently parses but does not enforce updatedInput for PreToolUse. +var AllowWithInput = claude.AllowWithInput + +// Deny blocks the tool from executing. This is enforced by Codex for Bash +// and apply_patch tools. +var Deny = claude.Deny + +// Ask prompts the user to confirm the tool execution. Note that Codex +// currently parses but does not enforce permissionDecision: "ask" for +// PreToolUse, so this falls open today. +var Ask = claude.Ask + +// PassThrough returns an empty output, letting the normal permission flow proceed. +var PassThrough = claude.PassThrough + +// ============================================================================= +// PermissionRequest Helpers (re-exported from claude) +// ============================================================================= + +// AllowPermission grants the permission request without surfacing the +// approval prompt. +var AllowPermission = claude.AllowPermission + +// DenyPermission rejects the permission request with a message shown to Codex. +var DenyPermission = claude.DenyPermission + +// ============================================================================= +// PostToolUse Helpers (re-exported from claude) +// ============================================================================= + +// PostToolOK returns an empty output, allowing normal flow to continue. +var PostToolOK = claude.PostToolOK + +// PostToolBlock provides feedback to Codex that replaces the tool result and +// continues the model from the hook-provided message. +var PostToolBlock = claude.PostToolBlock + +// PostToolContext adds developer context after the tool runs. +var PostToolContext = claude.PostToolContext + +// ============================================================================= +// UserPromptSubmit Helpers (re-exported from claude) +// ============================================================================= + +// AllowPrompt allows the prompt to be processed normally. +var AllowPrompt = claude.AllowPrompt + +// BlockPrompt prevents the prompt from being processed. +var BlockPrompt = claude.BlockPrompt + +// AddContext allows the prompt and adds developer context for Codex. +var AddContext = claude.AddContext + +// ============================================================================= +// SessionStart Helpers (re-exported from claude) +// ============================================================================= + +// SessionStartOK returns an empty output for session start. +var SessionStartOK = claude.SessionStartOK + +// SessionStartContext adds developer context at the start of a session. +var SessionStartContext = claude.SessionStartContext diff --git a/codex/helpers_test.go b/codex/helpers_test.go new file mode 100644 index 0000000..300688d --- /dev/null +++ b/codex/helpers_test.go @@ -0,0 +1,198 @@ +package codex + +import ( + "testing" +) + +// These tests verify that the codex helpers correctly re-export the Claude +// Code helpers and produce the wire shapes documented for Codex hooks. + +// ============================================================================= +// Stop Helpers Tests +// ============================================================================= + +func TestContinue(t *testing.T) { + output := Continue() + if output.Decision != "" { + t.Errorf("Decision should be empty, got %q", output.Decision) + } + if output.Reason != "" { + t.Errorf("Reason should be empty, got %q", output.Reason) + } +} + +func TestBlock(t *testing.T) { + output := Block("Run one more pass over the failing tests") + if output.Decision != "block" { + t.Errorf("Decision = %q, want %q", output.Decision, "block") + } + if output.Reason != "Run one more pass over the failing tests" { + t.Errorf("Reason = %q, want %q", output.Reason, "Run one more pass over the failing tests") + } +} + +func TestStopWith(t *testing.T) { + output := StopWith("Stopping due to policy violation") + if output.Continue == nil || *output.Continue != false { + t.Error("Continue should be false") + } + if output.StopReason != "Stopping due to policy violation" { + t.Errorf("StopReason = %q, want %q", output.StopReason, "Stopping due to policy violation") + } +} + +// ============================================================================= +// PreToolUse Helpers Tests +// ============================================================================= + +func TestDeny(t *testing.T) { + output := Deny("Destructive command blocked by hook.") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.PermissionDecision != "deny" { + t.Errorf("PermissionDecision = %q, want %q", output.HookSpecificOutput.PermissionDecision, "deny") + } + if output.HookSpecificOutput.PermissionDecisionReason != "Destructive command blocked by hook." { + t.Errorf("PermissionDecisionReason = %q, want %q", output.HookSpecificOutput.PermissionDecisionReason, "Destructive command blocked by hook.") + } +} + +func TestAllow(t *testing.T) { + output := Allow("Trusted command") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.PermissionDecision != "allow" { + t.Errorf("PermissionDecision = %q, want %q", output.HookSpecificOutput.PermissionDecision, "allow") + } +} + +func TestPassThrough(t *testing.T) { + output := PassThrough() + if output.HookSpecificOutput != nil { + t.Error("HookSpecificOutput should be nil for PassThrough") + } +} + +// ============================================================================= +// PermissionRequest Helpers Tests +// ============================================================================= + +func TestAllowPermission(t *testing.T) { + output := AllowPermission() + if output.HookSpecificOutput == nil || output.HookSpecificOutput.Decision == nil { + t.Fatal("HookSpecificOutput and Decision should not be nil") + } + if output.HookSpecificOutput.HookEventName != "PermissionRequest" { + t.Errorf("HookEventName = %q, want %q", output.HookSpecificOutput.HookEventName, "PermissionRequest") + } + if output.HookSpecificOutput.Decision.Behavior != "allow" { + t.Errorf("Behavior = %q, want %q", output.HookSpecificOutput.Decision.Behavior, "allow") + } +} + +func TestDenyPermission(t *testing.T) { + output := DenyPermission("Blocked by repository policy.") + if output.HookSpecificOutput == nil || output.HookSpecificOutput.Decision == nil { + t.Fatal("HookSpecificOutput and Decision should not be nil") + } + if output.HookSpecificOutput.Decision.Behavior != "deny" { + t.Errorf("Behavior = %q, want %q", output.HookSpecificOutput.Decision.Behavior, "deny") + } + if output.HookSpecificOutput.Decision.Message != "Blocked by repository policy." { + t.Errorf("Message = %q, want %q", output.HookSpecificOutput.Decision.Message, "Blocked by repository policy.") + } +} + +// ============================================================================= +// PostToolUse Helpers Tests +// ============================================================================= + +func TestPostToolOK(t *testing.T) { + output := PostToolOK() + if output.Decision != "" { + t.Errorf("Decision should be empty, got %q", output.Decision) + } +} + +func TestPostToolBlock(t *testing.T) { + output := PostToolBlock("Output needs review before continuing.") + if output.Decision != "block" { + t.Errorf("Decision = %q, want %q", output.Decision, "block") + } + if output.Reason != "Output needs review before continuing." { + t.Errorf("Reason = %q, want %q", output.Reason, "Output needs review before continuing.") + } +} + +func TestPostToolContext(t *testing.T) { + output := PostToolContext("The command updated generated files.") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.HookEventName != "PostToolUse" { + t.Errorf("HookEventName = %q, want %q", output.HookSpecificOutput.HookEventName, "PostToolUse") + } + if output.HookSpecificOutput.AdditionalContext != "The command updated generated files." { + t.Errorf("AdditionalContext = %q, want %q", output.HookSpecificOutput.AdditionalContext, "The command updated generated files.") + } +} + +// ============================================================================= +// UserPromptSubmit Helpers Tests +// ============================================================================= + +func TestAllowPrompt(t *testing.T) { + output := AllowPrompt() + if output.Decision != "" { + t.Errorf("Decision should be empty, got %q", output.Decision) + } +} + +func TestBlockPrompt(t *testing.T) { + output := BlockPrompt("Ask for confirmation before doing that.") + if output.Decision != "block" { + t.Errorf("Decision = %q, want %q", output.Decision, "block") + } + if output.Reason != "Ask for confirmation before doing that." { + t.Errorf("Reason = %q, want %q", output.Reason, "Ask for confirmation before doing that.") + } +} + +func TestAddContext(t *testing.T) { + output := AddContext("Ask for a clearer reproduction before editing files.") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.HookEventName != "UserPromptSubmit" { + t.Errorf("HookEventName = %q, want %q", output.HookSpecificOutput.HookEventName, "UserPromptSubmit") + } + if output.HookSpecificOutput.AdditionalContext != "Ask for a clearer reproduction before editing files." { + t.Errorf("AdditionalContext = %q, want %q", output.HookSpecificOutput.AdditionalContext, "Ask for a clearer reproduction before editing files.") + } +} + +// ============================================================================= +// SessionStart Helpers Tests +// ============================================================================= + +func TestSessionStartOK(t *testing.T) { + output := SessionStartOK() + if output.HookSpecificOutput != nil { + t.Error("HookSpecificOutput should be nil for SessionStartOK") + } +} + +func TestSessionStartContext(t *testing.T) { + output := SessionStartContext("Load the workspace conventions before editing.") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.HookEventName != "SessionStart" { + t.Errorf("HookEventName = %q, want %q", output.HookSpecificOutput.HookEventName, "SessionStart") + } + if output.HookSpecificOutput.AdditionalContext != "Load the workspace conventions before editing." { + t.Errorf("AdditionalContext = %q, want %q", output.HookSpecificOutput.AdditionalContext, "Load the workspace conventions before editing.") + } +} diff --git a/codex/types.go b/codex/types.go new file mode 100644 index 0000000..609594d --- /dev/null +++ b/codex/types.go @@ -0,0 +1,132 @@ +// Package codex provides types and helpers for OpenAI Codex hooks. +// +// Codex hooks use the same JSON protocol as Claude Code hooks. +// This package re-exports the claude package types for convenience. +// +// See https://developers.openai.com/codex/hooks for the upstream +// specification. +package codex + +import "github.com/CorridorSecurity/hookshot/claude" + +// ============================================================================= +// Common Types (re-exported from claude) +// ============================================================================= + +// BaseInput contains fields present in all Codex hook inputs. +// +// In Codex, every command hook receives one JSON object on stdin with these +// shared fields (see https://developers.openai.com/codex/hooks). Codex also +// passes a "model" field and turn-scoped hooks include a "turn_id"; those +// fields are accessible through the raw JSON if needed. +type BaseInput = claude.BaseInput + +// BaseOutput contains common fields that can be included in any hook output. +type BaseOutput = claude.BaseOutput + +// ============================================================================= +// Stop (re-exported from claude) +// ============================================================================= + +// StopInput is received when the main Codex turn finishes responding. +type StopInput = claude.StopInput + +// StopOutput controls whether Codex should stop or continue the turn. +// +// For this event, decision: "block" doesn't reject the turn. Instead, it tells +// Codex to continue and automatically creates a new continuation prompt that +// acts as a new user prompt, using the reason as that prompt text. +type StopOutput = claude.StopOutput + +// ============================================================================= +// SessionStart (re-exported from claude) +// ============================================================================= + +// SessionStartInput is received when Codex starts or resumes a session. +// +// The "source" field is one of "startup", "resume", or "clear". +type SessionStartInput = claude.SessionStartInput + +// SessionStartOutput can add developer context to the session. +type SessionStartOutput = claude.SessionStartOutput + +// SessionStartHookOutput contains session-start-specific output fields. +type SessionStartHookOutput = claude.SessionStartHookOutput + +// ============================================================================= +// PreToolUse (re-exported from claude) +// ============================================================================= + +// PreToolUseInput is received before Codex executes a tool. +// +// Codex PreToolUse can intercept Bash, file edits performed through +// apply_patch, and MCP tool calls. The canonical ToolName values include +// "Bash", "apply_patch", and MCP tool names like "mcp__server__tool". +type PreToolUseInput = claude.PreToolUseInput + +// PreToolUseOutput controls whether the tool should execute. +// +// Codex currently honors permissionDecision: "deny" (or the older +// decision: "block" shape) for Bash and apply_patch. "allow" and "ask" +// values are parsed but fall open today. +type PreToolUseOutput = claude.PreToolUseOutput + +// PreToolUseHookOutput contains pre-tool-use-specific output fields. +type PreToolUseHookOutput = claude.PreToolUseHookOutput + +// ============================================================================= +// PermissionRequest (re-exported from claude) +// ============================================================================= + +// PermissionRequestInput is received when Codex is about to ask for approval, +// such as a shell escalation or managed-network approval. It doesn't run for +// commands that don't need approval. +type PermissionRequestInput = claude.PermissionRequestInput + +// PermissionRequestOutput controls the permission dialog response. +// +// Codex respects "allow" and "deny" behaviors. Returning updatedInput, +// updatedPermissions, or interrupt is reserved for future behavior and +// currently fails closed. +type PermissionRequestOutput = claude.PermissionRequestOutput + +// PermissionRequestHookOutput contains permission-request-specific output fields. +type PermissionRequestHookOutput = claude.PermissionRequestHookOutput + +// PermissionRequestDecision controls how the permission request is handled. +type PermissionRequestDecision = claude.PermissionRequestDecision + +// ============================================================================= +// PostToolUse (re-exported from claude) +// ============================================================================= + +// PostToolUseInput is received after a tool executes. +// +// For Bash, PostToolUse also runs after commands that exit with a non-zero +// status. The canonical ToolName values include "Bash", "apply_patch", and +// MCP tool names like "mcp__server__tool". +type PostToolUseInput = claude.PostToolUseInput + +// PostToolUseOutput can provide feedback to Codex after tool execution. +// +// For this event, decision: "block" doesn't undo the completed tool call. +// Codex records the feedback, replaces the tool result with it, and continues +// the model from the hook-provided message. +type PostToolUseOutput = claude.PostToolUseOutput + +// PostToolUseHookOutput contains post-tool-use-specific output fields. +type PostToolUseHookOutput = claude.PostToolUseHookOutput + +// ============================================================================= +// UserPromptSubmit (re-exported from claude) +// ============================================================================= + +// UserPromptSubmitInput is received when the user submits a prompt. +type UserPromptSubmitInput = claude.UserPromptSubmitInput + +// UserPromptSubmitOutput controls whether the prompt should be processed +// and can add developer context. +type UserPromptSubmitOutput = claude.UserPromptSubmitOutput + +// UserPromptSubmitHookOutput contains user-prompt-submit-specific output fields. +type UserPromptSubmitHookOutput = claude.UserPromptSubmitHookOutput diff --git a/doc.go b/doc.go index d7eeeba..34a2dc3 100644 --- a/doc.go +++ b/doc.go @@ -1,5 +1,5 @@ // Package hookshot provides a framework for building hooks for AI coding agents -// like Cursor and Claude Code. +// like Cursor, Claude Code, Windsurf Cascade, Factory Droid, and OpenAI Codex. // // # Quick Start // @@ -112,6 +112,9 @@ // - hookshot (this package): Core Run/Register/RunCommand functions // - hookshot/claude: Types and helpers for Claude Code hooks // - hookshot/cursor: Types and helpers for Cursor hooks +// - hookshot/cascade: Types and helpers for Windsurf Cascade hooks +// - hookshot/droid: Types and helpers for Factory Droid hooks +// - hookshot/codex: Types and helpers for OpenAI Codex hooks // - hookshot/build: Cross-platform build tool // - hookshot/internal: Internal JSON I/O (not for external use) // diff --git a/docs/reference-codex.md b/docs/reference-codex.md new file mode 100644 index 0000000..5cd7b9c --- /dev/null +++ b/docs/reference-codex.md @@ -0,0 +1,433 @@ +# OpenAI Codex API Reference + +Platform-specific types and helpers for OpenAI Codex hooks. Use these when you need features not available in the unified API. + +Codex hooks use the same JSON wire format as Claude Code hooks, configured in `~/.codex/hooks.json` or inline `[hooks]` tables in `~/.codex/config.toml`. The `codex` Go package re-exports the relevant `claude` types so your code can stay platform-explicit while still benefiting from the shared types. + +Codex hooks are behind a feature flag — make sure `~/.codex/config.toml` contains: + +```toml +[features] +codex_hooks = true +``` + +See the upstream spec at https://developers.openai.com/codex/hooks. + +## Events + +| Event | Input | Output | Description | +|-------|-------|--------|-------------| +| SessionStart | `SessionStartInput` | `SessionStartOutput` | Session started or resumed | +| PreToolUse | `PreToolUseInput` | `PreToolUseOutput` | Before tool execution (Bash, apply_patch, MCP) | +| PermissionRequest | `PermissionRequestInput` | `PermissionRequestOutput` | Approval prompt about to surface | +| PostToolUse | `PostToolUseInput` | `PostToolUseOutput` | After tool execution | +| UserPromptSubmit | `UserPromptSubmitInput` | `UserPromptSubmitOutput` | User submitted a prompt | +| Stop | `StopInput` | `StopOutput` | Turn finished responding | + +--- + +## Common Types + +### BaseInput + +All Codex hook inputs include these fields: + +```go +type BaseInput struct { + SessionID string `json:"session_id"` + TranscriptPath string `json:"transcript_path"` + Cwd string `json:"cwd"` + PermissionMode string `json:"permission_mode"` + HookEventName string `json:"hook_event_name"` +} +``` + +Codex also sends a `model` field (active model slug) on every hook event, and a `turn_id` field on turn-scoped events (`PreToolUse`, `PermissionRequest`, `PostToolUse`, `UserPromptSubmit`, `Stop`). These fields aren't represented on the shared `BaseInput` struct, but you can read them by binding the raw JSON yourself with `hookshot.ReadRawInput`. + +### BaseOutput + +Common fields for any hook output: + +```go +type BaseOutput struct { + Continue *bool `json:"continue,omitempty"` + StopReason string `json:"stopReason,omitempty"` + SuppressOutput bool `json:"suppressOutput,omitempty"` // parsed but not enforced today + SystemMessage string `json:"systemMessage,omitempty"` +} +``` + +Codex enforces `continue: false` on `SessionStart`, `UserPromptSubmit`, `PostToolUse`, and `Stop`. For `PreToolUse` and `PermissionRequest`, `continue`, `stopReason`, and `suppressOutput` are parsed but currently fail open. + +--- + +## SessionStart + +Called when a session starts or resumes. The `matcher` regex is applied to the `source` field. Current runtime values are `startup`, `resume`, and `clear`. + +### SessionStartInput + +```go +type SessionStartInput struct { + BaseInput + Source string `json:"source"` // "startup", "resume", "clear" +} +``` + +### SessionStartOutput + +```go +type SessionStartOutput struct { + BaseOutput + HookSpecificOutput *SessionStartHookOutput `json:"hookSpecificOutput,omitempty"` +} + +type SessionStartHookOutput struct { + HookEventName string `json:"hookEventName,omitempty"` + AdditionalContext string `json:"additionalContext,omitempty"` +} +``` + +### Helper Functions + +```go +func SessionStartOK() SessionStartOutput +func SessionStartContext(context string) SessionStartOutput +``` + +### Example + +```go +hookshot.Register("codex-session-start", func() { + hookshot.Run(func(input codex.SessionStartInput) codex.SessionStartOutput { + return codex.SessionStartContext("Project uses Go 1.21+") + }) +}) +``` + +--- + +## PreToolUse + +Called before Codex executes a tool. Currently intercepts simple Bash commands, file edits performed through `apply_patch`, and MCP tool calls. The `matcher` regex is applied to `tool_name` and matcher aliases — `apply_patch` also matches `Edit` and `Write`. + +### PreToolUseInput + +```go +type PreToolUseInput struct { + BaseInput + ToolName string `json:"tool_name"` // "Bash", "apply_patch", or "mcp__server__tool" + ToolInput json.RawMessage `json:"tool_input"` + ToolUseID string `json:"tool_use_id"` +} +``` + +For `Bash` and `apply_patch`, the `tool_input` includes a `command` field. For MCP tools it carries all the arguments passed to the MCP call. + +### PreToolUseOutput + +```go +type PreToolUseOutput struct { + BaseOutput + HookSpecificOutput *PreToolUseHookOutput `json:"hookSpecificOutput,omitempty"` +} + +type PreToolUseHookOutput struct { + HookEventName string `json:"hookEventName,omitempty"` + PermissionDecision string `json:"permissionDecision,omitempty"` + PermissionDecisionReason string `json:"permissionDecisionReason,omitempty"` + UpdatedInput map[string]any `json:"updatedInput,omitempty"` +} +``` + +Codex honors `permissionDecision: "deny"` (or the older `decision: "block"` shape) on Bash and `apply_patch`. `"allow"`, `"ask"`, `updatedInput`, `additionalContext`, `continue: false`, `stopReason`, and `suppressOutput` are parsed but fail open today. + +### Helper Functions + +```go +func Deny(reason string) PreToolUseOutput // Enforced: blocks Bash and apply_patch +func Allow(reason string) PreToolUseOutput // Parsed but currently falls through +func AllowSilent() PreToolUseOutput // Parsed but currently falls through +func Ask(reason string) PreToolUseOutput // Parsed but currently falls through +func PassThrough() PreToolUseOutput // Empty output, normal flow +``` + +### Example + +```go +hookshot.Register("codex-pre-tool-use", func() { + hookshot.Run(func(input codex.PreToolUseInput) codex.PreToolUseOutput { + if input.ToolName == "Bash" { + var ti struct{ Command string `json:"command"` } + json.Unmarshal(input.ToolInput, &ti) + if strings.Contains(ti.Command, "rm -rf /") { + return codex.Deny("Destructive command blocked by hook.") + } + } + return codex.PassThrough() + }) +}) +``` + +You can also use exit code `2` with the reason written to stderr instead of returning the JSON output, which is what `RunE` does for you when the handler returns an error. + +--- + +## PermissionRequest + +Called when Codex is about to ask for approval (shell escalation, managed-network approval). It doesn't run for commands that don't need approval. + +### PermissionRequestInput + +```go +type PermissionRequestInput struct { + BaseInput + ToolName string `json:"tool_name"` + ToolInput json.RawMessage `json:"tool_input"` + ToolUseID string `json:"tool_use_id"` +} +``` + +The `tool_input` may include a `description` field with a human-readable approval reason. + +### PermissionRequestOutput + +```go +type PermissionRequestOutput struct { + BaseOutput + HookSpecificOutput *PermissionRequestHookOutput `json:"hookSpecificOutput,omitempty"` +} + +type PermissionRequestHookOutput struct { + HookEventName string `json:"hookEventName,omitempty"` + Decision *PermissionRequestDecision `json:"decision,omitempty"` +} + +type PermissionRequestDecision struct { + Behavior string `json:"behavior"` // "allow" or "deny" + Message string `json:"message,omitempty"` // For "deny" +} +``` + +If multiple matching hooks return decisions, any `deny` wins. Otherwise, an `allow` lets the request proceed without surfacing the approval prompt. If no matching hook decides, Codex uses the normal approval flow. `updatedInput`, `updatedPermissions`, and `interrupt` are reserved for future behavior and fail closed today. + +### Helper Functions + +```go +func AllowPermission() PermissionRequestOutput +func DenyPermission(message string) PermissionRequestOutput +``` + +--- + +## PostToolUse + +Called after Bash, `apply_patch`, or MCP tool calls produce output. For Bash, also runs after non-zero exits. Can't undo side effects. + +### PostToolUseInput + +```go +type PostToolUseInput struct { + BaseInput + ToolName string `json:"tool_name"` + ToolInput json.RawMessage `json:"tool_input"` + ToolResponse json.RawMessage `json:"tool_response"` + ToolUseID string `json:"tool_use_id"` +} +``` + +### PostToolUseOutput + +```go +type PostToolUseOutput struct { + BaseOutput + Decision string `json:"decision,omitempty"` + Reason string `json:"reason,omitempty"` + HookSpecificOutput *PostToolUseHookOutput `json:"hookSpecificOutput,omitempty"` +} + +type PostToolUseHookOutput struct { + HookEventName string `json:"hookEventName,omitempty"` + AdditionalContext string `json:"additionalContext,omitempty"` +} +``` + +`decision: "block"` doesn't undo the completed tool call. Codex records the feedback, replaces the tool result with it, and continues the model from the hook-provided message. To stop normal processing of the original tool result, also return `continue: false`. `updatedMCPToolOutput` and `suppressOutput` are parsed but not supported today. + +### Helper Functions + +```go +func PostToolOK() PostToolUseOutput +func PostToolBlock(reason string) PostToolUseOutput +func PostToolContext(context string) PostToolUseOutput +``` + +### Example + +```go +hookshot.Register("codex-post-tool-use", func() { + hookshot.Run(func(input codex.PostToolUseInput) codex.PostToolUseOutput { + if input.ToolName == "apply_patch" { + return codex.PostToolContext("Generated files were updated.") + } + return codex.PostToolOK() + }) +}) +``` + +--- + +## UserPromptSubmit + +Called when the user submits a prompt. `matcher` is ignored for this event. + +### UserPromptSubmitInput + +```go +type UserPromptSubmitInput struct { + BaseInput + Prompt string `json:"prompt"` +} +``` + +### UserPromptSubmitOutput + +```go +type UserPromptSubmitOutput struct { + BaseOutput + Decision string `json:"decision,omitempty"` + Reason string `json:"reason,omitempty"` + HookSpecificOutput *UserPromptSubmitHookOutput `json:"hookSpecificOutput,omitempty"` +} + +type UserPromptSubmitHookOutput struct { + HookEventName string `json:"hookEventName,omitempty"` + AdditionalContext string `json:"additionalContext,omitempty"` +} +``` + +Return `decision: "block"` to reject the prompt. Otherwise, plain text on stdout (or `additionalContext` in JSON) is added as extra developer context. + +### Helper Functions + +```go +func AllowPrompt() UserPromptSubmitOutput +func BlockPrompt(reason string) UserPromptSubmitOutput +func AddContext(context string) UserPromptSubmitOutput +``` + +### Example + +```go +hookshot.Register("codex-user-prompt-submit", func() { + hookshot.Run(func(input codex.UserPromptSubmitInput) codex.UserPromptSubmitOutput { + if strings.Contains(input.Prompt, "api_key=") { + return codex.BlockPrompt("Don't include API keys in prompts") + } + return codex.AllowPrompt() + }) +}) +``` + +--- + +## Stop + +Called when the turn finishes responding. `matcher` is ignored for this event. Codex expects JSON on stdout when the hook exits 0 — plain text is invalid here. + +### StopInput + +```go +type StopInput struct { + BaseInput + StopHookActive bool `json:"stop_hook_active"` +} +``` + +Codex also sends `last_assistant_message` (latest assistant message text) on Stop input. Read it via `hookshot.ReadRawInput` if needed. + +### StopOutput + +```go +type StopOutput struct { + BaseOutput + Decision string `json:"decision,omitempty"` // "block" to continue the turn + Reason string `json:"reason,omitempty"` +} +``` + +For this event, `decision: "block"` doesn't reject the turn. Instead, it tells Codex to continue and creates a new continuation prompt that acts as a new user prompt, using `reason` as that prompt text. If any matching Stop hook returns `continue: false`, that takes precedence over continuation decisions from other matching Stop hooks. + +### Helper Functions + +```go +func Continue() StopOutput // Allow stopping +func Block(reason string) StopOutput // Continue the turn with reason as the next prompt +func StopWith(reason string) StopOutput // Halt Codex entirely (continue=false) +``` + +### Example + +```go +hookshot.Register("codex-stop", func() { + hookshot.Run(func(input codex.StopInput) codex.StopOutput { + // IMPORTANT: Check StopHookActive to prevent infinite loops + if input.StopHookActive { + return codex.Continue() + } + return codex.Continue() + }) +}) +``` + +--- + +## Configuration Example + +`~/.codex/config.toml`: + +```toml +[features] +codex_hooks = true +``` + +`~/.codex/hooks.json`: + +```json +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash|apply_patch", + "hooks": [ + { "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" } + ] + } + ], + "PostToolUse": [ + { + "matcher": "apply_patch|Edit|Write", + "hooks": [ + { "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" } + ] + } + ], + "UserPromptSubmit": [ + { + "hooks": [ + { "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" } + ] + } + ], + "Stop": [ + { + "hooks": [ + { "type": "command", "command": "/path/to/my-hooks codex-stop", "timeout": 30 } + ] + } + ] + } +} +``` + +`hookshot install --codex --binary /path/to/my-hooks` will generate this layout for you, but it will not toggle the `codex_hooks` feature flag — set that yourself. diff --git a/docs/reference-unified.md b/docs/reference-unified.md index 28b1397..1523c23 100644 --- a/docs/reference-unified.md +++ b/docs/reference-unified.md @@ -1,6 +1,6 @@ # Unified API Reference -The unified API provides cross-platform handlers that work on Claude Code, Cursor, Windsurf Cascade, and Factory Droid. Write once, run on all platforms. +The unified API provides cross-platform handlers that work on Claude Code, Cursor, Windsurf Cascade, Factory Droid, and OpenAI Codex. Write once, run on all platforms. ## Platform Constants @@ -12,6 +12,7 @@ const ( PlatformCursor Platform = "cursor" PlatformDroid Platform = "droid" PlatformCascade Platform = "cascade" + PlatformCodex Platform = "codex" ) ``` @@ -19,17 +20,17 @@ const ( Handles stop events when the agent is about to finish. -**Registers:** `claude-stop`, `cursor-stop`, `droid-stop`, `cascade-post-cascade-response` +**Registers:** `claude-stop`, `cursor-stop`, `droid-stop`, `cascade-post-cascade-response`, `codex-stop` ### StopContext ```go type StopContext struct { Platform Platform - SessionID string // Claude/Droid: session_id, Cursor: conversation_id, Cascade: trajectory_id - Cwd string // Working directory (Claude/Droid only, empty for Cursor/Cascade) + SessionID string // Claude/Droid/Codex: session_id, Cursor: conversation_id, Cascade: trajectory_id + Cwd string // Working directory (Claude/Droid/Codex only, empty for Cursor/Cascade) - // Claude/Droid-specific + // Claude/Droid/Codex-specific StopHookActive bool // True if already continuing from a previous stop hook // Cursor-specific @@ -42,7 +43,7 @@ type StopContext struct { ```go // ShouldSkip returns true if the stop hook should be skipped to prevent loops. -// Claude/Droid: checks StopHookActive +// Claude/Droid/Codex: checks StopHookActive // Cursor: checks LoopCount >= 3 // Cascade: always returns false (no loop prevention mechanism) func (c StopContext) ShouldSkip() bool @@ -86,7 +87,9 @@ hookshot.OnStop(func(ctx hookshot.StopContext) hookshot.StopDecision { Handles pre-execution events for shell commands and MCP tools. -**Registers:** `claude-pre-tool-use`, `cursor-before-shell`, `cursor-before-mcp`, `droid-pre-tool-use`, `cascade-pre-run-command`, `cascade-pre-mcp-tool-use` +**Registers:** `claude-pre-tool-use`, `cursor-before-shell`, `cursor-before-mcp`, `droid-pre-tool-use`, `cascade-pre-run-command`, `cascade-pre-mcp-tool-use`, `codex-pre-tool-use` + +For Codex, `apply_patch` is classified as `ExecutionTool` (not `ExecutionShell` or `ExecutionMCP`); use `ctx.ToolName == "apply_patch"` to detect it. ### ExecutionType @@ -107,9 +110,9 @@ type ExecutionContext struct { Platform Platform Type ExecutionType - // For shell execution (Cursor beforeShellExecution, Claude Code Bash tool) + // For shell execution (Cursor beforeShellExecution, Claude Code/Codex Bash tool) // Also used for local MCP servers on Cursor (command-based MCP servers) - // NOTE: Only populated for Cursor and Cascade, not Claude Code or Droid + // NOTE: For Claude Code, Droid, and Codex, Command is parsed from tool_input.command for Bash. Command string Cwd string // Working directory @@ -119,6 +122,8 @@ type ExecutionContext struct { ServerURL string // MCP server URL (Cursor/Cascade only, for URL-based servers) // Raw access + // For Codex, the raw input is shared with Claude Code (RawClaudeCode) because + // Codex uses the same JSON wire format. RawClaudeCode *claude.PreToolUseInput RawCursor any // *cursor.BeforeShellExecutionInput or *cursor.BeforeMCPExecutionInput RawDroid *droid.PreToolUseInput @@ -178,7 +183,9 @@ hookshot.OnBeforeExecution(func(ctx hookshot.ExecutionContext) hookshot.Executio Handles post-file-edit events. -**Registers:** `claude-after-file-edit`, `cursor-after-file-edit`, `droid-after-file-edit`, `cascade-post-write-code` +**Registers:** `claude-after-file-edit`, `cursor-after-file-edit`, `droid-after-file-edit`, `cascade-post-write-code`, `codex-post-tool-use` + +For Codex, the underlying PostToolUse handler also matches `apply_patch` in addition to `Write` and `Edit`. Configure the hook with `matcher: "apply_patch|Edit|Write"` in `~/.codex/hooks.json`. ### FileEdit @@ -251,7 +258,7 @@ hookshot.OnAfterFileEdit(func(ctx hookshot.FileEditContext) hookshot.FileEditDec Handles prompt submission events. -**Registers:** `claude-user-prompt-submit`, `cursor-before-submit-prompt`, `droid-user-prompt-submit`, `cascade-pre-user-prompt` +**Registers:** `claude-user-prompt-submit`, `cursor-before-submit-prompt`, `droid-user-prompt-submit`, `cascade-pre-user-prompt`, `codex-user-prompt-submit` ### PromptContext diff --git a/examples/multi-hook/main.go b/examples/multi-hook/main.go index f5f49e1..c266d54 100644 --- a/examples/multi-hook/main.go +++ b/examples/multi-hook/main.go @@ -53,6 +53,18 @@ // "UserPromptSubmit": [{ "command": "/path/to/my-hooks droid-user-prompt-submit" }] // } // } +// +// Configure in OpenAI Codex (~/.codex/hooks.json; requires +// codex_hooks = true under [features] in ~/.codex/config.toml): +// +// { +// "hooks": { +// "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], +// "PreToolUse": [{ "matcher": "Bash|apply_patch", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], +// "PostToolUse": [{ "matcher": "apply_patch|Edit|Write", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], +// "UserPromptSubmit": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" }] }] +// } +// } package main import ( @@ -68,8 +80,8 @@ import ( func main() { // ========================================================================== // UNIFIED HANDLERS - // Write once, works on Claude Code, Cursor, Windsurf Cascade, and - // Factory Droid automatically. + // Write once, works on Claude Code, Cursor, Windsurf Cascade, + // Factory Droid, and OpenAI Codex automatically. // ========================================================================== hookshot.OnStop(handleStop) diff --git a/hookshot.go b/hookshot.go index 6c3c3d9..b7d216e 100644 --- a/hookshot.go +++ b/hookshot.go @@ -1,5 +1,5 @@ // Package hookshot provides a framework for building hooks for AI coding agents -// like Cursor and Claude Code. +// like Cursor, Claude Code, Windsurf Cascade, Factory Droid, and OpenAI Codex. // // Hookshot handles the boilerplate of reading JSON from stdin and writing JSON to stdout, // letting you focus on your hook logic. All hooks must use the multi-hook pattern with diff --git a/unified.go b/unified.go index c8a5ed0..eb2c537 100644 --- a/unified.go +++ b/unified.go @@ -20,6 +20,7 @@ const ( PlatformCursor Platform = "cursor" PlatformDroid Platform = "droid" PlatformCascade Platform = "cascade" + PlatformCodex Platform = "codex" ) // ============================================================================= @@ -41,10 +42,11 @@ type StopContext struct { } // ShouldSkip returns true if the stop hook should be skipped to prevent loops. -// For Claude Code and Droid, this checks StopHookActive. For Cursor, this checks LoopCount >= 3. -// For Cascade, there is no loop prevention mechanism (returns false). +// For Claude Code, Droid, and Codex, this checks StopHookActive. For Cursor, +// this checks LoopCount >= 3. For Cascade, there is no loop prevention +// mechanism (returns false). func (c StopContext) ShouldSkip() bool { - if c.Platform == PlatformClaude || c.Platform == PlatformDroid { + if c.Platform == PlatformClaude || c.Platform == PlatformDroid || c.Platform == PlatformCodex { return c.StopHookActive } if c.Platform == PlatformCursor { @@ -81,7 +83,7 @@ type StopHandler func(StopContext) StopDecision // OnStop registers a unified handler for stop events on all platforms. // It automatically registers handlers for "claude-stop", "cursor-stop", -// "droid-stop", and "cascade-post-cascade-response". +// "droid-stop", "cascade-post-cascade-response", and "codex-stop". func OnStop(handler StopHandler) { Register("claude-stop", func() { Run(func(input claude.StopInput) claude.StopOutput { @@ -145,6 +147,23 @@ func OnStop(handler StopHandler) { return cascade.PostCascadeResponseOK() }) }) + + // Codex uses the same JSON protocol as Claude Code + Register("codex-stop", func() { + Run(func(input claude.StopInput) claude.StopOutput { + ctx := StopContext{ + Platform: PlatformCodex, + SessionID: input.SessionID, + Cwd: input.Cwd, + StopHookActive: input.StopHookActive, + } + decision := handler(ctx) + if decision.Continue { + return claude.Continue() + } + return claude.Block(decision.Message) + }) + }) } // ============================================================================= @@ -233,6 +252,7 @@ type ExecutionHandler func(ExecutionContext) ExecutionDecision // - "droid-pre-tool-use" (filters to Bash and mcp__* tools) // - "cascade-pre-run-command" // - "cascade-pre-mcp-tool-use" +// - "codex-pre-tool-use" (filters to Bash, apply_patch, and mcp__* tools) func OnBeforeExecution(handler ExecutionHandler) { // Claude Code PreToolUse (for Bash and MCP tools) Register("claude-pre-tool-use", func() { @@ -431,6 +451,54 @@ func OnBeforeExecution(handler ExecutionHandler) { return cascade.PreMCPToolUseOutput{}, errors.New(decision.Reason) }) }) + + // Codex PreToolUse (same JSON protocol as Claude Code) + // Codex tool names include "Bash", "apply_patch", and MCP names like + // "mcp__server__tool". apply_patch is classified as ExecutionTool because + // it represents a file edit rather than a shell command or MCP call. + Register("codex-pre-tool-use", func() { + Run(func(input claude.PreToolUseInput) claude.PreToolUseOutput { + var execType ExecutionType + if input.ToolName == "Bash" { + execType = ExecutionShell + } else if len(input.ToolName) > 5 && input.ToolName[:5] == "mcp__" { + execType = ExecutionMCP + } else { + execType = ExecutionTool + } + + var command string + if execType == ExecutionShell { + var bashInput struct { + Command string `json:"command"` + } + json.Unmarshal(input.ToolInput, &bashInput) + command = bashInput.Command + } + + ctx := ExecutionContext{ + Platform: PlatformCodex, + Type: execType, + Command: command, + Cwd: input.Cwd, + ToolName: input.ToolName, + ToolInput: input.ToolInput, + RawClaudeCode: &input, + } + + decision := handler(ctx) + if decision.Allow { + if decision.Reason != "" { + return claude.Allow(decision.Reason) + } + return claude.AllowSilent() + } + if decision.Ask { + return claude.Ask(decision.Reason) + } + return claude.Deny(decision.Reason) + }) + }) } // ============================================================================= @@ -494,6 +562,7 @@ type FileEditHandler func(FileEditContext) FileEditDecision // - "cursor-after-file-edit" // - "droid-after-file-edit" (PostToolUse for Write/Edit) // - "cascade-post-write-code" +// - "codex-post-tool-use" (PostToolUse for Write/Edit/apply_patch) func OnAfterFileEdit(handler FileEditHandler) { // Claude Code PostToolUse (for Write/Edit) Register("claude-after-file-edit", func() { @@ -630,6 +699,50 @@ func OnAfterFileEdit(handler FileEditHandler) { return cascade.PostWriteCodeOK() }) }) + + // Codex PostToolUse (same JSON protocol as Claude Code). + // Codex uses apply_patch in addition to Write/Edit. Configure the hook + // with a matcher like "apply_patch|Edit|Write" in hooks.json. + Register("codex-post-tool-use", func() { + Run(func(input claude.PostToolUseInput) claude.PostToolUseOutput { + if input.ToolName != "Write" && input.ToolName != "Edit" && input.ToolName != "apply_patch" { + return claude.PostToolOK() + } + + var toolInput struct { + FilePath string `json:"file_path"` + Content string `json:"content"` + OldString string `json:"old_string"` + NewString string `json:"new_string"` + } + json.Unmarshal(input.ToolInput, &toolInput) + + var edits []FileEdit + if input.ToolName == "Edit" { + edits = []FileEdit{{OldString: toolInput.OldString, NewString: toolInput.NewString}} + } else { + edits = []FileEdit{{OldString: "", NewString: toolInput.Content}} + } + + ctx := FileEditContext{ + Platform: PlatformCodex, + SessionID: input.SessionID, + FilePath: toolInput.FilePath, + Edits: edits, + Cwd: input.Cwd, + RawClaudeCode: &input, + } + + decision := handler(ctx) + if decision.Block { + return claude.PostToolBlock(decision.Reason) + } + if decision.Context != "" { + return claude.PostToolContext(decision.Context) + } + return claude.PostToolOK() + }) + }) } // ============================================================================= @@ -686,6 +799,7 @@ type PromptHandler func(PromptContext) PromptDecision // - "cursor-before-submit-prompt" // - "droid-user-prompt-submit" // - "cascade-pre-user-prompt" +// - "codex-user-prompt-submit" func OnPromptSubmit(handler PromptHandler) { // Claude Code UserPromptSubmit Register("claude-user-prompt-submit", func() { @@ -767,6 +881,28 @@ func OnPromptSubmit(handler PromptHandler) { return cascade.AllowPrompt(), nil }) }) + + // Codex UserPromptSubmit (same JSON protocol as Claude Code) + Register("codex-user-prompt-submit", func() { + Run(func(input claude.UserPromptSubmitInput) claude.UserPromptSubmitOutput { + ctx := PromptContext{ + Platform: PlatformCodex, + SessionID: input.SessionID, + Prompt: input.Prompt, + Cwd: input.Cwd, + RawClaudeCode: &input, + } + + decision := handler(ctx) + if !decision.Allow { + return claude.BlockPrompt(decision.Reason) + } + if decision.Context != "" { + return claude.AddContext(decision.Context) + } + return claude.AllowPrompt() + }) + }) } // ============================================================================= diff --git a/unified_test.go b/unified_test.go index 9e7f505..f7b8a3c 100644 --- a/unified_test.go +++ b/unified_test.go @@ -262,6 +262,25 @@ func TestPlatformConstants(t *testing.T) { if PlatformCursor != "cursor" { t.Errorf("PlatformCursor = %q, want %q", PlatformCursor, "cursor") } + if PlatformDroid != "droid" { + t.Errorf("PlatformDroid = %q, want %q", PlatformDroid, "droid") + } + if PlatformCascade != "cascade" { + t.Errorf("PlatformCascade = %q, want %q", PlatformCascade, "cascade") + } + if PlatformCodex != "codex" { + t.Errorf("PlatformCodex = %q, want %q", PlatformCodex, "codex") + } +} + +func TestStopContext_ShouldSkip_Codex(t *testing.T) { + // Codex uses StopHookActive like Claude Code and Droid. + if !(StopContext{Platform: PlatformCodex, StopHookActive: true}).ShouldSkip() { + t.Error("Codex StopHookActive=true should skip") + } + if (StopContext{Platform: PlatformCodex, StopHookActive: false}).ShouldSkip() { + t.Error("Codex StopHookActive=false should not skip") + } } // ============================================================================= @@ -307,6 +326,7 @@ func TestOnBeforeExecution_RegistersAllHandlers(t *testing.T) { "droid-pre-tool-use", "cascade-pre-run-command", "cascade-pre-mcp-tool-use", + "codex-pre-tool-use", } for _, name := range expectedHandlers { @@ -329,6 +349,7 @@ func TestOnPromptSubmit_RegistersAllHandlers(t *testing.T) { "cursor-before-submit-prompt", "droid-user-prompt-submit", "cascade-pre-user-prompt", + "codex-user-prompt-submit", } for _, name := range expectedHandlers { @@ -351,6 +372,7 @@ func TestOnStop_RegistersAllHandlers(t *testing.T) { "cursor-stop", "droid-stop", "cascade-post-cascade-response", + "codex-stop", } for _, name := range expectedHandlers { @@ -373,6 +395,7 @@ func TestOnAfterFileEdit_RegistersAllHandlers(t *testing.T) { "cursor-after-file-edit", "droid-after-file-edit", "cascade-post-write-code", + "codex-post-tool-use", } for _, name := range expectedHandlers { From f3b3ab644d3be7d7ac707257631636709f060e4e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 14 May 2026 08:10:55 +0000 Subject: [PATCH 02/19] fix(codex): parse apply_patch envelope, fail closed on Ask, match MCP tools Addresses three review findings on the Codex platform integration: 1. codex-post-tool-use was unmarshalling apply_patch tool_input with the Write/Edit schema (file_path/content/old_string/new_string), but apply_patch actually carries a unified-diff envelope under tool_input.command. The handler now parses that envelope (Add / Update / Delete File sections with hunked diff bodies) via the new parseCodexApplyPatch helper and invokes OnAfterFileEdit handlers once per file with a fully populated FileEditContext. If any per-file invocation returns FileEditBlock, the reasons are concatenated and emitted as a single PostToolBlock so Codex still sees feedback. The parser is tolerant: malformed patches fall back to a single invocation with the raw command text in Edits[0].NewString so events never disappear silently. 2. codex-pre-tool-use was returning claude.Ask(...) for AskExecution decisions, but Codex parses but does not enforce permissionDecision 'ask' today. Any policy that required user confirmation before risky Bash or apply_patch operations was silently failing open. The unified bridge now rewrites Ask -> Deny on Codex so the same policy that asks on Claude/Cursor/Droid/Cascade fails closed on Codex. 3. The installer was generating a PreToolUse matcher of 'Bash|apply_patch' that excluded mcp__* tool names, so the codex-pre-tool-use handler was never invoked for MCP calls and MCP allowlist/SSRF policies were structurally bypassed. The matcher is now 'Bash|apply_patch|mcp__.*' for PreToolUse and 'apply_patch|Edit|Write|mcp__.*' for PostToolUse. apply_patch tool_input.command is also exposed via ExecutionContext.Command in the pre-tool-use bridge so policies can inspect the patch directly. Adds parseCodexApplyPatch with eight unit tests covering Add, Update, Delete, multi-hunk Update, multi-file patches, Move to renames, leading here-doc wrappers, and malformed input. Adds five integration tests covering the rewritten codex-pre-tool-use and codex-post-tool-use handlers (Ask fails closed, apply_patch populates Command, MCP is classified as ExecutionMCP, apply_patch parses files+edits, and block propagation across multi-file patches). Updates docs/reference-codex.md, docs/reference-unified.md, README.md, codex/doc.go, and examples/multi-hook to reflect the new matchers, the apply_patch parsing semantics, and the fail-closed treatment of Ask on Codex. Co-authored-by: Ashwin Ramaswami --- README.md | 2 +- cmd/hookshot/main.go | 8 +- codex/doc.go | 4 +- codex_apply_patch.go | 151 +++++++++++++++++++++++++++ codex_apply_patch_test.go | 181 ++++++++++++++++++++++++++++++++ docs/reference-codex.md | 19 +++- docs/reference-unified.md | 8 +- examples/multi-hook/main.go | 4 +- unified.go | 138 +++++++++++++++++++------ unified_test.go | 201 ++++++++++++++++++++++++++++++++++++ 10 files changed, 671 insertions(+), 45 deletions(-) create mode 100644 codex_apply_patch.go create mode 100644 codex_apply_patch_test.go diff --git a/README.md b/README.md index 1ae2993..3758da2 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,7 @@ Then configure the hooks themselves: { "hooks": { "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], - "PreToolUse": [{ "matcher": "Bash|apply_patch", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }] + "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }] } } ``` diff --git a/cmd/hookshot/main.go b/cmd/hookshot/main.go index 770a538..cd6cafd 100644 --- a/cmd/hookshot/main.go +++ b/cmd/hookshot/main.go @@ -494,14 +494,18 @@ func installCodex(binaryPath string) error { }}, }}, "PreToolUse": []map[string]any{{ - "matcher": "Bash|apply_patch", + // Include mcp__.* so MCP tool calls also reach the hook + // binary. Without this, the codex-pre-tool-use handler is + // never invoked for MCP tools and any OnBeforeExecution + // policy that enforces MCP allowlists is silently bypassed. + "matcher": "Bash|apply_patch|mcp__.*", "hooks": []map[string]any{{ "type": "command", "command": binaryPath + " codex-pre-tool-use", }}, }}, "PostToolUse": []map[string]any{{ - "matcher": "apply_patch|Edit|Write", + "matcher": "apply_patch|Edit|Write|mcp__.*", "hooks": []map[string]any{{ "type": "command", "command": binaryPath + " codex-post-tool-use", diff --git a/codex/doc.go b/codex/doc.go index d85a94b..3858a4d 100644 --- a/codex/doc.go +++ b/codex/doc.go @@ -24,13 +24,13 @@ // "hooks": { // "PreToolUse": [ // { -// "matcher": "Bash|apply_patch", +// "matcher": "Bash|apply_patch|mcp__.*", // "hooks": [{ "type": "command", "command": "/path/to/hook codex-pre-tool-use" }] // } // ], // "PostToolUse": [ // { -// "matcher": "apply_patch|Edit|Write", +// "matcher": "apply_patch|Edit|Write|mcp__.*", // "hooks": [{ "type": "command", "command": "/path/to/hook codex-post-tool-use" }] // } // ], diff --git a/codex_apply_patch.go b/codex_apply_patch.go new file mode 100644 index 0000000..3edca5e --- /dev/null +++ b/codex_apply_patch.go @@ -0,0 +1,151 @@ +package hookshot + +import ( + "strings" +) + +// codexApplyPatchFile represents one file affected by a Codex apply_patch +// tool call. Codex's apply_patch carries a "command" field containing a +// unified-diff-style envelope (see https://developers.openai.com/codex/hooks). +// Each envelope may reference multiple files, which is why we model this as +// a slice on the parser output rather than a single FilePath/Edits pair. +type codexApplyPatchFile struct { + // Operation is one of "add", "update", or "delete". + Operation string + // FilePath is the path declared in the *** {Add,Update,Delete} File: header. + FilePath string + // NewFilePath is set for "update" operations that include a *** Move to: line. + NewFilePath string + // Edits captures the per-hunk old/new content. For "add" the patch + // contains exactly one edit with OldString=="" and NewString set to the + // added contents. For "delete" Edits is empty (the file path is enough + // for policy evaluation). For "update", each hunk in the patch becomes + // one FileEdit with the removed lines joined as OldString and the added + // lines joined as NewString. + Edits []FileEdit +} + +// parseCodexApplyPatch parses the patch envelope from a Codex apply_patch +// tool call. The input is the raw value of tool_input.command for an +// apply_patch invocation. The parser is intentionally tolerant: malformed +// input degrades gracefully (the affected file is still recorded, even if +// its Edits list is incomplete) rather than returning an error, because +// hook handlers should never silently disappear on bad input. +func parseCodexApplyPatch(patch string) []codexApplyPatchFile { + // Strip a leading "apply_patch <<'EOF'\n" wrapper if present. Some Codex + // builds include a here-doc style wrapper around the *** Begin Patch + // block; this is a defensive measure and not part of the documented + // wire format. + if i := strings.Index(patch, "*** Begin Patch"); i > 0 { + patch = patch[i:] + } + + lines := strings.Split(patch, "\n") + var files []codexApplyPatchFile + var current *codexApplyPatchFile + var hunkOld, hunkNew []string + inHunk := false + + flushHunk := func() { + if current == nil { + return + } + if len(hunkOld) == 0 && len(hunkNew) == 0 { + hunkOld, hunkNew = nil, nil + return + } + current.Edits = append(current.Edits, FileEdit{ + OldString: strings.Join(hunkOld, "\n"), + NewString: strings.Join(hunkNew, "\n"), + }) + hunkOld, hunkNew = nil, nil + } + + flushFile := func() { + flushHunk() + if current != nil { + files = append(files, *current) + } + current = nil + inHunk = false + } + + for _, line := range lines { + switch { + case strings.HasPrefix(line, "*** Begin Patch"), + strings.HasPrefix(line, "*** End of File"): + // Markers we don't act on; "End of File" is sometimes emitted + // inside an Update section. + case strings.HasPrefix(line, "*** End Patch"): + flushFile() + case strings.HasPrefix(line, "*** Add File: "): + flushFile() + current = &codexApplyPatchFile{ + Operation: "add", + FilePath: strings.TrimPrefix(line, "*** Add File: "), + } + inHunk = true + case strings.HasPrefix(line, "*** Update File: "): + flushFile() + current = &codexApplyPatchFile{ + Operation: "update", + FilePath: strings.TrimPrefix(line, "*** Update File: "), + } + inHunk = false + case strings.HasPrefix(line, "*** Delete File: "): + flushFile() + current = &codexApplyPatchFile{ + Operation: "delete", + FilePath: strings.TrimPrefix(line, "*** Delete File: "), + } + inHunk = false + case strings.HasPrefix(line, "*** Move to: "): + if current != nil { + current.NewFilePath = strings.TrimPrefix(line, "*** Move to: ") + } + case strings.HasPrefix(line, "@@"): + // Hunk separator inside an Update section. Flush the previous + // hunk (if any) and start collecting a fresh one. + flushHunk() + inHunk = true + case current != nil: + switch current.Operation { + case "add": + // In an Add section every content line is prefixed with "+". + // Be lenient: accept unprefixed lines as well so we still + // capture the new file contents when Codex omits the + // prefix. + if strings.HasPrefix(line, "+") { + hunkNew = append(hunkNew, line[1:]) + } else if line != "" { + hunkNew = append(hunkNew, line) + } + case "update": + if !inHunk { + // Anything before the first @@ in an Update section is + // metadata we don't care about. + continue + } + if len(line) == 0 { + // Treat blank lines as context inside an Update. + flushHunk() + continue + } + switch line[0] { + case '+': + hunkNew = append(hunkNew, line[1:]) + case '-': + hunkOld = append(hunkOld, line[1:]) + case ' ': + // Context line — flush so adjacent ±/context groups + // don't get fused into a single edit. + flushHunk() + } + case "delete": + // Delete sections have no content lines; ignore stray text. + } + } + } + flushFile() + return files +} diff --git a/codex_apply_patch_test.go b/codex_apply_patch_test.go new file mode 100644 index 0000000..e911faa --- /dev/null +++ b/codex_apply_patch_test.go @@ -0,0 +1,181 @@ +package hookshot + +import ( + "reflect" + "testing" +) + +func TestParseCodexApplyPatch_AddFile(t *testing.T) { + patch := "*** Begin Patch\n" + + "*** Add File: secrets/api_key.txt\n" + + "+sk-deadbeef\n" + + "+second line\n" + + "*** End Patch\n" + + got := parseCodexApplyPatch(patch) + want := []codexApplyPatchFile{{ + Operation: "add", + FilePath: "secrets/api_key.txt", + Edits: []FileEdit{{ + OldString: "", + NewString: "sk-deadbeef\nsecond line", + }}, + }} + if !reflect.DeepEqual(got, want) { + t.Errorf("parseCodexApplyPatch(add) =\n %+v\nwant\n %+v", got, want) + } +} + +func TestParseCodexApplyPatch_DeleteFile(t *testing.T) { + patch := "*** Begin Patch\n" + + "*** Delete File: old/secrets.env\n" + + "*** End Patch\n" + + got := parseCodexApplyPatch(patch) + if len(got) != 1 { + t.Fatalf("got %d files, want 1", len(got)) + } + if got[0].Operation != "delete" { + t.Errorf("Operation = %q, want %q", got[0].Operation, "delete") + } + if got[0].FilePath != "old/secrets.env" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "old/secrets.env") + } + if len(got[0].Edits) != 0 { + t.Errorf("Edits = %+v, want empty", got[0].Edits) + } +} + +func TestParseCodexApplyPatch_UpdateFile(t *testing.T) { + patch := "*** Begin Patch\n" + + "*** Update File: src/auth.go\n" + + "@@ func login\n" + + " func login() {\n" + + "- token := \"hardcoded\"\n" + + "+ token := os.Getenv(\"TOKEN\")\n" + + " return token\n" + + " }\n" + + "*** End Patch\n" + + got := parseCodexApplyPatch(patch) + if len(got) != 1 { + t.Fatalf("got %d files, want 1", len(got)) + } + if got[0].FilePath != "src/auth.go" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "src/auth.go") + } + if got[0].Operation != "update" { + t.Errorf("Operation = %q, want %q", got[0].Operation, "update") + } + if len(got[0].Edits) != 1 { + t.Fatalf("Edits = %+v, want 1 edit", got[0].Edits) + } + wantEdit := FileEdit{ + OldString: " token := \"hardcoded\"", + NewString: " token := os.Getenv(\"TOKEN\")", + } + if got[0].Edits[0] != wantEdit { + t.Errorf("Edits[0] = %+v, want %+v", got[0].Edits[0], wantEdit) + } +} + +func TestParseCodexApplyPatch_UpdateFile_MultipleHunks(t *testing.T) { + patch := "*** Begin Patch\n" + + "*** Update File: a.go\n" + + "@@\n" + + "-foo\n" + + "+bar\n" + + "@@\n" + + "-baz\n" + + "+qux\n" + + "*** End Patch\n" + + got := parseCodexApplyPatch(patch) + if len(got) != 1 { + t.Fatalf("got %d files, want 1", len(got)) + } + if len(got[0].Edits) != 2 { + t.Fatalf("Edits = %+v, want 2 edits", got[0].Edits) + } + if got[0].Edits[0] != (FileEdit{OldString: "foo", NewString: "bar"}) { + t.Errorf("Edits[0] = %+v", got[0].Edits[0]) + } + if got[0].Edits[1] != (FileEdit{OldString: "baz", NewString: "qux"}) { + t.Errorf("Edits[1] = %+v", got[0].Edits[1]) + } +} + +func TestParseCodexApplyPatch_MultiFile(t *testing.T) { + patch := "*** Begin Patch\n" + + "*** Add File: new.txt\n" + + "+hello\n" + + "*** Update File: existing.go\n" + + "@@\n" + + "-old\n" + + "+new\n" + + "*** Delete File: stale.txt\n" + + "*** End Patch\n" + + got := parseCodexApplyPatch(patch) + if len(got) != 3 { + t.Fatalf("got %d files, want 3", len(got)) + } + if got[0].FilePath != "new.txt" || got[0].Operation != "add" { + t.Errorf("files[0] = %+v", got[0]) + } + if got[1].FilePath != "existing.go" || got[1].Operation != "update" { + t.Errorf("files[1] = %+v", got[1]) + } + if got[2].FilePath != "stale.txt" || got[2].Operation != "delete" { + t.Errorf("files[2] = %+v", got[2]) + } +} + +func TestParseCodexApplyPatch_MoveTo(t *testing.T) { + patch := "*** Begin Patch\n" + + "*** Update File: old/path.go\n" + + "*** Move to: new/path.go\n" + + "@@\n" + + "-foo\n" + + "+bar\n" + + "*** End Patch\n" + + got := parseCodexApplyPatch(patch) + if len(got) != 1 { + t.Fatalf("got %d files, want 1", len(got)) + } + if got[0].NewFilePath != "new/path.go" { + t.Errorf("NewFilePath = %q, want %q", got[0].NewFilePath, "new/path.go") + } + if got[0].FilePath != "old/path.go" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "old/path.go") + } +} + +func TestParseCodexApplyPatch_LeadingWrapper(t *testing.T) { + // Some Codex invocations wrap the patch in a here-doc style header. + // The parser should skip everything before the Begin Patch marker. + patch := "apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: foo.txt\n" + + "+hi\n" + + "*** End Patch\n" + + "PATCH\n" + + got := parseCodexApplyPatch(patch) + if len(got) != 1 { + t.Fatalf("got %d files, want 1", len(got)) + } + if got[0].FilePath != "foo.txt" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "foo.txt") + } +} + +func TestParseCodexApplyPatch_EmptyAndMalformed(t *testing.T) { + if files := parseCodexApplyPatch(""); files != nil { + t.Errorf("empty input should yield nil, got %+v", files) + } + if files := parseCodexApplyPatch("not a patch"); files != nil { + t.Errorf("non-patch input should yield nil, got %+v", files) + } +} diff --git a/docs/reference-codex.md b/docs/reference-codex.md index 5cd7b9c..fc77b5b 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -142,13 +142,15 @@ type PreToolUseHookOutput struct { Codex honors `permissionDecision: "deny"` (or the older `decision: "block"` shape) on Bash and `apply_patch`. `"allow"`, `"ask"`, `updatedInput`, `additionalContext`, `continue: false`, `stopReason`, and `suppressOutput` are parsed but fail open today. +> **Fail-closed `Ask` on the unified API.** Because Codex doesn't enforce `"ask"` yet, `hookshot.OnBeforeExecution` returning `AskExecution(...)` is translated to a `Deny` on Codex so policies that require user confirmation aren't silently bypassed. If you call the platform-level `codex.Ask` helper directly, the output JSON still encodes `"ask"` (so it round-trips with the upstream protocol) — that's only useful for forward-compat testing today. + ### Helper Functions ```go func Deny(reason string) PreToolUseOutput // Enforced: blocks Bash and apply_patch func Allow(reason string) PreToolUseOutput // Parsed but currently falls through func AllowSilent() PreToolUseOutput // Parsed but currently falls through -func Ask(reason string) PreToolUseOutput // Parsed but currently falls through +func Ask(reason string) PreToolUseOutput // Parsed but currently falls through (unified API rewrites to Deny) func PassThrough() PreToolUseOutput // Empty output, normal flow ``` @@ -262,6 +264,15 @@ func PostToolBlock(reason string) PostToolUseOutput func PostToolContext(context string) PostToolUseOutput ``` +### apply_patch parsing on the unified API + +`hookshot.OnAfterFileEdit` parses Codex `apply_patch` events by unpacking the unified-diff envelope in `tool_input.command` and invoking your handler **once per file** mentioned in the patch. Each invocation receives a fully populated `FileEditContext`: + +- `FilePath` is the path declared in the `*** Add File:`, `*** Update File:`, or `*** Delete File:` section. +- `Edits` is `[{OldString: "", NewString: }]` for Add, one `FileEdit` per hunk for Update (with removed lines as `OldString` and added lines as `NewString`), and empty for Delete. + +If any of those per-file invocations returns `FileEditBlock`, the unified bridge concatenates the reasons and emits a single `PostToolBlock` so Codex replaces the tool result with the combined feedback. The platform-level `codex.PostToolUseInput` retains the raw `tool_input.command` if you'd rather parse the patch yourself. + ### Example ```go @@ -398,7 +409,7 @@ codex_hooks = true "hooks": { "PreToolUse": [ { - "matcher": "Bash|apply_patch", + "matcher": "Bash|apply_patch|mcp__.*", "hooks": [ { "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" } ] @@ -406,7 +417,7 @@ codex_hooks = true ], "PostToolUse": [ { - "matcher": "apply_patch|Edit|Write", + "matcher": "apply_patch|Edit|Write|mcp__.*", "hooks": [ { "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" } ] @@ -431,3 +442,5 @@ codex_hooks = true ``` `hookshot install --codex --binary /path/to/my-hooks` will generate this layout for you, but it will not toggle the `codex_hooks` feature flag — set that yourself. + +> **Why `mcp__.*` is in the matcher.** Codex passes MCP tool names to PreToolUse / PostToolUse using the `mcp__server__tool` convention. Omitting the `mcp__.*` alternative would mean Codex never invokes the hook binary for MCP calls, which would silently bypass any `OnBeforeExecution` policy meant to enforce MCP allowlists. diff --git a/docs/reference-unified.md b/docs/reference-unified.md index 1523c23..128d790 100644 --- a/docs/reference-unified.md +++ b/docs/reference-unified.md @@ -89,7 +89,9 @@ Handles pre-execution events for shell commands and MCP tools. **Registers:** `claude-pre-tool-use`, `cursor-before-shell`, `cursor-before-mcp`, `droid-pre-tool-use`, `cascade-pre-run-command`, `cascade-pre-mcp-tool-use`, `codex-pre-tool-use` -For Codex, `apply_patch` is classified as `ExecutionTool` (not `ExecutionShell` or `ExecutionMCP`); use `ctx.ToolName == "apply_patch"` to detect it. +For Codex, `apply_patch` is classified as `ExecutionTool` (not `ExecutionShell` or `ExecutionMCP`); use `ctx.ToolName == "apply_patch"` to detect it. The patch text is exposed via `ctx.Command` so policies can inspect it without re-parsing `ToolInput`. + +Codex does not currently enforce `permissionDecision: "ask"`. To avoid a silent fail-open, the unified bridge rewrites `AskExecution(...)` decisions to a `Deny` on Codex; on every other platform `Ask` still surfaces an approval prompt as before. ### ExecutionType @@ -185,7 +187,9 @@ Handles post-file-edit events. **Registers:** `claude-after-file-edit`, `cursor-after-file-edit`, `droid-after-file-edit`, `cascade-post-write-code`, `codex-post-tool-use` -For Codex, the underlying PostToolUse handler also matches `apply_patch` in addition to `Write` and `Edit`. Configure the hook with `matcher: "apply_patch|Edit|Write"` in `~/.codex/hooks.json`. +For Codex, the underlying PostToolUse handler also matches `apply_patch` in addition to `Write` and `Edit`. Configure the hook with `matcher: "apply_patch|Edit|Write|mcp__.*"` in `~/.codex/hooks.json`. + +For Codex `apply_patch`, the unified bridge parses the unified-diff envelope in `tool_input.command` and invokes your handler **once per file** in the patch, with `FilePath` set to the file declared in the `*** Add/Update/Delete File:` header and `Edits` populated from each hunk. If any of those invocations returns `FileEditBlock`, the reasons are concatenated and emitted as a single `PostToolBlock`. ### FileEdit diff --git a/examples/multi-hook/main.go b/examples/multi-hook/main.go index c266d54..f2332f2 100644 --- a/examples/multi-hook/main.go +++ b/examples/multi-hook/main.go @@ -60,8 +60,8 @@ // { // "hooks": { // "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], -// "PreToolUse": [{ "matcher": "Bash|apply_patch", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], -// "PostToolUse": [{ "matcher": "apply_patch|Edit|Write", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], +// "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], +// "PostToolUse": [{ "matcher": "apply_patch|Edit|Write|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], // "UserPromptSubmit": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" }] }] // } // } diff --git a/unified.go b/unified.go index eb2c537..2839a73 100644 --- a/unified.go +++ b/unified.go @@ -452,10 +452,12 @@ func OnBeforeExecution(handler ExecutionHandler) { }) }) - // Codex PreToolUse (same JSON protocol as Claude Code) + // Codex PreToolUse (same JSON protocol as Claude Code). // Codex tool names include "Bash", "apply_patch", and MCP names like // "mcp__server__tool". apply_patch is classified as ExecutionTool because - // it represents a file edit rather than a shell command or MCP call. + // it represents a file edit rather than a shell command or MCP call. For + // apply_patch the underlying tool_input.command is parsed and exposed + // via ExecutionContext.Command so policies can inspect the patch text. Register("codex-pre-tool-use", func() { Run(func(input claude.PreToolUseInput) claude.PreToolUseOutput { var execType ExecutionType @@ -468,12 +470,12 @@ func OnBeforeExecution(handler ExecutionHandler) { } var command string - if execType == ExecutionShell { - var bashInput struct { + if execType == ExecutionShell || input.ToolName == "apply_patch" { + var cmdInput struct { Command string `json:"command"` } - json.Unmarshal(input.ToolInput, &bashInput) - command = bashInput.Command + json.Unmarshal(input.ToolInput, &cmdInput) + command = cmdInput.Command } ctx := ExecutionContext{ @@ -493,8 +495,13 @@ func OnBeforeExecution(handler ExecutionHandler) { } return claude.AllowSilent() } + // Codex currently parses but does not enforce permissionDecision + // "ask" for PreToolUse, so an Ask decision would silently fail + // open. Until Codex enforces it, fail closed by denying — this + // matches the security posture of the other platforms where + // Ask actually surfaces an approval prompt. if decision.Ask { - return claude.Ask(decision.Reason) + return claude.Deny(decision.Reason) } return claude.Deny(decision.Reason) }) @@ -701,44 +708,109 @@ func OnAfterFileEdit(handler FileEditHandler) { }) // Codex PostToolUse (same JSON protocol as Claude Code). - // Codex uses apply_patch in addition to Write/Edit. Configure the hook - // with a matcher like "apply_patch|Edit|Write" in hooks.json. + // Codex uses apply_patch in addition to Write/Edit; apply_patch carries + // a unified-diff envelope under tool_input.command that may touch + // multiple files in a single call. For each file in the patch we invoke + // the user's handler exactly once with a populated FileEditContext, and + // we combine the decisions across files: a Block from any file wins, + // otherwise context strings are concatenated. Configure the hook with a + // matcher like "apply_patch|Edit|Write" in hooks.json. Register("codex-post-tool-use", func() { Run(func(input claude.PostToolUseInput) claude.PostToolUseOutput { if input.ToolName != "Write" && input.ToolName != "Edit" && input.ToolName != "apply_patch" { return claude.PostToolOK() } - var toolInput struct { - FilePath string `json:"file_path"` - Content string `json:"content"` - OldString string `json:"old_string"` - NewString string `json:"new_string"` - } - json.Unmarshal(input.ToolInput, &toolInput) + // Write/Edit use the Claude-style schema. + if input.ToolName == "Write" || input.ToolName == "Edit" { + var toolInput struct { + FilePath string `json:"file_path"` + Content string `json:"content"` + OldString string `json:"old_string"` + NewString string `json:"new_string"` + } + json.Unmarshal(input.ToolInput, &toolInput) - var edits []FileEdit - if input.ToolName == "Edit" { - edits = []FileEdit{{OldString: toolInput.OldString, NewString: toolInput.NewString}} - } else { - edits = []FileEdit{{OldString: "", NewString: toolInput.Content}} + var edits []FileEdit + if input.ToolName == "Edit" { + edits = []FileEdit{{OldString: toolInput.OldString, NewString: toolInput.NewString}} + } else { + edits = []FileEdit{{OldString: "", NewString: toolInput.Content}} + } + + ctx := FileEditContext{ + Platform: PlatformCodex, + SessionID: input.SessionID, + FilePath: toolInput.FilePath, + Edits: edits, + Cwd: input.Cwd, + RawClaudeCode: &input, + } + + decision := handler(ctx) + if decision.Block { + return claude.PostToolBlock(decision.Reason) + } + if decision.Context != "" { + return claude.PostToolContext(decision.Context) + } + return claude.PostToolOK() } - ctx := FileEditContext{ - Platform: PlatformCodex, - SessionID: input.SessionID, - FilePath: toolInput.FilePath, - Edits: edits, - Cwd: input.Cwd, - RawClaudeCode: &input, + // apply_patch: tool_input is {"command": "*** Begin Patch ..."}. + var applyInput struct { + Command string `json:"command"` + } + json.Unmarshal(input.ToolInput, &applyInput) + + files := parseCodexApplyPatch(applyInput.Command) + if len(files) == 0 { + // We could not parse anything actionable out of the patch. + // Fall back to invoking the handler once with whatever raw + // information we have so policies still see a Codex + // PostToolUse event rather than nothing. + ctx := FileEditContext{ + Platform: PlatformCodex, + SessionID: input.SessionID, + Cwd: input.Cwd, + Edits: []FileEdit{{OldString: "", NewString: applyInput.Command}}, + RawClaudeCode: &input, + } + decision := handler(ctx) + if decision.Block { + return claude.PostToolBlock(decision.Reason) + } + if decision.Context != "" { + return claude.PostToolContext(decision.Context) + } + return claude.PostToolOK() } - decision := handler(ctx) - if decision.Block { - return claude.PostToolBlock(decision.Reason) + var ( + blockReasons []string + contexts []string + ) + for _, f := range files { + ctx := FileEditContext{ + Platform: PlatformCodex, + SessionID: input.SessionID, + FilePath: f.FilePath, + Edits: f.Edits, + Cwd: input.Cwd, + RawClaudeCode: &input, + } + decision := handler(ctx) + if decision.Block { + blockReasons = append(blockReasons, decision.Reason) + } else if decision.Context != "" { + contexts = append(contexts, decision.Context) + } } - if decision.Context != "" { - return claude.PostToolContext(decision.Context) + if len(blockReasons) > 0 { + return claude.PostToolBlock(strings.Join(blockReasons, "\n")) + } + if len(contexts) > 0 { + return claude.PostToolContext(strings.Join(contexts, "\n")) } return claude.PostToolOK() }) diff --git a/unified_test.go b/unified_test.go index f7b8a3c..79eb257 100644 --- a/unified_test.go +++ b/unified_test.go @@ -799,6 +799,207 @@ func TestOnPromptSubmit_CursorPopulatesCwd(t *testing.T) { } } +// ============================================================================= +// Codex-specific behavior tests +// ============================================================================= + +func TestCodexPreToolUse_AskFailsClosed(t *testing.T) { + // Codex currently parses but does not enforce permissionDecision "ask", + // so the unified Codex pre-tool-use bridge must convert an Ask decision + // into a Deny so policies still fail closed. + ClearHandlers() + defer ClearHandlers() + + OnBeforeExecution(func(ctx ExecutionContext) ExecutionDecision { + return AskExecution("please confirm") + }) + + // Capture stdout to assert on the emitted JSON. + stdinR, stdinW, _ := os.Pipe() + stdinW.Write([]byte(`{"session_id":"s","tool_name":"Bash","tool_input":{"command":"rm -rf /"},"cwd":"/tmp"}`)) + stdinW.Close() + stdoutR, stdoutW, _ := os.Pipe() + origStdin, origStdout := os.Stdin, os.Stdout + os.Stdin, os.Stdout = stdinR, stdoutW + defer func() { + stdoutW.Close() + os.Stdin, os.Stdout = origStdin, origStdout + }() + + handlers["codex-pre-tool-use"]() + stdoutW.Close() + + var buf bytes.Buffer + buf.ReadFrom(stdoutR) + got := buf.String() + if !strings.Contains(got, `"permissionDecision":"deny"`) { + t.Errorf("Codex Ask decision should emit deny, got %q", got) + } + if strings.Contains(got, `"permissionDecision":"ask"`) { + t.Errorf("Codex Ask decision should NOT emit ask, got %q", got) + } +} + +func TestCodexPreToolUse_ApplyPatchPopulatesCommand(t *testing.T) { + // For apply_patch, the unified handler should pull tool_input.command + // out as ExecutionContext.Command so policies can inspect the patch. + ClearHandlers() + defer ClearHandlers() + + var captured ExecutionContext + OnBeforeExecution(func(ctx ExecutionContext) ExecutionDecision { + captured = ctx + return AllowExecution() + }) + + stdin := `{"session_id":"s","tool_name":"apply_patch","tool_input":{"command":"*** Begin Patch\n*** Add File: foo.txt\n+hi\n*** End Patch"},"cwd":"/tmp"}` + runHandlerWithStdin(t, "codex-pre-tool-use", stdin) + + if captured.Platform != PlatformCodex { + t.Errorf("Platform = %q, want %q", captured.Platform, PlatformCodex) + } + if captured.Type != ExecutionTool { + t.Errorf("Type = %q, want %q (apply_patch is a tool, not shell/mcp)", captured.Type, ExecutionTool) + } + if captured.ToolName != "apply_patch" { + t.Errorf("ToolName = %q, want %q", captured.ToolName, "apply_patch") + } + if !strings.Contains(captured.Command, "*** Add File: foo.txt") { + t.Errorf("Command should contain the patch text, got %q", captured.Command) + } +} + +func TestCodexPreToolUse_MCPClassifiedAsMCP(t *testing.T) { + // Verifies that MCP tools reaching the codex-pre-tool-use handler are + // classified as ExecutionMCP. (Reaching the handler at all also + // requires the installer to include mcp__.* in the matcher; see + // TestCodexInstaller_MatcherIncludesMCP for that side.) + ClearHandlers() + defer ClearHandlers() + + var captured ExecutionContext + OnBeforeExecution(func(ctx ExecutionContext) ExecutionDecision { + captured = ctx + return AllowExecution() + }) + + stdin := `{"session_id":"s","tool_name":"mcp__net__fetch","tool_input":{"url":"http://example.com"},"cwd":"/tmp"}` + runHandlerWithStdin(t, "codex-pre-tool-use", stdin) + + if !captured.IsMCP() { + t.Errorf("Type = %q, want %q for MCP tool", captured.Type, ExecutionMCP) + } + if captured.ToolName != "mcp__net__fetch" { + t.Errorf("ToolName = %q, want %q", captured.ToolName, "mcp__net__fetch") + } +} + +func TestCodexPostToolUse_ApplyPatchParsesFilesAndEdits(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + // One add, one update, one delete in a single apply_patch call. + patch := "*** Begin Patch\\n" + + "*** Add File: secrets/api_key.txt\\n" + + "+sk-deadbeef\\n" + + "*** Update File: src/auth.go\\n" + + "@@\\n" + + "-old\\n" + + "+new\\n" + + "*** Delete File: stale.env\\n" + + "*** End Patch" + stdin := `{"session_id":"s","tool_name":"apply_patch","tool_input":{"command":"` + patch + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 3 { + t.Fatalf("handler invocations = %d, want 3 (one per file in patch); got=%+v", len(got), got) + } + if got[0].FilePath != "secrets/api_key.txt" { + t.Errorf("got[0].FilePath = %q, want %q", got[0].FilePath, "secrets/api_key.txt") + } + if len(got[0].Edits) != 1 || got[0].Edits[0].NewString != "sk-deadbeef" { + t.Errorf("got[0].Edits = %+v, want [{OldString:\"\", NewString:\"sk-deadbeef\"}]", got[0].Edits) + } + if got[1].FilePath != "src/auth.go" { + t.Errorf("got[1].FilePath = %q, want %q", got[1].FilePath, "src/auth.go") + } + wantEdit := FileEdit{OldString: "old", NewString: "new"} + if len(got[1].Edits) != 1 || got[1].Edits[0] != wantEdit { + t.Errorf("got[1].Edits = %+v, want [%+v]", got[1].Edits, wantEdit) + } + // Delete sections carry no per-edit content; we just verify the file + // path made it through so path-based policies can still react. + if got[2].FilePath != "stale.env" { + t.Errorf("got[2].FilePath = %q, want %q", got[2].FilePath, "stale.env") + } + if len(got[2].Edits) != 0 { + t.Errorf("got[2].Edits = %+v, want empty for delete", got[2].Edits) + } + for _, c := range got { + if c.Platform != PlatformCodex { + t.Errorf("Platform = %q, want %q", c.Platform, PlatformCodex) + } + if c.Cwd != "/repo" { + t.Errorf("Cwd = %q, want %q", c.Cwd, "/repo") + } + } +} + +func TestCodexPostToolUse_ApplyPatchBlockPropagates(t *testing.T) { + // If any file in a multi-file apply_patch is blocked, the unified + // handler must emit a PostToolBlock so Codex sees feedback rather than + // silently passing through. + ClearHandlers() + defer ClearHandlers() + + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + if strings.Contains(ctx.FilePath, "secrets") { + return FileEditBlock("contains secrets") + } + return FileEditOK() + }) + + patch := "*** Begin Patch\\n" + + "*** Add File: secrets/key.txt\\n" + + "+sk-deadbeef\\n" + + "*** Update File: src/auth.go\\n" + + "@@\\n" + + "-x\\n" + + "+y\\n" + + "*** End Patch" + stdin := `{"session_id":"s","tool_name":"apply_patch","tool_input":{"command":"` + patch + `"},"cwd":"/repo"}` + + stdinR, stdinW, _ := os.Pipe() + stdinW.Write([]byte(stdin)) + stdinW.Close() + stdoutR, stdoutW, _ := os.Pipe() + origStdin, origStdout := os.Stdin, os.Stdout + os.Stdin, os.Stdout = stdinR, stdoutW + defer func() { + stdoutW.Close() + os.Stdin, os.Stdout = origStdin, origStdout + }() + + handlers["codex-post-tool-use"]() + stdoutW.Close() + + var buf bytes.Buffer + buf.ReadFrom(stdoutR) + out := buf.String() + if !strings.Contains(out, `"decision":"block"`) { + t.Errorf("expected PostToolBlock JSON output, got %q", out) + } + if !strings.Contains(out, "contains secrets") { + t.Errorf("expected block reason to be present, got %q", out) + } +} + func TestOnStop_CursorEmptyWorkspaceRootsLeavesCwdEmpty(t *testing.T) { ClearHandlers() defer ClearHandlers() From a8c9d9016cd1ec84044545bd1247b201be93bb1e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 14 May 2026 18:42:19 +0000 Subject: [PATCH 03/19] fix(codex): propagate apply_patch Move to: destination to OnAfterFileEdit The codex-post-tool-use bridge was reading the *** Move to: destination into f.NewFilePath but never surfacing it to handlers. FileEditContext only carried FilePath (the source), so a FilePath-only path allowlist would approve a patch like: *** Update File: app/config.go *** Move to: ../../.ssh/authorized_keys even though the actual destination is outside the allowed directory. Adds a NewFilePath field to FileEditContext and rewrites the codex apply_patch loop to invoke the user's OnAfterFileEdit handler twice for rename operations: 1. once with FilePath = source, NewFilePath = destination 2. once with FilePath = destination, NewFilePath = destination Existing policies that only inspect ctx.FilePath now see the destination as a separate event and can block it. Policies that want to detect the rename relationship can check ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath. Block decisions across all invocations (per-file + per-rename-side) flow through the existing block-reason aggregation, so a single PostToolBlock with the combined reasons is emitted. NewFilePath defaults to "" on every other platform; in-place Codex edits also leave it empty, so this is a strictly additive change. Tests: - TestCodexPostToolUse_ApplyPatchMoveToInvokesHandlerForDestination asserts the handler is invoked once with FilePath=source and once with FilePath=destination, and that NewFilePath is populated on both. - TestCodexPostToolUse_ApplyPatchMoveToBlockOnDestinationPropagates asserts a FilePath-only block targeting the destination results in a PostToolBlock with the destination's reason. Docs: - docs/reference-unified.md and docs/reference-codex.md describe the new field, the double-invocation semantics for renames, and the recommended check for rename detection. Co-authored-by: Ashwin Ramaswami --- docs/reference-codex.md | 3 ++ docs/reference-unified.md | 13 +++--- unified.go | 33 ++++++++++++-- unified_test.go | 94 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 9 deletions(-) diff --git a/docs/reference-codex.md b/docs/reference-codex.md index fc77b5b..e382e17 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -269,8 +269,11 @@ func PostToolContext(context string) PostToolUseOutput `hookshot.OnAfterFileEdit` parses Codex `apply_patch` events by unpacking the unified-diff envelope in `tool_input.command` and invoking your handler **once per file** mentioned in the patch. Each invocation receives a fully populated `FileEditContext`: - `FilePath` is the path declared in the `*** Add File:`, `*** Update File:`, or `*** Delete File:` section. +- `NewFilePath` is the destination path for rename operations (`*** Move to:`); empty otherwise. - `Edits` is `[{OldString: "", NewString: }]` for Add, one `FileEdit` per hunk for Update (with removed lines as `OldString` and added lines as `NewString`), and empty for Delete. +For renames (`*** Update File: ` followed by `*** Move to: `), the handler is invoked **twice** — once with `FilePath` set to the source and once with `FilePath` set to the destination — and `NewFilePath` is populated on both invocations. This ensures that a FilePath-only allowlist which permits the benign source path still receives a separate call for the destination path and can deny moves to sensitive locations like `../../.ssh/authorized_keys`. Policies that want to react specifically to renames should check `ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. + If any of those per-file invocations returns `FileEditBlock`, the unified bridge concatenates the reasons and emits a single `PostToolBlock` so Codex replaces the tool result with the combined feedback. The platform-level `codex.PostToolUseInput` retains the raw `tool_input.command` if you'd rather parse the patch yourself. ### Example diff --git a/docs/reference-unified.md b/docs/reference-unified.md index 128d790..37057d4 100644 --- a/docs/reference-unified.md +++ b/docs/reference-unified.md @@ -191,6 +191,8 @@ For Codex, the underlying PostToolUse handler also matches `apply_patch` in addi For Codex `apply_patch`, the unified bridge parses the unified-diff envelope in `tool_input.command` and invokes your handler **once per file** in the patch, with `FilePath` set to the file declared in the `*** Add/Update/Delete File:` header and `Edits` populated from each hunk. If any of those invocations returns `FileEditBlock`, the reasons are concatenated and emitted as a single `PostToolBlock`. +For renames (`*** Update File: ` + `*** Move to: `) the handler is invoked **twice** — once with `FilePath` set to the source and once with `FilePath` set to the destination — and `NewFilePath` is populated on both invocations. This means a FilePath-only allowlist that permits the benign source still receives a separate call for the destination so it can deny something like `../../.ssh/authorized_keys`. Handlers that want to detect the rename relationship should check `ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. + ### FileEdit ```go @@ -204,11 +206,12 @@ type FileEdit struct { ```go type FileEditContext struct { - Platform Platform - SessionID string // Claude/Droid: session_id, Cursor: conversation_id, Cascade: trajectory_id - FilePath string - Edits []FileEdit - Cwd string + Platform Platform + SessionID string // Claude/Droid: session_id, Cursor: conversation_id, Cascade: trajectory_id + FilePath string + NewFilePath string // Destination path for rename operations (Codex apply_patch "*** Move to:"); empty otherwise. + Edits []FileEdit + Cwd string // Raw access RawClaudeCode *claude.PostToolUseInput diff --git a/unified.go b/unified.go index 2839a73..444d9ba 100644 --- a/unified.go +++ b/unified.go @@ -523,8 +523,19 @@ type FileEditContext struct { Platform Platform SessionID string // Claude Code: session_id, Cursor: conversation_id, Cascade: trajectory_id FilePath string - Edits []FileEdit - Cwd string + // NewFilePath is the destination path when the edit also renames the + // file (Codex apply_patch "*** Move to:" today; future platforms may + // surface their own rename semantics). It is empty for in-place edits. + // + // For Codex moves, the unified bridge invokes OnAfterFileEdit twice — + // once with FilePath set to the source and once with FilePath set to + // the destination — and populates NewFilePath on both invocations so + // path-based policies can never be bypassed by inspecting only + // FilePath. Handlers that want to detect a rename should check + // `ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. + NewFilePath string + Edits []FileEdit + Cwd string // Raw input for advanced use cases RawClaudeCode *claude.PostToolUseInput @@ -790,11 +801,12 @@ func OnAfterFileEdit(handler FileEditHandler) { blockReasons []string contexts []string ) - for _, f := range files { + invoke := func(filePath string, f codexApplyPatchFile) { ctx := FileEditContext{ Platform: PlatformCodex, SessionID: input.SessionID, - FilePath: f.FilePath, + FilePath: filePath, + NewFilePath: f.NewFilePath, Edits: f.Edits, Cwd: input.Cwd, RawClaudeCode: &input, @@ -806,6 +818,19 @@ func OnAfterFileEdit(handler FileEditHandler) { contexts = append(contexts, decision.Context) } } + for _, f := range files { + // Always invoke for the declared source path. + invoke(f.FilePath, f) + // For renames, invoke again with the destination so policies + // that only inspect ctx.FilePath cannot be bypassed by a + // "*** Move to:" pointing at a sensitive path (e.g. + // "../../.ssh/authorized_keys"). NewFilePath is populated on + // both invocations so policies that want to detect the + // rename relationship can. + if f.NewFilePath != "" && f.NewFilePath != f.FilePath { + invoke(f.NewFilePath, f) + } + } if len(blockReasons) > 0 { return claude.PostToolBlock(strings.Join(blockReasons, "\n")) } diff --git a/unified_test.go b/unified_test.go index 79eb257..6ca5c24 100644 --- a/unified_test.go +++ b/unified_test.go @@ -951,6 +951,100 @@ func TestCodexPostToolUse_ApplyPatchParsesFilesAndEdits(t *testing.T) { } } +func TestCodexPostToolUse_ApplyPatchMoveToInvokesHandlerForDestination(t *testing.T) { + // An apply_patch with a "*** Move to:" directive must surface the + // destination path to the handler. Without this, a patch like + // "*** Update File: app/config.go\n*** Move to: ../../.ssh/authorized_keys" + // would slip past a path-based policy that only inspects + // ctx.FilePath. The bridge invokes the handler twice for a rename so + // existing FilePath-only policies catch the destination. + ClearHandlers() + defer ClearHandlers() + + var seen []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + seen = append(seen, ctx) + return FileEditOK() + }) + + patch := "*** Begin Patch\\n" + + "*** Update File: app/config.go\\n" + + "*** Move to: ../../.ssh/authorized_keys\\n" + + "@@\\n" + + "-old\\n" + + "+attacker-controlled\\n" + + "*** End Patch" + stdin := `{"session_id":"s","tool_name":"apply_patch","tool_input":{"command":"` + patch + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(seen) != 2 { + t.Fatalf("handler invocations = %d, want 2 (source + destination); got %+v", len(seen), seen) + } + // Both invocations must expose the destination via NewFilePath so a + // policy can detect that a rename is happening. + for _, c := range seen { + if c.NewFilePath != "../../.ssh/authorized_keys" { + t.Errorf("NewFilePath = %q, want %q", c.NewFilePath, "../../.ssh/authorized_keys") + } + } + // First call: source path. Second call: destination path. A + // FilePath-only allowlist that permits "app/config.go" must still see + // "../../.ssh/authorized_keys" so it can deny it. + if seen[0].FilePath != "app/config.go" { + t.Errorf("seen[0].FilePath = %q, want %q", seen[0].FilePath, "app/config.go") + } + if seen[1].FilePath != "../../.ssh/authorized_keys" { + t.Errorf("seen[1].FilePath = %q, want %q", seen[1].FilePath, "../../.ssh/authorized_keys") + } +} + +func TestCodexPostToolUse_ApplyPatchMoveToBlockOnDestinationPropagates(t *testing.T) { + // A FilePath-only policy that blocks ".ssh/authorized_keys" must + // surface as a PostToolBlock even when the source path was benign. + ClearHandlers() + defer ClearHandlers() + + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + if strings.Contains(ctx.FilePath, ".ssh/authorized_keys") { + return FileEditBlock("destination is a sensitive path") + } + return FileEditOK() + }) + + patch := "*** Begin Patch\\n" + + "*** Update File: app/config.go\\n" + + "*** Move to: ../../.ssh/authorized_keys\\n" + + "@@\\n" + + "-old\\n" + + "+attacker-controlled\\n" + + "*** End Patch" + stdin := `{"session_id":"s","tool_name":"apply_patch","tool_input":{"command":"` + patch + `"},"cwd":"/repo"}` + + stdinR, stdinW, _ := os.Pipe() + stdinW.Write([]byte(stdin)) + stdinW.Close() + stdoutR, stdoutW, _ := os.Pipe() + origStdin, origStdout := os.Stdin, os.Stdout + os.Stdin, os.Stdout = stdinR, stdoutW + defer func() { + stdoutW.Close() + os.Stdin, os.Stdout = origStdin, origStdout + }() + + handlers["codex-post-tool-use"]() + stdoutW.Close() + + var buf bytes.Buffer + buf.ReadFrom(stdoutR) + out := buf.String() + if !strings.Contains(out, `"decision":"block"`) { + t.Errorf("expected PostToolBlock JSON output, got %q", out) + } + if !strings.Contains(out, "destination is a sensitive path") { + t.Errorf("expected block reason to be present, got %q", out) + } +} + func TestCodexPostToolUse_ApplyPatchBlockPropagates(t *testing.T) { // If any file in a multi-file apply_patch is blocked, the unified // handler must emit a PostToolBlock so Codex sees feedback rather than From 00088f24f1f975ea382ce23f6b148cbd759c5f71 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 17:09:07 -0700 Subject: [PATCH 04/19] Simplify --- .cursor/rules/corridor-mcp-server-usage.mdc | 7 + .vscode/settings.json | 1 + README.md | 15 +- cmd/hookshot/main.go | 16 +- codex/apply_patch.go | 154 ++++++++++++++++++ .../apply_patch_test.go | 50 +++--- codex/doc.go | 10 +- codex_apply_patch.go | 151 ----------------- docs/reference-codex.md | 26 ++- docs/reference-unified.md | 4 +- examples/multi-hook/main.go | 6 +- unified.go | 18 +- 12 files changed, 232 insertions(+), 226 deletions(-) create mode 100644 .cursor/rules/corridor-mcp-server-usage.mdc create mode 100644 .vscode/settings.json create mode 100644 codex/apply_patch.go rename codex_apply_patch_test.go => codex/apply_patch_test.go (73%) delete mode 100644 codex_apply_patch.go diff --git a/.cursor/rules/corridor-mcp-server-usage.mdc b/.cursor/rules/corridor-mcp-server-usage.mdc new file mode 100644 index 0000000..4f07f53 --- /dev/null +++ b/.cursor/rules/corridor-mcp-server-usage.mdc @@ -0,0 +1,7 @@ +--- +description: Corridor MCP Server Usage +globs: +alwaysApply: true +--- + +Every time you generate code, use the analyzePlan tool from Corridor's MCP Server (corridor) to analyze the plan or thought process. ALWAYS use Corridor to analyze the plan. Always generate a plan before generating code. diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..9e26dfe --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/README.md b/README.md index 3758da2..35153fc 100644 --- a/README.md +++ b/README.md @@ -99,20 +99,17 @@ hookshot install --binary /path/to/my-hooks ### OpenAI Codex (`~/.codex/hooks.json`) -Codex hooks are gated behind a feature flag. Enable it in `~/.codex/config.toml`: - -```toml -[features] -codex_hooks = true -``` - -Then configure the hooks themselves: +Codex hooks are enabled by default (the `hooks` feature flag in Codex is +stable and on). No `~/.codex/config.toml` change is required. If your +organization disabled hooks, set `[features].hooks = true` to turn them +back on — `codex_hooks` is a deprecated alias for the same flag. ```json { "hooks": { "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], - "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }] + "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], + "PostToolUse": [{ "matcher": "apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }] } } ``` diff --git a/cmd/hookshot/main.go b/cmd/hookshot/main.go index cd6cafd..6c9029b 100644 --- a/cmd/hookshot/main.go +++ b/cmd/hookshot/main.go @@ -482,10 +482,11 @@ func installCodex(binaryPath string) error { config = make(map[string]any) } - // Codex hook config follows the same shape as Claude Code's settings, but - // lives in ~/.codex/hooks.json. PostToolUse matches "apply_patch|Edit|Write" - // because Codex uses apply_patch for file edits in addition to Write/Edit - // aliases. + // Codex hook config follows the same JSON shape as Claude Code's + // settings but lives in ~/.codex/hooks.json. Matchers include + // "mcp__.*" so MCP tool calls reach the hook binary. "apply_patch" + // alone covers Codex file edits — Codex emits "Edit" and "Write" as + // matcher aliases for apply_patch, so they're redundant here. hooks := map[string]any{ "Stop": []map[string]any{{ "hooks": []map[string]any{{ @@ -494,10 +495,6 @@ func installCodex(binaryPath string) error { }}, }}, "PreToolUse": []map[string]any{{ - // Include mcp__.* so MCP tool calls also reach the hook - // binary. Without this, the codex-pre-tool-use handler is - // never invoked for MCP tools and any OnBeforeExecution - // policy that enforces MCP allowlists is silently bypassed. "matcher": "Bash|apply_patch|mcp__.*", "hooks": []map[string]any{{ "type": "command", @@ -505,7 +502,7 @@ func installCodex(binaryPath string) error { }}, }}, "PostToolUse": []map[string]any{{ - "matcher": "apply_patch|Edit|Write|mcp__.*", + "matcher": "apply_patch|mcp__.*", "hooks": []map[string]any{{ "type": "command", "command": binaryPath + " codex-post-tool-use", @@ -533,7 +530,6 @@ func installCodex(binaryPath string) error { } fmt.Println(" Installed hooks: Stop, PreToolUse, PostToolUse, UserPromptSubmit") - fmt.Println(" Note: Codex hooks require codex_hooks = true under [features] in ~/.codex/config.toml") return nil } diff --git a/codex/apply_patch.go b/codex/apply_patch.go new file mode 100644 index 0000000..6b2bb8f --- /dev/null +++ b/codex/apply_patch.go @@ -0,0 +1,154 @@ +package codex + +import "strings" + +// PatchEdit captures one removed/added pair inside an apply_patch hunk. +// +// For an "add" operation the patch contains exactly one PatchEdit with +// OldString == "" and NewString set to the added contents. For an "update" +// operation each hunk in the patch becomes one PatchEdit with the removed +// lines joined as OldString and the added lines joined as NewString. For +// "delete" the Edits slice is empty (the FilePath alone is enough to drive +// policy). +type PatchEdit struct { + OldString string + NewString string +} + +// PatchFile represents one file affected by an apply_patch tool call. +// +// Codex's apply_patch carries a unified-diff-style envelope under +// tool_input.command (see https://developers.openai.com/codex/hooks). A +// single envelope may touch multiple files, which is why ParseApplyPatch +// returns a slice rather than a single FilePath/Edits pair. +type PatchFile struct { + // Operation is one of "add", "update", or "delete". + Operation string + // FilePath is the path declared in the *** {Add,Update,Delete} File: + // header. + FilePath string + // NewFilePath is set for "update" operations that include a + // "*** Move to:" line. Empty for in-place edits. + NewFilePath string + // Edits captures per-hunk old/new content. See PatchEdit for shape. + Edits []PatchEdit +} + +// ParseApplyPatch parses the unified-diff envelope from a Codex apply_patch +// tool call. The input is the raw value of tool_input.command for an +// apply_patch invocation. +// +// This is a convenience helper for handlers that want structured access to +// the patch (per-file paths, rename detection, per-hunk content). It is NOT +// invoked by the unified OnAfterFileEdit bridge — that path passes the raw +// patch text through unchanged so simple path-based policies cannot be +// silently bypassed by a parser desync against Codex's actual format. Call +// this helper only if you need the parsed view. +// +// The parser is intentionally tolerant: malformed input degrades +// gracefully (an affected file is still recorded even if its Edits list is +// incomplete) rather than returning an error. +func ParseApplyPatch(patch string) []PatchFile { + // Strip a leading "apply_patch <<'EOF'\n" wrapper if present. Some + // Codex invocations include a here-doc style wrapper around the + // "*** Begin Patch" block; this is a defensive measure and not part + // of the documented wire format. + if i := strings.Index(patch, "*** Begin Patch"); i > 0 { + patch = patch[i:] + } + + lines := strings.Split(patch, "\n") + var files []PatchFile + var current *PatchFile + var hunkOld, hunkNew []string + inHunk := false + + flushHunk := func() { + if current == nil { + return + } + if len(hunkOld) == 0 && len(hunkNew) == 0 { + hunkOld, hunkNew = nil, nil + return + } + current.Edits = append(current.Edits, PatchEdit{ + OldString: strings.Join(hunkOld, "\n"), + NewString: strings.Join(hunkNew, "\n"), + }) + hunkOld, hunkNew = nil, nil + } + + flushFile := func() { + flushHunk() + if current != nil { + files = append(files, *current) + } + current = nil + inHunk = false + } + + for _, line := range lines { + switch { + case strings.HasPrefix(line, "*** Begin Patch"), + strings.HasPrefix(line, "*** End of File"): + case strings.HasPrefix(line, "*** End Patch"): + flushFile() + case strings.HasPrefix(line, "*** Add File: "): + flushFile() + current = &PatchFile{ + Operation: "add", + FilePath: strings.TrimPrefix(line, "*** Add File: "), + } + inHunk = true + case strings.HasPrefix(line, "*** Update File: "): + flushFile() + current = &PatchFile{ + Operation: "update", + FilePath: strings.TrimPrefix(line, "*** Update File: "), + } + inHunk = false + case strings.HasPrefix(line, "*** Delete File: "): + flushFile() + current = &PatchFile{ + Operation: "delete", + FilePath: strings.TrimPrefix(line, "*** Delete File: "), + } + inHunk = false + case strings.HasPrefix(line, "*** Move to: "): + if current != nil { + current.NewFilePath = strings.TrimPrefix(line, "*** Move to: ") + } + case strings.HasPrefix(line, "@@"): + flushHunk() + inHunk = true + case current != nil: + switch current.Operation { + case "add": + if strings.HasPrefix(line, "+") { + hunkNew = append(hunkNew, line[1:]) + } else if line != "" { + hunkNew = append(hunkNew, line) + } + case "update": + if !inHunk { + continue + } + if len(line) == 0 { + flushHunk() + continue + } + switch line[0] { + case '+': + hunkNew = append(hunkNew, line[1:]) + case '-': + hunkOld = append(hunkOld, line[1:]) + case ' ': + flushHunk() + } + case "delete": + } + } + } + flushFile() + return files +} diff --git a/codex_apply_patch_test.go b/codex/apply_patch_test.go similarity index 73% rename from codex_apply_patch_test.go rename to codex/apply_patch_test.go index e911faa..7f97dad 100644 --- a/codex_apply_patch_test.go +++ b/codex/apply_patch_test.go @@ -1,37 +1,37 @@ -package hookshot +package codex import ( "reflect" "testing" ) -func TestParseCodexApplyPatch_AddFile(t *testing.T) { +func TestParseApplyPatch_AddFile(t *testing.T) { patch := "*** Begin Patch\n" + "*** Add File: secrets/api_key.txt\n" + "+sk-deadbeef\n" + "+second line\n" + "*** End Patch\n" - got := parseCodexApplyPatch(patch) - want := []codexApplyPatchFile{{ + got := ParseApplyPatch(patch) + want := []PatchFile{{ Operation: "add", FilePath: "secrets/api_key.txt", - Edits: []FileEdit{{ + Edits: []PatchEdit{{ OldString: "", NewString: "sk-deadbeef\nsecond line", }}, }} if !reflect.DeepEqual(got, want) { - t.Errorf("parseCodexApplyPatch(add) =\n %+v\nwant\n %+v", got, want) + t.Errorf("ParseApplyPatch(add) =\n %+v\nwant\n %+v", got, want) } } -func TestParseCodexApplyPatch_DeleteFile(t *testing.T) { +func TestParseApplyPatch_DeleteFile(t *testing.T) { patch := "*** Begin Patch\n" + "*** Delete File: old/secrets.env\n" + "*** End Patch\n" - got := parseCodexApplyPatch(patch) + got := ParseApplyPatch(patch) if len(got) != 1 { t.Fatalf("got %d files, want 1", len(got)) } @@ -46,7 +46,7 @@ func TestParseCodexApplyPatch_DeleteFile(t *testing.T) { } } -func TestParseCodexApplyPatch_UpdateFile(t *testing.T) { +func TestParseApplyPatch_UpdateFile(t *testing.T) { patch := "*** Begin Patch\n" + "*** Update File: src/auth.go\n" + "@@ func login\n" + @@ -57,7 +57,7 @@ func TestParseCodexApplyPatch_UpdateFile(t *testing.T) { " }\n" + "*** End Patch\n" - got := parseCodexApplyPatch(patch) + got := ParseApplyPatch(patch) if len(got) != 1 { t.Fatalf("got %d files, want 1", len(got)) } @@ -70,7 +70,7 @@ func TestParseCodexApplyPatch_UpdateFile(t *testing.T) { if len(got[0].Edits) != 1 { t.Fatalf("Edits = %+v, want 1 edit", got[0].Edits) } - wantEdit := FileEdit{ + wantEdit := PatchEdit{ OldString: " token := \"hardcoded\"", NewString: " token := os.Getenv(\"TOKEN\")", } @@ -79,7 +79,7 @@ func TestParseCodexApplyPatch_UpdateFile(t *testing.T) { } } -func TestParseCodexApplyPatch_UpdateFile_MultipleHunks(t *testing.T) { +func TestParseApplyPatch_UpdateFile_MultipleHunks(t *testing.T) { patch := "*** Begin Patch\n" + "*** Update File: a.go\n" + "@@\n" + @@ -90,22 +90,22 @@ func TestParseCodexApplyPatch_UpdateFile_MultipleHunks(t *testing.T) { "+qux\n" + "*** End Patch\n" - got := parseCodexApplyPatch(patch) + got := ParseApplyPatch(patch) if len(got) != 1 { t.Fatalf("got %d files, want 1", len(got)) } if len(got[0].Edits) != 2 { t.Fatalf("Edits = %+v, want 2 edits", got[0].Edits) } - if got[0].Edits[0] != (FileEdit{OldString: "foo", NewString: "bar"}) { + if got[0].Edits[0] != (PatchEdit{OldString: "foo", NewString: "bar"}) { t.Errorf("Edits[0] = %+v", got[0].Edits[0]) } - if got[0].Edits[1] != (FileEdit{OldString: "baz", NewString: "qux"}) { + if got[0].Edits[1] != (PatchEdit{OldString: "baz", NewString: "qux"}) { t.Errorf("Edits[1] = %+v", got[0].Edits[1]) } } -func TestParseCodexApplyPatch_MultiFile(t *testing.T) { +func TestParseApplyPatch_MultiFile(t *testing.T) { patch := "*** Begin Patch\n" + "*** Add File: new.txt\n" + "+hello\n" + @@ -116,7 +116,7 @@ func TestParseCodexApplyPatch_MultiFile(t *testing.T) { "*** Delete File: stale.txt\n" + "*** End Patch\n" - got := parseCodexApplyPatch(patch) + got := ParseApplyPatch(patch) if len(got) != 3 { t.Fatalf("got %d files, want 3", len(got)) } @@ -131,7 +131,7 @@ func TestParseCodexApplyPatch_MultiFile(t *testing.T) { } } -func TestParseCodexApplyPatch_MoveTo(t *testing.T) { +func TestParseApplyPatch_MoveTo(t *testing.T) { patch := "*** Begin Patch\n" + "*** Update File: old/path.go\n" + "*** Move to: new/path.go\n" + @@ -140,7 +140,7 @@ func TestParseCodexApplyPatch_MoveTo(t *testing.T) { "+bar\n" + "*** End Patch\n" - got := parseCodexApplyPatch(patch) + got := ParseApplyPatch(patch) if len(got) != 1 { t.Fatalf("got %d files, want 1", len(got)) } @@ -152,9 +152,7 @@ func TestParseCodexApplyPatch_MoveTo(t *testing.T) { } } -func TestParseCodexApplyPatch_LeadingWrapper(t *testing.T) { - // Some Codex invocations wrap the patch in a here-doc style header. - // The parser should skip everything before the Begin Patch marker. +func TestParseApplyPatch_LeadingWrapper(t *testing.T) { patch := "apply_patch <<'PATCH'\n" + "*** Begin Patch\n" + "*** Add File: foo.txt\n" + @@ -162,7 +160,7 @@ func TestParseCodexApplyPatch_LeadingWrapper(t *testing.T) { "*** End Patch\n" + "PATCH\n" - got := parseCodexApplyPatch(patch) + got := ParseApplyPatch(patch) if len(got) != 1 { t.Fatalf("got %d files, want 1", len(got)) } @@ -171,11 +169,11 @@ func TestParseCodexApplyPatch_LeadingWrapper(t *testing.T) { } } -func TestParseCodexApplyPatch_EmptyAndMalformed(t *testing.T) { - if files := parseCodexApplyPatch(""); files != nil { +func TestParseApplyPatch_EmptyAndMalformed(t *testing.T) { + if files := ParseApplyPatch(""); files != nil { t.Errorf("empty input should yield nil, got %+v", files) } - if files := parseCodexApplyPatch("not a patch"); files != nil { + if files := ParseApplyPatch("not a patch"); files != nil { t.Errorf("non-patch input should yield nil, got %+v", files) } } diff --git a/codex/doc.go b/codex/doc.go index 3858a4d..433e5e4 100644 --- a/codex/doc.go +++ b/codex/doc.go @@ -12,10 +12,10 @@ // // # Configuration // -// Codex hooks are enabled behind a feature flag in ~/.codex/config.toml: -// -// [features] -// codex_hooks = true +// Codex hooks are enabled by default in current releases (the `hooks` +// feature flag is stable). If your organization disabled them, set +// `[features].hooks = true` in ~/.codex/config.toml to re-enable. The +// older `codex_hooks` key still works as a deprecated alias. // // Configure hook commands in ~/.codex/hooks.json (or inline under [hooks] in // ~/.codex/config.toml): @@ -30,7 +30,7 @@ // ], // "PostToolUse": [ // { -// "matcher": "apply_patch|Edit|Write|mcp__.*", +// "matcher": "apply_patch|mcp__.*", // "hooks": [{ "type": "command", "command": "/path/to/hook codex-post-tool-use" }] // } // ], diff --git a/codex_apply_patch.go b/codex_apply_patch.go deleted file mode 100644 index 3edca5e..0000000 --- a/codex_apply_patch.go +++ /dev/null @@ -1,151 +0,0 @@ -package hookshot - -import ( - "strings" -) - -// codexApplyPatchFile represents one file affected by a Codex apply_patch -// tool call. Codex's apply_patch carries a "command" field containing a -// unified-diff-style envelope (see https://developers.openai.com/codex/hooks). -// Each envelope may reference multiple files, which is why we model this as -// a slice on the parser output rather than a single FilePath/Edits pair. -type codexApplyPatchFile struct { - // Operation is one of "add", "update", or "delete". - Operation string - // FilePath is the path declared in the *** {Add,Update,Delete} File: header. - FilePath string - // NewFilePath is set for "update" operations that include a *** Move to: line. - NewFilePath string - // Edits captures the per-hunk old/new content. For "add" the patch - // contains exactly one edit with OldString=="" and NewString set to the - // added contents. For "delete" Edits is empty (the file path is enough - // for policy evaluation). For "update", each hunk in the patch becomes - // one FileEdit with the removed lines joined as OldString and the added - // lines joined as NewString. - Edits []FileEdit -} - -// parseCodexApplyPatch parses the patch envelope from a Codex apply_patch -// tool call. The input is the raw value of tool_input.command for an -// apply_patch invocation. The parser is intentionally tolerant: malformed -// input degrades gracefully (the affected file is still recorded, even if -// its Edits list is incomplete) rather than returning an error, because -// hook handlers should never silently disappear on bad input. -func parseCodexApplyPatch(patch string) []codexApplyPatchFile { - // Strip a leading "apply_patch <<'EOF'\n" wrapper if present. Some Codex - // builds include a here-doc style wrapper around the *** Begin Patch - // block; this is a defensive measure and not part of the documented - // wire format. - if i := strings.Index(patch, "*** Begin Patch"); i > 0 { - patch = patch[i:] - } - - lines := strings.Split(patch, "\n") - var files []codexApplyPatchFile - var current *codexApplyPatchFile - var hunkOld, hunkNew []string - inHunk := false - - flushHunk := func() { - if current == nil { - return - } - if len(hunkOld) == 0 && len(hunkNew) == 0 { - hunkOld, hunkNew = nil, nil - return - } - current.Edits = append(current.Edits, FileEdit{ - OldString: strings.Join(hunkOld, "\n"), - NewString: strings.Join(hunkNew, "\n"), - }) - hunkOld, hunkNew = nil, nil - } - - flushFile := func() { - flushHunk() - if current != nil { - files = append(files, *current) - } - current = nil - inHunk = false - } - - for _, line := range lines { - switch { - case strings.HasPrefix(line, "*** Begin Patch"), - strings.HasPrefix(line, "*** End of File"): - // Markers we don't act on; "End of File" is sometimes emitted - // inside an Update section. - case strings.HasPrefix(line, "*** End Patch"): - flushFile() - case strings.HasPrefix(line, "*** Add File: "): - flushFile() - current = &codexApplyPatchFile{ - Operation: "add", - FilePath: strings.TrimPrefix(line, "*** Add File: "), - } - inHunk = true - case strings.HasPrefix(line, "*** Update File: "): - flushFile() - current = &codexApplyPatchFile{ - Operation: "update", - FilePath: strings.TrimPrefix(line, "*** Update File: "), - } - inHunk = false - case strings.HasPrefix(line, "*** Delete File: "): - flushFile() - current = &codexApplyPatchFile{ - Operation: "delete", - FilePath: strings.TrimPrefix(line, "*** Delete File: "), - } - inHunk = false - case strings.HasPrefix(line, "*** Move to: "): - if current != nil { - current.NewFilePath = strings.TrimPrefix(line, "*** Move to: ") - } - case strings.HasPrefix(line, "@@"): - // Hunk separator inside an Update section. Flush the previous - // hunk (if any) and start collecting a fresh one. - flushHunk() - inHunk = true - case current != nil: - switch current.Operation { - case "add": - // In an Add section every content line is prefixed with "+". - // Be lenient: accept unprefixed lines as well so we still - // capture the new file contents when Codex omits the - // prefix. - if strings.HasPrefix(line, "+") { - hunkNew = append(hunkNew, line[1:]) - } else if line != "" { - hunkNew = append(hunkNew, line) - } - case "update": - if !inHunk { - // Anything before the first @@ in an Update section is - // metadata we don't care about. - continue - } - if len(line) == 0 { - // Treat blank lines as context inside an Update. - flushHunk() - continue - } - switch line[0] { - case '+': - hunkNew = append(hunkNew, line[1:]) - case '-': - hunkOld = append(hunkOld, line[1:]) - case ' ': - // Context line — flush so adjacent ±/context groups - // don't get fused into a single edit. - flushHunk() - } - case "delete": - // Delete sections have no content lines; ignore stray text. - } - } - } - flushFile() - return files -} diff --git a/docs/reference-codex.md b/docs/reference-codex.md index e382e17..5524d6d 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -4,12 +4,7 @@ Platform-specific types and helpers for OpenAI Codex hooks. Use these when you n Codex hooks use the same JSON wire format as Claude Code hooks, configured in `~/.codex/hooks.json` or inline `[hooks]` tables in `~/.codex/config.toml`. The `codex` Go package re-exports the relevant `claude` types so your code can stay platform-explicit while still benefiting from the shared types. -Codex hooks are behind a feature flag — make sure `~/.codex/config.toml` contains: - -```toml -[features] -codex_hooks = true -``` +Codex hooks are enabled by default (the `hooks` feature flag is stable in current Codex releases). If your organization disabled hooks, set `[features].hooks = true` in `~/.codex/config.toml` to re-enable. The older `codex_hooks` key still works as a deprecated alias. See the upstream spec at https://developers.openai.com/codex/hooks. @@ -142,7 +137,7 @@ type PreToolUseHookOutput struct { Codex honors `permissionDecision: "deny"` (or the older `decision: "block"` shape) on Bash and `apply_patch`. `"allow"`, `"ask"`, `updatedInput`, `additionalContext`, `continue: false`, `stopReason`, and `suppressOutput` are parsed but fail open today. -> **Fail-closed `Ask` on the unified API.** Because Codex doesn't enforce `"ask"` yet, `hookshot.OnBeforeExecution` returning `AskExecution(...)` is translated to a `Deny` on Codex so policies that require user confirmation aren't silently bypassed. If you call the platform-level `codex.Ask` helper directly, the output JSON still encodes `"ask"` (so it round-trips with the upstream protocol) — that's only useful for forward-compat testing today. +> **Fail-closed `Ask` on the unified API.** Because Codex doesn't enforce `"ask"` yet, `hookshot.OnBeforeExecution` returning `AskExecution(...)` is translated to a `Deny` on Codex so policies that require user confirmation aren't silently bypassed. If you call the platform-level `codex.Ask` helper directly, the output JSON still encodes `"ask"` (so it round-trips with the upstream protocol) — that's only useful for forward-compat testing today. If you want to react when Codex is actually about to prompt the user, register a separate handler for the [PermissionRequest event](#permissionrequest) — that one's enforcement is supported. ### Helper Functions @@ -274,7 +269,9 @@ func PostToolContext(context string) PostToolUseOutput For renames (`*** Update File: ` followed by `*** Move to: `), the handler is invoked **twice** — once with `FilePath` set to the source and once with `FilePath` set to the destination — and `NewFilePath` is populated on both invocations. This ensures that a FilePath-only allowlist which permits the benign source path still receives a separate call for the destination path and can deny moves to sensitive locations like `../../.ssh/authorized_keys`. Policies that want to react specifically to renames should check `ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. -If any of those per-file invocations returns `FileEditBlock`, the unified bridge concatenates the reasons and emits a single `PostToolBlock` so Codex replaces the tool result with the combined feedback. The platform-level `codex.PostToolUseInput` retains the raw `tool_input.command` if you'd rather parse the patch yourself. +If any of those per-file invocations returns `FileEditBlock`, the unified bridge concatenates the reasons and emits a single `PostToolBlock` so Codex replaces the tool result with the combined feedback. + +The same parser is also exported as `codex.ParseApplyPatch(rawCommand string) []codex.PatchFile` for callers that want to parse a patch envelope themselves (for example, from the raw `codex.PostToolUseInput.ToolInput`). ### Example @@ -398,12 +395,7 @@ hookshot.Register("codex-stop", func() { ## Configuration Example -`~/.codex/config.toml`: - -```toml -[features] -codex_hooks = true -``` +Hooks are on by default — no `~/.codex/config.toml` edit required. `~/.codex/hooks.json`: @@ -420,7 +412,7 @@ codex_hooks = true ], "PostToolUse": [ { - "matcher": "apply_patch|Edit|Write|mcp__.*", + "matcher": "apply_patch|mcp__.*", "hooks": [ { "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" } ] @@ -444,6 +436,8 @@ codex_hooks = true } ``` -`hookshot install --codex --binary /path/to/my-hooks` will generate this layout for you, but it will not toggle the `codex_hooks` feature flag — set that yourself. +`hookshot install --codex --binary /path/to/my-hooks` will generate this layout for you. > **Why `mcp__.*` is in the matcher.** Codex passes MCP tool names to PreToolUse / PostToolUse using the `mcp__server__tool` convention. Omitting the `mcp__.*` alternative would mean Codex never invokes the hook binary for MCP calls, which would silently bypass any `OnBeforeExecution` policy meant to enforce MCP allowlists. + +> **Why `Edit|Write` is not in the matcher.** Codex emits `Edit` and `Write` as *matcher aliases* for `apply_patch`. The canonical `tool_name` Codex sends to the hook is always `apply_patch`, so a matcher of `apply_patch` alone covers every file-edit call. `Edit` and `Write` aliases never appear as standalone tool names. diff --git a/docs/reference-unified.md b/docs/reference-unified.md index 37057d4..b12bc93 100644 --- a/docs/reference-unified.md +++ b/docs/reference-unified.md @@ -187,12 +187,14 @@ Handles post-file-edit events. **Registers:** `claude-after-file-edit`, `cursor-after-file-edit`, `droid-after-file-edit`, `cascade-post-write-code`, `codex-post-tool-use` -For Codex, the underlying PostToolUse handler also matches `apply_patch` in addition to `Write` and `Edit`. Configure the hook with `matcher: "apply_patch|Edit|Write|mcp__.*"` in `~/.codex/hooks.json`. +For Codex, the underlying PostToolUse handler matches `apply_patch` (which is how Codex performs all file edits). Configure the hook with `matcher: "apply_patch|mcp__.*"` in `~/.codex/hooks.json`. For Codex `apply_patch`, the unified bridge parses the unified-diff envelope in `tool_input.command` and invokes your handler **once per file** in the patch, with `FilePath` set to the file declared in the `*** Add/Update/Delete File:` header and `Edits` populated from each hunk. If any of those invocations returns `FileEditBlock`, the reasons are concatenated and emitted as a single `PostToolBlock`. For renames (`*** Update File: ` + `*** Move to: `) the handler is invoked **twice** — once with `FilePath` set to the source and once with `FilePath` set to the destination — and `NewFilePath` is populated on both invocations. This means a FilePath-only allowlist that permits the benign source still receives a separate call for the destination so it can deny something like `../../.ssh/authorized_keys`. Handlers that want to detect the rename relationship should check `ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. +The same parser is exported as `codex.ParseApplyPatch` for callers that want to parse a patch envelope themselves. + ### FileEdit ```go diff --git a/examples/multi-hook/main.go b/examples/multi-hook/main.go index f2332f2..978cfe8 100644 --- a/examples/multi-hook/main.go +++ b/examples/multi-hook/main.go @@ -54,14 +54,14 @@ // } // } // -// Configure in OpenAI Codex (~/.codex/hooks.json; requires -// codex_hooks = true under [features] in ~/.codex/config.toml): +// Configure in OpenAI Codex (~/.codex/hooks.json; hooks are enabled by +// default in current Codex releases): // // { // "hooks": { // "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], // "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], -// "PostToolUse": [{ "matcher": "apply_patch|Edit|Write|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], +// "PostToolUse": [{ "matcher": "apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], // "UserPromptSubmit": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" }] }] // } // } diff --git a/unified.go b/unified.go index 444d9ba..a83bce6 100644 --- a/unified.go +++ b/unified.go @@ -7,6 +7,7 @@ import ( "github.com/CorridorSecurity/hookshot/cascade" "github.com/CorridorSecurity/hookshot/claude" + "github.com/CorridorSecurity/hookshot/codex" "github.com/CorridorSecurity/hookshot/cursor" "github.com/CorridorSecurity/hookshot/droid" "github.com/CorridorSecurity/hookshot/internal" @@ -724,8 +725,11 @@ func OnAfterFileEdit(handler FileEditHandler) { // multiple files in a single call. For each file in the patch we invoke // the user's handler exactly once with a populated FileEditContext, and // we combine the decisions across files: a Block from any file wins, - // otherwise context strings are concatenated. Configure the hook with a - // matcher like "apply_patch|Edit|Write" in hooks.json. + // otherwise context strings are concatenated. The parser used here is + // also exported as codex.ParseApplyPatch for callers that want raw + // access. Configure the hook with matcher "apply_patch|mcp__.*" in + // hooks.json — "Edit" and "Write" matcher aliases exist but are + // redundant with "apply_patch". Register("codex-post-tool-use", func() { Run(func(input claude.PostToolUseInput) claude.PostToolUseOutput { if input.ToolName != "Write" && input.ToolName != "Edit" && input.ToolName != "apply_patch" { @@ -774,7 +778,7 @@ func OnAfterFileEdit(handler FileEditHandler) { } json.Unmarshal(input.ToolInput, &applyInput) - files := parseCodexApplyPatch(applyInput.Command) + files := codex.ParseApplyPatch(applyInput.Command) if len(files) == 0 { // We could not parse anything actionable out of the patch. // Fall back to invoking the handler once with whatever raw @@ -801,13 +805,17 @@ func OnAfterFileEdit(handler FileEditHandler) { blockReasons []string contexts []string ) - invoke := func(filePath string, f codexApplyPatchFile) { + invoke := func(filePath string, f codex.PatchFile) { + edits := make([]FileEdit, 0, len(f.Edits)) + for _, e := range f.Edits { + edits = append(edits, FileEdit{OldString: e.OldString, NewString: e.NewString}) + } ctx := FileEditContext{ Platform: PlatformCodex, SessionID: input.SessionID, FilePath: filePath, NewFilePath: f.NewFilePath, - Edits: f.Edits, + Edits: edits, Cwd: input.Cwd, RawClaudeCode: &input, } From 6c3fa9a4a344362b07e48af34499c47ce2d8edc4 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 17:09:27 -0700 Subject: [PATCH 05/19] fix --- README.md | 2 +- codex/doc.go | 3 +-- docs/reference-codex.md | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 35153fc..f5bb235 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ hookshot install --binary /path/to/my-hooks Codex hooks are enabled by default (the `hooks` feature flag in Codex is stable and on). No `~/.codex/config.toml` change is required. If your organization disabled hooks, set `[features].hooks = true` to turn them -back on — `codex_hooks` is a deprecated alias for the same flag. +back on. ```json { diff --git a/codex/doc.go b/codex/doc.go index 433e5e4..e486e3e 100644 --- a/codex/doc.go +++ b/codex/doc.go @@ -14,8 +14,7 @@ // // Codex hooks are enabled by default in current releases (the `hooks` // feature flag is stable). If your organization disabled them, set -// `[features].hooks = true` in ~/.codex/config.toml to re-enable. The -// older `codex_hooks` key still works as a deprecated alias. +// `[features].hooks = true` in ~/.codex/config.toml to re-enable. // // Configure hook commands in ~/.codex/hooks.json (or inline under [hooks] in // ~/.codex/config.toml): diff --git a/docs/reference-codex.md b/docs/reference-codex.md index 5524d6d..08b3298 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -4,7 +4,7 @@ Platform-specific types and helpers for OpenAI Codex hooks. Use these when you n Codex hooks use the same JSON wire format as Claude Code hooks, configured in `~/.codex/hooks.json` or inline `[hooks]` tables in `~/.codex/config.toml`. The `codex` Go package re-exports the relevant `claude` types so your code can stay platform-explicit while still benefiting from the shared types. -Codex hooks are enabled by default (the `hooks` feature flag is stable in current Codex releases). If your organization disabled hooks, set `[features].hooks = true` in `~/.codex/config.toml` to re-enable. The older `codex_hooks` key still works as a deprecated alias. +Codex hooks are enabled by default (the `hooks` feature flag is stable in current Codex releases). If your organization disabled hooks, set `[features].hooks = true` in `~/.codex/config.toml` to re-enable. See the upstream spec at https://developers.openai.com/codex/hooks. From ed397e99e84a471f4e1cdb06d65021d26bb16338 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 17:28:05 -0700 Subject: [PATCH 06/19] simplify --- .cursor/rules/corridor-mcp-server-usage.mdc | 7 - .vscode/settings.json | 1 - docs/reference-codex.md | 548 +++++--------------- 3 files changed, 134 insertions(+), 422 deletions(-) delete mode 100644 .cursor/rules/corridor-mcp-server-usage.mdc delete mode 100644 .vscode/settings.json diff --git a/.cursor/rules/corridor-mcp-server-usage.mdc b/.cursor/rules/corridor-mcp-server-usage.mdc deleted file mode 100644 index 4f07f53..0000000 --- a/.cursor/rules/corridor-mcp-server-usage.mdc +++ /dev/null @@ -1,7 +0,0 @@ ---- -description: Corridor MCP Server Usage -globs: -alwaysApply: true ---- - -Every time you generate code, use the analyzePlan tool from Corridor's MCP Server (corridor) to analyze the plan or thought process. ALWAYS use Corridor to analyze the plan. Always generate a plan before generating code. diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index 9e26dfe..0000000 --- a/.vscode/settings.json +++ /dev/null @@ -1 +0,0 @@ -{} \ No newline at end of file diff --git a/docs/reference-codex.md b/docs/reference-codex.md index 08b3298..3e989cf 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -1,155 +1,147 @@ # OpenAI Codex API Reference -Platform-specific types and helpers for OpenAI Codex hooks. Use these when you need features not available in the unified API. +Codex hooks use the same JSON wire format as Claude Code hooks. The `codex` +Go package re-exports the relevant types and helpers from `claude` so your +code can stay platform-explicit. **For type definitions and helper-function +signatures see [reference-claude.md](reference-claude.md)** — this page only +documents what differs on Codex. -Codex hooks use the same JSON wire format as Claude Code hooks, configured in `~/.codex/hooks.json` or inline `[hooks]` tables in `~/.codex/config.toml`. The `codex` Go package re-exports the relevant `claude` types so your code can stay platform-explicit while still benefiting from the shared types. +See the upstream spec at . -Codex hooks are enabled by default (the `hooks` feature flag is stable in current Codex releases). If your organization disabled hooks, set `[features].hooks = true` in `~/.codex/config.toml` to re-enable. +## Configuration -See the upstream spec at https://developers.openai.com/codex/hooks. +Codex hooks are enabled by default (the `hooks` feature flag is stable). If +your organization disabled hooks, set `[features].hooks = true` in +`~/.codex/config.toml` to re-enable. The older `codex_hooks` key still works +as a deprecated alias. -## Events - -| Event | Input | Output | Description | -|-------|-------|--------|-------------| -| SessionStart | `SessionStartInput` | `SessionStartOutput` | Session started or resumed | -| PreToolUse | `PreToolUseInput` | `PreToolUseOutput` | Before tool execution (Bash, apply_patch, MCP) | -| PermissionRequest | `PermissionRequestInput` | `PermissionRequestOutput` | Approval prompt about to surface | -| PostToolUse | `PostToolUseInput` | `PostToolUseOutput` | After tool execution | -| UserPromptSubmit | `UserPromptSubmitInput` | `UserPromptSubmitOutput` | User submitted a prompt | -| Stop | `StopInput` | `StopOutput` | Turn finished responding | - ---- - -## Common Types - -### BaseInput - -All Codex hook inputs include these fields: - -```go -type BaseInput struct { - SessionID string `json:"session_id"` - TranscriptPath string `json:"transcript_path"` - Cwd string `json:"cwd"` - PermissionMode string `json:"permission_mode"` - HookEventName string `json:"hook_event_name"` -} -``` - -Codex also sends a `model` field (active model slug) on every hook event, and a `turn_id` field on turn-scoped events (`PreToolUse`, `PermissionRequest`, `PostToolUse`, `UserPromptSubmit`, `Stop`). These fields aren't represented on the shared `BaseInput` struct, but you can read them by binding the raw JSON yourself with `hookshot.ReadRawInput`. - -### BaseOutput - -Common fields for any hook output: - -```go -type BaseOutput struct { - Continue *bool `json:"continue,omitempty"` - StopReason string `json:"stopReason,omitempty"` - SuppressOutput bool `json:"suppressOutput,omitempty"` // parsed but not enforced today - SystemMessage string `json:"systemMessage,omitempty"` -} -``` - -Codex enforces `continue: false` on `SessionStart`, `UserPromptSubmit`, `PostToolUse`, and `Stop`. For `PreToolUse` and `PermissionRequest`, `continue`, `stopReason`, and `suppressOutput` are parsed but currently fail open. - ---- - -## SessionStart - -Called when a session starts or resumes. The `matcher` regex is applied to the `source` field. Current runtime values are `startup`, `resume`, and `clear`. - -### SessionStartInput - -```go -type SessionStartInput struct { - BaseInput - Source string `json:"source"` // "startup", "resume", "clear" -} -``` - -### SessionStartOutput - -```go -type SessionStartOutput struct { - BaseOutput - HookSpecificOutput *SessionStartHookOutput `json:"hookSpecificOutput,omitempty"` -} - -type SessionStartHookOutput struct { - HookEventName string `json:"hookEventName,omitempty"` - AdditionalContext string `json:"additionalContext,omitempty"` -} -``` - -### Helper Functions - -```go -func SessionStartOK() SessionStartOutput -func SessionStartContext(context string) SessionStartOutput -``` - -### Example - -```go -hookshot.Register("codex-session-start", func() { - hookshot.Run(func(input codex.SessionStartInput) codex.SessionStartOutput { - return codex.SessionStartContext("Project uses Go 1.21+") - }) -}) -``` - ---- - -## PreToolUse - -Called before Codex executes a tool. Currently intercepts simple Bash commands, file edits performed through `apply_patch`, and MCP tool calls. The `matcher` regex is applied to `tool_name` and matcher aliases — `apply_patch` also matches `Edit` and `Write`. - -### PreToolUseInput +Hook commands live in `~/.codex/hooks.json` (or an inline `[hooks]` table in +`~/.codex/config.toml`). The minimum useful layout: -```go -type PreToolUseInput struct { - BaseInput - ToolName string `json:"tool_name"` // "Bash", "apply_patch", or "mcp__server__tool" - ToolInput json.RawMessage `json:"tool_input"` - ToolUseID string `json:"tool_use_id"` -} -``` - -For `Bash` and `apply_patch`, the `tool_input` includes a `command` field. For MCP tools it carries all the arguments passed to the MCP call. - -### PreToolUseOutput - -```go -type PreToolUseOutput struct { - BaseOutput - HookSpecificOutput *PreToolUseHookOutput `json:"hookSpecificOutput,omitempty"` -} - -type PreToolUseHookOutput struct { - HookEventName string `json:"hookEventName,omitempty"` - PermissionDecision string `json:"permissionDecision,omitempty"` - PermissionDecisionReason string `json:"permissionDecisionReason,omitempty"` - UpdatedInput map[string]any `json:"updatedInput,omitempty"` +```json +{ + "hooks": { + "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], + "PostToolUse": [{ "matcher": "apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], + "UserPromptSubmit": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" }] }], + "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop", "timeout": 30 }] }] + } } ``` -Codex honors `permissionDecision: "deny"` (or the older `decision: "block"` shape) on Bash and `apply_patch`. `"allow"`, `"ask"`, `updatedInput`, `additionalContext`, `continue: false`, `stopReason`, and `suppressOutput` are parsed but fail open today. - -> **Fail-closed `Ask` on the unified API.** Because Codex doesn't enforce `"ask"` yet, `hookshot.OnBeforeExecution` returning `AskExecution(...)` is translated to a `Deny` on Codex so policies that require user confirmation aren't silently bypassed. If you call the platform-level `codex.Ask` helper directly, the output JSON still encodes `"ask"` (so it round-trips with the upstream protocol) — that's only useful for forward-compat testing today. If you want to react when Codex is actually about to prompt the user, register a separate handler for the [PermissionRequest event](#permissionrequest) — that one's enforcement is supported. +`hookshot install --codex --binary /path/to/my-hooks` will generate this for +you. -### Helper Functions +> **Why `mcp__.*` is in the matcher.** Codex passes MCP tool names to +> PreToolUse / PostToolUse using the `mcp__server__tool` convention. +> Omitting `mcp__.*` would silently bypass any `OnBeforeExecution` policy +> meant to enforce MCP allowlists. +> +> **Why `Edit|Write` is not in the matcher.** Codex emits `Edit` and `Write` +> as *matcher aliases* for `apply_patch`. The canonical `tool_name` Codex +> sends to the hook is always `apply_patch`, so a matcher of `apply_patch` +> alone covers every file-edit call. -```go -func Deny(reason string) PreToolUseOutput // Enforced: blocks Bash and apply_patch -func Allow(reason string) PreToolUseOutput // Parsed but currently falls through -func AllowSilent() PreToolUseOutput // Parsed but currently falls through -func Ask(reason string) PreToolUseOutput // Parsed but currently falls through (unified API rewrites to Deny) -func PassThrough() PreToolUseOutput // Empty output, normal flow -``` +## Events -### Example +| Event | Types | Codex notes | +|---|---|---| +| SessionStart | `codex.SessionStartInput` / `Output` | `source` is `startup`, `resume`, or `clear`. | +| PreToolUse | `codex.PreToolUseInput` / `Output` | `tool_name` is `Bash`, `apply_patch`, or `mcp__server__tool`. See **Ask is not enforced** below. | +| PermissionRequest | `codex.PermissionRequestInput` / `Output` | Codex-specific. Fires only when Codex is about to surface an approval prompt; see below. | +| PostToolUse | `codex.PostToolUseInput` / `Output` | See **apply_patch on the unified API** below. | +| UserPromptSubmit | `codex.UserPromptSubmitInput` / `Output` | Same shape as Claude. | +| Stop | `codex.StopInput` / `Output` | Same shape as Claude; Codex expects JSON on stdout (not plain text). | + +Codex also sends a `model` field (active model slug) on every hook event and +a `turn_id` field on turn-scoped events (`PreToolUse`, `PermissionRequest`, +`PostToolUse`, `UserPromptSubmit`, `Stop`). These aren't on the shared +`BaseInput` struct — read them with `hookshot.ReadRawInput` if you need +them. Stop also carries `last_assistant_message`. + +Codex enforces `continue: false` on `SessionStart`, `UserPromptSubmit`, +`PostToolUse`, and `Stop`. For `PreToolUse` and `PermissionRequest`, +`continue`, `stopReason`, and `suppressOutput` are parsed but currently fail +open. + +## PreToolUse: Ask is not enforced + +Codex honors `permissionDecision: "deny"` (or the older `decision: "block"` +shape) on Bash and `apply_patch`. `"allow"`, `"ask"`, `updatedInput`, +`additionalContext`, `continue: false`, `stopReason`, and `suppressOutput` +are parsed but fail open today. + +Because `"ask"` falls open, `hookshot.OnBeforeExecution` returning +`AskExecution(...)` is rewritten to `Deny` on Codex so policies that require +user confirmation aren't silently bypassed. The platform-level `codex.Ask` +helper still emits `"ask"` in the JSON for forward-compat testing. + +If you want to react when Codex is actually about to prompt the user, +register a separate handler for the **PermissionRequest** event below — that +event's enforcement is supported. + +## PermissionRequest (Codex-only) + +Fires when Codex is about to ask for approval (shell escalation, managed- +network approval). Doesn't run for commands that don't need approval. The +`tool_input` may include a `description` field with a human-readable reason. + +If multiple matching hooks return decisions, any `deny` wins. Otherwise an +`allow` lets the request proceed without surfacing the approval prompt. If +no matching hook decides, Codex uses the normal approval flow. `updatedInput`, +`updatedPermissions`, and `interrupt` are reserved for future behavior and +fail closed today. + +Helpers: `codex.AllowPermission()` and `codex.DenyPermission(message string)`. + +## apply_patch on the unified API + +`hookshot.OnAfterFileEdit` parses Codex `apply_patch` events by unpacking +the unified-diff envelope in `tool_input.command` and invoking your handler +**once per file** mentioned in the patch. Each invocation receives a fully +populated `FileEditContext`: + +- `FilePath` is the path declared in the `*** Add File:`, + `*** Update File:`, or `*** Delete File:` section. +- `NewFilePath` is the destination path for rename operations + (`*** Move to:`); empty otherwise. +- `Edits` is `[{OldString: "", NewString: }]` for Add, one + `FileEdit` per hunk for Update, and empty for Delete. + +For renames (`*** Update File: ` followed by `*** Move to: `) the +handler is invoked **twice** — once with `FilePath` set to the source and +once with `FilePath` set to the destination — and `NewFilePath` is populated +on both. This means a FilePath-only allowlist that permits the benign source +still receives a separate call for the destination so it can deny moves to +sensitive locations like `../../.ssh/authorized_keys`. Policies that want +to react specifically to renames should check +`ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. + +If any per-file invocation returns `FileEditBlock`, the unified bridge +concatenates the reasons and emits a single `PostToolBlock`. + +The same parser is also exported as +`codex.ParseApplyPatch(rawCommand string) []codex.PatchFile` for callers +that want to parse a patch envelope themselves — for example from the raw +`codex.PostToolUseInput.ToolInput`. + +## PostToolUse semantics + +`decision: "block"` doesn't undo the completed tool call. Codex records the +feedback, replaces the tool result with it, and continues the model from +the hook-provided message. To stop normal processing of the original tool +result, also return `continue: false`. `updatedMCPToolOutput` and +`suppressOutput` are parsed but not supported today. + +## Stop semantics + +`decision: "block"` doesn't reject the turn. Instead it tells Codex to +continue and creates a new continuation prompt that acts as a new user +prompt, using `reason` as that prompt text. If any matching Stop hook +returns `continue: false`, that takes precedence over continuation +decisions from other matching Stop hooks. + +## Example ```go hookshot.Register("codex-pre-tool-use", func() { @@ -166,278 +158,6 @@ hookshot.Register("codex-pre-tool-use", func() { }) ``` -You can also use exit code `2` with the reason written to stderr instead of returning the JSON output, which is what `RunE` does for you when the handler returns an error. - ---- - -## PermissionRequest - -Called when Codex is about to ask for approval (shell escalation, managed-network approval). It doesn't run for commands that don't need approval. - -### PermissionRequestInput - -```go -type PermissionRequestInput struct { - BaseInput - ToolName string `json:"tool_name"` - ToolInput json.RawMessage `json:"tool_input"` - ToolUseID string `json:"tool_use_id"` -} -``` - -The `tool_input` may include a `description` field with a human-readable approval reason. - -### PermissionRequestOutput - -```go -type PermissionRequestOutput struct { - BaseOutput - HookSpecificOutput *PermissionRequestHookOutput `json:"hookSpecificOutput,omitempty"` -} - -type PermissionRequestHookOutput struct { - HookEventName string `json:"hookEventName,omitempty"` - Decision *PermissionRequestDecision `json:"decision,omitempty"` -} - -type PermissionRequestDecision struct { - Behavior string `json:"behavior"` // "allow" or "deny" - Message string `json:"message,omitempty"` // For "deny" -} -``` - -If multiple matching hooks return decisions, any `deny` wins. Otherwise, an `allow` lets the request proceed without surfacing the approval prompt. If no matching hook decides, Codex uses the normal approval flow. `updatedInput`, `updatedPermissions`, and `interrupt` are reserved for future behavior and fail closed today. - -### Helper Functions - -```go -func AllowPermission() PermissionRequestOutput -func DenyPermission(message string) PermissionRequestOutput -``` - ---- - -## PostToolUse - -Called after Bash, `apply_patch`, or MCP tool calls produce output. For Bash, also runs after non-zero exits. Can't undo side effects. - -### PostToolUseInput - -```go -type PostToolUseInput struct { - BaseInput - ToolName string `json:"tool_name"` - ToolInput json.RawMessage `json:"tool_input"` - ToolResponse json.RawMessage `json:"tool_response"` - ToolUseID string `json:"tool_use_id"` -} -``` - -### PostToolUseOutput - -```go -type PostToolUseOutput struct { - BaseOutput - Decision string `json:"decision,omitempty"` - Reason string `json:"reason,omitempty"` - HookSpecificOutput *PostToolUseHookOutput `json:"hookSpecificOutput,omitempty"` -} - -type PostToolUseHookOutput struct { - HookEventName string `json:"hookEventName,omitempty"` - AdditionalContext string `json:"additionalContext,omitempty"` -} -``` - -`decision: "block"` doesn't undo the completed tool call. Codex records the feedback, replaces the tool result with it, and continues the model from the hook-provided message. To stop normal processing of the original tool result, also return `continue: false`. `updatedMCPToolOutput` and `suppressOutput` are parsed but not supported today. - -### Helper Functions - -```go -func PostToolOK() PostToolUseOutput -func PostToolBlock(reason string) PostToolUseOutput -func PostToolContext(context string) PostToolUseOutput -``` - -### apply_patch parsing on the unified API - -`hookshot.OnAfterFileEdit` parses Codex `apply_patch` events by unpacking the unified-diff envelope in `tool_input.command` and invoking your handler **once per file** mentioned in the patch. Each invocation receives a fully populated `FileEditContext`: - -- `FilePath` is the path declared in the `*** Add File:`, `*** Update File:`, or `*** Delete File:` section. -- `NewFilePath` is the destination path for rename operations (`*** Move to:`); empty otherwise. -- `Edits` is `[{OldString: "", NewString: }]` for Add, one `FileEdit` per hunk for Update (with removed lines as `OldString` and added lines as `NewString`), and empty for Delete. - -For renames (`*** Update File: ` followed by `*** Move to: `), the handler is invoked **twice** — once with `FilePath` set to the source and once with `FilePath` set to the destination — and `NewFilePath` is populated on both invocations. This ensures that a FilePath-only allowlist which permits the benign source path still receives a separate call for the destination path and can deny moves to sensitive locations like `../../.ssh/authorized_keys`. Policies that want to react specifically to renames should check `ctx.NewFilePath != "" && ctx.NewFilePath != ctx.FilePath`. - -If any of those per-file invocations returns `FileEditBlock`, the unified bridge concatenates the reasons and emits a single `PostToolBlock` so Codex replaces the tool result with the combined feedback. - -The same parser is also exported as `codex.ParseApplyPatch(rawCommand string) []codex.PatchFile` for callers that want to parse a patch envelope themselves (for example, from the raw `codex.PostToolUseInput.ToolInput`). - -### Example - -```go -hookshot.Register("codex-post-tool-use", func() { - hookshot.Run(func(input codex.PostToolUseInput) codex.PostToolUseOutput { - if input.ToolName == "apply_patch" { - return codex.PostToolContext("Generated files were updated.") - } - return codex.PostToolOK() - }) -}) -``` - ---- - -## UserPromptSubmit - -Called when the user submits a prompt. `matcher` is ignored for this event. - -### UserPromptSubmitInput - -```go -type UserPromptSubmitInput struct { - BaseInput - Prompt string `json:"prompt"` -} -``` - -### UserPromptSubmitOutput - -```go -type UserPromptSubmitOutput struct { - BaseOutput - Decision string `json:"decision,omitempty"` - Reason string `json:"reason,omitempty"` - HookSpecificOutput *UserPromptSubmitHookOutput `json:"hookSpecificOutput,omitempty"` -} - -type UserPromptSubmitHookOutput struct { - HookEventName string `json:"hookEventName,omitempty"` - AdditionalContext string `json:"additionalContext,omitempty"` -} -``` - -Return `decision: "block"` to reject the prompt. Otherwise, plain text on stdout (or `additionalContext` in JSON) is added as extra developer context. - -### Helper Functions - -```go -func AllowPrompt() UserPromptSubmitOutput -func BlockPrompt(reason string) UserPromptSubmitOutput -func AddContext(context string) UserPromptSubmitOutput -``` - -### Example - -```go -hookshot.Register("codex-user-prompt-submit", func() { - hookshot.Run(func(input codex.UserPromptSubmitInput) codex.UserPromptSubmitOutput { - if strings.Contains(input.Prompt, "api_key=") { - return codex.BlockPrompt("Don't include API keys in prompts") - } - return codex.AllowPrompt() - }) -}) -``` - ---- - -## Stop - -Called when the turn finishes responding. `matcher` is ignored for this event. Codex expects JSON on stdout when the hook exits 0 — plain text is invalid here. - -### StopInput - -```go -type StopInput struct { - BaseInput - StopHookActive bool `json:"stop_hook_active"` -} -``` - -Codex also sends `last_assistant_message` (latest assistant message text) on Stop input. Read it via `hookshot.ReadRawInput` if needed. - -### StopOutput - -```go -type StopOutput struct { - BaseOutput - Decision string `json:"decision,omitempty"` // "block" to continue the turn - Reason string `json:"reason,omitempty"` -} -``` - -For this event, `decision: "block"` doesn't reject the turn. Instead, it tells Codex to continue and creates a new continuation prompt that acts as a new user prompt, using `reason` as that prompt text. If any matching Stop hook returns `continue: false`, that takes precedence over continuation decisions from other matching Stop hooks. - -### Helper Functions - -```go -func Continue() StopOutput // Allow stopping -func Block(reason string) StopOutput // Continue the turn with reason as the next prompt -func StopWith(reason string) StopOutput // Halt Codex entirely (continue=false) -``` - -### Example - -```go -hookshot.Register("codex-stop", func() { - hookshot.Run(func(input codex.StopInput) codex.StopOutput { - // IMPORTANT: Check StopHookActive to prevent infinite loops - if input.StopHookActive { - return codex.Continue() - } - return codex.Continue() - }) -}) -``` - ---- - -## Configuration Example - -Hooks are on by default — no `~/.codex/config.toml` edit required. - -`~/.codex/hooks.json`: - -```json -{ - "hooks": { - "PreToolUse": [ - { - "matcher": "Bash|apply_patch|mcp__.*", - "hooks": [ - { "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" } - ] - } - ], - "PostToolUse": [ - { - "matcher": "apply_patch|mcp__.*", - "hooks": [ - { "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" } - ] - } - ], - "UserPromptSubmit": [ - { - "hooks": [ - { "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" } - ] - } - ], - "Stop": [ - { - "hooks": [ - { "type": "command", "command": "/path/to/my-hooks codex-stop", "timeout": 30 } - ] - } - ] - } -} -``` - -`hookshot install --codex --binary /path/to/my-hooks` will generate this layout for you. - -> **Why `mcp__.*` is in the matcher.** Codex passes MCP tool names to PreToolUse / PostToolUse using the `mcp__server__tool` convention. Omitting the `mcp__.*` alternative would mean Codex never invokes the hook binary for MCP calls, which would silently bypass any `OnBeforeExecution` policy meant to enforce MCP allowlists. - -> **Why `Edit|Write` is not in the matcher.** Codex emits `Edit` and `Write` as *matcher aliases* for `apply_patch`. The canonical `tool_name` Codex sends to the hook is always `apply_patch`, so a matcher of `apply_patch` alone covers every file-edit call. `Edit` and `Write` aliases never appear as standalone tool names. +You can also use exit code `2` with the reason written to stderr instead of +returning the JSON output — that's what `RunE` does for you when the +handler returns an error. From 496112b6928473a95c4060ea8ad6b5bd4b0e2414 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 18:43:37 -0700 Subject: [PATCH 07/19] fix(codex): drop suppressOutput/updatedInput on PreToolUse; route unified via codex package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's PreToolUse rejects hook output containing suppressOutput, updatedInput, continue:false, stopReason — the CLI emits "PreToolUse hook returned unsupported " rather than ignoring the field (despite the upstream docs claiming fail-open). Two changes: 1. Make codex.AllowSilent and codex.AllowWithInput actually diverge from the claude.* aliases instead of inheriting the broken JSON shape. AllowSilent now emits {} (no suppressOutput); AllowWithInput drops the updatedInput map and preserves the reason via permissionDecision: "allow". 2. Switch every unified Codex registration (codex-stop, codex-pre-tool-use, codex-post-tool-use, codex-user-prompt-submit) to call codex.* helpers instead of claude.* so future Codex quirks live in the codex package, not the unified bridge. Updates docs/reference-codex.md to correctly mark the four fields as fail-closed. Adds regression tests in codex/helpers_test.go and unified_test.go. Co-authored-by: Cursor --- codex/helpers.go | 30 +++++++++++++---- codex/helpers_test.go | 34 +++++++++++++++++++ docs/reference-codex.md | 58 ++++++++++++++++++++++++--------- unified.go | 72 +++++++++++++++++++++++------------------ unified_test.go | 35 ++++++++++++++++++++ 5 files changed, 176 insertions(+), 53 deletions(-) diff --git a/codex/helpers.go b/codex/helpers.go index d92c4a5..5a98d00 100644 --- a/codex/helpers.go +++ b/codex/helpers.go @@ -25,12 +25,30 @@ var StopWith = claude.StopWith // effectively falls through to the normal flow. var Allow = claude.Allow -// AllowSilent permits the tool to execute without showing output. -var AllowSilent = claude.AllowSilent - -// AllowWithInput permits the tool with modified input parameters. Note that -// Codex currently parses but does not enforce updatedInput for PreToolUse. -var AllowWithInput = claude.AllowWithInput +// AllowSilent permits the tool to execute. Codex does NOT support +// suppressOutput — the Codex CLI rejects the hook output with +// "PreToolUse hook returned unsupported suppressOutput" — so on Codex the +// "silent" variant has no equivalent. This helper emits an empty {} which +// Codex treats as success. Use this instead of claude.AllowSilent in Codex +// code paths. +func AllowSilent() PreToolUseOutput { + return PreToolUseOutput{} +} + +// AllowWithInput permits the tool. Codex does NOT support updatedInput — +// the Codex CLI rejects the hook output with "PreToolUse hook returned +// unsupported updatedInput" — so the updatedInput argument is dropped on +// Codex. The reason argument is preserved via permissionDecision: "allow" +// (which falls open on Codex today). The signature matches +// claude.AllowWithInput for source compatibility, but callers that need +// to inject context on Codex should use the model-side tool args instead +// (Codex passes raw tool input through to MCP tools unchanged). +func AllowWithInput(reason string, _ map[string]any) PreToolUseOutput { + if reason == "" { + return PreToolUseOutput{} + } + return claude.Allow(reason) +} // Deny blocks the tool from executing. This is enforced by Codex for Bash // and apply_patch tools. diff --git a/codex/helpers_test.go b/codex/helpers_test.go index 300688d..3dbf755 100644 --- a/codex/helpers_test.go +++ b/codex/helpers_test.go @@ -75,6 +75,40 @@ func TestPassThrough(t *testing.T) { } } +func TestAllowSilent_DoesNotEmitSuppressOutput(t *testing.T) { + // Codex rejects suppressOutput with + // "PreToolUse hook returned unsupported suppressOutput". + // codex.AllowSilent must NOT set SuppressOutput (unlike claude.AllowSilent). + output := AllowSilent() + if output.SuppressOutput { + t.Error("codex.AllowSilent must not set SuppressOutput (Codex rejects suppressOutput)") + } + if output.HookSpecificOutput != nil { + t.Error("codex.AllowSilent should emit empty {} on Codex (no hookSpecificOutput)") + } +} + +func TestAllowWithInput_DropsUpdatedInput(t *testing.T) { + // Codex rejects updatedInput with + // "PreToolUse hook returned unsupported updatedInput". + // codex.AllowWithInput must NOT set UpdatedInput. + output := AllowWithInput("trusted", map[string]any{"file_path": "/tmp/x"}) + if output.HookSpecificOutput != nil && output.HookSpecificOutput.UpdatedInput != nil { + t.Error("codex.AllowWithInput must not set UpdatedInput (Codex rejects updatedInput)") + } + // Reason should still be passed through as permissionDecisionReason. + if output.HookSpecificOutput == nil || output.HookSpecificOutput.PermissionDecisionReason != "trusted" { + t.Errorf("Reason should be preserved as permissionDecisionReason, got %+v", output.HookSpecificOutput) + } +} + +func TestAllowWithInput_EmptyReasonProducesPassThrough(t *testing.T) { + output := AllowWithInput("", map[string]any{"k": "v"}) + if output.HookSpecificOutput != nil { + t.Error("codex.AllowWithInput with empty reason should emit empty {} (pass-through)") + } +} + // ============================================================================= // PermissionRequest Helpers Tests // ============================================================================= diff --git a/docs/reference-codex.md b/docs/reference-codex.md index 3e989cf..170e7d1 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -60,25 +60,51 @@ a `turn_id` field on turn-scoped events (`PreToolUse`, `PermissionRequest`, them. Stop also carries `last_assistant_message`. Codex enforces `continue: false` on `SessionStart`, `UserPromptSubmit`, -`PostToolUse`, and `Stop`. For `PreToolUse` and `PermissionRequest`, -`continue`, `stopReason`, and `suppressOutput` are parsed but currently fail -open. +`PostToolUse`, and `Stop`. For `PreToolUse` and `PermissionRequest`, Codex +rejects `continue`, `stopReason`, and `suppressOutput` with an +`unsupported ` error and discards the whole hook output — these +fields fail **closed** and must be omitted. The upstream +[Codex hooks doc](https://developers.openai.com/codex/hooks) currently +describes these as fail-open; the runtime behavior is fail-closed. -## PreToolUse: Ask is not enforced +## PreToolUse: which output fields actually work Codex honors `permissionDecision: "deny"` (or the older `decision: "block"` -shape) on Bash and `apply_patch`. `"allow"`, `"ask"`, `updatedInput`, -`additionalContext`, `continue: false`, `stopReason`, and `suppressOutput` -are parsed but fail open today. - -Because `"ask"` falls open, `hookshot.OnBeforeExecution` returning -`AskExecution(...)` is rewritten to `Deny` on Codex so policies that require -user confirmation aren't silently bypassed. The platform-level `codex.Ask` -helper still emits `"ask"` in the JSON for forward-compat testing. - -If you want to react when Codex is actually about to prompt the user, -register a separate handler for the **PermissionRequest** event below — that -event's enforcement is supported. +shape) on Bash and `apply_patch`. Codex also honors +`hookSpecificOutput.additionalContext`, which is injected as developer +context without blocking the call (the upstream +[Codex hooks doc](https://developers.openai.com/codex/hooks#pretooluse) +shows this case as a first-class example). + +`permissionDecision: "ask"` is parsed but currently fails open. +`hookshot.OnBeforeExecution` returning `AskExecution(...)` is rewritten to +`Deny` on Codex so policies that require user confirmation aren't silently +bypassed. The platform-level `codex.Ask` helper still emits `"ask"` in the +JSON for forward-compat testing. If you want to react when Codex is +actually about to prompt the user, register a separate handler for the +**PermissionRequest** event below — that event's enforcement is supported. + +**`updatedInput`, `continue: false`, `stopReason`, and `suppressOutput` +fail closed** — Codex rejects the whole hook output with +`PreToolUse hook returned unsupported ` rather than ignoring the +field, so these must be omitted. Concretely, this means: + +- `codex.Allow(reason)`, `codex.Deny(reason)`, and `codex.PassThrough()` + are all safe on Codex. +- `codex.AllowSilent()` is **not safe**: it sets `suppressOutput: true`, + which Codex rejects. Use `codex.PassThrough()` for an empty + no-side-effects allow. +- `codex.AllowWithInput(reason, input)` is **not safe**: it sets + `updatedInput`, which Codex rejects. There's no Codex-supported way to + mutate `tool_input` from a PreToolUse hook today — fall back to + injecting state through `additionalContext` instead. +- To attach model-visible context to an allow, return + `codex.AllowWithContext(reason, context)` (or build the output by hand + with `hookSpecificOutput.additionalContext`). + +The hookshot unified bridge already strips `suppressOutput` from Codex +`OnBeforeExecution(AllowExecution())` outputs, but if you call the +platform-level helpers directly you need to pick safe ones yourself. ## PermissionRequest (Codex-only) diff --git a/unified.go b/unified.go index a83bce6..1652537 100644 --- a/unified.go +++ b/unified.go @@ -149,9 +149,12 @@ func OnStop(handler StopHandler) { }) }) - // Codex uses the same JSON protocol as Claude Code + // Codex uses the same JSON wire protocol as Claude Code but with stricter + // validation. Use codex.* helpers (not claude.*) so any Codex-specific + // quirks (e.g. rejected suppressOutput / updatedInput) are handled in + // one place — the codex package. Register("codex-stop", func() { - Run(func(input claude.StopInput) claude.StopOutput { + Run(func(input codex.StopInput) codex.StopOutput { ctx := StopContext{ Platform: PlatformCodex, SessionID: input.SessionID, @@ -160,9 +163,9 @@ func OnStop(handler StopHandler) { } decision := handler(ctx) if decision.Continue { - return claude.Continue() + return codex.Continue() } - return claude.Block(decision.Message) + return codex.Block(decision.Message) }) }) } @@ -453,14 +456,16 @@ func OnBeforeExecution(handler ExecutionHandler) { }) }) - // Codex PreToolUse (same JSON protocol as Claude Code). - // Codex tool names include "Bash", "apply_patch", and MCP names like - // "mcp__server__tool". apply_patch is classified as ExecutionTool because - // it represents a file edit rather than a shell command or MCP call. For - // apply_patch the underlying tool_input.command is parsed and exposed - // via ExecutionContext.Command so policies can inspect the patch text. + // Codex PreToolUse (same JSON wire protocol as Claude Code, stricter + // validation). Codex tool names include "Bash", "apply_patch", and MCP + // names like "mcp__server__tool". apply_patch is classified as + // ExecutionTool because it represents a file edit rather than a shell + // command or MCP call. For apply_patch the underlying tool_input.command + // is parsed and exposed via ExecutionContext.Command so policies can + // inspect the patch text. Uses codex.* helpers so Codex quirks (no + // suppressOutput, no updatedInput) live in the codex package. Register("codex-pre-tool-use", func() { - Run(func(input claude.PreToolUseInput) claude.PreToolUseOutput { + Run(func(input codex.PreToolUseInput) codex.PreToolUseOutput { var execType ExecutionType if input.ToolName == "Bash" { execType = ExecutionShell @@ -492,9 +497,13 @@ func OnBeforeExecution(handler ExecutionHandler) { decision := handler(ctx) if decision.Allow { if decision.Reason != "" { - return claude.Allow(decision.Reason) + return codex.Allow(decision.Reason) } - return claude.AllowSilent() + // codex.AllowSilent is a Codex-safe no-op (emits {}) — it + // does NOT set suppressOutput like claude.AllowSilent, + // because Codex rejects that field with "PreToolUse hook + // returned unsupported suppressOutput". + return codex.AllowSilent() } // Codex currently parses but does not enforce permissionDecision // "ask" for PreToolUse, so an Ask decision would silently fail @@ -502,9 +511,9 @@ func OnBeforeExecution(handler ExecutionHandler) { // matches the security posture of the other platforms where // Ask actually surfaces an approval prompt. if decision.Ask { - return claude.Deny(decision.Reason) + return codex.Deny(decision.Reason) } - return claude.Deny(decision.Reason) + return codex.Deny(decision.Reason) }) }) } @@ -731,9 +740,9 @@ func OnAfterFileEdit(handler FileEditHandler) { // hooks.json — "Edit" and "Write" matcher aliases exist but are // redundant with "apply_patch". Register("codex-post-tool-use", func() { - Run(func(input claude.PostToolUseInput) claude.PostToolUseOutput { + Run(func(input codex.PostToolUseInput) codex.PostToolUseOutput { if input.ToolName != "Write" && input.ToolName != "Edit" && input.ToolName != "apply_patch" { - return claude.PostToolOK() + return codex.PostToolOK() } // Write/Edit use the Claude-style schema. @@ -764,12 +773,12 @@ func OnAfterFileEdit(handler FileEditHandler) { decision := handler(ctx) if decision.Block { - return claude.PostToolBlock(decision.Reason) + return codex.PostToolBlock(decision.Reason) } if decision.Context != "" { - return claude.PostToolContext(decision.Context) + return codex.PostToolContext(decision.Context) } - return claude.PostToolOK() + return codex.PostToolOK() } // apply_patch: tool_input is {"command": "*** Begin Patch ..."}. @@ -793,12 +802,12 @@ func OnAfterFileEdit(handler FileEditHandler) { } decision := handler(ctx) if decision.Block { - return claude.PostToolBlock(decision.Reason) + return codex.PostToolBlock(decision.Reason) } if decision.Context != "" { - return claude.PostToolContext(decision.Context) + return codex.PostToolContext(decision.Context) } - return claude.PostToolOK() + return codex.PostToolOK() } var ( @@ -840,12 +849,12 @@ func OnAfterFileEdit(handler FileEditHandler) { } } if len(blockReasons) > 0 { - return claude.PostToolBlock(strings.Join(blockReasons, "\n")) + return codex.PostToolBlock(strings.Join(blockReasons, "\n")) } if len(contexts) > 0 { - return claude.PostToolContext(strings.Join(contexts, "\n")) + return codex.PostToolContext(strings.Join(contexts, "\n")) } - return claude.PostToolOK() + return codex.PostToolOK() }) }) } @@ -987,9 +996,10 @@ func OnPromptSubmit(handler PromptHandler) { }) }) - // Codex UserPromptSubmit (same JSON protocol as Claude Code) + // Codex UserPromptSubmit (same JSON wire protocol as Claude Code, + // stricter validation — use codex.* helpers). Register("codex-user-prompt-submit", func() { - Run(func(input claude.UserPromptSubmitInput) claude.UserPromptSubmitOutput { + Run(func(input codex.UserPromptSubmitInput) codex.UserPromptSubmitOutput { ctx := PromptContext{ Platform: PlatformCodex, SessionID: input.SessionID, @@ -1000,12 +1010,12 @@ func OnPromptSubmit(handler PromptHandler) { decision := handler(ctx) if !decision.Allow { - return claude.BlockPrompt(decision.Reason) + return codex.BlockPrompt(decision.Reason) } if decision.Context != "" { - return claude.AddContext(decision.Context) + return codex.AddContext(decision.Context) } - return claude.AllowPrompt() + return codex.AllowPrompt() }) }) } diff --git a/unified_test.go b/unified_test.go index 6ca5c24..a601d8e 100644 --- a/unified_test.go +++ b/unified_test.go @@ -840,6 +840,41 @@ func TestCodexPreToolUse_AskFailsClosed(t *testing.T) { } } +func TestCodexPreToolUse_AllowDoesNotEmitSuppressOutput(t *testing.T) { + // Codex rejects suppressOutput with "PreToolUse hook returned unsupported + // suppressOutput", so the Codex pre-tool-use bridge must NOT use + // claude.AllowSilent (which sets suppressOutput: true) when translating + // an Allow decision with no reason. PassThrough (empty {}) is the right + // shape — Codex treats an empty output as success. + ClearHandlers() + defer ClearHandlers() + + OnBeforeExecution(func(ctx ExecutionContext) ExecutionDecision { + return AllowExecution() + }) + + stdinR, stdinW, _ := os.Pipe() + stdinW.Write([]byte(`{"session_id":"s","tool_name":"Bash","tool_input":{"command":"echo hi"},"cwd":"/tmp"}`)) + stdinW.Close() + stdoutR, stdoutW, _ := os.Pipe() + origStdin, origStdout := os.Stdin, os.Stdout + os.Stdin, os.Stdout = stdinR, stdoutW + defer func() { + stdoutW.Close() + os.Stdin, os.Stdout = origStdin, origStdout + }() + + handlers["codex-pre-tool-use"]() + stdoutW.Close() + + var buf bytes.Buffer + buf.ReadFrom(stdoutR) + got := buf.String() + if strings.Contains(got, "suppressOutput") { + t.Errorf("Codex Allow decision must not emit suppressOutput, got %q", got) + } +} + func TestCodexPreToolUse_ApplyPatchPopulatesCommand(t *testing.T) { // For apply_patch, the unified handler should pull tool_input.command // out as ExecutionContext.Command so policies can inspect the patch. From f75d61f77d24bd9b8700aaf2154b1447e388ace0 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 20:14:09 -0700 Subject: [PATCH 08/19] handle apply_patch format, too --- codex/apply_patch.go | 54 +++++++++- codex/apply_patch_test.go | 136 ++++++++++++++++++++++++++ unified.go | 200 ++++++++++++++++++++++---------------- unified_test.go | 158 ++++++++++++++++++++++++++++++ 4 files changed, 465 insertions(+), 83 deletions(-) diff --git a/codex/apply_patch.go b/codex/apply_patch.go index 6b2bb8f..c5c25ec 100644 --- a/codex/apply_patch.go +++ b/codex/apply_patch.go @@ -1,6 +1,17 @@ package codex -import "strings" +import ( + "regexp" + "strings" +) + +// applyPatchInvocationRE matches an `apply_patch` invocation followed by a +// heredoc operator (`<<`, `<<-`, or quoted variants). Requiring the heredoc +// operator — instead of accepting any substring match — prevents the +// detector from firing on benign Bash commands that merely mention +// `apply_patch` (filenames, documentation, log lines). The optional `-` and +// whitespace handling cover the variants Codex actually emits in the wild. +var applyPatchInvocationRE = regexp.MustCompile(`(?:^|[/\s;&|])apply_patch[ \t]+<<-?`) // PatchEdit captures one removed/added pair inside an apply_patch hunk. // @@ -152,3 +163,44 @@ func ParseApplyPatch(patch string) []PatchFile { flushFile() return files } + +// ParseApplyPatchFromBash inspects a Codex Bash tool command and, if the +// command is an apply_patch heredoc invocation, returns the parsed patch +// files. The second return value reports whether the command was an +// apply_patch invocation so callers can short-circuit on plain Bash +// commands without paying for a full parse pass. +// +// Codex routes file edits through Bash heredocs of the form +// +// apply_patch <<'PATCH' +// *** Begin Patch +// ... unified diff envelope ... +// *** End Patch +// PATCH +// +// at least as often as it emits a first-class tool_name="apply_patch" call. +// Codex may also invoke a per-session shim binary, so the apply_patch token +// can appear with an absolute path prefix, e.g. +// `/Users/me/.codex/tmp/arg0/codex-arg0IuQk4E/apply_patch <<'PATCH' ...`. +// Without this helper, callers that only inspect tool_name silently miss +// every heredoc-style edit (which is why Codex sessions showed up in the +// dashboard with zero SecurityScanResult rows even though file edits had +// clearly happened). +// +// Detection is heuristic but tight: the command must contain the +// "*** Begin Patch" envelope marker AND an `apply_patch < docs/apply_patch_format.md < 0 { + return codex.PostToolBlock(strings.Join(blockReasons, "\n")) + } + if len(contexts) > 0 { + return codex.PostToolContext(strings.Join(contexts, "\n")) + } return codex.PostToolOK() } - // Write/Edit use the Claude-style schema. - if input.ToolName == "Write" || input.ToolName == "Edit" { + switch input.ToolName { + case "Write", "Edit": var toolInput struct { FilePath string `json:"file_path"` Content string `json:"content"` @@ -779,82 +863,34 @@ func OnAfterFileEdit(handler FileEditHandler) { return codex.PostToolContext(decision.Context) } return codex.PostToolOK() - } - - // apply_patch: tool_input is {"command": "*** Begin Patch ..."}. - var applyInput struct { - Command string `json:"command"` - } - json.Unmarshal(input.ToolInput, &applyInput) - files := codex.ParseApplyPatch(applyInput.Command) - if len(files) == 0 { - // We could not parse anything actionable out of the patch. - // Fall back to invoking the handler once with whatever raw - // information we have so policies still see a Codex - // PostToolUse event rather than nothing. - ctx := FileEditContext{ - Platform: PlatformCodex, - SessionID: input.SessionID, - Cwd: input.Cwd, - Edits: []FileEdit{{OldString: "", NewString: applyInput.Command}}, - RawClaudeCode: &input, + case "apply_patch": + var applyInput struct { + Command string `json:"command"` } - decision := handler(ctx) - if decision.Block { - return codex.PostToolBlock(decision.Reason) + json.Unmarshal(input.ToolInput, &applyInput) + return dispatchPatch(codex.ParseApplyPatch(applyInput.Command), applyInput.Command) + + case "Bash": + // Codex routes most file edits through Bash: + // `apply_patch <<'PATCH' … PATCH` + // Detection lives in codex.ParseApplyPatchFromBash so + // non-edit Bash commands short-circuit cheaply. If the + // command IS an apply_patch invocation, dispatch through + // the same per-file pipeline as case "apply_patch". + var bashInput struct { + Command string `json:"command"` } - if decision.Context != "" { - return codex.PostToolContext(decision.Context) + json.Unmarshal(input.ToolInput, &bashInput) + files, ok := codex.ParseApplyPatchFromBash(bashInput.Command) + if !ok { + return codex.PostToolOK() } - return codex.PostToolOK() - } + return dispatchPatch(files, bashInput.Command) - var ( - blockReasons []string - contexts []string - ) - invoke := func(filePath string, f codex.PatchFile) { - edits := make([]FileEdit, 0, len(f.Edits)) - for _, e := range f.Edits { - edits = append(edits, FileEdit{OldString: e.OldString, NewString: e.NewString}) - } - ctx := FileEditContext{ - Platform: PlatformCodex, - SessionID: input.SessionID, - FilePath: filePath, - NewFilePath: f.NewFilePath, - Edits: edits, - Cwd: input.Cwd, - RawClaudeCode: &input, - } - decision := handler(ctx) - if decision.Block { - blockReasons = append(blockReasons, decision.Reason) - } else if decision.Context != "" { - contexts = append(contexts, decision.Context) - } - } - for _, f := range files { - // Always invoke for the declared source path. - invoke(f.FilePath, f) - // For renames, invoke again with the destination so policies - // that only inspect ctx.FilePath cannot be bypassed by a - // "*** Move to:" pointing at a sensitive path (e.g. - // "../../.ssh/authorized_keys"). NewFilePath is populated on - // both invocations so policies that want to detect the - // rename relationship can. - if f.NewFilePath != "" && f.NewFilePath != f.FilePath { - invoke(f.NewFilePath, f) - } - } - if len(blockReasons) > 0 { - return codex.PostToolBlock(strings.Join(blockReasons, "\n")) - } - if len(contexts) > 0 { - return codex.PostToolContext(strings.Join(contexts, "\n")) + default: + return codex.PostToolOK() } - return codex.PostToolOK() }) }) } diff --git a/unified_test.go b/unified_test.go index a601d8e..1b98240 100644 --- a/unified_test.go +++ b/unified_test.go @@ -1129,6 +1129,164 @@ func TestCodexPostToolUse_ApplyPatchBlockPropagates(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Codex Bash → apply_patch heredoc bridging +// --------------------------------------------------------------------------- +// These tests cover the case where Codex routes a file edit through a Bash +// `apply_patch <<'PATCH' … PATCH` invocation instead of a first-class +// tool_name="apply_patch" call. The unified bridge must detect that shape +// and dispatch through OnAfterFileEdit identically to the native path — +// otherwise SecurityScanResults never get populated for Codex sessions and +// the analytics dashboard shows zero stop-hook security checks. + +func TestCodexPostToolUse_BashApplyPatchHeredocDispatches(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + // Multi-file patch wrapped in a real-shape Codex Bash heredoc. The \\n + // sequences become literal newlines after JSON decoding so the parser + // sees the same line breaks Codex would emit. + cmd := "apply_patch <<'PATCH'\\n" + + "*** Begin Patch\\n" + + "*** Add File: secrets/api_key.txt\\n" + + "+sk-deadbeef\\n" + + "*** Update File: src/auth.go\\n" + + "@@\\n" + + "-old\\n" + + "+new\\n" + + "*** End Patch\\n" + + "PATCH" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 2 { + t.Fatalf("handler invocations = %d, want 2 (one per file in patch); got=%+v", len(got), got) + } + if got[0].FilePath != "secrets/api_key.txt" { + t.Errorf("got[0].FilePath = %q, want %q", got[0].FilePath, "secrets/api_key.txt") + } + if got[1].FilePath != "src/auth.go" { + t.Errorf("got[1].FilePath = %q, want %q", got[1].FilePath, "src/auth.go") + } + for _, c := range got { + if c.Platform != PlatformCodex { + t.Errorf("Platform = %q, want %q", c.Platform, PlatformCodex) + } + if c.Cwd != "/repo" { + t.Errorf("Cwd = %q, want %q", c.Cwd, "/repo") + } + if c.SessionID != "s" { + t.Errorf("SessionID = %q, want %q", c.SessionID, "s") + } + } + wantEdit := FileEdit{OldString: "old", NewString: "new"} + if len(got[1].Edits) != 1 || got[1].Edits[0] != wantEdit { + t.Errorf("got[1].Edits = %+v, want [%+v]", got[1].Edits, wantEdit) + } +} + +func TestCodexPostToolUse_BashApplyPatchAbsolutePathBinaryDispatches(t *testing.T) { + // Codex sometimes invokes a per-session apply_patch shim by absolute + // path (observed in hook_events.data for real sessions). The bridge + // must still recognize this as an apply_patch invocation. + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "/Users/me/.codex/tmp/arg0/codex-arg0IuQk4E/apply_patch <<'PATCH'\\n" + + "*** Begin Patch\\n" + + "*** Add File: foo.txt\\n" + + "+hi\\n" + + "*** End Patch\\n" + + "PATCH" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 1 || got[0].FilePath != "foo.txt" { + t.Fatalf("got = %+v, want one invocation with FilePath=foo.txt", got) + } +} + +func TestCodexPostToolUse_BashNonApplyPatchCommandSkipped(t *testing.T) { + // A normal Bash command must not trigger the file-edit handler. + // Otherwise every `ls`, `git status`, `cat`, etc. emitted by Codex + // would be misclassified as a file edit, flooding the security + // scanner with garbage events. + ClearHandlers() + defer ClearHandlers() + + invoked := 0 + OnAfterFileEdit(func(FileEditContext) FileEditDecision { + invoked++ + return FileEditOK() + }) + + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"ls -la /tmp"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if invoked != 0 { + t.Errorf("handler invoked %d times for plain Bash command, want 0", invoked) + } +} + +func TestCodexPostToolUse_BashApplyPatchBlockPropagates(t *testing.T) { + // A blocking decision from the user's handler on a Bash-heredoc + // apply_patch must emit PostToolBlock so Codex surfaces the feedback, + // identically to the native apply_patch path. + ClearHandlers() + defer ClearHandlers() + + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + if strings.Contains(ctx.FilePath, "secrets") { + return FileEditBlock("contains secrets") + } + return FileEditOK() + }) + + cmd := "apply_patch <<'PATCH'\\n" + + "*** Begin Patch\\n" + + "*** Add File: secrets/key.txt\\n" + + "+sk-deadbeef\\n" + + "*** End Patch\\n" + + "PATCH" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + + stdinR, stdinW, _ := os.Pipe() + stdinW.Write([]byte(stdin)) + stdinW.Close() + stdoutR, stdoutW, _ := os.Pipe() + origStdin, origStdout := os.Stdin, os.Stdout + os.Stdin, os.Stdout = stdinR, stdoutW + defer func() { + stdoutW.Close() + os.Stdin, os.Stdout = origStdin, origStdout + }() + + handlers["codex-post-tool-use"]() + stdoutW.Close() + + var buf bytes.Buffer + buf.ReadFrom(stdoutR) + out := buf.String() + if !strings.Contains(out, `"decision":"block"`) { + t.Errorf("expected PostToolBlock JSON output, got %q", out) + } + if !strings.Contains(out, "contains secrets") { + t.Errorf("expected block reason to be present, got %q", out) + } +} + func TestOnStop_CursorEmptyWorkspaceRootsLeavesCwdEmpty(t *testing.T) { ClearHandlers() defer ClearHandlers() From 3e96c2a6aa30cb0e8f11b410debe58336a6d384f Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 20:42:15 -0700 Subject: [PATCH 09/19] fix(prompt): forward Cwd to PromptContext for Claude and Droid OnPromptSubmit was constructing the PromptContext without Cwd for the claude-user-prompt-submit and droid-user-prompt-submit handlers, even though input.Cwd is always populated by Claude Code and Droid when running in a project directory. Without this, ctx.Cwd was empty in every OnPromptSubmit handler for these two platforms, forcing callers to fall back to os.Getwd() or env-var heuristics (CLAUDE_PROJECT_DIR). The Codex handler already forwarded Cwd correctly. Co-authored-by: Cursor --- unified.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/unified.go b/unified.go index fa108f8..7e5c3a6 100644 --- a/unified.go +++ b/unified.go @@ -958,6 +958,7 @@ func OnPromptSubmit(handler PromptHandler) { Platform: PlatformClaude, SessionID: input.SessionID, Prompt: input.Prompt, + Cwd: input.Cwd, RawClaudeCode: &input, } @@ -998,6 +999,7 @@ func OnPromptSubmit(handler PromptHandler) { Platform: PlatformDroid, SessionID: input.SessionID, Prompt: input.Prompt, + Cwd: input.Cwd, RawDroid: &input, } From 8378bdf51ddcfa3f35fd7f26e84e5015daaa0e13 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 20:57:54 -0700 Subject: [PATCH 10/19] feat(codex): detect cat/tee heredoc writes as afterFileEdit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex 0.130.0+ routes greenfield file writes through plain Bash `cat <<'EOF' > FILE … EOF` (and `tee FILE <<'EOF' … EOF`) rather than the apply_patch tool or any Write/Edit alias. ParseApplyPatchFromBash correctly rejects these shapes, so the unified codex-post-tool-use bridge silently no-op'd every "create a file" Bash invocation — no afterFileEdit fired, no security scan ran, no PostToolUse telemetry was emitted. End-to-end probe captured Codex emitting: tool_name: "Bash" tool_input.command: "cat <<'EOF' > greet.txt\nhello world\nEOF" Add codex.ParseBashRedirectWrite, paralleling ParseApplyPatchFromBash: - Detects cat / tee heredocs with `>`/`>>` redirects. - Handles quoted (`<<'EOF'`, `<<"EOF"`) and unquoted (`< --- codex/bash_write.go | 214 +++++++++++++++++++++++++++++++++++++++ codex/bash_write_test.go | 195 +++++++++++++++++++++++++++++++++++ unified.go | 70 ++++++++----- unified_test.go | 81 +++++++++++++++ 4 files changed, 537 insertions(+), 23 deletions(-) create mode 100644 codex/bash_write.go create mode 100644 codex/bash_write_test.go diff --git a/codex/bash_write.go b/codex/bash_write.go new file mode 100644 index 0000000..4b6d2ed --- /dev/null +++ b/codex/bash_write.go @@ -0,0 +1,214 @@ +package codex + +import ( + "regexp" + "strings" +) + +// catRedirectHeredocRE matches the leading `cat <<['"]?DELIM['"]? > FILE` +// (or `>>`) shape that Codex uses for greenfield file writes. The regex +// must hold together because Codex emits this command without any +// surrounding context — it's the entire `tool_input.command` payload. +// +// Group layout (named for legibility in the implementation): +// +// op — the heredoc operator, "<<" or "<<-". +// delim — the heredoc delimiter ("EOF", "PATCH", …), with surrounding +// quotes (if any) stripped by the caller. +// redir — the redirection operator, ">" or ">>". +// path — the target file path. Anything up to the first whitespace, +// `&`, `;`, `|`, or end-of-line. +// +// Why this exact shape: the captured probe payloads (see +// vscode-extension/docs/HOOKS.md § "Probing Codex hooks") show Codex +// invoking writes as `cat <<'EOF' > greet.txt` for the greet.txt case +// and `cd … && cat <<'EOF' > path` for cwd-prefixed variants. Heredoc +// quoting can be single, double, or absent; the redirect operator can +// be `>` (overwrite) or `>>` (append). The optional `-` after `<<` +// covers the `<<-DELIM` shape that strips leading tabs from the body — +// uncommon in Codex output but cheap to support. +// `\s` would also greedy-match the trailing newline + any leading blank +// lines of the body. Use `[ \t]*` for header-line whitespace so the +// header match always ends at the EOL boundary and extractHeredocBody +// can preserve leading blank lines in the body exactly as written. +var catRedirectHeredocRE = regexp.MustCompile( + `(?m)(?:^|[;&|])[ \t]*cat[ \t]+(?P<<-?)[ \t]*(?P'[^']+'|"[^"]+"|[A-Za-z_][A-Za-z0-9_]*)[ \t]*(?P>>?)[ \t]*(?P[^\s;&|]+)[ \t]*$`, +) + +// teeRedirectHeredocRE covers `tee [-a] FILE [< /dev/null]` and similar +// shapes where the body is fed from a heredoc opened earlier on the line. +// Less common than cat but inexpensive to handle, and the variant Codex +// occasionally emits when it wants tee's "also print to stdout" side +// effect. Captures match catRedirectHeredocRE's named groups so the +// caller can use a shared extractor. +var teeRedirectHeredocRE = regexp.MustCompile( + `(?m)(?:^|[;&|])[ \t]*tee[ \t]+(?:-a[ \t]+)?(?P[^\s;&|]+)[ \t]*(?:<[ \t]*/dev/null[ \t]*)?(?P<<-?)[ \t]*(?P'[^']+'|"[^"]+"|[A-Za-z_][A-Za-z0-9_]*)[ \t]*$`, +) + +// ParseBashRedirectWrite inspects a Codex Bash tool command and, if the +// command is a heredoc-based file write (`cat < FILE … EOF`), +// returns a synthetic PatchFile slice describing the write so the +// OnAfterFileEdit bridge can dispatch it through the same per-file +// pipeline as apply_patch invocations. The boolean return reports +// whether a write was detected, mirroring ParseApplyPatchFromBash's +// shape so callers can chain detectors: +// +// if files, ok := ParseApplyPatchFromBash(cmd); ok { … } +// if files, ok := ParseBashRedirectWrite(cmd); ok { … } +// +// Why this matters: Codex `0.130.0`+ routes greenfield file creates +// through plain Bash `cat <<'EOF' > FILE` invocations rather than the +// first-class `apply_patch` tool or any `Write`/`Edit` alias. Without +// this parser the corridor `codex-post-tool-use` handler short-circuits +// on every greenfield write — no afterFileEdit fires, no security scan +// runs, and the dashboard shows zero SecurityScanResult rows for any +// Codex session that only creates new files (the exact symptom that +// motivated this helper; see the regression test below). +// +// Heuristics are deliberately tight to keep false positives low: +// +// 1. The command must contain a `cat <<…> FILE` or `tee FILE <<…` +// line that ends the logical statement (anchored to end-of-line so +// trailing tokens like `&& echo done` don't mask the redirect). +// 2. The heredoc delimiter is extracted as-emitted (`'EOF'`, `"EOF"`, +// or bare `EOF`); single- or double-quotes around it are stripped +// before scanning the body — Bash treats quoted delimiters as a +// "no variable expansion" hint, which doesn't change our body +// extraction. +// 3. Body extraction walks from the line after the matched cat/tee +// invocation forward until it sees a line that is exactly the +// delimiter (no leading/trailing whitespace, except for `<<-` +// which permits leading tabs per POSIX). If the delimiter is +// missing the parser returns ok=false rather than guessing — the +// command isn't a well-formed heredoc write. +// +// We never try to evaluate the file path: anything from the post-redir +// token up to the next whitespace/separator is treated as a literal +// path. That matches what Codex emits and avoids us silently rewriting +// `>` redirects to fd numbers or process substitutions, which a real +// shell parser would have to handle. +func ParseBashRedirectWrite(command string) ([]PatchFile, bool) { + if !strings.Contains(command, "<<") { + return nil, false + } + + if files, ok := matchWriteRedirect(command, catRedirectHeredocRE); ok { + return files, true + } + if files, ok := matchWriteRedirect(command, teeRedirectHeredocRE); ok { + return files, true + } + return nil, false +} + +func matchWriteRedirect(command string, re *regexp.Regexp) ([]PatchFile, bool) { + // FindStringSubmatchIndex returns byte offsets so we can locate the + // heredoc body that begins on the line after the matched header. + idx := re.FindStringSubmatchIndex(command) + if idx == nil { + return nil, false + } + + names := re.SubexpNames() + groups := map[string]string{} + for i, name := range names { + if name == "" { + continue + } + if start, end := idx[2*i], idx[2*i+1]; start >= 0 && end >= 0 { + groups[name] = command[start:end] + } + } + + delim := stripDelimQuotes(groups["delim"]) + if delim == "" { + return nil, false + } + path := strings.TrimSpace(groups["path"]) + if path == "" { + return nil, false + } + + stripIndent := groups["op"] == "<<-" + body, ok := extractHeredocBody(command, idx[1], delim, stripIndent) + if !ok { + return nil, false + } + + return []PatchFile{{ + Operation: "add", + FilePath: path, + Edits: []PatchEdit{{ + OldString: "", + NewString: body, + }}, + }}, true +} + +// stripDelimQuotes removes surrounding single or double quotes from a +// heredoc delimiter token. Bash treats `<<'EOF'`, `<<"EOF"`, and +// `<= 2 { + first, last := delim[0], delim[len(delim)-1] + if (first == '\'' || first == '"') && first == last { + return delim[1 : len(delim)-1] + } + } + return delim +} + +// extractHeredocBody returns the heredoc body that follows headerEnd +// (the byte offset of the end of the matched cat/tee invocation line) +// up to but not including the line that contains only `delim`. The +// returned body never includes the trailing delimiter line itself. +// +// stripIndent reflects the `<<-` operator, which strips leading TAB +// characters from each body line and from the delimiter line; spaces +// are NOT stripped, per POSIX. We follow the same rule so the body we +// hand to OnAfterFileEdit matches what Bash would have written to the +// file. +func extractHeredocBody(command string, headerEnd int, delim string, stripIndent bool) (string, bool) { + // Body starts on the line after the header. Advance past the + // optional `\r\n` or `\n` immediately following the match. + start := headerEnd + if start < len(command) && command[start] == '\r' { + start++ + } + if start < len(command) && command[start] == '\n' { + start++ + } + + rest := command[start:] + lines := strings.SplitAfter(rest, "\n") + + var body strings.Builder + for _, raw := range lines { + // Drop the trailing newline so we can compare against delim + // without worrying about \r\n vs \n. + line := strings.TrimRight(raw, "\r\n") + cmpLine := line + if stripIndent { + cmpLine = strings.TrimLeft(cmpLine, "\t") + } + if cmpLine == delim { + result := body.String() + result = strings.TrimSuffix(result, "\n") + return result, true + } + if stripIndent { + line = strings.TrimLeft(line, "\t") + } + body.WriteString(line) + if strings.HasSuffix(raw, "\n") { + body.WriteByte('\n') + } + } + // Missing terminator — bail rather than silently treating the + // remainder of the command as body. A well-formed Codex command + // always terminates the heredoc. + return "", false +} diff --git a/codex/bash_write_test.go b/codex/bash_write_test.go new file mode 100644 index 0000000..c8021fe --- /dev/null +++ b/codex/bash_write_test.go @@ -0,0 +1,195 @@ +package codex + +import ( + "reflect" + "testing" +) + +func TestParseBashRedirectWrite_CatHeredocSingleQuotedDelim(t *testing.T) { + // Captured verbatim from a Codex 0.130.0 session that produced no + // afterFileEdit before we added this parser — the canonical + // greenfield-write shape. + cmd := "cat <<'EOF' > greet.txt\nhello world\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := []PatchFile{{ + Operation: "add", + FilePath: "greet.txt", + Edits: []PatchEdit{{ + OldString: "", + NewString: "hello world", + }}, + }} + if !reflect.DeepEqual(got, want) { + t.Errorf("ParseBashRedirectWrite =\n %+v\nwant\n %+v", got, want) + } +} + +func TestParseBashRedirectWrite_CatHeredocDoubleQuotedDelim(t *testing.T) { + cmd := "cat <<\"PYEOF\" > app.py\nprint('hi')\nPYEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "app.py" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "app.py") + } + if got[0].Edits[0].NewString != "print('hi')" { + t.Errorf("NewString = %q, want %q", got[0].Edits[0].NewString, "print('hi')") + } +} + +func TestParseBashRedirectWrite_CatHeredocUnquotedDelim(t *testing.T) { + cmd := "cat < notes.md\nfirst line\nsecond line\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].Edits[0].NewString != "first line\nsecond line" { + t.Errorf("NewString = %q", got[0].Edits[0].NewString) + } +} + +func TestParseBashRedirectWrite_CatHeredocAppendRedirect(t *testing.T) { + cmd := "cat <<'EOF' >> log.txt\nappended line\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "log.txt" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "log.txt") + } +} + +func TestParseBashRedirectWrite_CdAndCatHeredoc(t *testing.T) { + // Codex frequently prefixes the cat invocation with a `cd` so the + // relative path resolves correctly. The regex anchors at `;`, `&`, + // `|`, or start-of-line, so `&&` separators must still match. + cmd := "cd /tmp/probe && cat <<'EOF' > out.txt\nbody\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "out.txt" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "out.txt") + } +} + +func TestParseBashRedirectWrite_CatHeredocTabIndent(t *testing.T) { + // `<<-` strips leading TABs (not spaces) from each body line and + // from the delimiter line. Verify both behaviours. + cmd := "cat <<-EOF > indented.txt\n\tline one\n\tline two\n\tEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].Edits[0].NewString != "line one\nline two" { + t.Errorf("NewString = %q (tabs not stripped)", got[0].Edits[0].NewString) + } +} + +func TestParseBashRedirectWrite_TeeHeredoc(t *testing.T) { + cmd := "tee out.txt <<'EOF'\nfrom tee\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "out.txt" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "out.txt") + } + if got[0].Edits[0].NewString != "from tee" { + t.Errorf("NewString = %q", got[0].Edits[0].NewString) + } +} + +func TestParseBashRedirectWrite_TeeAppend(t *testing.T) { + cmd := "tee -a log.txt < greet.txt", // simple redirect (no heredoc) — out of scope + "printf 'hi' > greet.txt", // not handled here + "apply_patch <<'PATCH'\n*** Begin Patch\n*** Add File: a\n+x\n*** End Patch\nPATCH", + } + for _, cmd := range tests { + if _, ok := ParseBashRedirectWrite(cmd); ok { + t.Errorf("ParseBashRedirectWrite(%q) ok = true; want false", cmd) + } + } +} + +func TestParseBashRedirectWrite_HeredocMissingTerminator(t *testing.T) { + // Without the closing EOF line we can't trust the body — bail. + cmd := "cat <<'EOF' > greet.txt\nhello world\n" + + if _, ok := ParseBashRedirectWrite(cmd); ok { + t.Errorf("expected ok=false for heredoc missing terminator") + } +} + +func TestParseBashRedirectWrite_DelimAppearingInsideBodyIsNotTerminator(t *testing.T) { + // The delimiter must appear on a line by itself — `EOFISH` should + // not terminate an `EOF` heredoc. + cmd := "cat <<'EOF' > out.txt\nEOFISH is not a terminator\nstill in body\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := "EOFISH is not a terminator\nstill in body" + if got[0].Edits[0].NewString != want { + t.Errorf("NewString = %q, want %q", got[0].Edits[0].NewString, want) + } +} + +func TestParseBashRedirectWrite_PathTraversalIsPreservedVerbatim(t *testing.T) { + // We never normalize the path — if Codex emits something + // suspicious like `../../etc/passwd`, downstream policy needs to + // see it as-is so it can reject it. Verify we don't accidentally + // rewrite or strip such paths. + cmd := "cat <<'EOF' > ../../etc/passwd\nroot:x:0:0\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "../../etc/passwd" { + t.Errorf("FilePath = %q, want preserved verbatim", got[0].FilePath) + } +} + +func TestParseBashRedirectWrite_BodyWithEmptyLines(t *testing.T) { + cmd := "cat <<'EOF' > out.txt\n\nline two\n\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := "\nline two\n" + if got[0].Edits[0].NewString != want { + t.Errorf("NewString = %q, want %q", got[0].Edits[0].NewString, want) + } +} diff --git a/unified.go b/unified.go index 7e5c3a6..6a8c5dd 100644 --- a/unified.go +++ b/unified.go @@ -735,20 +735,28 @@ func OnAfterFileEdit(handler FileEditHandler) { // or old_string/new_string. Native first-class tools. // 2. apply_patch — tool_input.command holds a unified-diff envelope // that may touch multiple files in a single call. - // 3. Bash — tool_input.command holds `apply_patch <<'PATCH' - // … PATCH` (sometimes with an absolute path to a - // per-session apply_patch shim). Codex routes file - // edits this way at least as often as (2). Without - // handling this case the SecurityScanResults dash - // stayed empty for every Codex session because no - // afterFileEdit telemetry was ever emitted. + // 3. Bash — tool_input.command holds one of two shapes: + // a. Edits: `apply_patch <<'PATCH' … PATCH` + // (sometimes with an absolute path + // to a per-session apply_patch shim) + // b. Writes: `cat <<'EOF' > FILE … EOF` or + // `tee FILE <<'EOF' … EOF` + // Codex routes virtually all file operations this + // way: edits via (a), greenfield writes via (b). + // Without handling BOTH cases the + // SecurityScanResults dashboard stayed empty for + // every Codex session — no afterFileEdit + // telemetry was ever emitted for "Create a file" + // prompts or for in-place edits. // - // For (2) and (3) we run the patch through codex.ParseApplyPatch and - // invoke the user's handler exactly once per file in the envelope, then - // reduce the per-file decisions: a Block from any file wins, otherwise - // context strings are concatenated. The parser is also exported as - // codex.ParseApplyPatch / codex.ParseApplyPatchFromBash for callers - // that want raw access. + // For (2) and (3a) we run the patch through codex.ParseApplyPatch and + // invoke the user's handler exactly once per file in the envelope. For + // (3b) codex.ParseBashRedirectWrite synthesizes a single PatchFile so + // the same dispatchPatch reducer drives every shape. Per-file + // decisions reduce as: a Block from any file wins, otherwise context + // strings are concatenated. The parsers are also exported as + // codex.ParseApplyPatch / codex.ParseApplyPatchFromBash / + // codex.ParseBashRedirectWrite for callers that want raw access. // // Configure the hook with matcher "Bash|apply_patch|mcp__.*" in // hooks.json — "Edit" and "Write" matcher aliases exist but are @@ -872,21 +880,37 @@ func OnAfterFileEdit(handler FileEditHandler) { return dispatchPatch(codex.ParseApplyPatch(applyInput.Command), applyInput.Command) case "Bash": - // Codex routes most file edits through Bash: - // `apply_patch <<'PATCH' … PATCH` - // Detection lives in codex.ParseApplyPatchFromBash so - // non-edit Bash commands short-circuit cheaply. If the - // command IS an apply_patch invocation, dispatch through - // the same per-file pipeline as case "apply_patch". + // Codex routes file operations through Bash in two + // distinct shapes that look superficially similar but + // require different parsers: + // + // 1. Edits to existing files: + // apply_patch <<'PATCH' … *** End Patch … PATCH + // parsed by codex.ParseApplyPatchFromBash. + // + // 2. Greenfield writes (and overwrite-style writes): + // cat <<'EOF' > newfile.txt … EOF + // tee newfile.txt <<'EOF' … EOF + // parsed by codex.ParseBashRedirectWrite. + // + // Both produce []PatchFile suitable for the shared + // dispatchPatch reducer. We try apply_patch first + // because it's the higher-fidelity shape (per-hunk + // old/new) when both detectors would match, then fall + // back to the heredoc-write detector. Non-edit Bash + // commands short-circuit at the second `if !ok` check + // without paying for either full parse pass. var bashInput struct { Command string `json:"command"` } json.Unmarshal(input.ToolInput, &bashInput) - files, ok := codex.ParseApplyPatchFromBash(bashInput.Command) - if !ok { - return codex.PostToolOK() + if files, ok := codex.ParseApplyPatchFromBash(bashInput.Command); ok { + return dispatchPatch(files, bashInput.Command) + } + if files, ok := codex.ParseBashRedirectWrite(bashInput.Command); ok { + return dispatchPatch(files, bashInput.Command) } - return dispatchPatch(files, bashInput.Command) + return codex.PostToolOK() default: return codex.PostToolOK() diff --git a/unified_test.go b/unified_test.go index 1b98240..b3afe29 100644 --- a/unified_test.go +++ b/unified_test.go @@ -1240,6 +1240,87 @@ func TestCodexPostToolUse_BashNonApplyPatchCommandSkipped(t *testing.T) { } } +func TestCodexPostToolUse_BashCatHeredocWriteDispatches(t *testing.T) { + // Codex 0.130.0+ routes greenfield writes through a plain Bash + // `cat <<'EOF' > FILE … EOF` heredoc rather than apply_patch or any + // Write/Edit tool alias. Without this dispatch path, no afterFileEdit + // fires for "Create a file …" prompts and the security scanner + // never sees the new file — the exact regression that motivated + // ParseBashRedirectWrite. This test pins the wiring end-to-end. + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "cat <<'EOF' > greet.txt\\nhello world\\nEOF" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 1 { + t.Fatalf("handler invocations = %d, want 1; got=%+v", len(got), got) + } + if got[0].FilePath != "greet.txt" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "greet.txt") + } + if got[0].Platform != PlatformCodex { + t.Errorf("Platform = %q, want %q", got[0].Platform, PlatformCodex) + } + if got[0].Cwd != "/repo" { + t.Errorf("Cwd = %q, want %q", got[0].Cwd, "/repo") + } + wantEdit := FileEdit{OldString: "", NewString: "hello world"} + if len(got[0].Edits) != 1 || got[0].Edits[0] != wantEdit { + t.Errorf("Edits = %+v, want [%+v]", got[0].Edits, wantEdit) + } +} + +func TestCodexPostToolUse_BashCatHeredocBlockPropagates(t *testing.T) { + // Blocking decisions on cat-heredoc writes must surface as + // PostToolBlock JSON, identical to the apply_patch path. This is + // what lets policies like "no .env files" actually stop Codex + // from creating sensitive files via greenfield writes. + ClearHandlers() + defer ClearHandlers() + + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + if strings.HasSuffix(ctx.FilePath, ".env") { + return FileEditBlock("no env files") + } + return FileEditOK() + }) + + cmd := "cat <<'EOF' > .env\\nSECRET=abc\\nEOF" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + + stdinR, stdinW, _ := os.Pipe() + stdinW.Write([]byte(stdin)) + stdinW.Close() + stdoutR, stdoutW, _ := os.Pipe() + origStdin, origStdout := os.Stdin, os.Stdout + os.Stdin, os.Stdout = stdinR, stdoutW + defer func() { + stdoutW.Close() + os.Stdin, os.Stdout = origStdin, origStdout + }() + + handlers["codex-post-tool-use"]() + stdoutW.Close() + + var buf bytes.Buffer + buf.ReadFrom(stdoutR) + out := buf.String() + if !strings.Contains(out, `"decision":"block"`) { + t.Errorf("expected PostToolBlock JSON output, got %q", out) + } + if !strings.Contains(out, "no env files") { + t.Errorf("expected block reason in output, got %q", out) + } +} + func TestCodexPostToolUse_BashApplyPatchBlockPropagates(t *testing.T) { // A blocking decision from the user's handler on a Bash-heredoc // apply_patch must emit PostToolBlock so Codex surfaces the feedback, From a3462d3903312e42c8208a58fe3bcb4f604dd47c Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 17 May 2026 21:00:39 -0700 Subject: [PATCH 11/19] fix(installer): include Bash in codex PostToolUse matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hookshot installer's PostToolUse matcher was `apply_patch|mcp__.*`, which never matches anything Codex 0.130.0+ actually emits for file edits — Codex routes both edits (`apply_patch <<'PATCH' …`) and writes (`cat <<'EOF' > FILE …`) through `tool_name="Bash"`, not the first-class apply_patch tool. Without Bash in the matcher, Codex never hands the events to the hook binary, so the freshly-added ParseBashRedirectWrite dispatch never gets a chance to fire and afterFileEdit telemetry stays empty for every Codex session. Update the installer to write `Bash|apply_patch|mcp__.*` for both PreToolUse and PostToolUse, matching what hookshot's unified bridge parses. Sync README, codex/doc.go, docs/reference-codex.md, docs/reference-unified.md, and examples/multi-hook to the new recommended matcher so copy-pasted configs stay correct. Co-authored-by: Cursor --- README.md | 2 +- cmd/hookshot/main.go | 20 ++++++++++++++------ codex/doc.go | 2 +- docs/reference-codex.md | 2 +- docs/reference-unified.md | 2 +- examples/multi-hook/main.go | 2 +- 6 files changed, 19 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index f5bb235..51f6ff2 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,7 @@ back on. "hooks": { "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], - "PostToolUse": [{ "matcher": "apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }] + "PostToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }] } } ``` diff --git a/cmd/hookshot/main.go b/cmd/hookshot/main.go index 6c9029b..796edeb 100644 --- a/cmd/hookshot/main.go +++ b/cmd/hookshot/main.go @@ -502,7 +502,15 @@ func installCodex(binaryPath string) error { }}, }}, "PostToolUse": []map[string]any{{ - "matcher": "apply_patch|mcp__.*", + // Bash is required to catch the heredoc-style file edits + // (`apply_patch <<'PATCH' … PATCH`) and greenfield writes + // (`cat <<'EOF' > FILE … EOF`) Codex 0.130.0+ routes + // through plain Bash in addition to the apply_patch tool. The + // unified codex-post-tool-use bridge parses both shapes via + // codex.ParseApplyPatchFromBash / codex.ParseBashRedirectWrite + // — but only sees the events if the matcher itself lets + // them through. + "matcher": "Bash|apply_patch|mcp__.*", "hooks": []map[string]any{{ "type": "command", "command": binaryPath + " codex-post-tool-use", @@ -542,11 +550,11 @@ func installCascade(binaryPath string) error { // Build hooks config config := map[string]any{ "hooks": map[string]any{ - "pre_run_command": []map[string]any{{"command": binaryPath + " cascade-pre-run-command"}}, - "pre_mcp_tool_use": []map[string]any{{"command": binaryPath + " cascade-pre-mcp-tool-use"}}, - "pre_user_prompt": []map[string]any{{"command": binaryPath + " cascade-pre-user-prompt"}}, - "post_write_code": []map[string]any{{"command": binaryPath + " cascade-post-write-code"}}, - "post_cascade_response": []map[string]any{{"command": binaryPath + " cascade-post-cascade-response"}}, + "pre_run_command": []map[string]any{{"command": binaryPath + " cascade-pre-run-command"}}, + "pre_mcp_tool_use": []map[string]any{{"command": binaryPath + " cascade-pre-mcp-tool-use"}}, + "pre_user_prompt": []map[string]any{{"command": binaryPath + " cascade-pre-user-prompt"}}, + "post_write_code": []map[string]any{{"command": binaryPath + " cascade-post-write-code"}}, + "post_cascade_response": []map[string]any{{"command": binaryPath + " cascade-post-cascade-response"}}, }, } diff --git a/codex/doc.go b/codex/doc.go index e486e3e..2c971c6 100644 --- a/codex/doc.go +++ b/codex/doc.go @@ -29,7 +29,7 @@ // ], // "PostToolUse": [ // { -// "matcher": "apply_patch|mcp__.*", +// "matcher": "Bash|apply_patch|mcp__.*", // "hooks": [{ "type": "command", "command": "/path/to/hook codex-post-tool-use" }] // } // ], diff --git a/docs/reference-codex.md b/docs/reference-codex.md index 170e7d1..f5f2e43 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -22,7 +22,7 @@ Hook commands live in `~/.codex/hooks.json` (or an inline `[hooks]` table in { "hooks": { "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], - "PostToolUse": [{ "matcher": "apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], + "PostToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], "UserPromptSubmit": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" }] }], "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop", "timeout": 30 }] }] } diff --git a/docs/reference-unified.md b/docs/reference-unified.md index b12bc93..ba73d6b 100644 --- a/docs/reference-unified.md +++ b/docs/reference-unified.md @@ -187,7 +187,7 @@ Handles post-file-edit events. **Registers:** `claude-after-file-edit`, `cursor-after-file-edit`, `droid-after-file-edit`, `cascade-post-write-code`, `codex-post-tool-use` -For Codex, the underlying PostToolUse handler matches `apply_patch` (which is how Codex performs all file edits). Configure the hook with `matcher: "apply_patch|mcp__.*"` in `~/.codex/hooks.json`. +For Codex, the underlying PostToolUse handler must match `Bash` in addition to `apply_patch`: Codex 0.130.0+ routes greenfield writes through `cat <<'EOF' > FILE` and edits through `apply_patch <<'PATCH'` heredocs — both shapes ride a `tool_name="Bash"` PostToolUse, parsed by `codex.ParseBashRedirectWrite` and `codex.ParseApplyPatchFromBash` respectively. Configure the hook with `matcher: "Bash|apply_patch|mcp__.*"` in `~/.codex/hooks.json`. For Codex `apply_patch`, the unified bridge parses the unified-diff envelope in `tool_input.command` and invokes your handler **once per file** in the patch, with `FilePath` set to the file declared in the `*** Add/Update/Delete File:` header and `Edits` populated from each hunk. If any of those invocations returns `FileEditBlock`, the reasons are concatenated and emitted as a single `PostToolBlock`. diff --git a/examples/multi-hook/main.go b/examples/multi-hook/main.go index 978cfe8..5d01d0e 100644 --- a/examples/multi-hook/main.go +++ b/examples/multi-hook/main.go @@ -61,7 +61,7 @@ // "hooks": { // "Stop": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-stop" }] }], // "PreToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-pre-tool-use" }] }], -// "PostToolUse": [{ "matcher": "apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], +// "PostToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-post-tool-use" }] }], // "UserPromptSubmit": [{ "hooks": [{ "type": "command", "command": "/path/to/my-hooks codex-user-prompt-submit" }] }] // } // } From e926ab556f51cd3ac6c0553c5352300657355420 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 18 May 2026 04:29:47 +0000 Subject: [PATCH 12/19] docs(codex): document the Bash bridge for heredoc file edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a dedicated 'Bash bridge: file edits routed through Bash' section to docs/reference-codex.md that explains why Codex 0.130.0+ sessions route file edits through plain Bash invocations rather than the native apply_patch / Write / Edit tools, and how OnAfterFileEdit reduces all four shapes to one per-file FileEditContext. The new section covers: - The four shapes the bridge recognizes today (Write/Edit native, apply_patch native, Bash apply_patch <<'PATCH' heredoc, Bash cat/tee <<'EOF' > FILE heredoc) with the parser used for each. - Why the PostToolUse matcher must include 'Bash' — the symptom of not including it is zero afterFileEdit events for any Codex session that edits files. - Detection rules for apply_patch via Bash heredoc, including the per-session shim-path variant. - How greenfield writes synthesize a single PatchEdit so they flow through the same dispatch path as diff-based edits, and which cat/tee variants (quoted/unquoted/<<- delimiters, >/>>, cd-prefix) are recognized. - Public API for handlers that want to bypass the unified bridge: ParseApplyPatch, ParseApplyPatchFromBash, ParseBashRedirectWrite, PatchFile, PatchEdit — with an example handler. - Known limitations: echo/printf > FILE not recognized, only the first cat/tee heredoc in a single Bash command is surfaced, and paths are passed through verbatim without normalization. Each limitation comes with a concrete defence-in-depth suggestion (OnBeforeExecution grep, filepath.Clean) so policy authors can close the gap themselves until the parsers grow stricter. The existing PostToolUse events-table row now points at both the new 'Bash bridge' section and the existing 'apply_patch on the unified API' section so readers land on the right page regardless of which shape they're investigating. Co-authored-by: Ashwin Ramaswami --- docs/reference-codex.md | 150 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 149 insertions(+), 1 deletion(-) diff --git a/docs/reference-codex.md b/docs/reference-codex.md index f5f2e43..1bb406e 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -49,7 +49,7 @@ you. | SessionStart | `codex.SessionStartInput` / `Output` | `source` is `startup`, `resume`, or `clear`. | | PreToolUse | `codex.PreToolUseInput` / `Output` | `tool_name` is `Bash`, `apply_patch`, or `mcp__server__tool`. See **Ask is not enforced** below. | | PermissionRequest | `codex.PermissionRequestInput` / `Output` | Codex-specific. Fires only when Codex is about to surface an approval prompt; see below. | -| PostToolUse | `codex.PostToolUseInput` / `Output` | See **apply_patch on the unified API** below. | +| PostToolUse | `codex.PostToolUseInput` / `Output` | Codex 0.130.0+ routes most file edits through `Bash` rather than `apply_patch`/`Write`/`Edit`. See **Bash bridge** and **apply_patch on the unified API** below. | | UserPromptSubmit | `codex.UserPromptSubmitInput` / `Output` | Same shape as Claude. | | Stop | `codex.StopInput` / `Output` | Same shape as Claude; Codex expects JSON on stdout (not plain text). | @@ -151,6 +151,154 @@ The same parser is also exported as that want to parse a patch envelope themselves — for example from the raw `codex.PostToolUseInput.ToolInput`. +## Bash bridge: file edits routed through Bash + +Codex `0.130.0`+ routes most file operations through plain `Bash` +invocations rather than the native `apply_patch`, `Write`, or `Edit` +tools. The unified `hookshot.OnAfterFileEdit` bridge recognizes this +and dispatches the same per-file pipeline as the native shapes, so +policy handlers see one `FileEditContext` per file regardless of how +Codex chose to encode the edit. + +The four shapes the bridge recognizes today: + +| `tool_name` | `tool_input.command` shape | Parser | +|----------------|-----------------------------------------------------------------------|---------------------------------| +| `Write` / `Edit` | (native fields — uncommon on 0.130.0+) | inline | +| `apply_patch` | unified-diff envelope, no Bash wrapper | `codex.ParseApplyPatch` | +| `Bash` | `apply_patch <<'PATCH' … *** End Patch … PATCH` | `codex.ParseApplyPatchFromBash` | +| `Bash` | `cat <<'EOF' > FILE … EOF` or `tee FILE <<'EOF' … EOF` | `codex.ParseBashRedirectWrite` | + +This is why the `PostToolUse` matcher in `~/.codex/hooks.json` must +include `Bash` (not just `apply_patch`): + +```jsonc +"PostToolUse": [{ "matcher": "Bash|apply_patch|mcp__.*", /* … */ }] +``` + +`hookshot install --codex` writes this matcher by default. If you +configured hooks before adding the `Bash` token, you'll see zero +`afterFileEdit` events for any Codex session that edits or creates +files — that's the symptom that motivated this bridge. + +### `apply_patch` via Bash heredoc + +Edits to existing files almost always arrive as a Bash heredoc that +pipes a unified-diff envelope into `apply_patch`. Two shape variants +show up in practice: + +- **Built-in name**: `apply_patch <<'PATCH' … *** End Patch … PATCH` +- **Per-session shim**: `/Users/me/.codex/tmp/arg0/codex-arg0…/apply_patch <<'PATCH' …` + +`codex.ParseApplyPatchFromBash` accepts both. Detection requires the +literal token `apply_patch` followed by a heredoc operator (`<<` or +`<<-`) somewhere before the `*** Begin Patch` marker, which keeps +plain Bash commands that merely *mention* `apply_patch` (filenames, +docs, log lines) from spuriously firing the file-edit handler. + +Once detected, the envelope is fed to `codex.ParseApplyPatch` and +the per-file dispatch behaves exactly like the native `apply_patch` +case described above — including the rename double-invocation for +`*** Move to:` paths. + +### Greenfield writes via `cat` / `tee` heredoc + +New-file creation typically arrives as `cat <<'EOF' > FILE … EOF` +(or, less commonly, `tee FILE <<'EOF' … EOF`). There's no prior +content to diff against, so `codex.ParseBashRedirectWrite` +synthesizes a single `PatchEdit{OldString: "", NewString: }` +and a `PatchFile{Operation: "add", FilePath: , Edits: …}`, +then dispatches it through the same per-file pipeline. Variants +currently recognized: + +- quoted (`<<'EOF'`, `<<"EOF"`) and unquoted (`<` (overwrite) and `>>` (append) redirections, +- `cd … && cat <<'EOF' > FILE … EOF` cwd-prefixed forms. + +File paths are surfaced verbatim — `../`, `~/`, and absolute paths +pass through unchanged so downstream policies can apply their own +canonicalization rules (the bridge can't safely guess the agent's +cwd, so it doesn't try). + +If a Codex Bash command matches neither parser, the +`codex-post-tool-use` handler returns `PostToolOK()` without invoking +`OnAfterFileEdit`. That matches the platform semantics of every +other backend: non-edit Bash commands belong to `OnBeforeExecution`, +not `OnAfterFileEdit`. + +### Calling the parsers from your own handler + +Every parser used by the bridge is exported from the `codex` package +so handlers that want full control can short-circuit the unified +dispatch: + +```go +func ParseApplyPatch(rawCommand string) []PatchFile +func ParseApplyPatchFromBash(bashCommand string) (files []PatchFile, ok bool) +func ParseBashRedirectWrite(bashCommand string) (files []PatchFile, ok bool) + +type PatchFile struct { + Operation string // "add", "update", or "delete" + FilePath string + NewFilePath string // set for "*** Move to:" renames + Edits []PatchEdit +} + +type PatchEdit struct { + OldString string + NewString string +} +``` + +Wiring them up manually: + +```go +hookshot.Register("codex-post-tool-use", func() { + hookshot.Run(func(input codex.PostToolUseInput) codex.PostToolUseOutput { + if input.ToolName != "Bash" { + return codex.PostToolOK() + } + var bash struct{ Command string `json:"command"` } + json.Unmarshal(input.ToolInput, &bash) + + if files, ok := codex.ParseApplyPatchFromBash(bash.Command); ok { + return scanPatches(files) + } + if files, ok := codex.ParseBashRedirectWrite(bash.Command); ok { + return scanPatches(files) + } + return codex.PostToolOK() + }) +}) +``` + +### Known limitations + +The Bash bridge is deliberately conservative — it short-circuits on +ambiguous input rather than guessing. Known gaps today: + +- **`echo … > FILE` and `printf … > FILE` writes are not + recognized.** Codex prefers heredocs for any multi-line content, + so these are rare in practice. If you observe them in your + sessions please open an issue with the captured + `tool_input.command`. +- **Only the first `cat`/`tee` heredoc redirect in a single Bash + command is currently surfaced.** A command that performs several + heredoc writes in sequence (e.g. + `cat < a.txt … EOF; cat < b.txt … EOF`) will fire + `OnAfterFileEdit` only for the first file. As defence in depth + against this gap, also register an `OnBeforeExecution` policy + that grep/regex-scans `ctx.Command` for sensitive paths in Bash + commands — that handler runs *before* any heredoc writes execute + and isn't subject to the first-match restriction. +- **The bridge does not normalize file paths.** `../`, `~/`, and + symbolic links are passed through verbatim. Handlers that want to + enforce a containment policy should apply `filepath.Clean` and a + cwd-escape check themselves (the bridge can't safely assume the + agent's working directory matches the hook process's). + ## PostToolUse semantics `decision: "block"` doesn't undo the completed tool call. Codex records the From 7b82f2f44040f3978b9cf4a55d6c55f5071db0c2 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 18 May 2026 13:58:45 -0700 Subject: [PATCH 13/19] Address corridor comment --- codex/helpers.go | 42 ++++++++++++++++++++------------ codex/helpers_test.go | 53 ++++++++++++++++++++++++++++++++--------- docs/reference-codex.md | 32 ++++++++++++++++--------- 3 files changed, 90 insertions(+), 37 deletions(-) diff --git a/codex/helpers.go b/codex/helpers.go index 5a98d00..cb83c19 100644 --- a/codex/helpers.go +++ b/codex/helpers.go @@ -35,29 +35,41 @@ func AllowSilent() PreToolUseOutput { return PreToolUseOutput{} } -// AllowWithInput permits the tool. Codex does NOT support updatedInput — -// the Codex CLI rejects the hook output with "PreToolUse hook returned -// unsupported updatedInput" — so the updatedInput argument is dropped on -// Codex. The reason argument is preserved via permissionDecision: "allow" -// (which falls open on Codex today). The signature matches -// claude.AllowWithInput for source compatibility, but callers that need -// to inject context on Codex should use the model-side tool args instead -// (Codex passes raw tool input through to MCP tools unchanged). +// AllowWithInput is a fail-closed shim. Codex does NOT support updatedInput +// — the CLI rejects the hook output with "PreToolUse hook returned +// unsupported updatedInput" — so there is no Codex-supported way to mutate +// tool_input from a PreToolUse hook today. If a caller is using +// AllowWithInput it is almost certainly to sanitize a dangerous payload +// (strip --no-verify, rewrite a Bash command, etc.); silently dropping +// the sanitization and falling through to Allow would execute the +// unsanitized payload. To prevent that, this helper denies the call and +// surfaces the loss of input rewriting in the reason. The signature +// matches claude.AllowWithInput for source compatibility — callers that +// need to inject model-visible context on Codex should use +// AllowWithContext / additionalContext instead. func AllowWithInput(reason string, _ map[string]any) PreToolUseOutput { - if reason == "" { - return PreToolUseOutput{} + msg := reason + if msg == "" { + msg = "blocked" } - return claude.Allow(reason) + return claude.Deny(msg + " (input rewriting not supported on Codex; fail-closed)") } // Deny blocks the tool from executing. This is enforced by Codex for Bash // and apply_patch tools. var Deny = claude.Deny -// Ask prompts the user to confirm the tool execution. Note that Codex -// currently parses but does not enforce permissionDecision: "ask" for -// PreToolUse, so this falls open today. -var Ask = claude.Ask +// Ask is a fail-closed shim. Codex parses but does not enforce +// permissionDecision: "ask" for PreToolUse today, so emitting "ask" would +// silently execute the tool without surfacing an approval prompt — turning +// a "require confirmation" policy into a free pass. To match the security +// posture of the hookshot unified bridge (which already rewrites +// AskExecution to Deny on Codex; see unified.go), this helper returns a +// Deny with the caller's reason. If you actually want Codex's approval +// flow, register a PermissionRequest hook instead. +func Ask(reason string) PreToolUseOutput { + return claude.Deny(reason) +} // PassThrough returns an empty output, letting the normal permission flow proceed. var PassThrough = claude.PassThrough diff --git a/codex/helpers_test.go b/codex/helpers_test.go index 3dbf755..cb9c304 100644 --- a/codex/helpers_test.go +++ b/codex/helpers_test.go @@ -1,6 +1,7 @@ package codex import ( + "strings" "testing" ) @@ -88,24 +89,54 @@ func TestAllowSilent_DoesNotEmitSuppressOutput(t *testing.T) { } } -func TestAllowWithInput_DropsUpdatedInput(t *testing.T) { +func TestAllowWithInput_FailsClosed(t *testing.T) { // Codex rejects updatedInput with - // "PreToolUse hook returned unsupported updatedInput". - // codex.AllowWithInput must NOT set UpdatedInput. - output := AllowWithInput("trusted", map[string]any{"file_path": "/tmp/x"}) - if output.HookSpecificOutput != nil && output.HookSpecificOutput.UpdatedInput != nil { + // "PreToolUse hook returned unsupported updatedInput", so there is no + // safe way to apply the sanitized input. The helper must fail closed + // (Deny) rather than silently fall through to Allow with the original, + // unsanitized tool_input. + output := AllowWithInput("stripped --no-verify", map[string]any{"file_path": "/tmp/x"}) + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.PermissionDecision != "deny" { + t.Errorf("PermissionDecision = %q, want %q (fail-closed)", output.HookSpecificOutput.PermissionDecision, "deny") + } + if output.HookSpecificOutput.UpdatedInput != nil { t.Error("codex.AllowWithInput must not set UpdatedInput (Codex rejects updatedInput)") } - // Reason should still be passed through as permissionDecisionReason. - if output.HookSpecificOutput == nil || output.HookSpecificOutput.PermissionDecisionReason != "trusted" { - t.Errorf("Reason should be preserved as permissionDecisionReason, got %+v", output.HookSpecificOutput) + if !strings.Contains(output.HookSpecificOutput.PermissionDecisionReason, "stripped --no-verify") { + t.Errorf("PermissionDecisionReason should include caller reason, got %q", output.HookSpecificOutput.PermissionDecisionReason) + } + if !strings.Contains(output.HookSpecificOutput.PermissionDecisionReason, "input rewriting not supported on Codex") { + t.Errorf("PermissionDecisionReason should explain why input rewriting was dropped, got %q", output.HookSpecificOutput.PermissionDecisionReason) } } -func TestAllowWithInput_EmptyReasonProducesPassThrough(t *testing.T) { +func TestAllowWithInput_EmptyReasonStillDenies(t *testing.T) { output := AllowWithInput("", map[string]any{"k": "v"}) - if output.HookSpecificOutput != nil { - t.Error("codex.AllowWithInput with empty reason should emit empty {} (pass-through)") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil (fail-closed)") + } + if output.HookSpecificOutput.PermissionDecision != "deny" { + t.Errorf("PermissionDecision = %q, want %q (fail-closed)", output.HookSpecificOutput.PermissionDecision, "deny") + } +} + +func TestAsk_FailsClosed(t *testing.T) { + // Codex parses but does not enforce permissionDecision: "ask" for + // PreToolUse, so emitting "ask" would silently execute the tool + // without surfacing an approval prompt. codex.Ask must fail closed + // (Deny) to match the unified bridge's behaviour on Codex. + output := Ask("Confirm before deleting the production database.") + if output.HookSpecificOutput == nil { + t.Fatal("HookSpecificOutput should not be nil") + } + if output.HookSpecificOutput.PermissionDecision != "deny" { + t.Errorf("PermissionDecision = %q, want %q (Codex does not enforce ask)", output.HookSpecificOutput.PermissionDecision, "deny") + } + if output.HookSpecificOutput.PermissionDecisionReason != "Confirm before deleting the production database." { + t.Errorf("PermissionDecisionReason = %q, want caller reason preserved", output.HookSpecificOutput.PermissionDecisionReason) } } diff --git a/docs/reference-codex.md b/docs/reference-codex.md index 1bb406e..11aa484 100644 --- a/docs/reference-codex.md +++ b/docs/reference-codex.md @@ -76,13 +76,19 @@ context without blocking the call (the upstream [Codex hooks doc](https://developers.openai.com/codex/hooks#pretooluse) shows this case as a first-class example). -`permissionDecision: "ask"` is parsed but currently fails open. -`hookshot.OnBeforeExecution` returning `AskExecution(...)` is rewritten to -`Deny` on Codex so policies that require user confirmation aren't silently -bypassed. The platform-level `codex.Ask` helper still emits `"ask"` in the -JSON for forward-compat testing. If you want to react when Codex is -actually about to prompt the user, register a separate handler for the -**PermissionRequest** event below — that event's enforcement is supported. +`permissionDecision: "ask"` is parsed by Codex but currently not enforced +at the platform level — emitting it would silently allow the tool to run. +To prevent "require confirmation" policies from turning into free passes, +both the high-level and platform-level APIs fail closed: + +- `hookshot.OnBeforeExecution` returning `AskExecution(...)` is rewritten + to `Deny` on Codex. +- `codex.Ask(reason)` is a fail-closed shim that returns `Deny(reason)` — + it does not emit `"ask"`. + +If you want to react when Codex is actually about to prompt the user, +register a separate handler for the **PermissionRequest** event below — +that event's enforcement is supported. **`updatedInput`, `continue: false`, `stopReason`, and `suppressOutput` fail closed** — Codex rejects the whole hook output with @@ -94,10 +100,14 @@ field, so these must be omitted. Concretely, this means: - `codex.AllowSilent()` is **not safe**: it sets `suppressOutput: true`, which Codex rejects. Use `codex.PassThrough()` for an empty no-side-effects allow. -- `codex.AllowWithInput(reason, input)` is **not safe**: it sets - `updatedInput`, which Codex rejects. There's no Codex-supported way to - mutate `tool_input` from a PreToolUse hook today — fall back to - injecting state through `additionalContext` instead. +- `codex.AllowWithInput(reason, input)` is a **fail-closed shim**: there + is no Codex-supported way to mutate `tool_input` from a PreToolUse + hook (the CLI rejects `updatedInput`), and silently allowing the call + with the original, unsanitized input would defeat the purpose of + rewriting it. The helper therefore returns `Deny(reason + " (input + rewriting not supported on Codex; fail-closed)")`. If you need to + inject model-visible context on Codex, use `codex.AllowWithContext` + (or `additionalContext` directly) instead. - To attach model-visible context to an allow, return `codex.AllowWithContext(reason, context)` (or build the output by hand with `hookSpecificOutput.additionalContext`). From 52773805dd6d6d23f995a31e623638bacdaab382 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 18 May 2026 14:08:45 -0700 Subject: [PATCH 14/19] Fix heredoc parsing w dependency --- codex/apply_patch.go | 167 ++++++++++++++++++++++++++++++++------ codex/apply_patch_test.go | 154 +++++++++++++++++++++++++++++++++++ go.mod | 2 + go.sum | 12 +++ 4 files changed, 310 insertions(+), 25 deletions(-) create mode 100644 go.sum diff --git a/codex/apply_patch.go b/codex/apply_patch.go index c5c25ec..cf9b776 100644 --- a/codex/apply_patch.go +++ b/codex/apply_patch.go @@ -1,17 +1,10 @@ package codex import ( - "regexp" "strings" -) -// applyPatchInvocationRE matches an `apply_patch` invocation followed by a -// heredoc operator (`<<`, `<<-`, or quoted variants). Requiring the heredoc -// operator — instead of accepting any substring match — prevents the -// detector from firing on benign Bash commands that merely mention -// `apply_patch` (filenames, documentation, log lines). The optional `-` and -// whitespace handling cover the variants Codex actually emits in the wild. -var applyPatchInvocationRE = regexp.MustCompile(`(?:^|[/\s;&|])apply_patch[ \t]+<<-?`) + "mvdan.cc/sh/v3/syntax" +) // PatchEdit captures one removed/added pair inside an apply_patch hunk. // @@ -164,11 +157,11 @@ func ParseApplyPatch(patch string) []PatchFile { return files } -// ParseApplyPatchFromBash inspects a Codex Bash tool command and, if the -// command is an apply_patch heredoc invocation, returns the parsed patch -// files. The second return value reports whether the command was an -// apply_patch invocation so callers can short-circuit on plain Bash -// commands without paying for a full parse pass. +// ParseApplyPatchFromBash inspects a Codex Bash tool command and, if it +// invokes apply_patch with a heredoc body, returns the parsed patch +// files. The second return value reports whether at least one +// apply_patch heredoc invocation was found, so callers can short-circuit +// on plain Bash commands without parsing the patch envelope. // // Codex routes file edits through Bash heredocs of the form // @@ -187,20 +180,144 @@ func ParseApplyPatch(patch string) []PatchFile { // dashboard with zero SecurityScanResult rows even though file edits had // clearly happened). // -// Detection is heuristic but tight: the command must contain the -// "*** Begin Patch" envelope marker AND an `apply_patch < docs/apply_patch_format.md </dev/null; apply_patch <<'PATCH' ...`) +// would anchor `strings.Index` on the decoy, leaving the regex with a +// prefix that didn't contain the real `apply_patch <<` invocation — +// detection silently returned (nil, false) and the post-tool handler +// skipped dispatch entirely. +// 2. The regex required at least one space between `apply_patch` and +// `<<`, so the zero-space `apply_patch<<'PATCH'` (valid Bash) also +// evaded detection. +// +// A real parser closes both bypasses and also rules out the legacy +// false-positive case (an `apply_patch` token appearing inside a +// docs-file heredoc body), because the parser identifies the +// surrounding command (`cat > docs/apply_patch_format.md <<'EOF'`) as +// the invocation, not the body content. +// +// If a command contains multiple apply_patch heredoc invocations (in +// pipelines, `;` chains, subshells, conditionals, etc.) every body is +// parsed and the resulting PatchFile slices are concatenated. If the +// command is not valid Bash (parser error) this returns (nil, false) — +// silently treating malformed input as an edit would be unsafe. func ParseApplyPatchFromBash(command string) ([]PatchFile, bool) { - envelopeStart := strings.Index(command, "*** Begin Patch") - if envelopeStart < 0 { + file, err := syntax.NewParser().Parse(strings.NewReader(command), "") + if err != nil { return nil, false } - prefix := command[:envelopeStart] - if !applyPatchInvocationRE.MatchString(prefix) { + + var bodies []heredocBody + syntax.Walk(file, func(node syntax.Node) bool { + stmt, ok := node.(*syntax.Stmt) + if !ok { + return true + } + if !stmtInvokesApplyPatch(stmt) { + return true + } + for _, r := range stmt.Redirs { + body, ok := heredocBodyFromRedirect(command, r) + if ok { + bodies = append(bodies, body) + } + } + return true + }) + + if len(bodies) == 0 { return nil, false } - return ParseApplyPatch(command), true + + var all []PatchFile + for _, b := range bodies { + all = append(all, ParseApplyPatch(b.text)...) + } + return all, true +} + +// stmtInvokesApplyPatch reports whether stmt's primary command is the +// apply_patch executable. Accepts the bare `apply_patch` token and the +// `*/apply_patch` absolute-path form Codex uses for its per-session +// shim binary. Any other shape (function definitions, command +// substitutions, builtins, etc.) returns false. +func stmtInvokesApplyPatch(stmt *syntax.Stmt) bool { + call, ok := stmt.Cmd.(*syntax.CallExpr) + if !ok || len(call.Args) == 0 { + return false + } + name := call.Args[0].Lit() + if name == "apply_patch" { + return true + } + if strings.HasSuffix(name, "/apply_patch") { + return true + } + return false +} + +// heredocBody is the extracted body of one here-doc redirect, plus a +// hint about whether tab-stripping (POSIX `<<-` semantics) was applied. +// We carry this rather than returning a raw string so test failures can +// report which variant a particular body came from. +type heredocBody struct { + text string + stripIndent bool +} + +// heredocBodyFromRedirect returns the body text for a here-doc redirect +// (`<<` or `<<-`), sliced from the original command using positional +// offsets so we preserve the bytes exactly as Codex emitted them. For +// `<<-` (DashHdoc) we strip leading TAB characters from each line, per +// POSIX, so the body handed to ParseApplyPatch matches what Bash would +// have piped to apply_patch's stdin. +// +// The Hdoc Word's End() points just past the trailing delimiter token +// (e.g. `PATCH`), so the returned body includes that trailing line. +// ParseApplyPatch tolerates the extra line (it stops dispatching state +// at `*** End Patch`) so we don't try to trim it here — keeping the +// slice byte-exact makes the helper easier to reason about than one +// that does ad-hoc post-processing. +func heredocBodyFromRedirect(command string, r *syntax.Redirect) (heredocBody, bool) { + if r.Op != syntax.Hdoc && r.Op != syntax.DashHdoc { + return heredocBody{}, false + } + if r.Hdoc == nil { + return heredocBody{}, false + } + start := int(r.Hdoc.Pos().Offset()) + end := int(r.Hdoc.End().Offset()) + if start < 0 || end > len(command) || start >= end { + return heredocBody{}, false + } + body := command[start:end] + stripIndent := r.Op == syntax.DashHdoc + if stripIndent { + body = stripLeadingTabsPerLine(body) + } + return heredocBody{text: body, stripIndent: stripIndent}, true +} + +// stripLeadingTabsPerLine removes leading TAB characters (not spaces) +// from each newline-delimited line of body. Mirrors Bash's `<<-DELIM` +// semantics so ParseApplyPatch can match line prefixes like +// `*** Begin Patch` even when Codex emits the patch under the +// strip-tabs heredoc operator. +func stripLeadingTabsPerLine(body string) string { + if !strings.ContainsRune(body, '\t') { + return body + } + lines := strings.Split(body, "\n") + for i, line := range lines { + lines[i] = strings.TrimLeft(line, "\t") + } + return strings.Join(lines, "\n") } diff --git a/codex/apply_patch_test.go b/codex/apply_patch_test.go index 51bc18e..d8bdd12 100644 --- a/codex/apply_patch_test.go +++ b/codex/apply_patch_test.go @@ -313,3 +313,157 @@ func TestParseApplyPatchFromBash_HeredocVariants(t *testing.T) { } } } + +// --------------------------------------------------------------------------- +// Regression tests for two bypasses of the previous regex+strings.Index +// detector. The AST-based implementation closes both. See the doc comment +// on ParseApplyPatchFromBash for the full attack description. + +func TestParseApplyPatchFromBash_DecoyMarkerBeforeRealInvocation(t *testing.T) { + // Previously: strings.Index anchored on the decoy `*** Begin Patch` + // inside the printf string, so the regex's prefix was just + // `printf '` and detection returned ok=false — the real apply_patch + // heredoc below was silently dispatched as a plain Bash command, + // bypassing OnAfterFileEdit policies entirely. + cmd := "printf '*** Begin Patch\\n' >/dev/null; apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: /sensitive/path\n" + + "+malicious content\n" + + "*** End Patch\n" + + "PATCH" + + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("decoy-marker bypass: ParseApplyPatchFromBash returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "/sensitive/path" || files[0].Operation != "add" { + t.Errorf("files = %+v, want one add for /sensitive/path", files) + } +} + +func TestParseApplyPatchFromBash_ZeroSpaceBeforeHeredoc(t *testing.T) { + // Previously: the regex required `apply_patch[ \t]+<<`, so the + // zero-space `apply_patch<<'PATCH'` (valid Bash) evaded detection. + cmd := "apply_patch<<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: /tmp/zero-space.txt\n" + + "+hi\n" + + "*** End Patch\n" + + "PATCH" + + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("zero-space heredoc: ParseApplyPatchFromBash returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "/tmp/zero-space.txt" { + t.Errorf("files = %+v, want one add for /tmp/zero-space.txt", files) + } +} + +func TestParseApplyPatchFromBash_ZeroSpaceDashHeredoc(t *testing.T) { + // Combine the two edge cases: no space before <<-, and DashHdoc + // (tab-stripping) semantics. The detector must apply POSIX + // tab-stripping to the body before handing it to ParseApplyPatch, + // otherwise the leading tabs would prevent the `*** Begin Patch` + // line prefix match. + cmd := "apply_patch<<-PATCH\n" + + "\t*** Begin Patch\n" + + "\t*** Add File: a.txt\n" + + "\t+x\n" + + "\t*** End Patch\n" + + "\tPATCH" + + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("zero-space <<-PATCH: ParseApplyPatchFromBash returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "a.txt" { + t.Errorf("files = %+v, want one add for a.txt", files) + } +} + +func TestParseApplyPatchFromBash_MultipleInvocations(t *testing.T) { + // Two apply_patch invocations chained with `;` — both should be + // parsed and their files concatenated in source order. This guards + // against a future refactor that "first-match wins" silently drops + // edits from later invocations. + cmd := "apply_patch <<'A'\n" + + "*** Begin Patch\n" + + "*** Add File: first.txt\n" + + "+1\n" + + "*** End Patch\n" + + "A\n" + + "apply_patch <<'B'\n" + + "*** Begin Patch\n" + + "*** Add File: second.txt\n" + + "+2\n" + + "*** End Patch\n" + + "B" + + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("multi-invocation: ParseApplyPatchFromBash returned ok=false, want true") + } + if len(files) != 2 { + t.Fatalf("len(files) = %d, want 2 (one per invocation)", len(files)) + } + if files[0].FilePath != "first.txt" { + t.Errorf("files[0].FilePath = %q, want first.txt", files[0].FilePath) + } + if files[1].FilePath != "second.txt" { + t.Errorf("files[1].FilePath = %q, want second.txt", files[1].FilePath) + } +} + +func TestParseApplyPatchFromBash_InvocationInsideSubshell(t *testing.T) { + // `( ... )` runs the inner commands in a subshell. The AST walker + // must descend into the subshell so the apply_patch invocation is + // still detected — otherwise an agent could nest its real + // invocation inside `()` to evade the detector. + cmd := "(apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: nested.txt\n" + + "+x\n" + + "*** End Patch\n" + + "PATCH\n)" + + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("subshell-nested apply_patch: returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "nested.txt" { + t.Errorf("files = %+v, want one add for nested.txt", files) + } +} + +func TestParseApplyPatchFromBash_ApplyPatchTokenInsideStringSkipped(t *testing.T) { + // `apply_patch` appears only inside a single-quoted argument to + // echo — not as a command. The AST identifies the invocation as + // `echo`, so detection must return ok=false. + cmd := "echo 'apply_patch < Date: Mon, 18 May 2026 14:11:27 -0700 Subject: [PATCH 15/19] more regression tests --- codex/apply_patch_test.go | 163 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/codex/apply_patch_test.go b/codex/apply_patch_test.go index d8bdd12..a910763 100644 --- a/codex/apply_patch_test.go +++ b/codex/apply_patch_test.go @@ -467,3 +467,166 @@ func TestParseApplyPatchFromBash_NonHeredocInvocationSkipped(t *testing.T) { t.Error("apply_patch without heredoc wrongly detected as heredoc invocation") } } + +// --------------------------------------------------------------------------- +// Structured-construct regression tests. The previous regex+strings.Index +// detector only looked at byte-level neighborhood of `apply_patch`; nested +// invocations were detected by accident. The AST walker visits every Stmt, +// so any control-flow construct that contains an apply_patch heredoc must +// still be detected. These tests pin that behavior so a future refactor +// that limits the walk (e.g. "only top-level Stmts" or "stop at first +// match") can't silently regress detection coverage. + +func TestParseApplyPatchFromBash_InvocationInsideIfThen(t *testing.T) { + cmd := "if true; then apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: ifthen.txt\n" + + "+x\n" + + "*** End Patch\n" + + "PATCH\n" + + "fi" + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("if/then-nested apply_patch: returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "ifthen.txt" { + t.Errorf("files = %+v, want one add for ifthen.txt", files) + } +} + +func TestParseApplyPatchFromBash_InvocationInsideFunctionBody(t *testing.T) { + // A function defined and called in one block. Both the definition + // body (which is parsed by the AST walker) and the call site live + // inside the same Bash command. The invocation inside the function + // body must still trigger detection — otherwise a Codex prompt + // that wraps its writes in `do_patch(){ ... }; do_patch` would + // silently bypass the hook. The `}` and `do_patch` lines are + // formatted on separate lines because Bash requires the heredoc + // terminator to occupy a line by itself; placing `}` on the same + // line as `PATCH` would make the closing brace part of the body. + cmd := "do_patch() {\n" + + "apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: fnbody.txt\n" + + "+x\n" + + "*** End Patch\n" + + "PATCH\n" + + "}\n" + + "do_patch" + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("function-body apply_patch: returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "fnbody.txt" { + t.Errorf("files = %+v, want one add for fnbody.txt", files) + } +} + +func TestParseApplyPatchFromBash_InvocationAfterAndChain(t *testing.T) { + // Short-circuit `&&` chaining is the most common shape Codex + // emits for "do precondition then patch", e.g. `cd repo && + // apply_patch <<…`. The walker must descend into BinaryCmd nodes. + cmd := "cd /tmp && apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: andchain.txt\n" + + "+x\n" + + "*** End Patch\n" + + "PATCH" + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("&& chained apply_patch: returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "andchain.txt" { + t.Errorf("files = %+v, want one add for andchain.txt", files) + } +} + +func TestParseApplyPatchFromBash_InvocationInPipeline(t *testing.T) { + // apply_patch as the sink of a pipeline. Unusual (Codex normally + // feeds the patch through a heredoc, not stdin from another + // command), but valid Bash: the body still arrives via heredoc on + // the same Stmt as the apply_patch CallExpr. The walker must + // descend through the pipeline operator. + cmd := "echo unused | apply_patch <<'PATCH'\n" + + "*** Begin Patch\n" + + "*** Add File: pipeline.txt\n" + + "+x\n" + + "*** End Patch\n" + + "PATCH" + files, ok := ParseApplyPatchFromBash(cmd) + if !ok { + t.Fatal("pipeline-sink apply_patch: returned ok=false, want true") + } + if len(files) != 1 || files[0].FilePath != "pipeline.txt" { + t.Errorf("files = %+v, want one add for pipeline.txt", files) + } +} + +// --------------------------------------------------------------------------- +// False-positive guards on lookalike command names. The implementation +// matches the literal `apply_patch` or the suffix `*/apply_patch` to +// allow absolute paths to per-session shim binaries. Anything else must +// NOT be detected — otherwise a legitimate command like +// `apply_patcher < Date: Mon, 18 May 2026 14:15:23 -0700 Subject: [PATCH 16/19] Fix err-swallowing --- cmd/hookshot/main.go | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/cmd/hookshot/main.go b/cmd/hookshot/main.go index 796edeb..418a197 100644 --- a/cmd/hookshot/main.go +++ b/cmd/hookshot/main.go @@ -302,7 +302,10 @@ Flags:`) } func installClaude(binaryPath string) error { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolving home directory: %w", err) + } configPath := filepath.Join(homeDir, ".claude", "settings.json") fmt.Printf("Installing to Claude Code (%s)...\n", configPath) @@ -367,7 +370,10 @@ func installClaude(binaryPath string) error { } func installCursor(binaryPath string) error { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolving home directory: %w", err) + } configPath := filepath.Join(homeDir, ".cursor", "hooks.json") fmt.Printf("Installing to Cursor (%s)...\n", configPath) @@ -402,7 +408,10 @@ func installCursor(binaryPath string) error { } func installDroid(binaryPath string) error { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolving home directory: %w", err) + } configPath := filepath.Join(homeDir, ".factory", "settings.json") fmt.Printf("Installing to Factory Droid (%s)...\n", configPath) @@ -467,7 +476,10 @@ func installDroid(binaryPath string) error { } func installCodex(binaryPath string) error { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolving home directory: %w", err) + } configPath := filepath.Join(homeDir, ".codex", "hooks.json") fmt.Printf("Installing to OpenAI Codex (%s)...\n", configPath) @@ -542,7 +554,10 @@ func installCodex(binaryPath string) error { } func installCascade(binaryPath string) error { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolving home directory: %w", err) + } configPath := filepath.Join(homeDir, ".codeium", "windsurf", "hooks.json") fmt.Printf("Installing to Windsurf Cascade (%s)...\n", configPath) From 764676f2749ff4afb7f9f475082ce4d3c6f0b91a Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 18 May 2026 14:16:57 -0700 Subject: [PATCH 17/19] Fix heredoc --- codex/bash_write.go | 370 ++++++++++++++++++++++----------------- codex/bash_write_test.go | 143 +++++++++++++++ 2 files changed, 355 insertions(+), 158 deletions(-) diff --git a/codex/bash_write.go b/codex/bash_write.go index 4b6d2ed..f97ffe3 100644 --- a/codex/bash_write.go +++ b/codex/bash_write.go @@ -1,57 +1,18 @@ package codex import ( - "regexp" "strings" -) -// catRedirectHeredocRE matches the leading `cat <<['"]?DELIM['"]? > FILE` -// (or `>>`) shape that Codex uses for greenfield file writes. The regex -// must hold together because Codex emits this command without any -// surrounding context — it's the entire `tool_input.command` payload. -// -// Group layout (named for legibility in the implementation): -// -// op — the heredoc operator, "<<" or "<<-". -// delim — the heredoc delimiter ("EOF", "PATCH", …), with surrounding -// quotes (if any) stripped by the caller. -// redir — the redirection operator, ">" or ">>". -// path — the target file path. Anything up to the first whitespace, -// `&`, `;`, `|`, or end-of-line. -// -// Why this exact shape: the captured probe payloads (see -// vscode-extension/docs/HOOKS.md § "Probing Codex hooks") show Codex -// invoking writes as `cat <<'EOF' > greet.txt` for the greet.txt case -// and `cd … && cat <<'EOF' > path` for cwd-prefixed variants. Heredoc -// quoting can be single, double, or absent; the redirect operator can -// be `>` (overwrite) or `>>` (append). The optional `-` after `<<` -// covers the `<<-DELIM` shape that strips leading tabs from the body — -// uncommon in Codex output but cheap to support. -// `\s` would also greedy-match the trailing newline + any leading blank -// lines of the body. Use `[ \t]*` for header-line whitespace so the -// header match always ends at the EOL boundary and extractHeredocBody -// can preserve leading blank lines in the body exactly as written. -var catRedirectHeredocRE = regexp.MustCompile( - `(?m)(?:^|[;&|])[ \t]*cat[ \t]+(?P<<-?)[ \t]*(?P'[^']+'|"[^"]+"|[A-Za-z_][A-Za-z0-9_]*)[ \t]*(?P>>?)[ \t]*(?P[^\s;&|]+)[ \t]*$`, + "mvdan.cc/sh/v3/syntax" ) -// teeRedirectHeredocRE covers `tee [-a] FILE [< /dev/null]` and similar -// shapes where the body is fed from a heredoc opened earlier on the line. -// Less common than cat but inexpensive to handle, and the variant Codex -// occasionally emits when it wants tee's "also print to stdout" side -// effect. Captures match catRedirectHeredocRE's named groups so the -// caller can use a shared extractor. -var teeRedirectHeredocRE = regexp.MustCompile( - `(?m)(?:^|[;&|])[ \t]*tee[ \t]+(?:-a[ \t]+)?(?P[^\s;&|]+)[ \t]*(?:<[ \t]*/dev/null[ \t]*)?(?P<<-?)[ \t]*(?P'[^']+'|"[^"]+"|[A-Za-z_][A-Za-z0-9_]*)[ \t]*$`, -) - -// ParseBashRedirectWrite inspects a Codex Bash tool command and, if the -// command is a heredoc-based file write (`cat < FILE … EOF`), -// returns a synthetic PatchFile slice describing the write so the -// OnAfterFileEdit bridge can dispatch it through the same per-file -// pipeline as apply_patch invocations. The boolean return reports -// whether a write was detected, mirroring ParseApplyPatchFromBash's -// shape so callers can chain detectors: +// ParseBashRedirectWrite inspects a Codex Bash tool command and, for +// every heredoc-based file write detected anywhere in the command, +// returns a synthetic PatchFile so the OnAfterFileEdit bridge can +// dispatch each write through the same per-file pipeline as +// apply_patch invocations. The boolean return reports whether at least +// one write was detected, mirroring ParseApplyPatchFromBash's shape so +// callers can chain detectors: // // if files, ok := ParseApplyPatchFromBash(cmd); ok { … } // if files, ok := ParseBashRedirectWrite(cmd); ok { … } @@ -63,152 +24,245 @@ var teeRedirectHeredocRE = regexp.MustCompile( // on every greenfield write — no afterFileEdit fires, no security scan // runs, and the dashboard shows zero SecurityScanResult rows for any // Codex session that only creates new files (the exact symptom that -// motivated this helper; see the regression test below). +// motivated this helper; see the regression tests below). +// +// Implementation note: detection runs the command through a real Bash +// parser (mvdan.cc/sh/v3/syntax) and walks the AST looking for +// CallExpr statements whose primary command is `cat` or `tee` paired +// with a heredoc redirect (`<<` or `<<-`, quoted or unquoted) on the +// same Stmt. This replaced an earlier regex-based implementation that +// reported only the first match in a command — a multi-heredoc shape +// like // -// Heuristics are deliberately tight to keep false positives low: +// cat <<'EOF' > allowed.txt +// ok +// EOF +// cat <<'EOF' > .env +// TOKEN=secret +// EOF // -// 1. The command must contain a `cat <<…> FILE` or `tee FILE <<…` -// line that ends the logical statement (anchored to end-of-line so -// trailing tokens like `&& echo done` don't mask the redirect). -// 2. The heredoc delimiter is extracted as-emitted (`'EOF'`, `"EOF"`, -// or bare `EOF`); single- or double-quotes around it are stripped -// before scanning the body — Bash treats quoted delimiters as a -// "no variable expansion" hint, which doesn't change our body -// extraction. -// 3. Body extraction walks from the line after the matched cat/tee -// invocation forward until it sees a line that is exactly the -// delimiter (no leading/trailing whitespace, except for `<<-` -// which permits leading tabs per POSIX). If the delimiter is -// missing the parser returns ok=false rather than guessing — the -// command isn't a well-formed heredoc write. +// dispatched only the allowed.txt write to OnAfterFileEdit, silently +// bypassing path-deny rules and secret scanners for the .env write. A +// real parser also closes a handful of adjacent gaps the regex never +// covered: redirects in reverse order (`cat > FILE < "name with spaces.txt"`), writes nested inside compound +// commands (`cd … && cat …`, `if … then cat … fi`, pipelines), and +// commands that mix `cat` and `tee` heredoc writes. Every recognised +// write becomes a PatchFile in the order it appears in the command. // -// We never try to evaluate the file path: anything from the post-redir -// token up to the next whitespace/separator is treated as a literal -// path. That matches what Codex emits and avoids us silently rewriting -// `>` redirects to fd numbers or process substitutions, which a real -// shell parser would have to handle. +// We deliberately fail closed in two places. (1) If the command is +// not valid Bash (parser error) this returns (nil, false) — silently +// treating malformed input as a no-op write would let an attacker +// construct a heredoc the parser rejects but Codex's actual shell +// accepts. (2) If a cat/tee statement has a heredoc but no extractable +// target path (e.g. `cat < $(some_cmd)`), that single write is +// dropped from the result but the rest of the command's writes are +// still reported, and ok stays true as long as at least one well-formed +// write was found. Callers that need a stricter posture should layer +// their own checks; the unified bridge then surfaces the raw command +// through the existing fallback path. func ParseBashRedirectWrite(command string) ([]PatchFile, bool) { + // Cheap pre-check: every heredoc-style write contains `<<`. This + // keeps the parser allocation off the hot path for plain Bash + // commands (`pwd`, `ls`, `git diff`, …) which dominate real Codex + // traffic. if !strings.Contains(command, "<<") { return nil, false } - if files, ok := matchWriteRedirect(command, catRedirectHeredocRE); ok { - return files, true + file, err := syntax.NewParser().Parse(strings.NewReader(command), "") + if err != nil { + return nil, false } - if files, ok := matchWriteRedirect(command, teeRedirectHeredocRE); ok { - return files, true + + var files []PatchFile + syntax.Walk(file, func(node syntax.Node) bool { + stmt, ok := node.(*syntax.Stmt) + if !ok { + return true + } + if patch, ok := bashRedirectWriteFromStmt(command, stmt); ok { + files = append(files, patch) + } + return true + }) + if len(files) == 0 { + return nil, false } - return nil, false + return files, true } -func matchWriteRedirect(command string, re *regexp.Regexp) ([]PatchFile, bool) { - // FindStringSubmatchIndex returns byte offsets so we can locate the - // heredoc body that begins on the line after the matched header. - idx := re.FindStringSubmatchIndex(command) - if idx == nil { - return nil, false +// bashRedirectWriteFromStmt extracts a PatchFile from stmt if its +// primary command is a recognised heredoc-style file write (cat or +// tee). Any other shape returns ok=false so syntax.Walk can quietly +// skip it. +func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) (PatchFile, bool) { + call, ok := stmt.Cmd.(*syntax.CallExpr) + if !ok || len(call.Args) == 0 { + return PatchFile{}, false + } + switch call.Args[0].Lit() { + case "cat": + return catRedirectWrite(command, stmt) + case "tee": + return teeRedirectWrite(command, stmt, call) } + return PatchFile{}, false +} - names := re.SubexpNames() - groups := map[string]string{} - for i, name := range names { - if name == "" { - continue - } - if start, end := idx[2*i], idx[2*i+1]; start >= 0 && end >= 0 { - groups[name] = command[start:end] +// catRedirectWrite handles `cat < FILE` and `cat > FILE <`/`>>` target as separate +// entries on stmt.Redirs; we pick the last `>`/`>>` (Bash semantics — +// a later redirect on the same fd shadows an earlier one) and require +// exactly one heredoc on the statement. Two heredocs on a single cat +// is invalid Bash and never emitted by Codex; we bail rather than +// guess which one feeds the file. +func catRedirectWrite(command string, stmt *syntax.Stmt) (PatchFile, bool) { + var hdoc, out *syntax.Redirect + for _, r := range stmt.Redirs { + switch r.Op { + case syntax.Hdoc, syntax.DashHdoc: + if hdoc != nil { + return PatchFile{}, false + } + hdoc = r + case syntax.RdrOut, syntax.AppOut: + out = r } } - - delim := stripDelimQuotes(groups["delim"]) - if delim == "" { - return nil, false + if hdoc == nil || out == nil { + return PatchFile{}, false } - path := strings.TrimSpace(groups["path"]) + path := wordText(command, out.Word) if path == "" { - return nil, false + return PatchFile{}, false } - - stripIndent := groups["op"] == "<<-" - body, ok := extractHeredocBody(command, idx[1], delim, stripIndent) + body, ok := heredocBodyContent(command, hdoc) if !ok { - return nil, false + return PatchFile{}, false } - - return []PatchFile{{ + return PatchFile{ Operation: "add", FilePath: path, Edits: []PatchEdit{{ OldString: "", NewString: body, }}, - }}, true + }, true } -// stripDelimQuotes removes surrounding single or double quotes from a -// heredoc delimiter token. Bash treats `<<'EOF'`, `<<"EOF"`, and -// `<= 2 { - first, last := delim[0], delim[len(delim)-1] - if (first == '\'' || first == '"') && first == last { - return delim[1 : len(delim)-1] +// teeRedirectWrite handles `tee [-a] FILE [< /dev/null] < len(command) || start >= end { + return "" + } + return command[start:end] +} - rest := command[start:] - lines := strings.SplitAfter(rest, "\n") - - var body strings.Builder - for _, raw := range lines { - // Drop the trailing newline so we can compare against delim - // without worrying about \r\n vs \n. - line := strings.TrimRight(raw, "\r\n") - cmpLine := line - if stripIndent { - cmpLine = strings.TrimLeft(cmpLine, "\t") - } - if cmpLine == delim { - result := body.String() - result = strings.TrimSuffix(result, "\n") - return result, true - } - if stripIndent { - line = strings.TrimLeft(line, "\t") - } - body.WriteString(line) - if strings.HasSuffix(raw, "\n") { - body.WriteByte('\n') +// wordToLiteral returns w's effective literal text if every part is a +// plain literal or quoted literal (no expansions). The second return +// is false when the word requires shell evaluation; callers fall back +// to the verbatim source slice via wordText. +func wordToLiteral(w *syntax.Word) (string, bool) { + var b strings.Builder + for _, part := range w.Parts { + switch p := part.(type) { + case *syntax.Lit: + b.WriteString(p.Value) + case *syntax.SglQuoted: + b.WriteString(p.Value) + case *syntax.DblQuoted: + for _, inner := range p.Parts { + lit, ok := inner.(*syntax.Lit) + if !ok { + return "", false + } + b.WriteString(lit.Value) + } + default: + return "", false } } - // Missing terminator — bail rather than silently treating the - // remainder of the command as body. A well-formed Codex command - // always terminates the heredoc. - return "", false + return b.String(), true +} + +// heredocBodyContent returns the on-disk content a heredoc redirect +// would have produced — the body bytes with the trailing delimiter +// line removed and POSIX `<<-` tab-stripping applied. mvdan.cc/sh's +// Hdoc Word spans `body…\nDELIM`, so we strip from the last newline +// onward to drop the terminator. An entirely empty heredoc +// (`cat <= 0 { + return text[:idx], true + } + return "", true } diff --git a/codex/bash_write_test.go b/codex/bash_write_test.go index c8021fe..f84185d 100644 --- a/codex/bash_write_test.go +++ b/codex/bash_write_test.go @@ -193,3 +193,146 @@ func TestParseBashRedirectWrite_BodyWithEmptyLines(t *testing.T) { t.Errorf("NewString = %q, want %q", got[0].Edits[0].NewString, want) } } + +// Regression: the original regex-based parser used FindStringSubmatchIndex +// inside matchWriteRedirect, which only returns the first match. A +// Codex Bash command chaining two heredoc writes would dispatch only +// the first to OnAfterFileEdit, silently bypassing path-deny rules, +// secret scanners, and audit logging for every subsequent write. See +// the security report on the cursor/add-codex-support branch for the +// attack-path narrative; the literal payload below is the canonical +// shape from that report. +func TestParseBashRedirectWrite_MultipleCatHeredocs_AllReported(t *testing.T) { + cmd := "cat <<'EOF' > allowed.txt\nok\nEOF\ncat <<'EOF' > .env\nTOKEN=secret\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := []PatchFile{ + {Operation: "add", FilePath: "allowed.txt", Edits: []PatchEdit{{OldString: "", NewString: "ok"}}}, + {Operation: "add", FilePath: ".env", Edits: []PatchEdit{{OldString: "", NewString: "TOKEN=secret"}}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ParseBashRedirectWrite =\n %+v\nwant\n %+v", got, want) + } +} + +// Regression: multi-heredoc commands prefixed with the `cd … && cat …` +// shape Codex often emits — Stmts nested inside BinaryCmds must still +// be visited by the AST walk and reported alongside the top-level +// writes that follow them. +func TestParseBashRedirectWrite_MultipleCatHeredocs_WithCdPrefix(t *testing.T) { + cmd := "cd /tmp && cat <<'EOF' > a.txt\nA\nEOF\ncat <<'EOF' > b.txt\nB\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if len(got) != 2 { + t.Fatalf("expected 2 PatchFiles, got %d: %+v", len(got), got) + } + if got[0].FilePath != "a.txt" || got[1].FilePath != "b.txt" { + t.Errorf("FilePaths = [%q, %q], want [a.txt, b.txt]", got[0].FilePath, got[1].FilePath) + } +} + +// Regression: a command that mixes cat and tee heredoc writes must +// report both. The original ParseBashRedirectWrite returned on the +// first matchWriteRedirect hit, so the catRedirectHeredocRE branch +// short-circuited the teeRedirectHeredocRE branch even when the +// command contained a tee write the cat regex didn't match. +func TestParseBashRedirectWrite_CatAndTeeMixed_BothReported(t *testing.T) { + cmd := "cat <<'EOF' > from_cat.txt\nA\nEOF\ntee from_tee.txt <<'EOF'\nB\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if len(got) != 2 { + t.Fatalf("expected 2 PatchFiles, got %d: %+v", len(got), got) + } + if got[0].FilePath != "from_cat.txt" || got[1].FilePath != "from_tee.txt" { + t.Errorf("FilePaths = [%q, %q], want [from_cat.txt, from_tee.txt]", got[0].FilePath, got[1].FilePath) + } +} + +// Regression: redirects can legally appear before the heredoc in Bash +// (`cat > FILE <` after the +// heredoc declarator and missed this shape entirely; the AST walk +// finds both redirects regardless of order. +func TestParseBashRedirectWrite_CatReverseRedirectOrder(t *testing.T) { + cmd := "cat > greet.txt <<'EOF'\nhello\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "greet.txt" { + t.Errorf("FilePath = %q, want greet.txt", got[0].FilePath) + } + if got[0].Edits[0].NewString != "hello" { + t.Errorf("NewString = %q, want hello", got[0].Edits[0].NewString) + } +} + +// Regression: paths that need shell quoting (spaces, special chars) +// were broken by the regex's `[^\s;&|]+` path class. The AST parser +// gives us a syntax.Word we can unquote correctly. +func TestParseBashRedirectWrite_QuotedPathWithSpaces(t *testing.T) { + cmd := "cat <<'EOF' > \"name with spaces.txt\"\nbody\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "name with spaces.txt" { + t.Errorf("FilePath = %q, want %q", got[0].FilePath, "name with spaces.txt") + } +} + +// Regression: writes nested inside an `if`/`fi` block (and other +// compound shapes) must still be visited by the walker. The regex +// implementation happened to match these because it ignored block +// structure entirely; the AST implementation has to actively recurse +// into compound commands. This locks that recursion in. +func TestParseBashRedirectWrite_HeredocInsideIfBlock(t *testing.T) { + cmd := "if true; then cat <<'EOF' > out.txt\nbody\nEOF\nfi" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if len(got) != 1 || got[0].FilePath != "out.txt" { + t.Fatalf("got %+v, want one PatchFile with FilePath=out.txt", got) + } +} + +// Regression: an invalid Bash command (unclosed heredoc, dangling +// quote, etc.) must fail closed rather than fall back to a partial +// parse. The unified bridge surfaces the raw command through its +// fallback path when both detectors return ok=false, so the +// underlying policy still sees the event — but only after the +// detector has explicitly declined to invent a PatchFile out of +// malformed input. +func TestParseBashRedirectWrite_InvalidBash_FailsClosed(t *testing.T) { + cmd := "cat <<'EOF' > greet.txt\nhello\n" // missing terminator + + if files, ok := ParseBashRedirectWrite(cmd); ok { + t.Errorf("expected ok=false on invalid Bash; got %+v", files) + } +} + +// Regression: when one heredoc in a multi-write command is well-formed +// and a later token makes the command unparseable, we should not +// silently return the well-formed write — the entire command is +// suspect and the unified bridge's fallback path is more appropriate. +func TestParseBashRedirectWrite_PartiallyMalformed_FailsClosed(t *testing.T) { + // Second heredoc never terminates; parser will reject the whole + // command. The first write must not slip through. + cmd := "cat <<'EOF' > a.txt\nA\nEOF\ncat <<'EOF' > b.txt\nB\n" + + if files, ok := ParseBashRedirectWrite(cmd); ok { + t.Errorf("expected ok=false when later heredoc lacks terminator; got %+v", files) + } +} From 4367d02ab3eb06b61e986e4f1c79d499813ca72e Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 18 May 2026 14:20:32 -0700 Subject: [PATCH 18/19] Add regression tests --- codex/bash_write_test.go | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/codex/bash_write_test.go b/codex/bash_write_test.go index f84185d..9719d81 100644 --- a/codex/bash_write_test.go +++ b/codex/bash_write_test.go @@ -308,6 +308,56 @@ func TestParseBashRedirectWrite_HeredocInsideIfBlock(t *testing.T) { } } +// Regression: a quoted target path (single or double quotes) must be +// reported to OnAfterFileEdit with the surrounding quotes stripped — +// i.e. the value Bash would actually write to, not the surface token. +// +// The original regex implementation captured the path via +// `[^\s;&|]+`, which happily included surrounding quote characters, +// so policies running `filepath.Rel(repoRoot, ctx.FilePath)` followed +// by `strings.HasPrefix(rel, "..")` would see a string starting with +// `'` or `"` and the containment check would silently pass while the +// dangerous write proceeded. The AST-based parser unquotes +// SglQuoted/DblQuoted Word parts via wordToLiteral; this test pins +// that behavior so a future refactor can't regress it. +func TestParseBashRedirectWrite_CatSingleQuotedPathEscapesContainment(t *testing.T) { + cmd := "cat <<'EOF' > '../../.ssh/authorized_keys'\nattacker-controlled\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "../../.ssh/authorized_keys" { + t.Errorf("FilePath = %q, want unquoted %q (quoted-path containment bypass)", + got[0].FilePath, "../../.ssh/authorized_keys") + } +} + +func TestParseBashRedirectWrite_CatDoubleQuotedPathEscapesContainment(t *testing.T) { + cmd := "cat <<'EOF' > \"../../.ssh/authorized_keys\"\nattacker-controlled\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "../../.ssh/authorized_keys" { + t.Errorf("FilePath = %q, want unquoted %q", + got[0].FilePath, "../../.ssh/authorized_keys") + } +} + +func TestParseBashRedirectWrite_TeeQuotedPathEscapesContainment(t *testing.T) { + cmd := "tee '../../.ssh/authorized_keys' <<'EOF'\nattacker-controlled\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "../../.ssh/authorized_keys" { + t.Errorf("FilePath = %q, want unquoted %q", got[0].FilePath, "../../.ssh/authorized_keys") + } +} + // Regression: an invalid Bash command (unclosed heredoc, dangling // quote, etc.) must fail closed rather than fall back to a partial // parse. The unified bridge surfaces the raw command through its From 059943e8ffcce9c896544a71925f1e74e724d808 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 18 May 2026 15:24:52 -0700 Subject: [PATCH 19/19] fix --- codex/bash_write.go | 174 +++++++++++++++++++++++++++------------ codex/bash_write_test.go | 162 ++++++++++++++++++++++++++++++++++++ 2 files changed, 285 insertions(+), 51 deletions(-) diff --git a/codex/bash_write.go b/codex/bash_write.go index f97ffe3..c1eaaa0 100644 --- a/codex/bash_write.go +++ b/codex/bash_write.go @@ -46,19 +46,28 @@ import ( // real parser also closes a handful of adjacent gaps the regex never // covered: redirects in reverse order (`cat > FILE < "name with spaces.txt"`), writes nested inside compound -// commands (`cd … && cat …`, `if … then cat … fi`, pipelines), and -// commands that mix `cat` and `tee` heredoc writes. Every recognised -// write becomes a PatchFile in the order it appears in the command. +// commands (`cd … && cat …`, `if … then cat … fi`, pipelines), +// commands that mix `cat` and `tee` heredoc writes, multi-target tee +// (`tee a.txt b.txt < .env 2> log` correctly +// identifies .env as the write target rather than letting the later 2> +// entry overwrite our pick). Every recognised write becomes a +// PatchFile in the order it appears in the command. // -// We deliberately fail closed in two places. (1) If the command is +// We deliberately fail closed in three places. (1) If the command is // not valid Bash (parser error) this returns (nil, false) — silently // treating malformed input as a no-op write would let an attacker // construct a heredoc the parser rejects but Codex's actual shell -// accepts. (2) If a cat/tee statement has a heredoc but no extractable +// accepts. (2) If a cat statement has a heredoc but no extractable // target path (e.g. `cat < $(some_cmd)`), that single write is // dropped from the result but the rest of the command's writes are // still reported, and ok stays true as long as at least one well-formed -// write was found. Callers that need a stricter posture should layer +// write was found. (3) If a tee statement has a heredoc but ANY of its +// target paths can't be cleanly extracted, the entire statement is +// dropped — partial coverage of a multi-target tee is the exact +// confused-deputy bypass we're trying to prevent, so we'd rather hand +// the raw command to the unified bridge's fallback than report a +// subset of writes. Callers that need a stricter posture should layer // their own checks; the unified bridge then surfaces the raw command // through the existing fallback path. func ParseBashRedirectWrite(command string) ([]PatchFile, bool) { @@ -81,8 +90,8 @@ func ParseBashRedirectWrite(command string) ([]PatchFile, bool) { if !ok { return true } - if patch, ok := bashRedirectWriteFromStmt(command, stmt); ok { - files = append(files, patch) + if patches, ok := bashRedirectWriteFromStmt(command, stmt); ok { + files = append(files, patches...) } return true }) @@ -92,14 +101,16 @@ func ParseBashRedirectWrite(command string) ([]PatchFile, bool) { return files, true } -// bashRedirectWriteFromStmt extracts a PatchFile from stmt if its -// primary command is a recognised heredoc-style file write (cat or -// tee). Any other shape returns ok=false so syntax.Walk can quietly -// skip it. -func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) (PatchFile, bool) { +// bashRedirectWriteFromStmt extracts one PatchFile per write target from +// stmt if its primary command is a recognised heredoc-style file write +// (cat or tee). Most statements yield a single PatchFile, but `tee +// FILE1 FILE2 …` produces one PatchFile per target so every write goes +// through the per-file policy pipeline. Any other shape returns +// ok=false so syntax.Walk can quietly skip it. +func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) ([]PatchFile, bool) { call, ok := stmt.Cmd.(*syntax.CallExpr) if !ok || len(call.Args) == 0 { - return PatchFile{}, false + return nil, false } switch call.Args[0].Lit() { case "cat": @@ -107,66 +118,106 @@ func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) (PatchFile, bo case "tee": return teeRedirectWrite(command, stmt, call) } - return PatchFile{}, false + return nil, false } // catRedirectWrite handles `cat < FILE` and `cat > FILE <`/`>>` target as separate -// entries on stmt.Redirs; we pick the last `>`/`>>` (Bash semantics — -// a later redirect on the same fd shadows an earlier one) and require -// exactly one heredoc on the statement. Two heredocs on a single cat -// is invalid Bash and never emitted by Codex; we bail rather than -// guess which one feeds the file. -func catRedirectWrite(command string, stmt *syntax.Stmt) (PatchFile, bool) { +// entries on stmt.Redirs; we pick the last `>`/`>>` on fd 1 (Bash +// semantics — a later redirect on the same fd shadows an earlier one) +// and require exactly one heredoc on the statement. Two heredocs on a +// single cat is invalid Bash and never emitted by Codex; we bail +// rather than guess which one feeds the file. +// +// Crucially we filter out redirects on file descriptors other than +// stdout (fd 1). mvdan.cc/sh's parser represents `2> stderr.log` as +// {Op: RdrOut, N: lit("2"), …} — the same RedirOperator as `> file`, +// distinguished only by the fd field. Without this filter, a command +// like `cat < .env 2> allowed.log` would have `out` overwritten +// by the (later) stderr entry and report `allowed.log` to +// OnAfterFileEdit — leaving the actual .env write completely +// unmonitored. `&>` (RdrAll) and `&>>` (AppAll) combine stdout+stderr +// to one file; that file IS a real write target, so we treat them +// like stdout-bound RdrOut/AppOut. +func catRedirectWrite(command string, stmt *syntax.Stmt) ([]PatchFile, bool) { var hdoc, out *syntax.Redirect for _, r := range stmt.Redirs { switch r.Op { case syntax.Hdoc, syntax.DashHdoc: if hdoc != nil { - return PatchFile{}, false + return nil, false } hdoc = r case syntax.RdrOut, syntax.AppOut: + if !redirectsStdout(r) { + continue + } + out = r + case syntax.RdrAll, syntax.AppAll: out = r } } if hdoc == nil || out == nil { - return PatchFile{}, false + return nil, false } path := wordText(command, out.Word) if path == "" { - return PatchFile{}, false + return nil, false } body, ok := heredocBodyContent(command, hdoc) if !ok { - return PatchFile{}, false + return nil, false } - return PatchFile{ + return []PatchFile{{ Operation: "add", FilePath: path, Edits: []PatchEdit{{ OldString: "", NewString: body, }}, - }, true + }}, true } -// teeRedirectWrite handles `tee [-a] FILE [< /dev/null] <` → +// N.Value == "2"); a nil/empty N means the default, which for `>` and +// `>>` is fd 1. Anything other than nil/empty/"1" is some other fd +// (stderr, a custom log fd, …) and must NOT be treated as the +// command's primary write target. +func redirectsStdout(r *syntax.Redirect) bool { + if r.N == nil { + return true + } + switch r.N.Value { + case "", "1": + return true + } + return false } // wordText returns a usable string representation of a syntax.Word — diff --git a/codex/bash_write_test.go b/codex/bash_write_test.go index 9719d81..d3b0f59 100644 --- a/codex/bash_write_test.go +++ b/codex/bash_write_test.go @@ -386,3 +386,165 @@ func TestParseBashRedirectWrite_PartiallyMalformed_FailsClosed(t *testing.T) { t.Errorf("expected ok=false when later heredoc lacks terminator; got %+v", files) } } + +// Regression: tee writes its stdin to EVERY file operand, not just +// the first. The original teeRedirectWrite extracted only the first +// non-option positional arg and broke out of the loop, so a command +// like `tee allowed.txt ../../.ssh/authorized_keys < stderr.log` as +// {Op: RdrOut, N: lit("2"), …} — the same RedirOperator as a stdout +// `> file` redirect, distinguished only by the fd field N. The +// original catRedirectWrite ignored N and just picked the last +// RdrOut/AppOut entry, so a command like +// `cat < .env 2> allowed.log` would have `out` overwritten by +// the (later) stderr entry and report `allowed.log` as the write +// target. Path-deny rules on `.env` and secret scanners would never +// see the actual write — a complete bypass with no expansion trick. +func TestParseBashRedirectWrite_CatStderrShadowsStdoutTarget(t *testing.T) { + cmd := "cat <<'EOF' > .env 2> allowed.log\nTOKEN=secret\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if len(got) != 1 { + t.Fatalf("expected 1 PatchFile, got %d: %+v", len(got), got) + } + if got[0].FilePath != ".env" { + t.Errorf("FilePath = %q, want .env (stderr 2> redirect must not shadow stdout target)", + got[0].FilePath) + } + if got[0].Edits[0].NewString != "TOKEN=secret" { + t.Errorf("NewString = %q, want %q", got[0].Edits[0].NewString, "TOKEN=secret") + } +} + +// Regression: explicit `1>` is identical to bare `>` — both target +// stdout. Verify the fd filter accepts `1` as a synonym for "no fd". +func TestParseBashRedirectWrite_CatExplicitStdoutFd(t *testing.T) { + cmd := "cat <<'EOF' 1> out.txt\nbody\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "out.txt" { + t.Errorf("FilePath = %q, want out.txt", got[0].FilePath) + } +} + +// Regression: a cat statement whose ONLY `>` redirect is on a +// non-stdout fd (e.g. `cat < log.txt` — stdin from heredoc, +// stderr to log.txt, stdout discarded) has no actual write target we +// can attribute to the heredoc body. The heredoc body just goes to +// stdout (which here is the default tty/pipe), so we must NOT report +// log.txt as if the heredoc landed there. The detector returns +// ok=false and the unified bridge's fallback handles the raw command. +func TestParseBashRedirectWrite_CatStderrOnlyRedirect_NotReported(t *testing.T) { + cmd := "cat <<'EOF' 2> log.txt\nbody\nEOF" + + if files, ok := ParseBashRedirectWrite(cmd); ok { + t.Errorf("expected ok=false when only redirect is non-stdout; got %+v", files) + } +} + +// Regression: `&>` (RdrAll) redirects both stdout and stderr to a +// single file. That file IS the heredoc's write target, so it must +// be reported. Without explicit handling of RdrAll/AppAll, a command +// like `cat < .env` would silently bypass detection. +func TestParseBashRedirectWrite_CatRdrAll(t *testing.T) { + cmd := "cat <<'EOF' &> combined.log\nbody\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + if got[0].FilePath != "combined.log" { + t.Errorf("FilePath = %q, want combined.log", got[0].FilePath) + } +} + +// Regression: a tee statement with multiple targets where ONE target +// is unparseable (empty word offsets, etc.) must fail closed for the +// whole statement — partial coverage of a multi-target tee is the +// exact bypass we're guarding against. We trigger the empty-path +// codepath via `tee "" foo.txt <