feat(cli): polite deep merge for settings.json and support JSONC by RbBtSn0w · Pull Request #1863 · github/spec-kit

RbBtSn0w · 2026-03-16T03:31:00Z

Description

This PR addresses issue #1799, where the Specify CLI would overwrite the user's existing .vscode/settings.json file during initialization or upgrades, leading to data loss of personal preferences.

Key Changes & Architectural Decisions

JSONC Parsing Refactor (The json5 library):
Initially, a custom state-machine tokenizer was implemented to handle VS Code's JSONC format (comments and trailing commas). However, given the complexity of edge cases (e.g., trailing commas immediately followed by block comments before a closing brace) and to future-proof the parser against other JSON5/JSONC quirks, the custom parser was entirely removed.
Instead, the robust json5 library has been integrated. It natively, effortlessly, and safely parses files with single/multi-line comments, trailing commas, and BOMs without the maintenance overhead of a custom tokenizer.
Polite Deep Merge: Implemented deep_merge_polite which strictly ensures that:
- New recommended settings from templates are added.
- Existing user settings are preserved and are never overwritten by template defaults.
- Nested dictionaries are merged recursively to maintain complex structures.
Non-Destructive Fallbacks (Zero Data Loss Guarantee):
Added extreme defensive programming to the merge pipeline. If the user's existing settings.json is completely unparseable (e.g., severe syntax error) or is not a JSON object (dictionary), the merge_json_files function immediately returns None. The CLI catches this and gracefully skips the file writing step entirely, guaranteeing that the user's original (even if invalid) configuration is never wiped by an empty dictionary or overridden by the template.
Unit Tests: Added comprehensive tests in tests/test_merge.py that specifically target the deep-merge preservation rules, type mismatch handling, and the secure fallback behavior.

Verification

Verified that .vscode/settings.json containing comments and trailing commas can be parsed cleanly via json5.
Verified that user-defined keys (e.g., editor.fontSize) remain strictly intact.
Verified that unparseable user files do not result in a destructive overwrite.
Verified all tests pass with uv run pytest tests/test_merge.py.

Fixes #1799

Copilot

Pull request overview

This PR updates Specify CLI’s .vscode/settings.json handling to avoid overwriting user settings during init/upgrade by parsing JSON-with-comments and performing a “polite” deep merge.

Changes:

Added strip_json_comments() and used it when reading template and existing VSCode settings.
Implemented merge_json_files() with a deep-merge policy that preserves existing (user) values while adding new keys.
Added unit tests for comment stripping and merge fallback behavior; updated changelog entries.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`src/specify_cli/__init__.py`	Adds JSONC-ish parsing and polite deep-merge logic for `.vscode/settings.json`.
`tests/test_merge.py`	Introduces tests for comment stripping and merge behavior/fallbacks.
`CHANGELOG.md`	Documents the new merge/JSONC support behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/test_merge.py

src/specify_cli/__init__.py

CHANGELOG.md

RbBtSn0w · 2026-03-16T03:37:48Z

I've addressed the feedback regarding JSONC parsing:

Implemented a state-machine tokenizer for strip_json_comments that correctly handles string literals (e.g., URLs).
Added support for trailing commas in objects and arrays.
Fixed indentation issues in src/specify_cli/__init__.py.
Added new test cases in tests/test_merge.py to cover these scenarios.

Copilot

Pull request overview

This PR updates Specify CLI’s VSCode .vscode/settings.json handling to avoid overwriting user settings by adding JSONC parsing support and a “polite” deep-merge strategy during init/upgrade flows.

Changes:

Added strip_json_comments() to support VSCode-style JSONC (line and block comments) and attempted trailing-comma handling.
Implemented “polite” deep merge logic in merge_json_files() to preserve existing user values while adding missing recommended keys.
Added unit tests for comment stripping and merge behavior; updated CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`src/specify_cli/__init__.py`	Adds JSONC stripping + polite deep merge, and uses it when handling `.vscode/settings.json`.
`tests/test_merge.py`	New tests for JSONC stripping and merge behavior (including invalid/non-object existing JSON).
`CHANGELOG.md`	Documents the new JSONC + polite merge behavior in Unreleased notes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

tests/test_merge.py

src/specify_cli/__init__.py

RbBtSn0w · 2026-03-16T03:44:29Z

Expanded test coverage across multiple dimensions:

String literals: Verified URL-like strings and escaped characters are not corrupted.
Trailing commas: Verified support for trailing commas in nested objects and arrays.
BOM handling: Added support for UTF-8 Byte Order Mark (BOM).
Merge conflicts: Verified polite merging preserves existing settings when types mismatch (e.g., string vs object).
Realistic scenarios: Simulated complex settings.json file merging.
All 9 test cases in tests/test_merge.py are passing.

RbBtSn0w · 2026-03-16T03:50:18Z

Addressing final review comments:

Refined Backslash Handling: strip_json_comments now determines whether a quote is escaped by correctly counting consecutive backslashes (odd = escaped, even = terminator). Added test cases for even_backslashes and odd_backslashes.
Improved Validation: merge_json_files now explicitly validates that new_content is a dictionary and returns an empty dictionary with a warning if it is not.
Cleanup: Removed the unused pytest import in tests/test_merge.py.
Refined Test Cases: Updated tests to reflect the new validation logic and cover complex escape sequences.

Copilot

Pull request overview

This PR updates Specify CLI’s VSCode initialization/upgrade flow to avoid overwriting a user’s existing .vscode/settings.json by adding JSONC parsing (comments/trailing commas) and performing a “polite” deep merge that preserves existing user values.

Changes:

Add strip_json_comments() to parse VSCode-style JSONC (comments + trailing commas).
Update settings handling to merge template settings into existing settings without overwriting user values (recursive for dicts).
Add unit tests for JSONC stripping + merge behavior and document the feature in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`src/specify_cli/__init__.py`	Adds JSONC stripping + polite deep merge and uses it during `.vscode/settings.json` handling.
`tests/test_merge.py`	Adds tests for comment stripping, deep merge behavior, BOM handling, and error cases.
`CHANGELOG.md`	Notes JSONC support and polite merge behavior.

Comments suppressed due to low confidence (1)

src/specify_cli/init.py:673

Merging .vscode/settings.json rewrites the file via json.dump(...), which will drop any existing comments and original formatting even though JSONC parsing is now supported. If preserving comments is a goal, you’ll need a non-destructive update strategy (e.g., patching the text AST) or at least document/warn that comments will be removed on merge.

        if dest_file.exists():
            merged = merge_json_files(dest_file, new_settings, verbose=verbose and not tracker)
            with open(dest_file, 'w', encoding='utf-8') as f:
                json.dump(merged, f, indent=4)
                f.write('\n')

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/test_merge.py

src/specify_cli/__init__.py

RbBtSn0w · 2026-03-16T04:00:38Z

Final updates to address all Copilot and Maintainer concerns:

String-Safe Trailing Commas: Integrated trailing comma removal into the state-machine loop of strip_json_comments. This ensures commas inside strings (e.g. "val, ]") are strictly preserved.
Secure Fallback Mechanism: Updated merge_json_files to return the existing configuration instead of an empty dictionary when the template (new_content) is invalid. This prevents accidental data loss.
Typo Fix: Corrected "Correctlly" to "Correctly".
Robustness: Added a specific test case test_strip_json_comments_safe_commas_in_strings to verify the state-machine's precision.
Indentation: Normalized all indentations to 4-space multiples.

Copilot

Pull request overview

This PR updates the Specify CLI’s VSCode settings handling to avoid overwriting user .vscode/settings.json during init/upgrade by supporting JSONC parsing and performing a “polite” deep merge that preserves existing user values.

Changes:

Add strip_json_comments() to support VSCode-style JSONC (comments + trailing commas) parsing.
Implement a polite recursive merge in merge_json_files() to add missing template keys without overwriting existing user settings.
Add unit tests for comment stripping and merge behavior; update the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
`src/specify_cli/__init__.py`	Adds JSONC stripping and updates `merge_json_files()` to preserve existing settings via polite deep merge.
`tests/test_merge.py`	Introduces tests for JSONC stripping, merge semantics, and some error-handling cases.
`CHANGELOG.md`	Documents the new JSONC + polite merge behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

tests/test_merge.py

mnriem

Please address Copilot feedback. If not applicable, please explain why

RbBtSn0w · 2026-03-17T01:31:39Z

All Copilot feedback has been addressed:

Removed unused imports (re in __init__.py, and Path/pytest in test_merge.py).
Fixed the fallback behavior so that it acts securely as a non-destructive operation: if the template or the existing JSON is invalid/non-dict, it safely returns None and the merge is skipped, ensuring user's data is strictly preserved and never overwritten with {}.
Added edge-case test coverage for an unparseable existing settings.json.

All tests pass. Could you please take another look?

Copilot

Pull request overview

This PR updates the Specify CLI’s VS Code settings initialization/upgrade flow to avoid clobbering an existing .vscode/settings.json, adding JSONC parsing support (comments + trailing commas) and a “polite” deep-merge strategy that preserves user-defined values.

Changes:

Add strip_json_comments() to support JSONC-style comments and trailing commas for VS Code settings parsing.
Implement merge_json_files() “polite deep merge” behavior that only adds missing keys and recursively merges nested objects without overwriting existing user values.
Add unit tests for JSONC stripping + merge behavior, and document the change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/specify_cli/__init__.py`	Adds JSONC stripping and a non-destructive merge path for `.vscode/settings.json` during init/upgrade.
`tests/test_merge.py`	Introduces coverage for comment stripping, trailing commas, merge preservation rules, and failure-mode behavior.
`CHANGELOG.md`	Documents the new JSONC support and polite merge behavior under Unreleased.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

tests/test_merge.py

RbBtSn0w

I have addressed the final edge case regarding trailing commas followed by comments. The strip_json_comments state-machine lookahead has been updated to safely skip over // and /* */ blocks when searching for the closing bracket/brace. Associated tests have been added.

RbBtSn0w

I have completely refactored the JSON parsing approach. To eliminate all edge cases and future-proof the JSONC compatibility (including complex comment patterns and trailing commas), I removed the custom state-machine implementation and integrated the json5 library as a dependency. It natively and safely handles the VS Code JSONC format. All merge strategies and non-destructive fallbacks remain intact.

Copilot

Pull request overview

This PR updates the Specify CLI’s VSCode settings handling to avoid overwriting user .vscode/settings.json during init/upgrade by parsing JSONC and performing a “polite” deep merge that only adds missing recommended keys.

Changes:

Parse VSCode settings.json as JSONC using json5 (comments + trailing commas).
Implement a polite deep-merge strategy that preserves existing user values and only adds new keys; skip writing entirely on parse/type issues to prevent data loss.
Add unit tests covering merge behavior (type mismatches, deep nesting, BOM, invalid JSON) and update dependencies/changelog.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`src/specify_cli/__init__.py`	Switch VSCode settings parsing to `json5` and implement “polite” deep merge with safe skip-write behavior (`None`).
`tests/test_merge.py`	Adds unit tests for merge preservation, nesting, BOM handling, and safe fallbacks on invalid inputs.
`pyproject.toml`	Adds `json5` runtime dependency for JSONC parsing support.
`CHANGELOG.md`	Documents the new settings merge/JSONC behavior (but see comment about `strip_json_comments`).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/test_merge.py

src/specify_cli/__init__.py

CHANGELOG.md

RbBtSn0w

Refactoring Update: Replacing Custom Tokenizer with `json5`

During the review process, we identified several edge cases in the custom state-machine used to parse JSONC (specifically around Lookaheads for trailing commas followed by block comments).

To eliminate these edge cases entirely, reduce code complexity, and future-proof the CLI against any other VS Code JSONC/JSON5 quirks, I have completely removed the custom tokenizer and integrated the json5 library.

Why json5?

Bulletproof Parsing: It natively handles single/multi-line comments, trailing commas, BOMs, and even unquoted keys if they ever occur.
Reduced Maintenance: We no longer need to maintain 100+ lines of state-machine logic and the associated parsing unit tests.
Simplicity: The file read logic is now as simple as json5.load(f).

The core business logic—the Polite Deep Merge and the Non-destructive fallbacks (returning None to safely skip overwriting bad files)—remains fully intact. The test suite has been updated to focus purely on the merge strategies rather than parsing mechanics.

All CI checks should now pass cleanly without any linting or edge-case failures.

Copilot

Pull request overview

This PR updates the Specify CLI’s .vscode/settings.json handling to avoid overwriting user configuration by parsing VS Code’s JSONC format and performing a “polite” deep merge that only adds missing template keys while preserving existing user values.

Changes:

Switch .vscode/settings.json template parsing to json5 to support JSONC features (comments/trailing commas/BOM).
Implement a non-destructive merge pipeline (merge_json_files) that returns None to skip writes when existing content/template content is unsafe to merge.
Add unit tests for deep-merge preservation and safety fallbacks; add json5 as a project dependency.

Reviewed changes

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

File	Description
`src/specify_cli/__init__.py`	Uses `json5` for parsing and adds a polite deep-merge function with safe “skip write” fallbacks.
`tests/test_merge.py`	Adds tests covering deep merge behavior and defensive non-destructive fallback scenarios.
`pyproject.toml`	Adds `json5` runtime dependency.
`CHANGELOG.md`	Documents the new JSONC + merge behavior (one entry appears stale).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

CHANGELOG.md

tests/test_merge.py

RbBtSn0w

Addressed. I've removed the unused import json from tests/test_merge.py to keep the test module perfectly clean and free of lint warnings.

Copilot

Pull request overview

This PR updates the Specify CLI’s VS Code initialization/upgrade flow to avoid clobbering an existing .vscode/settings.json, adding JSONC parsing support and a “polite” deep-merge strategy that preserves user-defined settings.

Changes:

Switch .vscode/settings.json parsing to json5 to support JSONC (comments, trailing commas, BOM).
Implement “polite” deep merge behavior and a non-destructive fallback (merge_json_files() returns None to skip writing on unsafe conditions).
Add unit tests covering merge behavior and safety fallbacks; add json5 dependency and changelog entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`src/specify_cli/__init__.py`	Uses `json5` for parsing and adds `merge_json_files()` with polite deep merge + safe “skip write” fallbacks.
`tests/test_merge.py`	Adds tests for deep-merge preservation, realistic VS Code JSONC, BOM handling, and unparseable-file safety.
`pyproject.toml`	Adds runtime dependency on `json5`.
`CHANGELOG.md`	Documents the new CLI behavior in the Unreleased section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

Copilot

Pull request overview

Prevents destructive overwrites of users’ existing VS Code .vscode/settings.json during CLI init/upgrade by parsing JSONC via json5 and performing a “polite” deep merge that only adds missing recommended settings.

Changes:

Switch VS Code settings parsing to json5 to support JSONC features (comments/trailing commas) and adjust merge/write flow to skip writing on unsafe merges.
Implement merge_json_files() with a deep “preserve existing values” strategy and defensive None return to avoid clobbering unparseable/non-object settings.
Add unit tests covering deep-merge behavior and safety fallbacks; add json5 dependency and changelog entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
`src/specify_cli/__init__.py`	Uses `json5` for JSONC parsing and introduces polite deep merge with non-destructive fallbacks during VS Code settings handling.
`tests/test_merge.py`	Adds tests validating preservation rules, deep nesting merges, BOM handling, and “skip write” behavior via `None` fallbacks.
`pyproject.toml`	Adds `json5` runtime dependency.
`CHANGELOG.md`	Documents the new polite merge + JSONC support feature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

Copilot

Pull request overview

This PR updates the Specify CLI’s .vscode/settings.json handling to avoid clobbering user settings during init/upgrade by parsing VS Code JSONC and performing a “polite” deep merge that preserves existing user values.

Changes:

Switch settings parsing to json5 to support JSONC (comments, trailing commas, BOM).
Implement merge_json_files() polite deep-merge behavior with “skip write” fallbacks (None) to prevent destructive overwrites.
Add unit tests covering merge behavior and defensive fallback cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`src/specify_cli/__init__.py`	Uses `json5` for JSONC parsing, adds polite deep merge + skip-write behavior, adjusts settings merge/copy flow
`tests/test_merge.py`	Adds tests for deep-merge preservation rules and safe fallbacks
`pyproject.toml`	Adds `json5` dependency
`CHANGELOG.md`	Documents the CLI settings merge/JSONC support feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

Copilot

Pull request overview

This PR updates the Specify CLI’s VS Code settings handling to avoid overwriting user .vscode/settings.json by parsing JSONC via json5 and performing a “polite” deep merge that only adds missing keys, with defensive fallbacks to skip writing on unsafe/invalid inputs.

Changes:

Parse both template and existing VS Code settings using json5 (JSONC/JSON5 support) and add atomic writes when a merge produces changes.
Implement merge_json_files() polite deep-merge semantics with None return to signal “don’t rewrite”.
Add unit tests covering preservation rules, deep nesting, BOM handling, and unparseable/non-dict fallbacks.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
`src/specify_cli/__init__.py`	Switch settings parsing to `json5`, add polite deep merge + safe skip behavior, and atomic writes on changes.
`tests/test_merge.py`	New tests for merge semantics, JSONC parsing, and safety fallbacks.
`pyproject.toml`	Add `json5` as a runtime dependency.
`CHANGELOG.md`	Document the new settings merge behavior and JSONC support.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

Copilot

Pull request overview

Prevents Specify CLI init/upgrade from clobbering an existing .vscode/settings.json by parsing JSONC and performing a “polite” deep merge that preserves user-defined settings and skips writes when there are no effective changes.

Changes:

Switch VS Code settings parsing to json5 to support JSONC features (comments/trailing commas/BOM).
Implement merge_json_files() polite deep merge with non-destructive None fallback and “no-op => don’t rewrite” behavior.
Add unit tests covering deep merge rules, type mismatches, BOM handling, and unparseable-file fallback.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`src/specify_cli/__init__.py`	Uses `json5` for JSONC parsing, adds polite deep merge + atomic write behavior, and skips overwriting on unsafe conditions.
`tests/test_merge.py`	Adds focused tests for merge semantics and safety fallbacks.
`pyproject.toml`	Adds `json5` as a runtime dependency.
`CHANGELOG.md`	Documents the new settings merge/JSONC support behavior in Unreleased notes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/specify_cli/__init__.py

Copilot

Pull request overview

This PR updates the Specify CLI’s init/upgrade behavior for .vscode/settings.json to avoid overwriting user configuration by parsing VS Code JSONC (comments/trailing commas) and performing a non-destructive “polite” deep merge.

Changes:

Add JSONC/JSON5 parsing via json5 and update merge logic to preserve existing user values, merging only nested dict additions.
Implement atomic JSON rewriting that preserves file mode bits and skips rewriting when there are no changes (or when parsing/shape is unsafe).
Add unit tests covering deep merge behavior, type mismatches, JSONC inputs, BOM handling, and non-destructive fallback behavior.

Reviewed changes

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

File	Description
`src/specify_cli/__init__.py`	Switch settings parsing to `json5`, implement polite deep merge with “skip rewrite” semantics, and add atomic write preserving mode/ownership best-effort.
`tests/test_merge.py`	Adds targeted tests for merge preservation rules, JSONC parsing cases, and atomic-write mode preservation.
`pyproject.toml`	Adds `json5` as a runtime dependency.
`CHANGELOG.md`	Documents the new settings merge + JSONC support behavior under Unreleased.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…omic write

RbBtSn0w · 2026-03-17T06:07:56Z

This PR is superseded by #1874 to provide a cleaner review history with the same final code changes. Please continue review there.

RbBtSn0w · 2026-03-17T06:07:58Z

Closing in favor of #1874 (same content, cleaner commit/review history).

RbBtSn0w requested a review from mnriem as a code owner March 16, 2026 03:31

Copilot AI review requested due to automatic review settings March 16, 2026 03:31

Copilot started reviewing on behalf of RbBtSn0w March 16, 2026 03:31 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

tests/test_merge.py Show resolved Hide resolved

src/specify_cli/__init__.py Outdated Show resolved Hide resolved

src/specify_cli/__init__.py Outdated Show resolved Hide resolved

src/specify_cli/__init__.py Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

RbBtSn0w requested a review from Copilot March 16, 2026 03:39

Copilot started reviewing on behalf of RbBtSn0w March 16, 2026 03:40 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 16, 2026 03:50

Copilot started reviewing on behalf of RbBtSn0w March 16, 2026 03:50 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

mnriem requested a review from Copilot March 16, 2026 13:23

Copilot started reviewing on behalf of mnriem March 16, 2026 13:23 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

src/specify_cli/__init__.py Outdated Show resolved Hide resolved

src/specify_cli/__init__.py Show resolved Hide resolved

tests/test_merge.py Outdated Show resolved Hide resolved

tests/test_merge.py Outdated Show resolved Hide resolved

mnriem requested changes Mar 16, 2026

View reviewed changes

RbBtSn0w requested a review from Copilot March 17, 2026 02:02

Copilot started reviewing on behalf of RbBtSn0w March 17, 2026 02:02 View session