Skip to content

fix(cli): make model outputs JSON-safe - #2135

Merged
zerob13 merged 2 commits into
devfrom
fix/issue-2120-json-safe-model-routes
Aug 11, 2026
Merged

fix(cli): make model outputs JSON-safe#2135
zerob13 merged 2 commits into
devfrom
fix/issue-2120-json-safe-model-routes

Conversation

@yyhhyyyyyy

@yyhhyyyyyy yyhhyyyyyy commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Project runtime model entries into an explicit JSON-safe DTO before returning models.listRuntime.
  • Project public model configuration field by field for both models.getPublicConfig and models.setPublicConfig.
  • Omit explicitly undefined properties, including nested image, video, and TTS options.
  • Keep the CLI route boundary fail-closed for schema-valid outputs that are not valid JSON.
  • Log bounded validation metadata without exposing route values or dynamic record keys.

Root Cause

Zod preserves optional properties when the input explicitly contains those properties with an undefined value.

The affected model routes passed their domain schemas, but their parsed results then failed JsonValueSchema validation in the CLI server because undefined is not a valid JSON value. As a result, model list and model config-get returned internal_error.

Compatibility

  • Preserves all existing renderer-visible runtime model fields, including falsy values.
  • Does not normalize, clamp, or add defaults to model configuration values.
  • Continues stripping private and unknown configuration fields.
  • Does not change the public CLI protocol or command syntax.

Testing

  • Covered empty model lists, multiple providers, and custom models.
  • Covered every currently exposed optional runtime model field.
  • Covered sparse public configuration for both config-get and config-set.
  • Covered nested image, video, and TTS properties containing explicit undefined.
  • Covered unknown/private field redaction.
  • Added a generic CLI boundary test for schema-valid but non-JSON output.
  • Verified validation logging does not expose dynamic output keys.

Validation completed:

  • pnpm run format:check
  • pnpm run i18n
  • pnpm run lint
  • pnpm run typecheck
  • 40 focused Vitest tests

Fixes #2120

Summary by CodeRabbit

  • Bug Fixes

    • Model configuration responses now expose only approved, JSON-safe settings, excluding private or unknown fields for both retrieval and updates.
    • Runtime model listings no longer expose internal credentials or runtime-only fields.
    • Invalid route responses are rejected safely with improved diagnostics, without leaking sensitive values or being treated as successful responses.
  • Tests

    • Expanded coverage for configuration redaction, runtime model responses, JSON safety, and invalid route output handling.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7da17e5d-faa0-4f28-8b7c-f40bc3a6add6

📥 Commits

Reviewing files that changed from the base of the PR and between 8060ece and eeb0ed0.

📒 Files selected for processing (4)
  • src/main/cli/providerModelAdminRoutes.ts
  • src/main/provider/routes.ts
  • src/main/routes/routeRegistry.ts
  • test/main/routes/routeRegistry.test.ts

📝 Walkthrough

Walkthrough

The changes add a shared JSON-safe route-output projection. Model configuration and runtime model routes use it to remove undefined and private fields. CLI diagnostics now distinguish route-contract failures from JSON-value failures.

Changes

JSON-safe model outputs

Layer / File(s) Summary
Route output JSON projection
src/main/routes/routeRegistry.ts, test/main/routes/routeRegistry.test.ts
projectJsonRouteOutput validates route output, normalizes it through JSON serialization, and validates the JSON result. Tests cover nested removal of undefined and private values.
Public model route projection
src/main/cli/providerModelAdminRoutes.ts, src/main/provider/routes.ts, test/main/cli/providerModelAdminRoutes.test.ts, test/main/provider/routes.test.ts
Model configuration GET and SET routes and the runtime model list route use JSON-safe projection. Tests cover sparse configurations, redaction, empty results, custom models, and credential omission.
Route validation diagnostics
src/main/cli/server.ts, test/main/cli/server.test.ts
parseRouteOutput separates contract and JSON-value validation failures and logs bounded issue metadata. Tests verify safe diagnostics and a 500 response for non-JSON output.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: making CLI model outputs JSON-safe.
Linked Issues check ✅ Passed The changes address issue #2120 through JSON-safe projections, redaction, validation diagnostics, and regression tests for model and configuration routes.
Out of Scope Changes check ✅ Passed All production and test changes support the linked issue objectives and the stated goal of safe CLI model outputs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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 fix/issue-2120-json-safe-model-routes

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

Comment @coderabbitai help to get the list of available commands.

@yyhhyyyyyy
yyhhyyyyyy requested a review from zerob13 August 10, 2026 13:26

@zerob13 zerob13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is covered and CI is green, but both DTO helpers duplicate the canonical route schemas and create avoidable field-drift risk. Details are inline.

Comment thread src/main/cli/providerModelAdminRoutes.ts Outdated
Comment thread src/main/provider/routes.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes CLI failures for model list and model config-get by ensuring route outputs are projected into JSON-safe values (notably removing explicitly-undefined optional fields) before they reach the CLI’s strict JsonValueSchema boundary. It introduces a reusable projection helper, applies it to the affected model routes, and adds regression tests that cover both JSON-safety and redaction.

Changes:

  • Added projectJsonRouteOutput to normalize contract-valid outputs into JSON-safe values (omitting explicit undefined keys).
  • Applied JSON-safe projection to models.listRuntime, models.getPublicConfig, and models.setPublicConfig outputs.
  • Strengthened CLI boundary logging with bounded Zod validation metadata and added targeted regression tests for JSON-invalid-but-schema-valid outputs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/main/routes/routeRegistry.ts Adds projectJsonRouteOutput helper to project contract outputs through JSON serialization to omit explicit undefined.
src/main/provider/routes.ts Uses projectJsonRouteOutput for models.listRuntime to prevent undefined from reaching CLI JSON validation.
src/main/cli/providerModelAdminRoutes.ts Uses projectJsonRouteOutput for public model config get/set responses to ensure JSON-safe sparse configs and redaction.
src/main/cli/server.ts Improves fail-closed output validation with stage-aware, bounded error metadata logging.
test/main/routes/routeRegistry.test.ts Adds unit test for projection removing private/unknown values before serialization and dropping undefined optionals.
test/main/provider/routes.test.ts Adds coverage ensuring runtime model lists are JSON-safe and do not leak internal fields across providers.
test/main/cli/server.test.ts Adds regression test for schema-valid but non-JSON route output and verifies logs don’t include dynamic record keys.
test/main/cli/providerModelAdminRoutes.test.ts Adds coverage for sparse public configs (including nested undefined) being returned as redacted JSON-safe values for get/set.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@zerob13
zerob13 merged commit 3bb8c92 into dev Aug 11, 2026
13 checks passed
@zhangmo8
zhangmo8 deleted the fix/issue-2120-json-safe-model-routes branch August 11, 2026 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Make model list and config-get outputs JSON-safe

3 participants