type GPT-5.4 hosted tools and enforce model gates

[ndycode]

Summary

• add typed hosted-tool definitions for tool_search, mcp, computer, and namespace bundles
• remove model-incompatible hosted tools during request normalization
• cover GPT-5.4 capability gates in transformer and tool-utils tests

Stack

• base: #313

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

centralized tool type definitions to lib/types.ts, refactored tool cleanup logic with helper functions, added model-aware tool filtering to remove incompatible tools based on selected model capabilities, and extended test coverage accordingly.

Changes

Cohort / File(s)	Summary
Type Centralization lib/types.ts	Added comprehensive tool-related type definitions (ToolParametersSchema, ToolFunction, FunctionToolDefinition, ToolSearchToolDefinition, RemoteMcpToolDefinition, ComputerUseToolDefinition, ToolNamespaceDefinition, RequestToolDefinition) and updated RequestBody.tools property type constraint from unknown to RequestToolDefinition[] | unknown.
Tool Cleanup Refactoring lib/request/helpers/tool-utils.ts	Removed locally-exported ToolFunction and Tool interfaces, switched to importing type-only definitions from lib/types.ts, and refactored cleanupToolDefinitions to delegate per-tool handling to new cleanupToolDefinition and cleanupFunctionTool helper functions.
Model-Aware Tool Sanitization lib/request/request-transformer.ts	Integrated sanitizeModelIncompatibleTools logic to filter request tools based on model capabilities—removes tool_search and computer/computer_use_preview entries when unsupported, handles namespace nesting by either removing or updating nested tool lists, and emits warnings on removals.
Test Updates test/public-api-contract.test.ts, test/request-transformer.test.ts, test/tool-utils.test.ts	Extended public API contract test with tool_search and mcp entries; added three unit tests validating tool filtering by model capabilities (tool_search removal, computer use removal, namespace filtering); added tool-utils tests covering hosted tool variants and recursive namespace cleanup.

Sequence Diagram

sequenceDiagram
    participant Client
    participant RequestTransformer
    participant ModelCapabilities
    participant ToolUtils

    Client->>RequestTransformer: transformRequestBody(body, model)
    RequestTransformer->>ToolUtils: cleanupToolDefinitions(body.tools)
    ToolUtils-->>RequestTransformer: cleaned tools
    RequestTransformer->>ModelCapabilities: getModelCapabilities(model)
    ModelCapabilities-->>RequestTransformer: {supportsToolSearch, supportsComputerUse}
    RequestTransformer->>RequestTransformer: sanitizeModelIncompatibleTools(tools, capabilities)
    Note over RequestTransformer: Filter tool_search if unsupported<br/>Filter computer tools if unsupported<br/>Remove/update namespaces
    RequestTransformer->>Client: sanitized request body

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

---

specific concerns for review

regression test coverage gaps: lib/request/helpers/tool-utils.ts:line cleanupFunctionTool preserves "failing open" behavior on invalid inputs—confirm test/tool-utils.test.ts:line covers cloning failures and non-record parameter edge cases explicitly. test/request-transformer.test.ts adds good filter validation, but does it exercise deeply nested namespace scenarios where only some inner tools become incompatible?

namespace filtering logic risk: lib/request/request-transformer.ts:line the sanitization recursively updates namespace.tools, but confirm that removing all nested tools correctly removes the entire namespace entry (the summary says "drops the namespace") rather than leaving an empty namespace object.

model capability lookups: lib/request/request-transformer.ts:line ensure getModelCapabilities handles unknown or future model strings gracefully—does it return safe defaults (conservative: assume unsupported) or could it throw? confirm no concurrency issues if model list changes mid-flight.

no windows-specific paths: tool definitions use standardized server_url and server_label fields, so no platform-specific path separators or escaping issues expected.

public api contract stability: lib/types.ts exports now include tool types that were previously only in tool-utils.ts—verify no circular dependency risk or breaking changes for downstream consumers importing from either location.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	pr description is sparse and lacks required template sections like validation checklist, docs/governance updates, and risk/rollback analysis.	fill out the full template: add validation checkboxes (lint, typecheck, test, build), docs checklist items, risk level assessment, and rollback plan. verify what docs actually need updates.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	title uses correct conventional commits format (type: summary), stays under 72 chars, uses lowercase imperative, and accurately summarizes the main changes (typing hosted tools and enforcing model gates).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/openai-parity-pr6

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch feat/openai-parity-pr6

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/request/request-transformer.ts`:
- Around line 903-904: sanitizePlanOnlyTools currently only inspects top-level
tools so nested tool entries inside bundle/namespace objects (e.g.,
{type:"function", function:{name:"request_user_input"}}) bypass the
collaboration guard; update sanitizePlanOnlyTools in request-transformer.ts to
recursively walk into namespace/bundle entries (same traversal pattern used by
sanitizeModelIncompatibleTools or the existing recursive helper) and filter out
plan-only tools at any depth, ensure body.tools assignment still calls
sanitizePlanOnlyTools before sanitizeModelIncompatibleTools, and add a vitest
regression next to the existing tests in test/request-transformer.test.ts
(around 1943-1964) that covers a namespace/bundle containing a plan-only tool to
assert it is removed in non-collaboration mode.
- Around line 901-905: After sanitizing tools in the block that calls
cleanupToolDefinitions, sanitizePlanOnlyTools, and
sanitizeModelIncompatibleTools, collapse an empty tools array to undefined so
downstream checks that use !!body.tools don't treat [] as truthy; specifically,
after body.tools = sanitizeModelIncompatibleTools(body.tools, body.model) set
body.tools = undefined if Array.isArray(body.tools) && body.tools.length === 0.
Update the affected tests in test/request-transformer.test.ts (the cases around
the previous assertions for result.tools) to assert the normalized behavior
(tools becomes undefined when all tools are stripped) and add/adjust vitest
cases to cover the empty-array-to-undefined normalization.
- Around line 352-365: The namespace-handling branch in request-transformer.ts
(the block around the type === "namespace" case using
sanitizeModelIncompatibleToolEntry) assumes "no change" if nestedTools.length
=== record.tools.length, which lets rewritten nested namespaces be discarded;
change the logic to detect structural changes (not just removals) by performing
a deep comparison between nestedTools and record.tools (e.g., element-wise/deep
equality) and only return the original tool when they are deeply equal,
otherwise return the updated record with tools: nestedTools; update the code
path that returns tool and the surrounding logic in
sanitizeModelIncompatibleToolEntry/namespace handling accordingly, and add a
vitest regression in test/request-transformer.test.ts (extend the existing test
around lines 2011-2047) that covers multi-level nested namespaces (e.g.,
namespace -> namespace -> tool_search on a model without search) to assert that
rewritten inner namespaces are preserved and unsupported tools are filtered out.

In `@lib/types.ts`:
- Line 196: The tools property is effectively typed as unknown because
RequestToolDefinition[] | unknown collapses to unknown; change the request
boundary type so tools is a concrete type (e.g., RequestToolDefinition[] or
RequestToolDefinition[] | undefined) instead of unioning with unknown, update
the cleanupToolDefinitions function signature (cleanupToolDefinitions) to accept
and return that concrete type rather than unknown so type information flows
through the pipeline, and add an expectTypeOf public API test to validate the
exported type shape for tools to prevent regressions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c84f23a9-299d-49c1-950a-345d7a274175

📥 Commits

Reviewing files that changed from the base of the PR and between 456cbaf and cccf972.

📒 Files selected for processing (6)

• lib/request/helpers/tool-utils.ts
• lib/request/request-transformer.ts
• lib/types.ts
• test/public-api-contract.test.ts
• test/request-transformer.test.ts
• test/tool-utils.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/public-api-contract.test.ts
• test/request-transformer.test.ts
• test/tool-utils.test.ts

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

• lib/request/request-transformer.ts
• lib/types.ts
• lib/request/helpers/tool-utils.ts

[ndycode]

@coderabbitai review
@greptile @cubic review

[coderabbitai]

@ndycode: kicking off the review now. will check model-gate logic in lib/request/request-transformer.ts, the new type definitions in lib/types.ts, recursive cleanup in lib/request/helpers/tool-utils.ts, and test coverage across all three test files.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

All review threads are resolved and later commits addressed this stale automated change request.

[ndycode]

Closing because this work is now included in main via #318 and #319.