fix: support Codex CLI gpt-5.5 structured outputs#1699
Open
seven017438 wants to merge 4 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: a0a0f36 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds GPT-5.5 to the Codex CLI model catalog, introduces a recursive JSON-schema normalizer for strict OpenAI structured outputs in BaseAIProvider, implements system Codex CLI path detection with caching, updates a dependency, and adds/updates tests to verify behavior. ChangesSchema Normalization & Model Support
Sequence DiagramsequenceDiagram
participant Client
participant BaseAIProvider
participant Normalizer
participant OpenAI
Client->>BaseAIProvider: generateObject(schema)
BaseAIProvider->>Normalizer: normalizeSchemaForStructuredOutputs
Normalizer-->>BaseAIProvider: strict sanitized schema
BaseAIProvider->>OpenAI: generateObject with strict schema
OpenAI-->>BaseAIProvider: structured output
BaseAIProvider-->>Client: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/unit/ai-providers/codex-cli-supported-models.test.js (1)
4-10: 💤 Low valueResolve the catalog path relative to the test file, not
process.cwd().
process.cwd()makes the test pass only when Jest is invoked from the repo root. Resolving viaimport.meta.urlkeeps the test stable regardless of working directory and is the idiomatic ESM equivalent of__dirname.♻️ Proposed change
import fs from 'fs'; import path from 'path'; +import { fileURLToPath } from 'url'; -const supportedModelsPath = path.resolve( - process.cwd(), - 'scripts/modules/supported-models.json' -); +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const supportedModelsPath = path.resolve( + __dirname, + '../../../scripts/modules/supported-models.json' +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/ai-providers/codex-cli-supported-models.test.js` around lines 4 - 10, The test currently resolves supportedModelsPath using process.cwd(), which makes it fragile; change the resolution to be relative to the test file using import.meta.url: import { fileURLToPath } from 'url' and compute const __dirname = path.dirname(fileURLToPath(import.meta.url)), then build supportedModelsPath with path.resolve(__dirname, '../../scripts/modules/supported-models.json') (or the correct relative steps from the test file) so supportedModelsPath and the JSON read (supportedModels) are stable regardless of the working directory.tests/unit/ai-providers/codex-cli.test.js (1)
21-25: ⚡ Quick winAdd a test for the "no Codex anywhere" fallback.
You cover (a) system path detected and (b) explicit override, but not the case where
getSystemCodexPath()returnsnull(e.g.,execSyncthrows). Worth one more case asserting thatdefaultSettingsis created without acodexPathkey when neither config nor system provides one — that's the path most ChatGPT-OAuth users without@openai/codexglobally installed will hit.♻️ Suggested addition
+ it('omits codexPath when neither config nor system provides one', async () => { + const { execSync } = await import('child_process'); + execSync.mockImplementationOnce(() => { + throw new Error('not found'); + }); + + await provider.getClient({ commandName: 'parse-prd' }); + + const call = createCodexCli.mock.calls.at(-1)[0]; + expect(call.defaultSettings.codexPath).toBeUndefined(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/ai-providers/codex-cli.test.js` around lines 21 - 25, Add a unit test in tests/unit/ai-providers/codex-cli.test.js that simulates "no Codex anywhere" by mocking child_process.execSync to throw so getSystemCodexPath() returns null, then call the code-path that produces defaultSettings (the function/flow that builds defaultSettings in the codex-cli module) and assert the resulting defaultSettings object does NOT contain a codexPath key; this ensures the fallback when neither config nor system-provided Codex exists is covered.src/ai-providers/base-provider.js (1)
57-119: ⚡ Quick winEdge cases worth confirming in the recursive normalizer.
A few subtleties in
normalizeSchemaForStructuredOutputsthat you may want to nail down with explicit tests:
propertyKeys-based detection (isObjectSchema = next.type === 'object' || propertyKeys.length > 0) treats anything withpropertiesas an object even whentypeis anullableunion like['object','null']. That's likely fine, butadditionalProperties:falsewill then be added to nullable object schemas as well — verify that's desirable for structured outputs.oneOf/anyOf/allOfbranches are recursed, but the parent schema still getsadditionalProperties:falseif it happens to definepropertiesalongside the combinator. JSON Schema allows that combination but OpenAI strict mode is finicky about it.itemshandling at line 98 only normalizes when truthy; tuple-styleitems: [](empty array) is falsy in the existing check (if (next.items)is true for empty arrays actually — arrays are truthy), but you skip the array-of-schemas tuple form. ConsiderArray.isArray(next.items)and mapping each entry.Non-blocking, but the schema test in
tests/unit/ai-providers/base-provider-schema.test.jsonly covers a single happy-path object; adding a case with a pre-existingrequired, a nullable object, and a tuple-styleitemswould lock the contract down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-providers/base-provider.js` around lines 57 - 119, The recursive normalizer normalizeSchemaForStructuredOutputs should: treat type arrays (nullable unions) correctly by checking Array.isArray(next.type) and using next.type.includes('object') when deciding isObjectSchema; avoid forcing additionalProperties=false when the schema uses combinators (oneOf/anyOf/allOf) by skipping that assignment if any of those keys exist on next; preserve an existing next.required (don’t overwrite it with propertyKeys) and only set required = propertyKeys when next.required is undefined; and handle tuple-style items by checking Array.isArray(next.items) and mapping normalizeSchemaForStructuredOutputs over each element (and otherwise recurse for single-item schemas). Ensure these changes are applied inside normalizeSchemaForStructuredOutputs and reference next, propertyKeys, isObjectSchema, and next.items when implementing.src/ai-providers/codex-cli.js (2)
46-62: ⚡ Quick winCache the system Codex path lookup; it currently spawns a subprocess per
getClientcall.
getClientis invoked for every text/object generation, and each call now runsexecSync('command -v codex' | 'where codex')synchronously on the request thread when no explicitcodexPathis configured. Even withtimeout: 1000, that's avoidable overhead (and a synchronous syscall on the hot path). Cache it on the instance like the existing_codexCliCheckedflag.♻️ Proposed change
constructor() { super(); ... // CLI availability check cache this._codexCliChecked = false; this._codexCliAvailable = null; + // System Codex path detection cache + this._systemCodexPathChecked = false; + this._systemCodexPath = null; } + + _resolveSystemCodexPath() { + if (!this._systemCodexPathChecked) { + this._systemCodexPath = getSystemCodexPath(); + this._systemCodexPathChecked = true; + } + return this._systemCodexPath; + } @@ - const codexPath = settings.codexPath || getSystemCodexPath(); + const codexPath = settings.codexPath || this._resolveSystemCodexPath();Also applies to: 180-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-providers/codex-cli.js` around lines 46 - 62, The getSystemCodexPath function is currently invoked on every getClient call and runs a synchronous execSync lookup each time; cache the resolved system path on the instance (similar to the existing _codexCliChecked flag) so the subprocess is only spawned once: modify getClient (and the code paths around _codexCliChecked and the fallback logic referenced at the block around 180-206) to first check an instance property like _codexCliPath and only call getSystemCodexPath to populate it once, store null if not found, and then reuse that cached _codexCliPath on subsequent getClient calls to avoid repeated execSync invocations.
46-62: 💤 Low valueConsider adding a clarifying comment on the
command -vlogic.The shell implementation correctly uses
command -v codexon POSIX systems, which is reliably available in /bin/sh across Alpine, Debian (dash), and macOS. This is more portable thanwhich, which isn't guaranteed to exist on minimal Linux containers. A one-line comment (e.g.,// 'command -v' is POSIX-compliant; avoids 'which' which may not exist in minimal environments) would prevent future readers from refactoring it towhich.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-providers/codex-cli.js` around lines 46 - 62, In getSystemCodexPath(), add a one-line clarifying comment above the command assignment explaining why we use 'command -v codex' on POSIX systems (e.g., "'command -v' is POSIX-compliant and available in /bin/sh; prefer it over 'which' which may be missing in minimal containers") so future maintainers don't replace it with 'which'; place the comment right next to the const command = ... line to make the intent obvious.tests/unit/ai-providers/base-provider-schema.test.js (1)
90-125: ⚡ Quick winGood baseline — please extend coverage to lock down the
requiredcontract.The happy-path assertions look solid. To pin the contract precisely (and to catch regressions tied to the broader concern raised in
base-provider.js), add at least one case that:
- Passes a schema where
zodSchemamock returnsrequired: ['title'](caller-defined).- Asserts whether the normalizer preserves it or overwrites with all keys.
That single test will document the intended behavior unambiguously and fail loudly if it's later changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/ai-providers/base-provider-schema.test.js` around lines 90 - 125, Add a new unit test that calls TestProvider.generateObject (same pattern as the existing test) but mock the zod-to-json-schema call so it returns a schema with required: ['title']; then assert against mockJsonSchema.mock.calls[0][0].required to explicitly verify the normalizer's contract (for example, expect it to equal ['title'] if you intend to preserve caller-defined required, or expect the full set if you intend to override); reference TestProvider.generateObject and mockJsonSchema when adding the assertion so the behavior is pinned and will fail if changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ai-providers/base-provider.js`:
- Around line 102-116: The buildSafeSchema logic in
BaseAIProvider.generateObject currently overwrites caller-supplied required and
forces additionalProperties:false for every provider; change buildSafeSchema so
it only sets next.required = propertyKeys when next.required is undefined
(preserve any caller-provided required array) and only sets
next.additionalProperties = false if next.additionalProperties is undefined (do
not force it); additionally consider adding an opt-in boolean (e.g.
strictStructuredOutputs) on BaseAIProvider or generateObject so providers that
need OpenAI-strict semantics can opt into forcing all properties required (apply
the strict behavior only when strictStructuredOutputs is true).
---
Nitpick comments:
In `@src/ai-providers/base-provider.js`:
- Around line 57-119: The recursive normalizer
normalizeSchemaForStructuredOutputs should: treat type arrays (nullable unions)
correctly by checking Array.isArray(next.type) and using
next.type.includes('object') when deciding isObjectSchema; avoid forcing
additionalProperties=false when the schema uses combinators (oneOf/anyOf/allOf)
by skipping that assignment if any of those keys exist on next; preserve an
existing next.required (don’t overwrite it with propertyKeys) and only set
required = propertyKeys when next.required is undefined; and handle tuple-style
items by checking Array.isArray(next.items) and mapping
normalizeSchemaForStructuredOutputs over each element (and otherwise recurse for
single-item schemas). Ensure these changes are applied inside
normalizeSchemaForStructuredOutputs and reference next, propertyKeys,
isObjectSchema, and next.items when implementing.
In `@src/ai-providers/codex-cli.js`:
- Around line 46-62: The getSystemCodexPath function is currently invoked on
every getClient call and runs a synchronous execSync lookup each time; cache the
resolved system path on the instance (similar to the existing _codexCliChecked
flag) so the subprocess is only spawned once: modify getClient (and the code
paths around _codexCliChecked and the fallback logic referenced at the block
around 180-206) to first check an instance property like _codexCliPath and only
call getSystemCodexPath to populate it once, store null if not found, and then
reuse that cached _codexCliPath on subsequent getClient calls to avoid repeated
execSync invocations.
- Around line 46-62: In getSystemCodexPath(), add a one-line clarifying comment
above the command assignment explaining why we use 'command -v codex' on POSIX
systems (e.g., "'command -v' is POSIX-compliant and available in /bin/sh; prefer
it over 'which' which may be missing in minimal containers") so future
maintainers don't replace it with 'which'; place the comment right next to the
const command = ... line to make the intent obvious.
In `@tests/unit/ai-providers/base-provider-schema.test.js`:
- Around line 90-125: Add a new unit test that calls TestProvider.generateObject
(same pattern as the existing test) but mock the zod-to-json-schema call so it
returns a schema with required: ['title']; then assert against
mockJsonSchema.mock.calls[0][0].required to explicitly verify the normalizer's
contract (for example, expect it to equal ['title'] if you intend to preserve
caller-defined required, or expect the full set if you intend to override);
reference TestProvider.generateObject and mockJsonSchema when adding the
assertion so the behavior is pinned and will fail if changed.
In `@tests/unit/ai-providers/codex-cli-supported-models.test.js`:
- Around line 4-10: The test currently resolves supportedModelsPath using
process.cwd(), which makes it fragile; change the resolution to be relative to
the test file using import.meta.url: import { fileURLToPath } from 'url' and
compute const __dirname = path.dirname(fileURLToPath(import.meta.url)), then
build supportedModelsPath with path.resolve(__dirname,
'../../scripts/modules/supported-models.json') (or the correct relative steps
from the test file) so supportedModelsPath and the JSON read (supportedModels)
are stable regardless of the working directory.
In `@tests/unit/ai-providers/codex-cli.test.js`:
- Around line 21-25: Add a unit test in
tests/unit/ai-providers/codex-cli.test.js that simulates "no Codex anywhere" by
mocking child_process.execSync to throw so getSystemCodexPath() returns null,
then call the code-path that produces defaultSettings (the function/flow that
builds defaultSettings in the codex-cli module) and assert the resulting
defaultSettings object does NOT contain a codexPath key; this ensures the
fallback when neither config nor system-provided Codex exists is covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06e3aab8-e89f-41d4-9d61-15d99e0dfdb6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.changeset/fix-codex-cli-gpt55-structured-output.mdpackage.jsonscripts/modules/supported-models.jsonsrc/ai-providers/base-provider.jssrc/ai-providers/codex-cli.jstests/unit/ai-providers/base-provider-schema.test.jstests/unit/ai-providers/codex-cli-supported-models.test.jstests/unit/ai-providers/codex-cli.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/ai-providers/codex-cli.test.js (1)
104-118: ⚡ Quick winConsider verifying that system detection is skipped when config provides codexPath.
The test correctly verifies that the configured path takes precedence, but adding an assertion that
execSyncis not called would confirm the performance optimization and make the precedence behavior more explicit.🔍 Proposed enhancement
it('preserves explicitly configured codexPath over system Codex path', async () => { getCodexCliSettingsForCommand.mockReturnValueOnce({ allowNpx: true, codexPath: '/custom/bin/codex' }); await provider.getClient({ commandName: 'parse-prd' }); + // Verify system detection is skipped when config provides path + expect(execSync).not.toHaveBeenCalled(); + expect(createCodexCli).toHaveBeenCalledWith({ defaultSettings: expect.objectContaining({ allowNpx: true, codexPath: '/custom/bin/codex' }) }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/ai-providers/codex-cli.test.js` around lines 104 - 118, Add an assertion that system detection via execSync is skipped when an explicit codexPath is provided: after calling provider.getClient({ commandName: 'parse-prd' }) and the existing expect(createCodexCli)... check, add an expectation that execSync (the mocked system detection function) was not called (e.g., expect(execSync).not.toHaveBeenCalled()). This ensures getCodexCliSettingsForCommand returning codexPath prevents any fallback system discovery and makes the test assert the performance/precedence behavior explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/ai-providers/codex-cli.test.js`:
- Around line 104-118: Add an assertion that system detection via execSync is
skipped when an explicit codexPath is provided: after calling
provider.getClient({ commandName: 'parse-prd' }) and the existing
expect(createCodexCli)... check, add an expectation that execSync (the mocked
system detection function) was not called (e.g.,
expect(execSync).not.toHaveBeenCalled()). This ensures
getCodexCliSettingsForCommand returning codexPath prevents any fallback system
discovery and makes the test assert the performance/precedence behavior
explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b33946b-ea98-4490-b02b-bf0eda43875d
📒 Files selected for processing (5)
src/ai-providers/base-provider.jssrc/ai-providers/codex-cli.jstests/unit/ai-providers/base-provider-schema.test.jstests/unit/ai-providers/codex-cli-supported-models.test.jstests/unit/ai-providers/codex-cli.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/ai-providers/codex-cli-supported-models.test.js
- tests/unit/ai-providers/base-provider-schema.test.js
- src/ai-providers/base-provider.js
- src/ai-providers/codex-cli.js
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b88e357. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gpt-5.5to the Codex CLI model catalog and reasoning-effort support.ai-sdk-provider-codex-cli@^0.7.3path and prefer the systemcodexbinary (or explicitcodexCli.codexPath) to avoid stale bundled Codex versions for ChatGPT OAuth users.Validation
npm test -- --runTestsByPath tests/unit/ai-providers/codex-cli.test.js tests/unit/ai-providers/base-provider-schema.test.js tests/unit/ai-providers/codex-cli-supported-models.test.js tests/unit/ai-services-unified.test.js→ 4 suites / 23 tests passednpm run turbo:typecheck→ 9 successful / 9 totaltask-master parse-prd ... --tag gpt55-final-smokewith providercodex-cliand modelgpt-5.5generated tasks successfullyNotes
ai-sdk-provider-codex-cli@1.1.0, which currently targets a newer provider spec than Task Master's AI SDK 5 stack supports.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Note
Medium Risk
Changes structured-output JSON schema generation and Codex CLI invocation defaults, which can affect object-generation success and which
codexbinary is executed at runtime. Covered by new unit tests but still touches core provider behavior.Overview
Fixes Codex CLI structured object generation and adds
gpt-5.5support.Structured-output schemas are now recursively normalized for OpenAI strict mode (auto-populating
additionalProperties: falseand inferredrequiredfields for object schemas, while preserving the existing removal of integer min/max constraints) before being passed into the AI SDK.The Codex CLI provider now prefers an explicitly configured
codexPath, otherwise auto-detects the systemcodexbinary (cached) to avoid using a potentially stale bundled CLI;gpt-5.5is added to thecodex-climodel catalog and reasoning-effort support. Dependencyai-sdk-provider-codex-cliis bumped to^0.7.3, and new unit tests cover schema normalization, model catalog, and codex path resolution.Reviewed by Cursor Bugbot for commit a0a0f36. Bugbot is set up for automated code reviews on this repo. Configure here.