Skip to content

Handle per-key failures during settings import#401

Open
taltas wants to merge 1 commit into
mainfrom
import-fix
Open

Handle per-key failures during settings import#401
taltas wants to merge 1 commit into
mainfrom
import-fix

Conversation

@taltas
Copy link
Copy Markdown
Contributor

@taltas taltas commented May 29, 2026

Summary

  • make settings import resilient to invalid values on individual keys instead of failing the entire import
  • continue importing valid settings while collecting/reporting per-key failures
  • add coverage for resilient partial import behavior in both import/export and auto-import paths

Why

RooCode settings import could fail entirely when a single persisted setting was invalid for the current schema. One concrete case is imageGenerationProvider receiving the invalid value roo, which causes the import path to reject the whole payload.

This change makes import behavior more resilient by skipping only the invalid keys, preserving valid settings from the same import, and covering the behavior with regression tests.

Summary by CodeRabbit

  • New Features

    • Enhanced validation of global settings during import with improved handling of unsupported values
    • Refined warning messages to provide clearer feedback on import issues
  • Tests

    • Expanded test coverage for global settings import behavior and warning reporting

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

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: 0a6d5956-056a-4a9b-b2bf-73e2ca70f48b

📥 Commits

Reviewing files that changed from the base of the PR and between 45088a7 and cbdb2ff.

📒 Files selected for processing (4)
  • src/core/config/__tests__/importExport.spec.ts
  • src/core/config/importExport.ts
  • src/utils/__tests__/autoImportSettings.spec.ts
  • src/utils/autoImportSettings.ts

📝 Walkthrough

Walkthrough

Settings import now validates and sanitizes globalSettings field-by-field during import, accumulates warnings for invalid or unsupported entries (including clearing unsupported imageGenerationProvider: "roo"), propagates sanitized values through custom modes and context proxy updates, and reports warnings consistently through toast messages and auto-import output channels.

Changes

Global Settings Import Validation and Warning Reporting

Layer / File(s) Summary
Global settings sanitization schema and logic
src/core/config/importExport.ts
Imports GlobalSettings type and creates sanitizeGlobalSettings helper that derives a Zod shape from globalSettingsSchema, validates each field with safeParse, accumulates warnings for invalid or unknown entries, and clears unsupported imageGenerationProvider: "roo" value.
Settings import flow integration of sanitization
src/core/config/importExport.ts
Updates lenient import schema to treat globalSettings as optional z.unknown(), integrates sanitizeGlobalSettings during import to validate and collect warnings, appends warnings to import list, uses sanitized values for custom modes updates and contextProxy.setValues, and returns globalSettings: sanitizedGlobalSettings in result.
Global settings import validation test coverage
src/core/config/__tests__/importExport.spec.ts
Tests sanitization during import: verifies imageGenerationProvider: "roo" is normalized with warnings while preserving other fields, imports only valid fields when invalid top-level keys are present, skips invalid customModes entries without aborting import, and uses generic "item" wording when warnings stem from global settings only.
Warning message consistency updates
src/core/config/importExport.ts, src/core/config/__tests__/importExport.spec.ts
Changes warning summary wording from "profiles had issues during import" to "items had issues during import" in importSettingsWithFeedback and corresponding test expectations for both singular and plural cases.
Auto-import warning output reporting
src/utils/autoImportSettings.ts, src/utils/__tests__/autoImportSettings.spec.ts
Adds conditional warning reporting to output channel after successful auto-import, including total count and per-warning detail lines with pluralization; test verifies warnings are logged to output channel while import still succeeds and sanitized settings with imageGenerationProvider cleared are applied to context.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Hop skip, validate with care,
Settings fields parsed fair and square,
Warnings gathered, "roo" cast away,
Sanitized and safe to stay.
Items fixed, not profiles now,
Import warnings take a bow! 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description lacks required sections from the template: no related GitHub Issue link, no test procedure details, no pre-submission checklist completion, and missing discord username. While the Summary and Why sections provide useful context, critical template sections are absent. Add missing required sections: link the related GitHub Issue (Closes: #), provide detailed test procedure steps, complete the pre-submission checklist, and include Discord username for contact.
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: handling per-key failures during settings import instead of failing entirely, which directly aligns with the changeset's core objective.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 import-fix

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/core/config/__tests__/importExport.spec.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.

src/core/config/importExport.ts

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.

src/utils/__tests__/autoImportSettings.spec.ts

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.

  • 1 others

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 and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

❌ Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/core/config/importExport.ts 81.63% 8 Missing and 1 partial ⚠️
src/utils/autoImportSettings.ts 88.88% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

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.

1 participant