Repository navigation
fix(install): show each change in --dry-run and write nothing before the question - #1173
Merged
Merged
Conversation
…the question (#173) install --dry-run listed file names only, and a real install wrote the MCP config, skills and hooks before it asked about CLAUDE.md and the other instruction files. Every install read and write now goes through install_plan. Inside a plan a write is held in memory, later reads see it, and nothing reaches the disk until Plan.apply. --dry-run prints the plan as one unified diff per file (a new file as its content, cut at 60 lines with a note) and writes nothing. A real install lists the files it will change, asks its question, and then writes exactly that plan. Ctrl-C at the question writes nothing, a step that fails while planning writes nothing, and a file edited while the question was open is left alone and reported. JSONC configs go through the same splice in both modes, so comments are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
code-review-graph reviewOverall risk: 0.65 (MEDIUM) — 131 changed function(s)/class(es), 32 affected flow(s), 59 test gap(s) (11 with no tested caller found, 48 reached only through a caller) Risk-scored changes
Affected execution flows
Test gaps
Reached only through a callerNo 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.
Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Incomplete conflict protection can recreate instruction files deleted during confirmation and silently re-enable hooks whose execute permissions the user removed.
5 open issues
What changed in this PR
This PR partially addresses #173 by adding an in-memory installation plan, making installer changes visible before they reach disk.
Changes:
- Adds per-file diff previews for
install --dry-run. - Defers installation writes until after the instruction-file confirmation, with cancellation and conflict handling.
- Adds cross-platform preview tests and updates installation documentation.
| File | Description |
|---|---|
| tests/test_install_preview.py | Tests previews, deferred writes, cancellation, and conflicts. |
| README.md | Explains preview and confirmation behavior. |
| docs/USAGE.md | Documents installation options and write timing. |
| docs/COMMANDS.md | Clarifies the dry-run command description. |
| code_review_graph/skills.py | Routes configuration, skill, hook, and instruction changes through plans. |
| code_review_graph/install_plan.py | Adds buffered file operations, application, and diff rendering. |
| code_review_graph/incremental.py | Makes .gitignore updates plan-aware. |
| code_review_graph/codex_skill.py | Makes Codex skill installation operations plan-aware. |
| code_review_graph/cli.py | Coordinates planning, previews, confirmation, and application. |
| CLAUDE.md | Documents the installation-plan module. |
| CHANGELOG.md | Records the new installation behavior. |
Review effort: Balanced
| raise SystemExit(130) from None | ||
| with contextlib.redirect_stdout(report): | ||
| if inject: | ||
| outcomes = inject_instruction_files(repo_root, target=target) |
Owner
Author
There was a problem hiding this comment.
Fixed in 66a46de: instruction files are now planned before the question and dropped if the answer is no, so an edit made while it is open is left alone and reported.
| failures: list[tuple[Path, str]] = [] | ||
| for change in self.changes(): | ||
| try: | ||
| if _disk_state(change.path)[0] != change.before: |
Owner
Author
There was a problem hiding this comment.
Fixed in 66a46de: apply now checks the permission bits as well as the bytes, so a hook turned off while the question is open stays off.
Comment on lines
+321
to
+324
| if not skip_instructions: | ||
| _print_instruction_conflicts( | ||
| inject_instruction_files(repo_root, target=target) | ||
| ) |
Owner
Author
There was a problem hiding this comment.
Fixed in 66a46de: both modes now build the same instruction plan, the old Copilot file is listed before the question, and the real install removes it as the dry run showed.
| failures = plan.apply() | ||
|
|
||
| print() | ||
| sys.stdout.write(report.getvalue()) |
Owner
Author
There was a problem hiding this comment.
Fixed in 66a46de: when a write is left alone the progress messages are dropped and only the files actually written are named.
|
|
||
|
|
||
| def _key(path: PathLike) -> Path: | ||
| return Path(os.path.abspath(os.fspath(path))) |
Owner
Author
There was a problem hiding this comment.
Fixed in 66a46de: the plan keys each file by its real path, so CLAUDE.md linked to AGENTS.md is planned and written once; removing a symlink still removes only the link.
…des and symlinks Copilot review on #1173: - instruction files are planned before the question, so an edit made while it is open is left alone, and the dry run and the real install build the same plan, old Copilot instruction file included - apply also compares permission bits, so a hook turned off while the question is open is not turned back on - when a write is left alone, the progress messages are not replayed; only the files written are named - the plan keys each file by its real path, so CLAUDE.md linked to AGENTS.md is written once; removing a symlink still removes the link Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moving the hook chmod calls to install_plan.chmod(path, 0o755) put the mode in the place bandit B103 checks, so the security job failed. A hook has to be executable, so each call carries a nosec with that reason. 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>
Brings in the two CI test fixes on train/2026-11-09 (30c9427). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Nov 9, 2026
jay23606
pushed a commit
to jay23606/code-review-graph
that referenced
this pull request
Nov 9, 2026
Each claim in docs/, the get_docs_section_tool reference, CONTRIBUTING, SECURITY, AGENTS.md, the Copilot instructions, the skills and the VS Code README was checked against main.py, cli.py --help, PLATFORMS, migrations.py, action.yml and the parsers, and fixed where it was wrong or missing after the 2026-11-09 train and tirth8205#1173. The two changed skills are edited in skills.py and regenerated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #173.
What was wrong
install --dry-runlisted file names only, never what would be written..gitignore, skills and hooks before it asked aboutCLAUDE.mdand the other instruction files.What this does
Every install read and write now goes through a new module,
install_plan.py. Inside a plan a write is held in memory, later reads in the same run see it (Gemini's.gemini/settings.jsongets the MCP entry and then the hooks), and nothing reaches the disk untilPlan.apply(). Outside a plan the helpers write straight to disk, so direct callers and existing tests behave as before.install --dry-runprints one unified diff per file, for every writer of every platform inPLATFORMS: MCP configs (JSON, JSONC, TOML, YAML),.gitignore, Claude/Gemini/CodeBuddy/Hermes/Qoder/Codex skills, Claude/Qoder/CodeBuddy/Codex/Cursor/Gemini hooks, git hooks, the OpenCode plugin, the instruction files, the--steeringhook and the--gitignoreblock. A new file shows as its content. Each file is cut at 60 lines with... N more lines not shown. It writes nothing.Redone on top of the train (#1186)
The train's install writers now all go through the plan too:
_read_settings_for_mergereads through the plan and_write_merged_settingswrites the splice (as bytes, so CRLF survives), the plain JSON and the.bakcopy through it. So Claude, Qoder, CodeBuddy, Codex, Cursor and Gemini hook settings keep their comments in the dry run and the real write alike. A settings file that install will leave alone ("left unchanged to avoid data loss") is now named in the dry run as well; before, the dry run dropped those messages.--steeringmerges into.claude/settings.jsoninside the plan, after the other hooks, so the diff shows both.--no-steeringremoves it through the same plan:uninstall._remove_hooksreads and writes throughinstall_planwhen a plan is active, so it sees the hooks this run planned a moment before rather than the old file..gitignoreblock.install_gitignore.applyreads.gitignore, checks which paths exist and writes the block through the plan, so the block is worked out from the files the plan creates, the same way in both modes. It is planned after the instruction files. If you say no at the question, the block is planned again without them, so it never lists aCLAUDE.mdthat was not written.Proof
Throwaway HOME and throwaway git repo per platform, lived in: a
CLAUDE.md, a.gitignore, a commentedopencode.jsonc, a commented.claude/settings.json, a commented.gemini/settings.json, a commented~/.cursor/hooks.jsonand a~/.codex/config.tomlholding another server. For each of the 16 platforms inPLATFORMS, andall: hash the whole tree, runcode-review-graph install --dry-run --platform X, hash again; then run the real install and compare what it wrote with the dry run, both as rendered diffs and as the full bytes and modes of every file (the plan is read back from a second dry run in-process).Plain install:
With
--steering --gitignore:Nothing written before the question
install --platform X --steering --gitignorerun through a real pseudo terminal; the tree is hashed while the process waits at[Y/n], thennis typed.On the train (
train/2026-11-09, 30c9427):On this branch:
Dry run on the train and on this branch
install --dry-run --platform claude --steering --gitignoreon the lived-in tree. On the train it lists names only:On this branch (excerpts; 13 files). The commented hook settings keep their comment, and the steering hook sits after the other hooks:
A real install prints, before the question:
Tests
tests/test_install_preview.py(111 tests). For every platform, a dry run leaves the tree byte for byte unchanged and shows a diff for each planned file; a real install writes exactly the dry-run plan, fresh and lived in (commented hook settings, hooks with a mode change, TOML, YAML, JSONC), plain and with--steering --gitignore; nothing is written before the question (claudeandall, plain and with the opt-ins); Ctrl-C and a failing step write nothing; JSONC comments are kept; long files are cut with a note; a file edited after the preview is left alone.--no-steeringremoves what the same run planned; commented hook settings and their backup stay in the plan; a hook settings file install will leave alone is named in the dry run; the--gitignoreblock in a dry run matches the real write; saying no keepsCLAUDE.mdout of the block. With the train writers left writing straight to disk, 41 of these tests fail.uv run ruff check code_review_graph/: clean.uv run --extra dev python -m mypy code_review_graph/ --ignore-missing-imports --no-strict-optional: no issues in 92 files.uv run pytest tests/test_platform_lifecycle.py -m platform_lifecycle: 124 passed, 17 xfailed (as before).uv run pytest tests/ --tb=short -q: 7842 passed, 919 skipped, 3 xfailed, 3 xpassed, 6 failed. None of the six is from this branch: threetest_token_budget.pycases fail only because of the long temp path I ran under (144 passed with a short one), and threetest_http_multi_repo.pycases fail the same way on the train itself at 30c9427 with the localmcp2.3.0 (float + timedeltain the client).Left for a maintainer decision
The question still covers the instruction files only, as before: saying no skips them, and the MCP config, skills and hooks are still written after the answer. If a real install should instead ask one yes/no for every file (or print the diffs inline before asking), that is a change to what
installdoes by default, so I left it out. That is why this says "Part of #173".🤖 Generated with Claude Code