mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
9cd11e9e62c48ea6127f8b9f6ae2e013e4e4ff00
21
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  AbhinavandGitHub
|
5c20513a1b |
Default function tools into tool hooks (#23757)
# Why `PreToolUse`, `PostToolUse`, and `updatedInput` coverage for local function tools currently depends on each handler remembering to wire up the hook contract itself. That makes coverage easy to miss as new function tools are added, even though most of them share the same basic shape: a model-facing function call with JSON arguments. # What This makes `CoreToolRuntime` provide the default hook contract for ordinary local function tools: - build generic `PreToolUse` and `PostToolUse` payloads from the function tool name and arguments - apply `updatedInput` rewrites back into function-tool arguments through the same default path - let tool outputs override the post-hook input or response when they have a more stable hook-facing contract The exceptions stay explicit: - hosted tools remain outside the generic local function path - code-mode `wait` and `write_stdin` opt out for now - `PostToolUse` feedback replaces only the model-visible response, so code mode keeps its typed tool result With the generic path in place, the MCP and extension-tool adapters no longer need their own duplicate pre/post hook plumbing. The new coverage exercises the registry default plus end-to-end local function behavior for pre-hook blocking, `updatedInput` rewriting, and post-hook context. |
||
 jif-oaiandGitHub
|
516f134641 |
Make tool executor specs mandatory (#23870)
## Why `ToolExecutor` is the runtime contract that keeps a callable tool and its model-visible spec together. Leaving `spec()` optional lets a registered runtime silently omit that half of the contract, and it also overloads a missing spec as an exposure decision for tools that should stay dispatchable without being shown to the model. ## What - Make `ToolExecutor::spec()` required and update core, extension, and test tool executors to return a concrete `ToolSpec`. - Add `ToolExposure::Hidden` for dispatch-only tools. The legacy `shell_command` runtime in unified-exec sessions now uses that explicit exposure instead of hiding itself by omitting a spec. - Build MCP tool specs when `McpHandler` is constructed so invalid MCP specs are skipped before the handler is registered. - Keep tool planning aligned with the new contract for direct, deferred, hidden, code-mode, dynamic, and namespaced tool paths. ## Testing - Added tool-plan coverage that invalid MCP tool specs are not registered. - Updated shell-family coverage for the hidden legacy `shell_command` runtime and the affected tool executor test fixtures. |
||
|
jif-oaiandGitHub
|
c69cde3547 |
Add tool lifecycle extension contributor (#23309)
## Why Extensions that need to track runtime progress currently have no typed host signal for tool execution. The goal extension in particular needs to observe tool attempts without inspecting tool payloads, owning tool implementations, or staying coupled to core-only runtime plumbing. This adds a narrow lifecycle contributor API for host-owned tool execution: extensions can observe when an accepted tool call starts and how it finishes, while policy hooks and tool handlers continue to own payload rewriting, blocking, and execution. Relevant code: - [`ToolLifecycleContributor`](https://github.com/openai/codex/blob/3ad2850ffc7d8a1da19c65a92425637a59098f1b/codex-rs/ext/extension-api/src/contributors.rs#L119) defines the extension-facing observer contract. - [`tool_lifecycle.rs`](https://github.com/openai/codex/blob/3ad2850ffc7d8a1da19c65a92425637a59098f1b/codex-rs/ext/extension-api/src/contributors/tool_lifecycle.rs) defines the typed start/finish inputs, source, and outcome enums. - [`notify_tool_start` / `notify_tool_finish`](https://github.com/openai/codex/blob/3ad2850ffc7d8a1da19c65a92425637a59098f1b/codex-rs/core/src/tools/lifecycle.rs) bridges core tool dispatch into the extension registry. ## What Changed - Added `ToolLifecycleContributor` to `codex-extension-api`, including: - `ToolStartInput` - `ToolFinishInput` - `ToolCallSource` - `ToolCallOutcome` - Added registration and lookup support on `ExtensionRegistryBuilder` / `ExtensionRegistry`. - Wired core tool dispatch to notify lifecycle contributors for: - accepted tool starts - completed tool calls, including the tool output success marker - pre-tool-use blocks - failures before or after the handler runs - cancellation/abort in the parallel tool path - Registered the goal extension as a lifecycle contributor and added the outcome filter it will use for goal progress accounting. ## Test Coverage - Added `dispatch_notifies_tool_lifecycle_contributors` to cover lifecycle notification ordering and outcomes for successful and handler-failed tool calls. |
||
|
jif-oaiandGitHub
|
6f1a01fbdd |
Simplify tool executor and registry plumbing (#22636)
## Why The tool runtime path still had a typed output associated type on `ToolExecutor`, plus a core-only `RegisteredTool` adapter and extension-only executor aliases. That made every new shared tool runtime carry extra adapter plumbing before it could participate in core dispatch, extension tools, hook payloads, telemetry, and model-visible spec generation. This PR moves output erasure to the shared executor boundary so core and extension tools can use the same execution contract directly. ## What Changed - Changed `codex_tools::ToolExecutor` to return `Box<dyn ToolOutput>` instead of an associated `Output` type. - Removed the extension-specific `ExtensionToolExecutor` / `ExtensionToolOutput` aliases and exposed `ToolExecutor<ToolCall>` plus `ToolOutput` through `codex-extension-api`. - Reworked core tool registration around `CoreToolRuntime` and `ToolRegistry::from_tools`, removing the extra `RegisteredTool` / `ToolRegistryBuilder` layer. - Consolidated model-visible spec planning and registry construction in `core/src/tools/spec_plan.rs`, including deferred tool search and code-mode-only filtering. - Added `ToolOutput` helpers for post-tool-use hook ids and inputs so MCP, unified exec, extension, and other boxed outputs preserve the same hook payload behavior. - Updated core handlers, memories tools, and the related registry/spec/router tests to use the simplified contract. ## Test Coverage - Updated coverage for tool spec planning, registry lookup, deferred tool search registration, extension tool routing, post-tool-use hook payloads, dispatch tracing, guardian output extraction, and memories extension tool execution. |
||
|
jif-oaiandGitHub
|
6d65686313 |
feat: make ToolExecutor an async trait (#22560)
## Why `codex_tools::ToolExecutor` keeps a tool spec attached to its runtime handler, but extension tools still carried a parallel `ExtensionToolFuture` / `ExtensionToolExecutor` shape. That made extension-owned tools look different from host tools even though routing, registration, and execution need the same abstraction. This PR makes the shared executor contract directly async and lets extension tools implement it too, so host tools and extension tools can move through the same registration path. ## What changed - Changed `ToolExecutor::handle` to an `async fn` using `async-trait`, and updated built-in tool handlers to implement the async trait directly. - Replaced the bespoke `ExtensionToolFuture` contract with a marker `ExtensionToolExecutor` over `ToolExecutor<ToolCall, Output = JsonToolOutput>`, re-exporting `ToolExecutor` from `codex-extension-api`. - Updated the memories extension tools to implement the shared executor trait. - Split tool-router construction into collected executors plus hosted model specs, keeping hosted tools like web search and image generation separate from executable handlers. - Updated spec/router tests and extension-tool stubs for the new executor shape. ## Verification - Not run locally. |
||
|
jif-oaiandGitHub
|
fdda59c00b |
Introduce tool exposure for deferred registration (#22489)
## Why Deferred tools were tracked with separate side-channel filtering after tool specs had already been assembled. That made the registry responsible for executing tools while the router/spec planner separately decided whether those same tools should be exposed to the model up front. This PR makes exposure part of the tool handler contract so direct versus deferred availability travels with the executable tool registration. Next step will be to simplify registration ## What Changed - Adds `ToolExposure` to `codex-tools` and exposes it through `ToolExecutor`, defaulting tools to `Direct`. - Teaches dynamic tools and MCP handlers to mark deferred tools as `Deferred` at construction time. - Renames the registry object-safe wrapper from `AnyToolHandler` to `RegisteredTool` and uses `ToolExposure` when deciding whether to include a handler's spec in the initial model-visible tool list. - Refactors tool spec planning to derive direct specs and deferred search entries from registered handlers, removing the router's special-case deferred dynamic tool filtering. ## Verification - Not run. |
||
|
jif-oaiandGitHub
|
1824685a00 |
feat: extract shared tool executor interface (#22359)
## Why Codex still models model-visible tools and executable behavior largely inside `codex-core`, which makes it harder to evolve the tool system toward a single reusable abstraction for built-ins, MCP-backed tools, dynamic tools, and later tools injected from outside core. This PR takes the next incremental step in that direction by moving the common execution-facing pieces out of core and separating them from core-only orchestration. The intent is to let shared tool abstractions improve in one place, while `codex-core` keeps the parts that are still inherently host-specific today, such as `ToolInvocation`, dispatch wiring, and hook integration. This PR is mostly moving things around. The only interesting piece is this abstraction: https://github.com/openai/codex/pull/22359/changes#diff-81af519002548ba51ed102bdaaf77e081d40a1e73a6e5f9b104bbbc96a6f1b3dR13 ## What changed - Added `codex_tools::ToolExecutor<Invocation>` as the shared execution trait for model-visible tools. - Moved the reusable execution support types from `codex-core` into `codex-tools`: - `FunctionCallError` - `ToolPayload` - `ToolOutput` - Refactored core tool implementations so that execution behavior lives on `ToolExecutor<ToolInvocation>`, while `ToolHandler` remains the core-local extension point for hook payloads, telemetry tags, diff consumers, and other orchestration concerns. - Kept the registry and dispatch flow behaviorally unchanged while making the shared/extracted boundary explicit across built-in, MCP, dynamic, extension-backed, shell, and multi-agent tool handlers. ## Verification - `cargo test -p codex-tools` - `just fix -p codex-tools` - `just fix -p codex-core` - `cargo test -p codex-core` progressed through the updated tool surfaces and then hit the existing unrelated multi-agent stack overflow in `tools::handlers::multi_agents::tests::tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtrees_closed`. |
||
 pakrym-oaiandGitHub
|
0173f71143 |
Refactor namespaced tool spec registration (#22256)
## Summary This refactor makes tool handlers the owner of the specs they can publish, so registry construction can register handlers once and separately publish only the specs that should be model-visible. The main motivation is deferred tools: MCP and dynamic tools still need handlers registered up front, but deferred tools should be discoverable through `tool_search` rather than emitted in the initial tool spec list. ## What changed - `McpHandler` and `DynamicToolHandler` can return their own `ToolSpec`. - `build_tool_registry_builder` now collects handlers, registers them through the no-spec path, and publishes only non-deferred handler specs. - Deferred MCP and dynamic tool names are combined into one `all_deferred_tools` set that drives spec filtering, code-mode deferred-tool signaling, and `tool_search` registration. - `tool_search` registration now requires both deferred tools and `namespace_tools`. - Namespace specs are merged in `spec_plan`, preserving top-level spec order, sorting tools within each namespace, and backfilling empty namespace descriptions. - Hosted web search and image-generation specs are included in the collected spec vector before namespace merge/publication, and tool-name tests that should not care about hosted relative order now compare sets. ## Testing - `cargo test -p codex-core tools::spec::tests:: -- --nocapture` - `cargo test -p codex-core tools::spec_plan::tests:: -- --nocapture` - `cargo test -p codex-core tools::router::tests::specs_filter_deferred_dynamic_tools -- --nocapture` - `cargo test -p codex-core suite::prompt_caching::prompt_tools_are_consistent_across_requests -- --nocapture` - `just fmt` - `just fix -p codex-core` - `cargo test -p codex-core -- --skip tools::handlers::multi_agents::tests::tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtrees_closed` passed the library suite after skipping the known stack-overflowing unit test. Full `cargo test -p codex-core` currently hits a stack overflow in `tools::handlers::multi_agents::tests::tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtrees_closed`; the same focused test reproduces on `origin/main`. |
||
|
pakrym-oaiandGitHub
|
c9e46ed639 |
[codex] Make handlers own parallel tool support (#22254)
## Why `ToolRouter::tool_supports_parallel()` was still consulting configured specs when a handler lookup missed, even though parallel schedulability is really a property of the executable handler. Keeping that metadata on `ConfiguredToolSpec` duplicated state between the model-visible spec layer and the runtime handler layer. This change makes handlers the sole source of truth for parallel tool support and removes the extra spec wrapper that only existed to carry duplicated metadata. ## What changed - removed `ConfiguredToolSpec` and store plain `ToolSpec` values in the registry/router builder path - changed `ToolRouter::tool_supports_parallel()` to consult only the handler registry and fall back to `false` - simplified spec collection and test helpers to operate directly on `ToolSpec` - updated router/spec tests to cover handler-owned parallel behavior and the no-handler fallback ## Validation - `cargo test -p codex-tools` - `cargo test -p codex-core mcp_parallel_support_uses_handler_data` - `cargo test -p codex-core deferred_responses_api_tool_serializes_with_defer_loading` - `cargo test -p codex-core tools_without_handlers_do_not_support_parallel` - `cargo test -p codex-core request_plugin_install_can_be_registered_without_search_tool` ## Docs No documentation updates needed. |
||
|
pakrym-oaiandGitHub
|
ed5944ba1d |
Simplify MCP tool handler plumbing (#21595)
## Why The MCP tool path had accumulated a few core-owned special cases: a dedicated payload variant, resolver plumbing, a legacy `AfterToolUse` translation path, and a side channel for parallel-call metadata. That made `ToolRegistry` and the spec builder know more about MCP than they needed to. This change moves MCP-specific execution details back onto `ToolInfo` and `McpHandler` so `codex-core` can treat MCP calls like normal function calls while still preserving MCP-specific dispatch and telemetry behavior where it belongs. ## What changed - removed `resolve_mcp_tool_info`, `ToolPayload::Mcp`, `ToolKind`, and the remaining registry-side MCP resolver path - stored MCP routing metadata directly on `McpHandler` and `ToolInfo`, including `supports_parallel_tool_calls` - deleted the legacy `AfterToolUse` consumer in `core`, which removes the need for handler-specific `after_tool_use_payload` implementations - switched tool-result telemetry to handler-provided tags and kept MCP-specific dispatch payload construction inside the handler - simplified tool spec planning/building by passing `ToolInfo` directly and dropping the direct/deferred MCP wrapper structs and the parallel-server side table ## Testing - `cargo check -p codex-core -p codex-mcp -p codex-otel` - `cargo test -p codex-core mcp_parallel_support_uses_exact_payload_server` - `cargo test -p codex-core direct_mcp_tools_register_namespaced_handlers` - `cargo test -p codex-core search_tool_description_lists_each_mcp_source_once` - `cargo test -p codex-mcp list_all_tools_uses_startup_snapshot_while_client_is_pending` - `just fix -p codex-core -p codex-mcp -p codex-otel` |
||
|
jif-oaiandGitHub
|
8e12c12a07 |
feat: move extensions tool (#22163)
This PR is just moving stuff around |
||
|
jif-oaiandGitHub
|
672cc1f669 |
feat: wire extension tool bundles into core (#22147)
## Why This is the next narrow step toward moving concrete tool families out of core. After #22138 introduced `codex-tool-api`, we still needed a real end-to-end seam that lets an extension own an executable tool definition once and have core install it without the temporary `extension-api` wrapper or a dependency on `codex-tools`. `codex-tool-api` is the small extension-facing execution contract, while `codex-tools` still has a different job: host-side shared tool metadata and planning logic that is not “run this contributed tool”, like spec shaping, namespaces, discovery, code-mode augmentation, and MCP/dynamic-to-Responses API conversion ## What changed - Moved the shared leaf tool-spec and JSON Schema types into `codex-tool-api`, so the executable contract now lives with [`ToolBundle`](https://github.com/openai/codex/blob/c538758095337d4fe0a52a172363ccede4066bda/codex-rs/tool-api/src/bundle.rs#L19-L70). - Replaced the temporary extension-side tool wrapper with direct `ToolBundle` use in `codex-extension-api`. - Taught core to collect contributed bundles, include them in spec planning, register them through [`ToolRegistryBuilder::register_tool_bundle`](https://github.com/openai/codex/blob/c538758095337d4fe0a52a172363ccede4066bda/codex-rs/core/src/tools/registry.rs#L653-L667), and dispatch them through the existing router/runtime path. - Added focused coverage for contributed tools becoming model-visible and dispatchable, plus spec-planning coverage for contributed function and freeform tools. ## Verification - Added `extension_tool_bundles_are_model_visible_and_dispatchable` in `core/src/tools/router_tests.rs`. - Added spec-plan coverage in `core/src/tools/spec_plan_tests.rs` for contributed extension bundles. ## Related - Follow-up to #22138 |
||
|
pakrym-oaiandGitHub
|
566f2cb612 |
[codex] Move tool specs onto handlers (#21461)
## Why This is the next stacked step after deleting the tool-handler kind indirection. Specs should come from the registered handlers themselves so registry construction has a single source of truth for handler behavior and exposed tool definitions. ## What changed - Added `ToolHandler::spec()` plus handler-provided parallel/code-mode metadata, and made `ToolRegistryBuilder::register_handler` automatically collect specs from registered handlers. - Moved builtin tool spec construction into the corresponding handlers and their adjacent `_spec` modules, including shell, unified exec, apply patch, view image, request plugin install, tool search, MCP resource, goals, planning, permissions, agent jobs, and multi-agent tools. - Reworked configurable handlers to receive their tool-building options through constructors, with non-optional handler options where the handler is always spec-backed. Shell fallback handlers keep an explicit no-spec mode because they are also registered as hidden dispatch aliases. - Kept `CodeModeExecuteHandler` on the explicit configured wrapper so the code-mode exec spec can still be built from the nested registry. ## Verification - `cargo check -p codex-core` - `cargo test -p codex-core tools::spec_plan::tests` - `cargo test -p codex-core tools::spec::tests` - `cargo test -p codex-core tools::handlers::multi_agents_spec::tests` - `RUST_MIN_STACK=16777216 cargo test -p codex-core tools::handlers::multi_agents::tests` - `cargo test -p codex-core tools::handlers::apply_patch::tests` - `cargo test -p codex-core tools::handlers::unified_exec::tests` - `just fix -p codex-core` - `git diff --check` |
||
|
pakrym-oaiandGitHub
|
f593323ef1 |
[codex] Split tool handlers by tool name (#20687)
## Why Tool registration used to bind a tool name to a handler externally, which left ownership split between the registry plan and the handler implementation. Some built-in handlers also multiplexed multiple in-core tools by switching on the invoked tool name internally. This moves the registry identity onto the handler itself and makes built-in multi-tool areas use separate concrete handlers, so each registered handler instance owns exactly one tool name and one dispatch path. ## What Changed - Added `ToolHandler::tool_name()` and changed `ToolRegistryBuilder::register_handler` to derive the registry key from the handler. - Split built-in multiplexed handlers into concrete per-tool handlers for unified exec, shell/local shell/container exec, MCP resources, goal tools, and agent job tools. - Kept name-carrying handler instances only where the runtime target is inherently external or dynamic, such as MCP tools, dynamic tools, and unavailable placeholders. - Updated `ToolHandlerKind` and registry-plan construction so plan entries map directly to concrete handler registrations. ## Verification - `cargo test -p codex-tools tool_registry_plan` - `cargo test -p codex-core --lib tools::registry_tests` - `just fix -p codex-tools` - `just fix -p codex-core` |
||
 cassirer-openaiandGitHub
|
6d09b6752d |
[rollout_trace] Trace tool and code-mode boundaries (#18878)
## Summary Extends rollout tracing across tool dispatch and code-mode runtime boundaries. This records canonical tool-call lifecycle events and links code-mode execution/wait operations back to the model-visible calls that caused them. ## Stack This is PR 3/5 in the rollout trace stack. - [#18876](https://github.com/openai/codex/pull/18876): Add rollout trace crate - [#18877](https://github.com/openai/codex/pull/18877): Record core session rollout traces - [#18878](https://github.com/openai/codex/pull/18878): Trace tool and code-mode boundaries - [#18879](https://github.com/openai/codex/pull/18879): Trace sessions and multi-agent edges - [#18880](https://github.com/openai/codex/pull/18880): Add debug trace reduction command ## Review Notes This PR is about attribution. Reviewers should focus on whether direct tool calls, code-mode-originated tool calls, waits, outputs, and cancellation boundaries are recorded with enough source information for deterministic reduction without coupling the reducer to live runtime internals. The stack remains valid after this layer: tool and code-mode traces reduce through the existing crate model, while the broader session and multi-agent relationships are added in the next PR. |
||
 sayan-oaiandGitHub
|
1325bcd3f6 |
chore: refactor name and namespace to single type (#17402)
avoid passing them both around, unify on a type. this now also keys `ToolRegistry`. tests pass |
||
 Michael BolinandGitHub
|
3c7f013f97 |
core: cut codex-core compile time 63% with native async ToolHandler (#16630)
## Why `ToolHandler` was still paying a large compile-time tax from `#[async_trait]` on every concrete handler impl, even though the only object-safe boundary the registry actually stores is the internal `AnyToolHandler` adapter. This PR removes that macro-generated async wrapper layer from concrete `ToolHandler` impls while keeping the existing object-safe shim in `AnyToolHandler`. In practice, that gets essentially the same compile-time win as the larger type-erasure refactor in #16627, but with a much smaller diff and without changing the public shape of `ToolHandler<Output = T>`. That tradeoff matters here because this is a broad `codex-core` hotspot and reviewers should be able to judge the compile-time impact from hard numbers, not vibes. ## Headline result On a clean `codex-core` package rebuild (`cargo clean -p codex-core` before each command), rustc `total` dropped from **187.15s to 68.98s** versus the shared `0bd31dc382bd` baseline: **-63.1%**. The biggest hot passes dropped by roughly **71-72%**: | Metric | Baseline `0bd31dc382bd` | This PR `41f7ac0adeac` | Delta | |---|---:|---:|---:| | `total` | 187.15s | 68.98s | **-63.1%** | | `generate_crate_metadata` | 84.53s | 24.49s | **-71.0%** | | `MIR_borrow_checking` | 84.13s | 24.58s | **-70.8%** | | `monomorphization_collector_graph_walk` | 79.74s | 22.19s | **-72.2%** | | `evaluate_obligation` self-time | 180.62s | 46.91s | **-74.0%** | Important caveat: `-Z time-passes` timings are nested, so `generate_crate_metadata` and `monomorphization_collector_graph_walk` are mostly overlapping, not additive. ## Why this PR over #16627 #16627 already proved that the `ToolHandler` stack was the right hotspot, but it got there by making `ToolHandler` object-safe and changing every handler to return `BoxFuture<Result<AnyToolResult, _>>` directly. This PR keeps the lower-churn shape: - `ToolHandler` remains generic over `type Output`. - Concrete handlers use native RPITIT futures with explicit `Send` bounds. - `AnyToolHandler` remains the only object-safe adapter and still does the boxing at the registry boundary, as before. - The implementation diff is only **33 files, +28/-77**. The measurements are at least comparable, and in this run this PR is slightly faster than #16627 on the pass-level total: | Metric | #16627 | This PR | Delta | |---|---:|---:|---:| | `total` | 79.90s | 68.98s | **-13.7%** | | `generate_crate_metadata` | 25.88s | 24.49s | **-5.4%** | | `monomorphization_collector_graph_walk` | 23.54s | 22.19s | **-5.7%** | | `evaluate_obligation` self-time | 43.29s | 46.91s | +8.4% | ## Profile data ### Crate-level timings `cargo +nightly build -p codex-core --lib -Z unstable-options --timings=json` after `cargo clean -p codex-core`. Baseline data below is reused from the shared parent `0bd31dc382bd` profile because this PR and #16627 are both one commit on top of that same parent. | Crate | Baseline `duration` | This PR `duration` | Delta | Baseline `rmeta_time` | This PR `rmeta_time` | Delta | |---|---:|---:|---:|---:|---:|---:| | `codex_core` | 187.380776583s | 69.171113833s | **-63.1%** | 174.474507208s | 55.873015583s | **-68.0%** | | `starlark` | 17.90s | 16.773824125s | -6.3% | n/a | 8.8999965s | n/a | ### Pass-level timings `cargo +nightly rustc -p codex-core --lib -- -Z time-passes -Z time-passes-format=json` after `cargo clean -p codex-core`. | Pass | Baseline | This PR | Delta | |---|---:|---:|---:| | `total` | 187.150662083s | 68.978770375s | **-63.1%** | | `generate_crate_metadata` | 84.531864625s | 24.487462958s | **-71.0%** | | `MIR_borrow_checking` | 84.131389375s | 24.575553875s | **-70.8%** | | `monomorphization_collector_graph_walk` | 79.737515042s | 22.190207417s | **-72.2%** | | `codegen_crate` | 12.362532292s | 12.695237625s | +2.7% | | `type_check_crate` | 4.4765405s | 5.442019542s | +21.6% | | `coherence_checking` | 3.311121208s | 4.239935292s | +28.0% | | process `real` / `user` / `sys` | 187.70s / 201.87s / 4.99s | 69.52s / 85.90s / 2.92s | n/a | ### Self-profile query summary `cargo +nightly rustc -p codex-core --lib -- -Z self-profile=... -Z self-profile-events=default,query-keys,args,llvm,artifact-sizes` after `cargo clean -p codex-core`, summarized with `measureme summarize -p 0.5`. | Query / phase | Baseline self time | This PR self time | Delta | Baseline total time | This PR total time | Baseline item count | This PR item count | Baseline cache hits | This PR cache hits | |---|---:|---:|---:|---:|---:|---:|---:|---:|---:| | `evaluate_obligation` | 180.62s | 46.91s | **-74.0%** | 182.08s | 48.37s | 572,234 | 388,659 | 1,130,998 | 1,058,553 | | `mir_borrowck` | 1.42s | 1.49s | +4.9% | 93.77s | 29.59s | n/a | 6,184 | n/a | 15,298 | | `typeck` | 1.84s | 1.87s | +1.6% | 2.38s | 2.44s | n/a | 9,367 | n/a | 79,247 | | `LLVM_module_codegen_emit_obj` | n/a | 17.12s | n/a | 17.01s | 17.12s | n/a | 256 | n/a | 0 | | `LLVM_passes` | n/a | 13.07s | n/a | 12.95s | 13.07s | n/a | 1 | n/a | 0 | | `codegen_module` | n/a | 12.33s | n/a | 12.22s | 13.64s | n/a | 256 | n/a | 0 | | `items_of_instance` | n/a | 676.00ms | n/a | n/a | 24.96s | n/a | 99,990 | n/a | 0 | | `type_op_prove_predicate` | n/a | 660.79ms | n/a | n/a | 24.78s | n/a | 78,762 | n/a | 235,877 | | Summary | Baseline | This PR | |---|---:|---:| | `evaluate_obligation` % of total CPU | 70.821% | 38.880% | | self-profile total CPU time | 255.042999997s | 120.661175956s | | process `real` / `user` / `sys` | 220.96s / 235.02s / 7.09s | 86.35s / 103.66s / 3.54s | ### Artifact sizes From the same `measureme summarize` output: | Artifact | Baseline | This PR | Delta | |---|---:|---:|---:| | `crate_metadata` | 26,534,471 bytes | 26,545,248 bytes | +10,777 | | `dep_graph` | 253,181,425 bytes | 239,240,806 bytes | -13,940,619 | | `linked_artifact` | 565,366,624 bytes | 562,673,176 bytes | -2,693,448 | | `object_file` | 513,127,264 bytes | 510,464,096 bytes | -2,663,168 | | `query_cache` | 137,440,945 bytes | 136,982,566 bytes | -458,379 | | `cgu_instructions` | 3,586,307 bytes | 3,575,121 bytes | -11,186 | | `codegen_unit_size_estimate` | 2,084,846 bytes | 2,078,773 bytes | -6,073 | | `work_product_index` | 19,565 bytes | 19,565 bytes | 0 | ### Baseline hotspots before this change These are the top normalized obligation buckets from the shared baseline profile: | Obligation bucket | Samples | Duration | |---|---:|---:| | `outlives:tasks::review::ReviewTask` | 1,067 | 6.33s | | `outlives:tools::handlers::unified_exec::UnifiedExecHandler` | 896 | 5.63s | | `trait:T as tools::registry::ToolHandler` | 876 | 5.45s | | `outlives:tools::handlers::shell::ShellHandler` | 888 | 5.37s | | `outlives:tools::handlers::shell::ShellCommandHandler` | 870 | 5.29s | | `outlives:tools::runtimes::shell::unix_escalation::CoreShellActionProvider` | 637 | 3.73s | | `outlives:tools::handlers::mcp::McpHandler` | 695 | 3.61s | | `outlives:tasks::regular::RegularTask` | 726 | 3.57s | Top `items_of_instance` entries before this change were mostly concrete async handler/task impls: | Instance | Duration | |---|---:| | `tasks::regular::{impl#2}::run` | 3.79s | | `tools::handlers::mcp::{impl#0}::handle` | 3.27s | | `tools::runtimes::shell::unix_escalation::{impl#2}::determine_action` | 3.09s | | `tools::handlers::agent_jobs::{impl#11}::handle` | 3.07s | | `tools::handlers::multi_agents::spawn::{impl#1}::handle` | 2.84s | | `tasks::review::{impl#4}::run` | 2.82s | | `tools::handlers::multi_agents_v2::spawn::{impl#2}::handle` | 2.80s | | `tools::handlers::multi_agents::resume_agent::{impl#1}::handle` | 2.73s | | `tools::handlers::unified_exec::{impl#2}::handle` | 2.54s | | `tasks::compact::{impl#4}::run` | 2.45s | ## What changed Relevant pre-change registry shape: [`codex-rs/core/src/tools/registry.rs`](https://github.com/openai/codex/blob/0bd31dc382bd1c33dc2bb6b97069c76aa10ba14b/codex-rs/core/src/tools/registry.rs#L38-L219) Current registry shape in this PR: [`codex-rs/core/src/tools/registry.rs`](https://github.com/openai/codex/blob/41f7ac0adeac81d667541853d6546267d6083613/codex-rs/core/src/tools/registry.rs#L38-L203) - `ToolHandler::{is_mutating, handle}` now return native `impl Future + Send` futures instead of using `#[async_trait]`. - `AnyToolHandler` remains the object-safe adapter and boxes those futures at the registry boundary with explicit lifetimes. - Concrete handlers and the registry test handler drop `#[async_trait]` but otherwise keep their async method bodies intact. - Representative examples: [`codex-rs/core/src/tools/handlers/shell.rs`](https://github.com/openai/codex/blob/41f7ac0adeac81d667541853d6546267d6083613/codex-rs/core/src/tools/handlers/shell.rs#L223-L379), [`codex-rs/core/src/tools/handlers/unified_exec.rs`](https://github.com/openai/codex/blob/41f7ac0adeac81d667541853d6546267d6083613/codex-rs/core/src/tools/handlers/unified_exec.rs), [`codex-rs/core/src/tools/registry_tests.rs`](https://github.com/openai/codex/blob/41f7ac0adeac81d667541853d6546267d6083613/codex-rs/core/src/tools/registry_tests.rs) ## Tradeoff This is intentionally less invasive than #16627: it does **not** move result boxing into every concrete handler and does **not** change `ToolHandler` into an object-safe trait. Instead, it keeps the existing registry-level type-erasure boundary and only removes the macro-generated async wrapper layer from concrete impls. So the runtime boxing story stays basically the same as before, while the compile-time savings are still large. ## Verification Existing verification for this branch still applies: - Ran `cargo test -p codex-core`; this change compiled and the suite reached the known unrelated `config::tests::*guardian*` failures, with no local diff under `codex-rs/core/src/config/`. Profiling commands used for the tables above: - `cargo clean -p codex-core` - `cargo +nightly build -p codex-core --lib -Z unstable-options --timings=json` - `cargo +nightly rustc -p codex-core --lib -- -Z time-passes -Z time-passes-format=json` - `cargo +nightly rustc -p codex-core --lib -- -Z self-profile=... -Z self-profile-events=default,query-keys,args,llvm,artifact-sizes` - `measureme summarize -p 0.5` |
||
|
Michael BolinandGitHub
|
61dfe0b86c |
chore: clean up argument-comment lint and roll out all-target CI on macOS (#16054)
## Why `argument-comment-lint` was green in CI even though the repo still had many uncommented literal arguments. The main gap was target coverage: the repo wrapper did not force Cargo to inspect test-only call sites, so examples like the `latest_session_lookup_params(true, ...)` tests in `codex-rs/tui_app_server/src/lib.rs` never entered the blocking CI path. This change cleans up the existing backlog, makes the default repo lint path cover all Cargo targets, and starts rolling that stricter CI enforcement out on the platform where it is currently validated. ## What changed - mechanically fixed existing `argument-comment-lint` violations across the `codex-rs` workspace, including tests, examples, and benches - updated `tools/argument-comment-lint/run-prebuilt-linter.sh` and `tools/argument-comment-lint/run.sh` so non-`--fix` runs default to `--all-targets` unless the caller explicitly narrows the target set - fixed both wrappers so forwarded cargo arguments after `--` are preserved with a single separator - documented the new default behavior in `tools/argument-comment-lint/README.md` - updated `rust-ci` so the macOS lint lane keeps the plain wrapper invocation and therefore enforces `--all-targets`, while Linux and Windows temporarily pass `-- --lib --bins` That temporary CI split keeps the stricter all-targets check where it is already cleaned up, while leaving room to finish the remaining Linux- and Windows-specific target-gated cleanup before enabling `--all-targets` on those runners. The Linux and Windows failures on the intermediate revision were caused by the wrapper forwarding bug, not by additional lint findings in those lanes. ## Validation - `bash -n tools/argument-comment-lint/run.sh` - `bash -n tools/argument-comment-lint/run-prebuilt-linter.sh` - shell-level wrapper forwarding check for `-- --lib --bins` - shell-level wrapper forwarding check for `-- --tests` - `just argument-comment-lint` - `cargo test` in `tools/argument-comment-lint` - `cargo test -p codex-terminal-detection` ## Follow-up - Clean up remaining Linux-only target-gated callsites, then switch the Linux lint lane back to the plain wrapper invocation. - Clean up remaining Windows-only target-gated callsites, then switch the Windows lint lane back to the plain wrapper invocation. |
||
 Andrei EternalandGitHub
|
c4d9887f9a |
[hooks] add non-streaming (non-stdin style) shell-only PostToolUse support (#15531)
CHAINED PR - note that base is eternal/hooks-pretooluse-bash, not main -- so the following PR should be first Matching post-tool hook to the pre-tool functionality here: https://github.com/openai/codex/pull/15211 So, PreToolUse calls for plain shell calls, allows blocking. This PostToolUse call runs after the command executed example run: ``` › as a test, run in parallel the following commands: - echo 'one' - echo '[block-pre-tool-use]' - echo '[block-post-tool-use]' ⚠ MCP startup incomplete (failed: notion, linear) • Cruising through those three commands in parallel now, and I’ll share the exact outputs right after they land. • Running PreToolUse hook: checking the observatory runes • Running PreToolUse hook: checking the observatory runes • Running PreToolUse hook: checking the observatory runes PreToolUse hook (blocked) warning: wizard-tower PreToolUse demo blocked a Bash command on purpose. feedback: PreToolUse demo blocked the command. Remove [block-pre-tool-use] to continue. PreToolUse hook (completed) warning: wizard-tower PreToolUse demo inspected Bash: echo 'one' PreToolUse hook (completed) warning: wizard-tower PreToolUse demo inspected Bash: echo '[block-post-tool-use]' • Ran echo '[block-post-tool-use]' └ [block-post-tool-use] • Ran echo 'one' └ one • Running PostToolUse hook: reading the observatory aftermath • Running PostToolUse hook: reading the observatory aftermath PostToolUse hook (blocked) warning: wizard-tower PostToolUse demo blocked the Bash follow-through on purpose. feedback: PostToolUse demo blocked the result after execution. Remove [block-post-tool-use] to continue. PostToolUse hook (completed) warning: wizard-tower PostToolUse demo inspected Bash: echo 'one' hook context: PostToolUse demo saw the Bash result. Response preview: one ────────────────────────────────────────────────────────────────────────────────────────────────────── • Ran them in parallel. Little wave report: - echo 'one' completed and returned one. - echo '[block-pre-tool-use]' was blocked before execution by the PreToolUse hook. - echo '[block-post-tool-use]' ran, but its result was blocked afterward by the PostToolUse hook. The hook messages were: - PreToolUse demo blocked the command. Remove [block-pre-tool-use] to continue. - PostToolUse demo blocked the result after execution. Remove [block-post-tool-use] to continue. › for the blocked messages (correctly blocked in this test) did the tool output land back into context? • Yeah, from this run: - PreToolUse block: the command itself did not execute, so no command output landed in context. Only the block message did. - PostToolUse block: the command did execute, but the actual stdout did not come back into context. Only the post-tool block message did. So the blocked notifications landed in context, but the blocked command payloads themselves did not. Nice clean guardrail, aloha. ``` |
||
|
Andrei EternalandGitHub
|
73bbb07ba8 |
[hooks] add non-streaming (non-stdin style) shell-only PreToolUse support (#15211)
- add `PreToolUse` hook for bash-like tool execution only at first - block shell execution before dispatch with deny-only hook behavior - introduces common.rs matcher framework for matching when hooks are run example run: ``` › run three parallel echo commands, and the second one should echo "[block-pre-tool-use]" as a test • Running the three echo commands in parallel now and I’ll report the output directly. • Running PreToolUse hook: name for demo pre tool use hook • Running PreToolUse hook: name for demo pre tool use hook • Running PreToolUse hook: name for demo pre tool use hook PreToolUse hook (completed) warning: wizard-tower PreToolUse demo inspected Bash: echo "first parallel echo" PreToolUse hook (blocked) warning: wizard-tower PreToolUse demo blocked a Bash command on purpose. feedback: PreToolUse demo blocked the command. Remove [block-pre-tool-use] to continue. PreToolUse hook (completed) warning: wizard-tower PreToolUse demo inspected Bash: echo "third parallel echo" • Ran echo "first parallel echo" └ first parallel echo • Ran echo "third parallel echo" └ third parallel echo • Three little waves went out in parallel. 1. printed first parallel echo 2. was blocked before execution because it contained the exact test string [block-pre-tool-use] 3. printed third parallel echo There was also an unrelated macOS defaults warning around the successful commands, but the echoes themselves worked fine. If you want, I can rerun the second one with a slightly modified string so it passes cleanly. ``` |
||
|
Michael BolinandGitHub
|
0c8a36676a |
fix: move inline codex-rs/core unit tests into sibling files (#14444)
## Why PR #13783 moved the `codex.rs` unit tests into `codex_tests.rs`. This applies the same extraction pattern across the rest of `codex-rs/core` so the production modules stay focused on runtime code instead of large inline test blocks. Keeping the tests in sibling files also makes follow-up edits easier to review because product changes no longer have to share a file with hundreds or thousands of lines of test scaffolding. ## What changed - replaced each inline `mod tests { ... }` in `codex-rs/core/src/**` with a path-based module declaration - moved each extracted unit test module into a sibling `*_tests.rs` file, using `mod_tests.rs` for `mod.rs` modules - preserved the existing `cfg(...)` guards and module-local structure so the refactor remains structural rather than behavioral ## Testing - `cargo test -p codex-core --lib` (`1653 passed; 0 failed; 5 ignored`) - `just fix -p codex-core` - `cargo fmt --check` - `cargo shear` |