Commit Graph

8 Commits

  • [codex] Use expect in integration tests (#28441)
    The workspace denies `clippy::expect_used` in production. Although
    `clippy.toml` allows `expect` in tests, Bazel Clippy compiles
    integration-test helper code in a way that does not receive that
    exemption, which encouraged verbose `unwrap_or_else(... panic!(...))`
    and equivalent `match`/`let else` forms.
    
    This allows `clippy::expect_used` once at each integration-test crate
    root (including aggregated suites and test-support libraries), then
    replaces manual panic-based Result and Option unwraps with
    `expect`/`expect_err`. Standalone `tests/*.rs` files remain their own
    crate roots. Intentional assertion and unexpected-variant panics remain
    unchanged, and the production `expect_used = "deny"` lint remains in
    place.
    
    The cleanup is mechanical and net-negative in line count.
  • [codex] Wait for MCP readiness in core integration tests (#24964)
    Ensures MCP-backed `codex-core` integration tests exercise initialized
    servers instead of racing server startup.
    
    I've been idly investigating a few flakes and the failure modes are much
    more confusing when a tool call fails because of a failed server start
    than when the failed server start causes the test to fail directly.
  • Move MCP tool naming mode into manager (#21576)
    ## Why
    
    The `non_prefixed_mcp_tool_names` feature should be applied where MCP
    tools become model-visible, not by remapping names later in core.
    Keeping the decision in `McpConnectionManager` construction makes
    `ToolInfo` the single shaped view that spec building, deferred tool
    search, routing, and unavailable-tool placeholders can consume directly.
    
    This also preserves the existing external behavior while the feature is
    off, and keeps the feature-on behavior for code mode and hooks explicit
    at the manager boundary.
    
    ## What Changed
    
    - Add `McpToolNameMode` to `codex-mcp` and flow it through `McpConfig`
    into `McpConnectionManager::new`.
    - Normalize MCP `ToolInfo` names in the manager using either
    legacy-prefixed namespaces or non-prefixed namespaces; the legacy path
    adds `mcp__` without restoring the old trailing namespace suffix.
    - Remove the core-side MCP name remapping path so specs, tool search,
    session resolution, and unavailable-tool placeholder construction use
    the manager-provided `ToolName` values directly.
    - Keep code mode flattening on the `__` namespace separator.
    - Preserve hook compatibility by giving non-prefixed MCP hook names
    legacy `mcp__...` matcher aliases.
    - Add/adjust integration and unit coverage for non-prefixed code-mode
    behavior, hook matching with the feature on and off, and manager-level
    legacy prefixing.
    
    ## Testing
    
    - `cargo test -p codex-mcp --lib`
    - `cargo test -p codex-core --lib tools::spec::tests -- --nocapture`
    - `cargo test -p codex-core --lib mcp_tools -- --nocapture`
    - `cargo test -p codex-core --lib mcp_tool_exposure -- --nocapture`
    - `cargo test -p codex-core --test all mcp_tool -- --nocapture`
    - `cargo test -p codex-core --test all search_tool -- --nocapture`
    - `cargo test -p codex-core --test all hooks_mcp -- --nocapture`
    - `cargo test -p codex-core --test all
    code_mode_uses_non_prefixed_mcp_tool_names_when_feature_enabled --
    --nocapture`
    - `cargo test -p codex-tools`
    - `cargo test -p codex-features`
  • Route MCP servers through explicit environments (#23583)
    ## Summary
    - route each configured MCP server through an explicit per-server
    `environment_id` instead of a manager-wide remote toggle
    - default omitted `environment_id` to `local`, resolve named ids through
    `EnvironmentManager`, and fail only the affected MCP server when an
    explicit id is unknown
    - keep local stdio on the existing local launcher path for now, while
    named-environment stdio uses the selected environment backend and
    requires an absolute `cwd`
    - allow local HTTP MCP servers to keep using the ambient HTTP client
    when no local `Environment` is configured; named-environment HTTP MCPs
    use that environment's HTTP client
    
    ## Validation
    - devbox Bazel build: `bazel build --bes_backend= --bes_results_url=
    //codex-rs/cli:codex //codex-rs/rmcp-client:test_stdio_server
    //codex-rs/rmcp-client:test_streamable_http_server`
    - devbox app-server config matrix with real `config.toml` /
    `environments.toml` files covering omitted local, explicit local,
    omitted local under remote default, explicit remote stdio, local HTTP
    without local env, explicit remote HTTP, local stdio without local env,
    unknown explicit env, and remote stdio without `cwd`
  • Support explicit MCP OAuth client IDs (#22575)
    ## Why
    Some MCP OAuth providers require a pre-registered public client ID and
    cannot rely on dynamic client registration. Codex already supports MCP
    OAuth, but it had no way to supply that client ID from config into the
    PKCE flow.
    
    ## What changed
    - add `oauth.client_id` under `[mcp_servers.<server>]` config, including
    config editing and schema generation
    - thread the configured client ID through CLI, app-server, plugin login,
    and MCP skill dependency OAuth entrypoints
    - configure RMCP authorization with the explicit client when present,
    while preserving the existing dynamic-registration path when it is
    absent
    - add focused coverage for config parsing/serialization and OAuth URL
    generation
    
    ## Verification
    - `cargo test -p codex-config -p codex-rmcp-client -p codex-mcp -p
    codex-core-plugins`
    - `cargo test -p codex-core blocking_replace_mcp_servers_round_trips
    --lib`
    - `cargo test -p codex-core
    replace_mcp_servers_streamable_http_serializes_oauth_resource --lib`
    - `cargo test -p codex-core config_schema_matches_fixture --lib`
    
    ## Notes
    Broader local package runs still hit unrelated pre-existing stack
    overflows in:
    - `codex-app-server::in_process_start_clamps_zero_channel_capacity`
    -
    `codex-core::resume_agent_from_rollout_uses_edge_data_when_descendant_metadata_source_is_stale`
  • Support PreToolUse updatedInput rewrites (#20527)
    ## Why
    
    `PreToolUse` already exposes `updatedInput` in its hook output schema,
    but Codex currently rejects it instead of applying the rewrite. That
    leaves hook authors unable to make the documented pre-execution
    adjustment to a tool call before it runs.
    
    ## What
    
    - Accept `updatedInput` from `PreToolUse` hooks when paired with
    `permissionDecision: "allow"`.
    - Apply the rewritten input before dispatch so the tool executes the
    updated payload, not the original one.
    - Preserve the stable hook-facing compatibility shapes that
    participating tool handlers expose:
    - Bash-like tools (`shell`, `container.exec`, `local_shell`,
    `shell_command`, `exec_command`) use `{ "command": ... }`.
    - `apply_patch` exposes its patch body through the same command-shaped
    hook contract.
      - MCP tools expose their JSON argument object directly.
    - Keep each participating tool handler responsible for translating
    hook-facing `updatedInput` back into its concrete invocation shape.
    
    ## Verification
    
    Direct Bash-like rewrite coverage:
    
    - `pre_tool_use_rewrites_shell_before_execution`
    - `pre_tool_use_rewrites_container_exec_before_execution`
    - `pre_tool_use_rewrites_local_shell_before_execution`
    - `pre_tool_use_rewrites_shell_command_before_execution`
    - `pre_tool_use_rewrites_exec_command_before_execution`
    
    These cases assert that each supported Bash-like surface runs only the
    rewritten command while the hook still observes the original `{
    "command": ... }` input.
    
    `pre_tool_use_rewrites_apply_patch_before_execution`
    
    - Model emits one patch.
    - Hook swaps in a different patch.
    - Asserts only the rewritten file is created, and the hook saw the
    original patch.
    
    `pre_tool_use_rewrites_code_mode_nested_exec_command_before_execution`
    
    - Model runs one nested shell command from code mode.
    - Hook rewrites it.
    - Asserts only the rewritten command runs, and the hook saw the original
    nested input.
    
    `pre_tool_use_rewrites_mcp_tool_before_execution`
    
    - Model calls the RMCP echo tool.
    - Hook rewrites the MCP arguments.
    - Asserts the MCP server receives and returns the rewritten message, not
    the original one.
  • hook trust metadata and enforcement (#20321)
    # Why
    
    We want shared hook trust that both the app and the TUI can build on,
    but the metadata is only useful if runtime behavior agrees with it. This
    PR adds a single backend trust model for hooks so unmanaged hooks cannot
    run until the current definition has been reviewed, while managed hooks
    remain runnable and non-configurable.
    
    # What
    
    - persist `trusted_hash` alongside hook state in `config.toml`
    - expose `currentHash` and derived `trustStatus` through `hooks/list`
    - derive trust from normalized hook definitions so equivalent hooks from
    `config.toml` and `hooks.json` share the same trust identity
    - gate unmanaged hooks on trust before they enter the runnable handler
    set
    
    # Reviewer Notes
    
    - key file to review is `codex-rs/hooks/src/engine/discovery.rs`
    - the only **core** change is schema related
  • Support MCP tools in hooks (#18385)
    ## Summary
    
    Lifecycle hooks currently treat `PreToolUse`, `PostToolUse`, and
    `PermissionRequest` as Bash-only flows
    - hook schema constrains `tool_name` to `Bash`
    - hook input assumes a command-shaped `tool_input`
    - core hook dispatch path passes only shell command strings
    
    That means hooks cannot target MCP tools even though MCP tool names are
    model-visible and stable
    
    This change generalizes those hook paths so they can match and receive
    payloads for MCP tools while preserving the existing Bash behavior.
    
    ## Reviewer Notes
    
    I think these are the key files
    - `codex-rs/core/src/tools/handlers/mcp.rs`
    - `codex-rs/core/src/mcp_tool_call.rs`
    
    Otherwise the changes across apply_patch, shell, and unified_exec are
    mainly to rewire everything to be `tool_input` based instead of just
    `command` so that it'll make sense for MCP tools.
    
    ## Changes
    
    - Allow `PreToolUse`, `PostToolUse`, and `PermissionRequest` hook inputs
    to carry arbitrary `tool_name` and `tool_input` values instead of
    hard-coding `Bash` and command-only payloads.
    - Add MCP hook payload support through `McpHandler`, using the
    model-visible tool name from `ToolInvocation` and the raw MCP arguments
    as `tool_input`.
    - Include MCP tool responses in `PostToolUse` by serializing
    `McpToolOutput` into the hook response payload.
    - Run `PermissionRequest` hooks for MCP approval requests after
    remembered approval checks and before falling back to user-facing MCP
    elicitation.
    - Preserve exact matching for literal hook matchers like `Bash` and
    `mcp__memory__create_entities`, while keeping regex matcher support for
    patterns like `mcp__memory__.*` and `mcp__.*__write.*`.
    
    ---------
    
    Co-authored-by: Andrei Eternal <eternal@openai.com>
    Co-authored-by: Codex <noreply@openai.com>