Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the CLI's configuration system by introducing robust type casting for environment variables used within settings.json. By allowing string-based environment variables to be correctly interpreted as boolean or number types, it resolves previous validation issues and enhances the overall flexibility and reliability of how settings are managed and applied.

Highlights

• Environment Variable Type Casting: Enabled automatic type casting for boolean and number environment variables when used in settings.json, allowing string representations like "true" or "42" to be correctly interpreted as their respective types.
• Delayed Settings Validation: Refactored the settings loading process to delay configuration validation until after environment variables have been expanded, preventing validation errors caused by string-typed environment variable values.
• Improved Configuration Robustness: Ensured that successfully cast and validated data from environment variables is preserved and correctly utilized by the application, enhancing the flexibility and reliability of CLI configuration.

/gemini review

Size Change: +1.04 kB (0%)

Total Size: 33.9 MB

_{compressed-size-action}

/gemini review

/gemini review

From /review-frontend

Summary

The PR correctly identifies and addresses the need for type casting when environment variables are used in settings.json. However, there are a few critical and structural issues that need to be addressed before merging.

Critical Issues

1. Environment Variable Placeholder Loss on Save

In packages/cli/src/config/settings.ts, the load function now returns expanded settings. These expanded settings are then used to populate originalSettings:

const userOriginalSettings = structuredClone(userResult.settings);

Since saveSettings uses originalSettings to write back to the file, any save operation (e.g., changing a setting in the UI) will replace the environment variable placeholders (like "$MY_VAR") with their current values (like "true"). This breaks the "source of truth" for users who want to keep placeholders in their config.

Recommendation: The load function should return both the raw settings (with placeholders) and the expanded settings. originalSettings should be derived from the raw settings.

2. Inconsistent Casting in Shared Definitions (refs)

The casting logic was added to buildPrimitiveSchema, but buildZodSchemaFromJsonSchema (which builds REF_SCHEMAS) still uses standard z.boolean() and z.number().
Impact: Settings using shared definitions (like MCPServerConfig for mcpServers) will still fail validation if environment variables are used. For example, mcpServers.my-server.trust: "$TRUST" will not be cast.

Recommendation: Refactor buildZodSchemaFromJsonSchema to use buildPrimitiveSchema for consistent casting across all definitions.

Suggestions for Improvement

3. Test Assertions

In packages/cli/src/config/settings-validation.test.ts, the test it('should reject invalid castable strings', ...) should verify the specific error message to ensure it's failing for the right reason (type mismatch) rather than just "any error".

4. Type Safety in Tests

Avoid using // eslint-disable-next-line @typescript-eslint/no-explicit-any in tests. Prefer narrowing or using unknown with a cast to a partial of the expected type if needed for assertion.

/gemini review