fix(install): never wipe a hook settings file it cannot read as strict JSON by tirth8205 · Pull Request #1157 · tirth8205/code-review-graph

tirth8205 · 2026-11-06T18:54:39Z

What was wrong

install could wipe a user's hook settings file. All four hook writers read their file with strict json.loads. One comment or trailing comma made that fail, the error was only logged, existing stayed {}, and the file was overwritten with nothing but our hooks. The backup line sat after the parse, so no .bak was written either. This loses every setting in the file, not just the comments.

Found while building #1155. jsonc.py already protected MCP config, but the hook writers never used it.

Files affected: .claude/settings.json, .qoder/settings.json, .codebuddy/settings.json, .gemini/settings.json, ~/.codex/hooks.json, ~/.cursor/hooks.json.

What changed

Hook files are read as JSONC through one shared path. A commented file gets only what changed spliced in, and every comment stays with the setting it describes. It gets a .bak first, and reinstalling leaves it byte-identical.
A file that cannot be merged is left byte for byte and named in the output, and install no longer says it installed those hooks. That covers broken JSON, a top level that is not an object, hooks or a hook event of the wrong shape, bad UTF-8, and a comment inside an event list we would have to replace. The refused Gemini and Cursor installs write no scripts either.
Cursor's file now gets a .bak like the others.
jsonc.set_member no longer moves a trailing comment onto a new member, and no longer drops a comment from an empty object. This also helps the MCP writer.

One behaviour change to check

Installing over a corrupt hook file now refuses instead of overwriting it. test_handles_corrupt_existing_json pinned the old overwrite for Cursor, so I rewrote it as test_leaves_corrupt_existing_json_alone. The MCP writer already refuses in this case. The cost is that someone with a truly broken file gets a message rather than hooks.

Real checks

Real code-review-graph install under a throwaway HOME, on every JSON file each of the 16 platforms writes, seeded with a comment, a trailing comma, both, and broken JSON (80 cases). Staging was a snapshot of origin/staging 115dccf.

	Staging	This PR
Cases that lost data	23 of 80	0 of 80

On the 6 hook files in this PR (24 cases):

18 of 18 mergeable cases kept the user's data, added our hooks and wrote a backup.
6 of 6 broken files stayed byte-identical.

Real install, then real uninstall, on a commented .claude/settings.json:

// after install
{
  // the whole team reads this file
  "permissions": {"allow": ["Bash(ls:*)"]}, // keep this short
  "model": "opus", /* pinned for now */
  "hooks": {

After uninstall all three comments and both settings are still there, and our hooks are gone. The only leftover is a blank line where the hooks were.

Not run: the real clients reading these files. This checks what install writes, not how each client treats a commented file. Windows was not run here.

Tests

New cases in tests/test_installer_ownership.py run against all six writers; all 75 failed on staging before the fix.
New cases in tests/test_jsonc_removals.py and tests/test_cli_install.py.
ruff and mypy pass. Full suite: 6,474 passed, 0 failed.

🤖 Generated with Claude Code

…t JSON All four hook writers read their file with json.loads. A comment or trailing comma made that fail, the error was only logged, and the file was overwritten with nothing but our hooks and no backup, so every setting in it was lost. They now share one path: JSONC is read as JSONC, a commented file gets only what changed spliced in, and a file that cannot be merged is left byte for byte and named in the output. Cursor's file now gets a backup like the others. jsonc.set_member no longer moves a trailing comment onto a new member or drops a comment from an empty object. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-11-06T18:55:31Z

code-review-graph review

Overall risk: 0.55 (MEDIUM) — 39 changed function(s)/class(es), 2 affected flow(s), 5 test gap(s) (0 with no tested caller found, 5 reached only through a caller)

Risk-scored changes

Risk	Level	Symbol	Location	Tested
0.55	medium	code_review_graph/cli.py::_handle_init	code_review_graph/cli.py:273	yes
0.40	medium	code_review_graph/jsonc.py::_past_trailing_comment	code_review_graph/jsonc.py:389	indirect
0.40	medium	code_review_graph/skills.py::_report_settings_left_alone	code_review_graph/skills.py:2191	indirect
0.35	low	code_review_graph/jsonc.py::has_unclosed_comment	code_review_graph/jsonc.py:92	yes
0.35	low	code_review_graph/jsonc.py::set_member	code_review_graph/jsonc.py:417	yes
0.30	low	code_review_graph/jsonc.py::value_has_comments	code_review_graph/jsonc.py:409	yes
0.30	low	tests/test_installer_ownership.py::hook_writer	tests/test_installer_ownership.py:516	(test)
0.30	low	tests/test_installer_ownership.py::test_hook_install_keeps_a_comment_with_the_setting_it_belongs_to	tests/test_installer_ownership.py:555	(test)
0.30	low	tests/test_installer_ownership.py::test_hook_reinstall_leaves_a_commented_file_byte_identical	tests/test_installer_ownership.py:609	(test)
0.30	low	tests/test_installer_ownership.py::test_hook_install_leaves_a_file_it_cannot_merge_alone	tests/test_installer_ownership.py:631	(test)

Affected execution flows

main — criticality 0.72, 1103 node(s) across 45 file(s)
_positive_int — criticality 0.32, 2 node(s) across 1 file(s)

Reached only through a caller

No test names these directly, but a tested caller reaches them along the call path shown. That is a path in the call graph, not a record of execution -- the caller's tests may never take this branch. Treat it as where to look, not as coverage: these count as gaps and are scored as untested.

code_review_graph/jsonc.py::_past_trailing_comment (code_review_graph/jsonc.py:389) — via code_review_graph/jsonc.py::set_member, 1 hop(s)
code_review_graph/skills.py::_report_settings_left_alone (code_review_graph/skills.py:2191) — via code_review_graph/skills.py::_merge_hooks_into_settings, 2 hop(s)
code_review_graph/skills.py::_read_settings_for_merge (code_review_graph/skills.py:2198) — via code_review_graph/skills.py::_merge_hooks_into_settings, 1 hop(s)
code_review_graph/skills.py::_splice_merged_settings (code_review_graph/skills.py:2249) — via code_review_graph/skills.py::_merge_hooks_into_settings, 2 hop(s)
code_review_graph/skills.py::_write_merged_settings (code_review_graph/skills.py:2290) — via code_review_graph/skills.py::_merge_hooks_into_settings, 1 hop(s)

Token savings: this graph-backed report used ~58,753 fewer tokens (~40%) than reading every changed file in full (estimated, chars/4 approximation).

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

Copilot

🟡 Changes recommended

Unterminated JSONC comments can leave hooks inactive despite reporting success, and rejected Gemini installs can still overwrite executable hook scripts.

2 open issues

What changed in this PR

This PR protects user settings during hook installation by sharing JSONC-aware merge logic across six hook files, addressing the data-loss issue identified while developing #1155.

Changes:

Preserve comments and user settings, back up edits, and refuse files that cannot be safely merged.
Report hook installation success only when the writer succeeds.
Improve JSONC comment placement and add regression coverage for preservation, refusal, and reinstall behavior.

File	Description
tests/test_skills.py	Require corrupt Cursor settings to remain unchanged.
tests/test_jsonc_removals.py	Test comment placement and detection within values.
tests/test_installer_ownership.py	Cover safe merges and refusal across six hook writers.
tests/test_cli_install.py	Check that refused hooks are not reported as installed.
code_review_graph/skills.py	Centralize JSONC reading, safe merging, backups, and refusal reporting.
code_review_graph/jsonc.py	Preserve comments during member insertion and detect comments within values.
code_review_graph/cli.py	Make installation messages conditional on successful writes.
CHANGELOG.md	Document hook-settings protection and JSONC fixes.

_{Review effort: Balanced}

tirth8205 · 2026-11-09T15:49:16Z

+    stripped = _strip_jsonc(raw)
+    if not stripped.strip():
+        return raw, {}


Fixed in cc14b85: a file with a /* that never closes is now refused and left byte for byte as it was, including a comment-only file, with tests for all six writers.

tirth8205 · 2026-11-09T15:49:19Z

+    if not _write_merged_settings(settings_path, raw, existing, label):
+        return None


Fixed in cc14b85: Gemini now writes its hooks folder and scripts only after the settings merge succeeds, as Cursor does, and a test checks a refused install keeps the scripts it found.

… last A /* with no */ made the hooks land inside the comment: written, reported as installed, never run. A refused Gemini CLI install still rewrote the two hook scripts the unchanged settings may run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

#1157 in the train rewrote the hook settings reads and writes to keep comments, and #1155, #1172 and #1180 build on that. Each of them now goes through the install plan, so --dry-run still writes nothing and a real install still writes exactly what the dry run showed: - _read_settings_for_merge and _write_merged_settings read, splice, back up and write through install_plan (the splice as bytes, so CRLF stays). - The steering hook is planned after the other hooks, and --no-steering removes it through the plan, so it sees what this run planned. - install_gitignore.apply reads, checks paths and writes through the plan. The block is planned after the instruction files and again without them when the question is answered no. - Gemini's hook scripts and the Cursor and Codex hook folders are no longer created before the plan is applied. - A hook settings file install will leave alone is named in the dry run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

tirth8205 requested a balanced review from Copilot November 9, 2026 14:59

Copilot started reviewing on behalf of tirth8205 November 9, 2026 15:00

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

tirth8205 mentioned this pull request Nov 9, 2026

Merge train: 43 reviewed PRs (2026-11-09) #1186

Merged

tirth8205 mentioned this pull request Nov 9, 2026

fix(install): show each change in --dry-run and write nothing before the question #1173

Merged

tirth8205 closed this pull request by merging all changes into staging in d01c1fd Nov 9, 2026

Navigation Menu

Repository navigation

fix(install): never wipe a hook settings file it cannot read as strict JSON - #1157

fix(install): never wipe a hook settings file it cannot read as strict JSON#1157
2 commits merged into
stagingfrom
fix/install-keeps-commented-settings

tirth8205 commented Nov 6, 2026

Uh oh!

github-actions Bot commented Nov 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

tirth8205 Nov 9, 2026

Uh oh!

tirth8205 Nov 9, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if not _write_merged_settings(settings_path, raw, existing, label):
		return None

Conversation

tirth8205 commented Nov 6, 2026

What was wrong

What changed

One behaviour change to check

Real checks

Tests

Uh oh!

github-actions Bot commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

code-review-graph review

Risk-scored changes

Affected execution flows

Reached only through a caller

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

tirth8205 Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

tirth8205 Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Nov 6, 2026 •

edited

Loading