fix(installer): take ownership of minimal shell init marker - #729
Conversation
📝 WalkthroughWalkthroughThe installer now replaces stale shell rc marker blocks with the current init sourcing line, preserves malformed rc files unchanged, continues uninstall processing non-fatally, and verifies these behaviors through updated specifications and Zsh integration tests. ChangesRC marker handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.sh (1)
602-617: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep stale-block replacement non-mutating in dry-run mode.
When
dry_run=1,strip_rc_block()returns success after printing “would remove”; this code then proceeds to append the new block because the write path has no dry-run guard. A dry run can therefore modify the rc file and leave both old and new blocks. Branch before the append (and preserve the intended “would” status output).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install.sh` around lines 602 - 617, Guard the append path in the stale-block replacement flow with dry_run, so strip_rc_block’s successful “would remove” result cannot write to the rc file when dry_run=1. Preserve the existing non-dry-run mkdir/printf behavior and the intended dry-run status output.
🧹 Nitpick comments (1)
scripts/install_test.sh (1)
437-447: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact block and file identity on rerun.
The test currently proves idempotence only through log output and an opening-marker count. Capture the rc file before
oldblock2and compare it afterward; also assert exactly one opening and closing marker plus the current hook line inside that block. Add a case where the current hook exists outside the stale block to catch whole-file matching regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install_test.sh` around lines 437 - 447, Strengthen the R9.2 rerun assertions around oldblock2 by capturing H14/.zshrc before and after execution and requiring the files to match exactly. Assert exactly one opening and closing minimal-block marker and verify the current shell-init/zsh.sh hook appears inside that block; also add a fixture where the current hook is outside the stale block to ensure replacement targets only the stale block rather than matching the whole file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.sh`:
- Around line 602-605: Update strip_rc_block to detect any start marker without
a matching end marker and fail before modifying the rc file; ensure the existing
subshell invocation leaves the original file intact when validation fails, while
preserving normal replacement for properly terminated blocks.
- Around line 598-600: Update the current-hook check in the install script to
parse the exact “>>> minimal >>>” and matching end-marker block instead of
scanning the entire rc file. Treat the file as configured only when exactly one
fenced block exists and that block contains the hook path in $2; otherwise
continue replacing the stale block.
---
Outside diff comments:
In `@scripts/install.sh`:
- Around line 602-617: Guard the append path in the stale-block replacement flow
with dry_run, so strip_rc_block’s successful “would remove” result cannot write
to the rc file when dry_run=1. Preserve the existing non-dry-run mkdir/printf
behavior and the intended dry-run status output.
---
Nitpick comments:
In `@scripts/install_test.sh`:
- Around line 437-447: Strengthen the R9.2 rerun assertions around oldblock2 by
capturing H14/.zshrc before and after execution and requiring the files to match
exactly. Assert exactly one opening and closing minimal-block marker and verify
the current shell-init/zsh.sh hook appears inside that block; also add a fixture
where the current hook is outside the stale block to ensure replacement targets
only the stale block rather than matching the whole file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c10350fa-a38e-46ba-879f-98d82f5ea343
📒 Files selected for processing (3)
docs/specs/07-spec-installer/07-spec-installer.mdscripts/install.shscripts/install_test.sh
76d579e to
bb393bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.sh`:
- Around line 187-190: Update the awk validation in the install script’s
marker-checking block to reject malformed sequences before rewriting: exit with
status 2 when a start marker appears while a block is already open or an end
marker appears while closed. Preserve the existing unterminated-block check and
warning behavior, and ensure valid single start/end pairs continue to pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d41953e-b4ff-4f1b-bf5b-f9682460e72a
📒 Files selected for processing (3)
docs/specs/07-spec-installer/07-spec-installer.mdscripts/install.shscripts/install_test.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/specs/07-spec-installer/07-spec-installer.md
| if ! awk -v s="$marker_start" -v e="$marker_end" \ | ||
| '$0==s {open=1} $0==e {open=0} END {exit open}' "$1"; then | ||
| say " warning: unterminated minimal block in $1, left untouched" | ||
| return 2 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject nested or unmatched marker sequences before rewriting.
Line 188 only checks whether the last marker leaves the block open. A start → user content → start → end sequence passes validation, then Line 198 removes everything from the first start through the end—including user content. Track a start while already open and an end while closed as malformed, then return 2.
Proposed fix
- if ! awk -v s="$marker_start" -v e="$marker_end" \
- '$0==s {open=1} $0==e {open=0} END {exit open}' "$1"; then
+ if ! awk -v s="$marker_start" -v e="$marker_end" '
+ $0 == s {
+ if (open) malformed = 1
+ open = 1
+ next
+ }
+ $0 == e {
+ if (!open) malformed = 1
+ else open = 0
+ }
+ END { exit (malformed || open) }
+ ' "$1"; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! awk -v s="$marker_start" -v e="$marker_end" \ | |
| '$0==s {open=1} $0==e {open=0} END {exit open}' "$1"; then | |
| say " warning: unterminated minimal block in $1, left untouched" | |
| return 2 | |
| if ! awk -v s="$marker_start" -v e="$marker_end" ' | |
| $0 == s { | |
| if (open) malformed = 1 | |
| open = 1 | |
| next | |
| } | |
| $0 == e { | |
| if (!open) malformed = 1 | |
| else open = 0 | |
| } | |
| END { exit (malformed || open) } | |
| ' "$1"; then | |
| say " warning: unterminated minimal block in $1, left untouched" | |
| return 2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 187 - 190, Update the awk validation in the
install script’s marker-checking block to reject malformed sequences before
rewriting: exit with status 2 when a start marker appears while a block is
already open or an end marker appears while closed. Preserve the existing
unterminated-block check and warning behavior, and ensure valid single start/end
pairs continue to pass.
If you had installed legacy minimal, the installer would see the existing
>>> minimal >>>block and think you already had wiring installed.We now take ownership of blocks of this name, so the installer rewrites it for the new install. This also allows us to tweak shell sections in the future.
Summary by CodeRabbit