Skip to content

[codex] Split tool handlers into separate files#21395

Merged
pakrym-oai merged 3 commits into
mainfrom
pakrym/split-tool-handlers-one-per-file
May 6, 2026
Merged

[codex] Split tool handlers into separate files#21395
pakrym-oai merged 3 commits into
mainfrom
pakrym/split-tool-handlers-one-per-file

Conversation

@pakrym-oai
Copy link
Copy Markdown
Collaborator

Why

Several tool handler modules still bundled multiple ToolHandler implementations in one file. That made the handler directory harder to navigate and made otherwise local handler edits land in large shared modules.

What

  • Split grouped tool handlers into one handler file each for agent jobs, goals, MCP resources, shell tools, and unified exec.
  • Kept shared parsing, payload, and runtime helpers in the existing parent modules, with re-exports preserving the existing handler import paths.
  • Updated the shell handler tests to construct ShellCommandHandler through the existing ShellCommandBackendConfig conversion now that the backend detail lives with the shell-command handler.

Validation

  • cargo check -p codex-core
  • cargo clippy -p codex-core --lib -- -D warnings
  • git diff --check -- codex-rs/core/src/tools/handlers

Targeted codex-core handler tests did not run locally because core_test_support currently fails to compile before reaching these tests due to an unresolved similar import.

@pakrym-oai pakrym-oai marked this pull request as ready for review May 6, 2026 18:39
@pakrym-oai pakrym-oai requested a review from a team as a code owner May 6, 2026 18:39
Copy link
Copy Markdown
Collaborator

@jif-oai jif-oai left a comment

Choose a reason for hiding this comment

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

That looks like a very big PR for a still non-final architecture. This will creates a lot of conflicts and make any revert of PRs before this one more difficult

I understand the rational though so I let you decide if you want to merge

}
}

pub(super) fn resolve_use_login_shell(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ultra nit but putting think in pub just for testing is in general an hint of a design issue

Good for now though. This is still in design

@pakrym-oai pakrym-oai enabled auto-merge (squash) May 6, 2026 19:50
@pakrym-oai pakrym-oai disabled auto-merge May 6, 2026 20:12
@pakrym-oai pakrym-oai merged commit b9c50a5 into main May 6, 2026
26 checks passed
@pakrym-oai pakrym-oai deleted the pakrym/split-tool-handlers-one-per-file branch May 6, 2026 20:12
@github-actions github-actions Bot locked and limited conversation to collaborators May 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants