Skip to content

fix(jsonc): preserve strict JSON after removing adjacent hook entries - #1078

Merged
1 commit merged into
tirth8205:stagingfrom
azizur100389:codex/fix-uninstall-json
Oct 15, 2026
Merged

1 commit merged into
tirth8205:stagingfrom
azizur100389:codex/fix-uninstall-json

Conversation

@azizur100389

@azizur100389 azizur100389 commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Linked issue

Closes #1068.

What & why

Uninstalling CRG from Claude settings with user-owned hooks can remove adjacent final members and leave the preceding separator behind. Fix the shared token-preserving removal helper to remove newly exposed trailing commas, while retaining pre-existing JSONC commas, comments, formatting, and literal commas inside strings. Includes an actual install/uninstall cycle that strictly parses the resulting settings and verifies that user permissions and hooks survive.

How it was tested

Following the same staged regression/baseline methodology used in my earlier PRs (#274, #276, #279, #353 and #354), against the current contribution target staging at 6b12d11625cbec3b6773e076cb3d136464fa90e5.

  • New regression tests: 8/8 passed on Windows, Python 3.13.14. The reported bug reproduces on unchanged staging before the fix.
  • Initial affected-module run: 328 passed, 12 baseline failures, 1 skipped. Every failure also reproduces on unchanged staging (compared by JUnit test identity). The final expanded regression module passed separately; no new affected-module failures.
  • ruff check code_review_graph tests/test_jsonc_removals.py: clean.
  • bandit -r code_review_graph/ -c pyproject.toml: zero findings.
  • mypy code_review_graph/ --ignore-missing-imports --no-strict-optional: the same three existing Windows fcntl.LOCK_EX/LOCK_NB/flock errors in daemon.py as unchanged staging; no new errors.
  • Schema remains version 13 on both Python and VS Code sides.
  • GitHub CI: 17 checks passed on the final commit. Python 3.10–3.13, browser tests, Windows native tests, and the Linux/macOS/Windows MCP client checks are green. Python 3.13 full-suite result: 4276 passed, 804 skipped, 46 deselected, 2 xfailed, 2 xpassed; 85.89% coverage (65% required). The manual upgrade job is skipped by design.

Exact test commands (using a local virtual environment with .[dev], as uv is not installed):

python -m pytest tests/test_jsonc_removals.py -q --tb=short
python -m pytest tests/test_jsonc_removals.py tests/test_uninstall.py tests/test_installer_ownership.py tests/test_skills.py -q --tb=short
$env:GIT_CEILING_DIRECTORIES = $env:TEMP.Replace('\', '/')
python -m pytest --tb=short -q -m 'not browser and not upgrade' --cov=code_review_graph --cov-report=term --cov-fail-under=65

Full local Windows suite completed: 4,151 passed, 110 failed, 819 skipped, 46 deselected, 2 xfailed, 2 xpassed; 86% coverage. Unchanged staging: 4,143 passed, 110 failed, with identical skip/xfail/xpass counts. Failure identities match unchanged staging exactly; no new failures. The Git ceiling isolates temporary test repositories from an unrelated enclosing home-directory Git repository; PATH/PYTHONPATH point to each checkout under test. Browser tests passed in CI; browser and opt-in upgrade suites were excluded locally.

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

code-review-graph review

Overall risk: 0.65 (MEDIUM) — 5 changed function(s)/class(es), 1 affected flow(s), 0 test gap(s)

Risk-scored changes

Risk Level Symbol Location Tested
0.65 medium code_review_graph/jsonc.py::remove_paths code_review_graph/jsonc.py:288 yes
0.15 low tests/test_jsonc_removals.py::test_multi_removal_preserves_strict_json tests/test_jsonc_removals.py:18 (test)
0.15 low tests/test_jsonc_removals.py::test_removal_preserves_comments_and_string_commas tests/test_jsonc_removals.py:22 (test)
0.15 low tests/test_jsonc_removals.py::test_existing_jsonc_trailing_comma_is_preserved tests/test_jsonc_removals.py:30 (test)
0.05 low tests/test_jsonc_removals.py::test_install_uninstall_preserves_strict_claude_settings tests/test_jsonc_removals.py:36 (test)

Affected execution flows

  • main — criticality 0.72, 923 node(s) across 40 file(s)

Powered by code-review-graph — local-first analysis; no code leaves the CI runner.

@azizur100389
azizur100389 marked this pull request as ready for review October 1, 2026 16:46
@tirth8205 tirth8205 closed this pull request by merging all changes into tirth8205:staging in 03a5c7d Oct 15, 2026
@tirth8205

Copy link
Copy Markdown
Owner

Thank you! I added tests for nested JSONC removals with comments, top-level arrays, and Gemini and Claude settings after uninstall. The tests and your change are merged into staging in #1115.

azizur100389 pushed a commit to azizur100389/code-review-graph that referenced this pull request Oct 15, 2026
- every removal subset of object members and array elements (sizes 1-5,
  compact and indented), adjacent or not, parses as strict JSON
- commented JSONC: all comments survive and no new trailing comma appears
- empty path list returns the document unchanged
- a surviving member keeps its own JSONC trailing comma; a removed tail
  that carried one still leaves strict JSON
- duplicate paths, parent+child overlapping paths, removals in several
  containers at once, nested hook groups emptied at the end
- unicode, escaped quotes and Windows paths before the edit point; CRLF
- large documents (300 members/elements)
- unresolvable paths still raise KeyError/IndexError/ValueError
- install -> uninstall leaves strict JSON with user hooks for Claude,
  Qoder, CodeBuddy, Codex, Gemini (own and shared events) and Cursor
- Continue duplicate registrations at the end of mcpServers
- hooks written by an earlier release, owned hooks inside a user group
- second uninstall is byte-stable; install/uninstall cycles; dry run
- the issue tirth8205#1068 reproduction through the CLI

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
azizur100389 pushed a commit to azizur100389/code-review-graph that referenced this pull request Oct 15, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants