fix: strip commandSpecific before passing settings to createCodexCli#1688
Open
octo-patch wants to merge 2 commits into
Open
Conversation
…ixes eyaltoledano#1679) codexCli.commandSpecific is a Task Master config feature used for per-command overrides. It was being spread directly into defaultSettings and forwarded to createCodexCli(), which rejects it as an unrecognized key. Destructure commandSpecific out of the resolved settings object before building defaultSettings so only Codex CLI supported keys are forwarded. Add a regression test verifying commandSpecific is not present in createCodexCli call args.
🦋 Changeset detectedLatest commit: 752d96f The changes in this PR will be included in the next version bump. 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA patch prevents the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
tests/unit/ai-providers/codex-cli.test.js (1)
106-115: Good coverage of the regression.Test asserts both the negative (no
commandSpecificforwarded) and positive (other keys likeallowNpxstill pass through) cases.mockReturnValueOncecomposes correctly with thejest.clearAllMocks()inbeforeEach.One optional strengthening: also assert
createCodexCliwas called exactly once and thatdefaultSettingsdoes not contain any nested reference to the'parse-prd'override keys (e.g.,approvalMode: 'never'), to guard against a future partial-merge regression. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/ai-providers/codex-cli.test.js` around lines 106 - 115, Add two extra assertions to harden the test: assert that createCodexCli was called exactly once after calling provider.getClient({ commandName: 'parse-prd' }) and assert that the produced call.defaultSettings has no nested override keys from commandSpecific (e.g., does not contain approvalMode or any property 'parse-prd'). Locate the existing test that mocks getCodexCliSettingsForCommand and inspects createCodexCli.mock.calls[0][0] (symbols: getCodexCliSettingsForCommand, provider.getClient, createCodexCli, call.defaultSettings) and add expectations for createCodexCli.mock.calls.length === 1 and that call.defaultSettings neither has property 'approvalMode' nor any nested object keyed by 'parse-prd'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/ai-providers/codex-cli.test.js`:
- Around line 106-115: Add two extra assertions to harden the test: assert that
createCodexCli was called exactly once after calling provider.getClient({
commandName: 'parse-prd' }) and assert that the produced call.defaultSettings
has no nested override keys from commandSpecific (e.g., does not contain
approvalMode or any property 'parse-prd'). Locate the existing test that mocks
getCodexCliSettingsForCommand and inspects createCodexCli.mock.calls[0][0]
(symbols: getCodexCliSettingsForCommand, provider.getClient, createCodexCli,
call.defaultSettings) and add expectations for createCodexCli.mock.calls.length
=== 1 and that call.defaultSettings neither has property 'approvalMode' nor any
nested object keyed by 'parse-prd'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9526fd1e-3d5c-494a-9723-36c680ba5bb4
📒 Files selected for processing (3)
.changeset/fix-codex-commandspecific-leak.mdsrc/ai-providers/codex-cli.jstests/unit/ai-providers/codex-cli.test.js
Address coderabbit nitpick on eyaltoledano#1688: - Assert createCodexCli was called exactly once after the single getClient invocation (previously the test only checked the first call's args). - Assert that no override key from the nested commandSpecific block (e.g. approvalMode) leaked into the top-level defaultSettings, guarding against a future partial-merge regression in case someone changes the destructuring strategy.
Author
|
Thanks @coderabbitai — applied the optional strengthening in 752d96f:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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.
Fixes #1679
Problem
When
codexCli.commandSpecificis present in the Task Master config, the resolved settings object (returned bygetCodexCliSettingsForCommand) includes thecommandSpecifickey. This object is spread directly intodefaultSettingsand forwarded tocreateCodexCli().Codex CLI does not know about
commandSpecific— it's a Task Master-level concept — so it rejects the settings with:Solution
Destructure
commandSpecificout of the resolvedsettingsobject before constructingdefaultSettings. Only Task Master–agnostic keys are forwarded tocreateCodexCli().Testing
getCodexCliSettingsForCommandreturns an object that includescommandSpecific, the resultingcreateCodexClicall does not containcommandSpecificindefaultSettings.Note
Low Risk
Low risk: small, localized change to Codex CLI provider settings sanitization with a targeted unit test; main risk is unintended omission of other provider settings during the destructuring/forwarding step.
Overview
Fixes Codex CLI initialization failures by stripping Task Master-only
codexCli.commandSpecificfrom the settings object before buildingdefaultSettingspassed tocreateCodexCli().Adds a unit test to ensure
commandSpecific(and any nested override keys) are not forwarded, and includes a patch changeset documenting the bug fix.Reviewed by Cursor Bugbot for commit 752d96f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests