You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Close the Bootstrap Kept-Tree Removal and Report Gaps - #1794
Closes the remaining gaps from #1770 in the bootstrap loaders' kept-tree handling and in the skills report. #1773 had already closed the issue's first two items, the rollback that could leave no tree and the report's commit label.
Changes
Marker-last removal in bash. An owned tree now loses its contents first and its .bootstrap-owned marker last, as the PowerShell loader already did. A removal that stops part way leaves a tree the next run still recognizes.
Symlinks at managed paths. Bash counts a dangling symlink as present and never treats a symlink as owned. PowerShell's Test-Ownership rejects a reparse point, so a marker-last removal can't empty a link's target.
Swap ordering.
The old tree's name is freed only when a live tree is about to take it, so a locked leftover .old beside an empty name no longer blocks the new tree from landing.
Rollback runs only when this run moved the tree aside, so a failed swap never renames a foreign .old into place.
A failed rollback in bash now says so.
Exit cleanup order. Both loaders restore the old tree before removing the staging tree, and remove staging best-effort and marker-last. The "move it by hand" warning fires only for a kept tree.
--report --intended in a bootstrap tree is refused, since git there would answer for an enclosing repository.
Report message. The loaders' skills-report line now says only that no current copy was reported and points at the output above. Every not-current snapshot now carries a reason, which the stale and dirty paths previously lacked.
The PowerShell functions were run in scratch pwsh probes on Linux (swap, restore, symlink refusal). Nothing in this PR has been run on native Windows, so the locked-file behavior these fixes target is not verified there.
Linters run locally: shellcheck, shfmt, PSScriptAnalyzer, editorconfig-checker and markdownlint all pass, as do ruff, mypy and spec/validate.py.
Seven local strict-review passes ran. Every finding on this change's own text was fixed, and the last pass came back clean.
Follow-ups filed
#1790, #1791 and #1792. #1793 was folded in here, since this change's explicit return status widened it: the final transient-tree removal is now best-effort in both loaders.
Carries three explanatory comments, hence the comments label.
Work through the remaining #1770 gaps in the loaders and the skills report.
- Remove an owned tree contents first and its marker last in bash, as
PowerShell already did, so a partial removal leaves a tree the next run
still owns.
- Treat a symlink at a managed path as present and not ours, so a dangling
one gets the loader's own refusal.
- Free the old tree's name only when the live tree is about to take it, so
a locked leftover beside an empty name no longer blocks the new tree.
- In PowerShell cleanup, restore the old tree before removing the staging
tree, remove it marker-last and best-effort, and warn about a hand move
only for a kept tree.
- Refuse --report --intended in a bootstrap tree, where git would answer
for an enclosing repository.
- Word the loaders' report message for every not-current reason, not just
missing or stale.
The rollback and the report-label gaps were already closed by #1773.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…reign Paths
- Roll the old tree back only when this run moved it aside, so a failed
swap never renames a leftover it did not create into the tree's name.
- Reject a reparse point in PowerShell's ownership check, as bash already
rejects a symlink, so a marker-last removal never empties a link's target.
- Skip the partial-removal test under root, which ignores the read-only
mode it relies on.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bring the bash exit cleanup in line with PowerShell's order: put the old
tree back first, then remove the staging tree best-effort, so a staging tree
that will not go no longer aborts the trap before the restore. A failed
rollback in the swap now says the previous tree could not be put back.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The previous wording pointed at a report reason that a stale install, and
an installer that failed before reporting, never print. State only what
holds for every non-zero exit.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The installer wrappers can exit before any report runs, on a missing Python,
a tree without the installer, or a sudo refusal, so the loader message now
says only that no current copy was reported and defers to the output above.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The stale and dirty paths set current to false with no reason, so the
loaders' "the output above says why" did not hold there. Each not-current
snapshot now names its cause and, where re-installing fixes it, says so.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A stamp with no recorded commit is not known to hold another revision, and
re-running the installer from the same kind of tree writes the same stamp,
so each reason now states only what the stamp records.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ptr727
added
script
A defect in hub tooling
comments
Permits the comment lines the pull request adds or edits, which the prose gate otherwise refuses
labels
Sep 24, 2026
Reviewing files that changed from the base of the PR and between aea350c and ade1608.
📒 Files selected for processing (6)
host-setup/bootstrap.ps1
host-setup/bootstrap.sh
scripts/README.md
scripts/skills_install.py
scripts/tests/test_bootstrap.py
scripts/tests/test_skills_install.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📝 Walkthrough
Walkthrough
Bootstrap scripts change owned-tree removal, swap recovery, and cleanup. Skills reporting now distinguishes dirty checkouts from revision mismatches and rejects intended-revision comparisons for bootstrap trees.
Changes
Bootstrap tree lifecycle
Layer / File(s)
Summary
Ownership checks and marker-preserving removal host-setup/bootstrap.ps1, host-setup/bootstrap.sh
Ownership checks reject reparse-point or symlink roots. Removal of owned trees preserves the ownership marker until the final removal step.
Swap recovery and cleanup host-setup/bootstrap.ps1, host-setup/bootstrap.sh, scripts/tests/test_bootstrap.py
Swap and cleanup paths restore retired trees when needed and warn on specified cleanup failures. Linux tests cover partial removal, dangling symlinks, swap failures, and cleanup restoration.
Report messages distinguish dirty checkouts from revision mismatches. Bootstrap trees reject --intended and continue to use the loader-resolved commit. Tests check the report reasons and rejection behavior.
The bootstrap recovery and skills-report changes appear mergeable after normal checks. Native Windows locked-file behavior has not been validated.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. (2 skipped: …
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title clearly summarizes the main changes: fixing bootstrap kept-tree removal and skills-report gaps.
Full details: Docstring Coverage
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Commit to this branch
Create a new PR
🧪 Generate unit tests (beta)
Commit to this branch
Create a new PR
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The bash loader cleanup currently suppresses failures when removing an owned retired tree in kept-tree mode, which can leave a blocking .old behind with no warning after an interrupted swap.
Get a fresh assessment by requesting another Copilot review.
This PR tightens bootstrap kept-tree lifecycle handling (bash + PowerShell) and improves skills_install.py --report diagnostics so bootstrap-owned trees remain safely recognizable, swaps are safer, and non-current reports consistently explain why.
Changes:
Make kept-tree removal marker-last and harden swap/cleanup ordering (including symlink/reparse-point handling) in both bootstrap loaders.
Refuse skills_install.py --report --intended when running inside a bootstrap-owned (archive) tree, and enrich snapshot reporting with explicit reason strings for not-current outcomes.
Add focused tests for kept-tree behaviors in bootstrap.sh and for --intended refusal + snapshot reason coverage in skills_install.py.
File
Description
scripts/tests/test_skills_install.py
Adds assertions for snapshot reason text and verifies --intended is refused in bootstrap trees.
scripts/tests/test_bootstrap.py
Adds Linux-only tests that source bootstrap.sh functions to validate marker-last removal, swap ordering, and cleanup restore behavior.
scripts/skills_install.py
Adds consistent non-current reason reporting and rejects --intended in bootstrap-owned trees.
scripts/README.md
Updates documentation to reflect --intended refusal in bootstrap trees.
The exit cleanup discarded a failed removal of the old tree, so a kept-tree
run could leave a leftover that blocks the next run with no warning. It now
warns for a kept tree, as the swap itself does.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A bare return inside a function the EXIT trap calls returns the status the
script exited with, so on a successful run a failed removal read as success
and cleanup's warning never fired. Return find's status explicitly, test
through a real exit trap, and have the bash warnings say to remove the
leftover by hand, since on Linux nothing clears it on its own.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A transient tree is loaded by nothing, so failing to remove it now warns
rather than failing a run whose work succeeded. The previous commit's
explicit return status made that failure reach the exit code on more
paths, so both loaders take the fix here.
Fixes#1793.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
rm -rf invocations lack -- for hyphen-prefixed paths
host-setup/bootstrap.sh:147
remove_tree() calls rm -rf without -- (both in the find -exec and the final directory removal). If an extracted entry or the tree path itself begins with -, rm can treat it as an option, causing the cleanup to fail or behave unexpectedly. Add -- to ensure paths are always treated as operands.
A relative --dir beginning with a hyphen reached find, rm, and mkdir as an
option. Prefixing a relative directory with ./ once, where it is resolved,
keeps every path this loader builds an operand.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rm -rf invocations lack -- for hyphen-prefixed paths (host-setup/bootstrap.sh:147): If an extracted entry or the tree path itself begins with -, rm can treat it as an option.
Fixed in eb0f000, at the root rather than per command. An extracted entry reaches rm as <tree>/<name>, so only the tree path itself could lead with a hyphen. parse_args already refuses a relative --dir. The one remaining route is a relative XDG_DATA_HOME or XDG_CACHE_HOME feeding default_dir. main() now prefixes a relative DIR with ./ where it is resolved, which keeps every path built from it an operand for find, rm, mkdir, mv, tar -C, and curl -o alike. The PowerShell loader has no such case, since Resolve-Directory only returns a rooted path.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
bootstrap.sh cleanup can delete an owned .old tree when the live tree path is a symlink, risking loss of the recoverable previous tree.
Review effort: Lite Findings: None
Previously missed (1)
In code that hasn't changed since last review
Cleanup trap deletes retired tree when live path is a symlink
host-setup/bootstrap.sh:223
cleanup(): When the live tree path is a symlink, exists "$(tree_path)" is true, so the exit trap will delete an owned $(retired_path) instead of restoring it or leaving it for manual recovery. This can lose the previous owned tree on a run that aborts because it refuses a symlinked tree path.
Exit cleanup removed an owned old tree whenever anything sat at the tree's
name, including a symlink this loader refused to replace, which could
discard the last owned copy. It now removes the old tree only beside a live
tree it owns, restores it to an empty name, and otherwise leaves it.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Cleanup trap deletes retired tree when live path is a symlink (host-setup/bootstrap.sh:223): When the live tree path is a symlink, exists "$(tree_path)" is true, so the exit trap will delete an owned $(retired_path) instead of restoring it or leaving it for manual recovery.
Fixed in 0724a06, in both loaders. Exit cleanup now removes the old tree only beside a live tree this loader owns. It restores the old tree to an empty name and leaves it in place when the name holds anything else: a foreign directory, a symlink, or a dangling one. A test pins the symlink case.
…udit to Main (#1796)
Promotes `develop` to `main`.
## Carried
- #1794: closes the bootstrap loaders' kept-tree removal and report gaps
from #1770, and makes the final transient-tree removal best-effort
(#1793).
- #1789: grades HomeAutomation-Config operational in its third audit
run. It came from the HomeAutomation-Config session.
Closes#1770Closes#1793
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved setup and cleanup so existing owned installations can be
restored if an update fails. Cleanup now preserves unowned trees and
warns about leftovers rather than failing the entire run.
* Clarified installation reports when a snapshot comes from a different
revision or a dirty checkout.
* `--intended` is rejected for bootstrap-installed trees; those trees
are checked against the revision resolved by their loader.
* **Documentation**
* Clarified when `--intended` is available.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
commentsPermits the comment lines the pull request adds or edits, which the prose gate otherwise refusesscriptA defect in hub tooling
2 participants
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.
ptr727
added
script
Closes the remaining gaps from #1770 in the bootstrap loaders' kept-tree handling and in the skills report. #1773 had already closed the issue's first two items, the rollback that could leave no tree and the report's commit label.
Changes
.bootstrap-ownedmarker last, as the PowerShell loader already did. A removal that stops part way leaves a tree the next run still recognizes.Test-Ownershiprejects a reparse point, so a marker-last removal can't empty a link's target..oldbeside an empty name no longer blocks the new tree from landing..oldinto place.--report --intendedin a bootstrap tree is refused, since git there would answer for an enclosing repository.reason, which the stale and dirty paths previously lacked.Verification
scripts/tests/test_bootstrap.pydrives the bash loader's own functions. Each new test was checked against the pre-fix loader and fails there, except the cleanup-restore pin, which covers behavior Keep the Bootstrap's Skills Tree So the Claude Code Plugin Outlives the Run #1773 already had.pwshprobes on Linux (swap, restore, symlink refusal). Nothing in this PR has been run on native Windows, so the locked-file behavior these fixes target is not verified there.spec/validate.py.Follow-ups filed
#1790, #1791 and #1792. #1793 was folded in here, since this change's explicit return status widened it: the final transient-tree removal is now best-effort in both loaders.
Carries three explanatory comments, hence the
commentslabel.🤖 Generated with Claude Code