fix(agents): include codex mcp resume install setting in shape#113
fix(agents): include codex mcp resume install setting in shape#113cdenneen wants to merge 1 commit intohappier-dev:devfrom
Conversation
|
@coderabbitai review |
|
@greptile review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes add a new Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Greptile SummaryThis PR fixes a shape/default mismatch in Key changes:
Confidence Score: 5/5
Last reviewed commit: 3635b0c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/agents/src/providerSettings/definitions/codex.settingsShape.test.ts (1)
13-14: Avoid pinning the exact default string here.
packages/agents/src/providerSettings/registry.tsalready covers shape/default key parity across providers, so the highest-value check in this test is that the field exists. Locking the default to''makes this test noisier than necessary unless that exact value is part of the public contract.♻️ Suggested tightening
expect(shape).toHaveProperty('codexMcpResumeInstallSpec'); - expect(CODEX_PROVIDER_SETTINGS_DEFAULTS).toHaveProperty('codexMcpResumeInstallSpec', ''); + expect(CODEX_PROVIDER_SETTINGS_DEFAULTS).toHaveProperty('codexMcpResumeInstallSpec'); + expect(typeof CODEX_PROVIDER_SETTINGS_DEFAULTS.codexMcpResumeInstallSpec).toBe('string');Based on learnings: Applies to **/*.{test,spec}.{ts,tsx} : Avoid brittle 'content policing' tests that pin default config values, exact Markdown wording/format/length, or user-facing copy unless copy itself is the product requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agents/src/providerSettings/definitions/codex.settingsShape.test.ts` around lines 13 - 14, The test currently asserts an exact default value for CODEX_PROVIDER_SETTINGS_DEFAULTS.codexMcpResumeInstallSpec which is brittle; change the test to only assert the presence of the key/shape (keep expect(shape).toHaveProperty('codexMcpResumeInstallSpec')) and remove the assertion that pins CODEX_PROVIDER_SETTINGS_DEFAULTS to ''. Update the test to not reference the exact string default (leave CODEX_PROVIDER_SETTINGS_DEFAULTS existence checks only) so shape/default parity is still covered by registry.ts without locking this test to a specific default value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/agents/src/providerSettings/definitions/codex.settingsShape.test.ts`:
- Around line 13-14: The test currently asserts an exact default value for
CODEX_PROVIDER_SETTINGS_DEFAULTS.codexMcpResumeInstallSpec which is brittle;
change the test to only assert the presence of the key/shape (keep
expect(shape).toHaveProperty('codexMcpResumeInstallSpec')) and remove the
assertion that pins CODEX_PROVIDER_SETTINGS_DEFAULTS to ''. Update the test to
not reference the exact string default (leave CODEX_PROVIDER_SETTINGS_DEFAULTS
existence checks only) so shape/default parity is still covered by registry.ts
without locking this test to a specific default value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72084cdb-7de7-497a-88fe-c218b941b809
📒 Files selected for processing (2)
packages/agents/src/providerSettings/definitions/codex.settingsShape.test.tspackages/agents/src/providerSettings/definitions/codex.ts
WalkthroughThe changes add a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/agents/src/providerSettings/definitions/codex.settingsShape.test.ts (1)
9-16: Test structure looks good, though the value assertion is somewhat redundant.The registry validation in
registry.tsalready enforces shape/defaults parity at build time (throws if keys are missing). This test adds fast unit-level feedback, which is useful.One minor note: asserting the exact default value
''on line 14 could be slightly brittle if someone needs to change the default later. The presence check on line 13 already verifies the field exists in the shape. Consider whether pinning the exact value is necessary, or if just checkingtoHaveProperty('codexMcpResumeInstallSpec')for both would suffice.That said, for a "spec" field, an empty string default is a reasonable invariant to verify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agents/src/providerSettings/definitions/codex.settingsShape.test.ts` around lines 9 - 16, The test currently asserts an exact default value for codexMcpResumeInstallSpec which is brittle; update the test so it only checks that the defaults object has the key instead of pinning the value: locate the test using buildCodexProviderSettingsShape and CODEX_PROVIDER_SETTINGS_DEFAULTS and replace the expectation expect(CODEX_PROVIDER_SETTINGS_DEFAULTS).toHaveProperty('codexMcpResumeInstallSpec', '') with a presence check like toHaveProperty('codexMcpResumeInstallSpec') so the shape/default parity is still verified without locking the specific default string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/agents/src/providerSettings/definitions/codex.settingsShape.test.ts`:
- Around line 9-16: The test currently asserts an exact default value for
codexMcpResumeInstallSpec which is brittle; update the test so it only checks
that the defaults object has the key instead of pinning the value: locate the
test using buildCodexProviderSettingsShape and CODEX_PROVIDER_SETTINGS_DEFAULTS
and replace the expectation
expect(CODEX_PROVIDER_SETTINGS_DEFAULTS).toHaveProperty('codexMcpResumeInstallSpec',
'') with a presence check like toHaveProperty('codexMcpResumeInstallSpec') so
the shape/default parity is still verified without locking the specific default
string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 988aa271-3d28-43df-ba9d-d7b78a1bfe92
📒 Files selected for processing (2)
packages/agents/src/providerSettings/definitions/codex.settingsShape.test.tspackages/agents/src/providerSettings/definitions/codex.ts
1 participant
Summary
codexMcpResumeInstallSpecto Codex provider defaults inpackages/agentscodexMcpResumeInstallSpecto Codex provider settings shape validationWhy
apps/uiregisters this field in the Codex provider plugin, butpackages/agentsdid not define it in shape/defaults. This caused runtime plugin-registry validation failures in UI tests.Validation
yarn workspace @happier-dev/agents test src/providerSettings/definitions/codex.settingsShape.test.tsSummary by CodeRabbit
Release Notes
New Features
Tests