Repository navigation
fix: keep completed skill installs when backup cleanup fails - #3853
Conversation
Keep backup deletion outside the swap rollback boundary. Verify the built CLI with real filesystem operations and an injected EBUSY, preserving the replacement and matching lockfile. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 30, 2026, 2:18 AM ET / 06:18 UTC. ClawSweeper reviewWhat this changesThe ClawHub CLI keeps a completed skill install or update when old-backup deletion fails, with CLI documentation, a changelog entry, and unit and built-CLI regressions. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 5 items remain Current main still rolls back a completed skill replacement when backup deletion fails. This PR fixes that boundary, but a retained backup is discoverable by Priority: P2 Review scores
Verification
How this fits togetherThe ClawHub CLI downloads a skill into a work directory, replaces an installed copy, and records its origin and version in a lockfile. The sync command scans that same skills directory and can publish folders it recognizes as skills. flowchart LR
A[Registry skill ZIP] --> B[CLI install or update]
B --> C[Staged replacement and backup]
C --> D{Replacement complete?}
D -->|No| E[Restore old skill]
D -->|Yes| F[Installed skill and lockfile]
D -->|Cleanup fails| G[Retained old backup]
G --> H[Sync discovery and publish]
Before merge
Findings
Agent review detailsSecurityNeeds attention: The retained backup can become an unintended authenticated sync upload; the diff adds no dependency or CI supply-chain change. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep the successful replacement and exclude installer-created backup directories from skill discovery; record the rollback boundary in a focused spec note. Do we have a high-confidence way to reproduce the issue? Yes. On current main, make backup deletion fail after both renames: the catch removes the replacement and restores the old tree; the PR's built-CLI test encodes that fault path. Is this the best way to solve the issue? Not yet. Moving cleanup outside rollback is the right repair for install and update, but retained backups must also be excluded from sync discovery; retrying deletion alone would not protect that path. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6520846333a8. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
Acceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Landed in d26329b, preserving Sebastien Tardif's Co-authored-by credit and Unreleased thanks. #3733 is closed and #3734 is closed as superseded; the contributor branch remains intact. Before/after proof ran the built CLI with real ZIP/filesystem operations against a synthetic loopback registry and child-only EBUSY injection. The new artifact regression failed before the fix (21 others passed); afterward all 22 artifact tests pass and verify the new content, matching lockfile, and retained old backup. The static, coverage (7,250 passed / 3 skipped), packages (461 CLI source tests), types/build, and HTTP gates passed on Blacksmith Testbox Exact PR-head CI and post-merge main CI passed on their first attempts. No CLI release was published. |
A completed
clawhub install --forceor update could restore the old skill merely because deleting its backup failed withEBUSYorEPERM. Move backup cleanup outside the successful replacement's rollback boundary. Earlier swap failures still roll back; a completed replacement keeps its new files and matching lockfile even if the old backup remains.Completes and supersedes #3734 on a maintainer-owned branch after its changelog conflicted with current main. Preserves @SebTardif's fix and regression coverage, the built-CLI filesystem regression, CLI documentation, and Unreleased credit. The contributor's branch is unchanged.
Fixes #3733.
The before/after artifact test runs the real built Node CLI against a synthetic loopback registry, extracts a ZIP into real directories, and injects only backup-removal
EBUSYin the CLI child. Before the fix, that test failed with exit 1 while the other 21 artifact tests passed. Afterward, all 22 pass: new content remains, the lockfile records version 1.0.0, and the previous backup is retained. This is filesystem fault-injection proof, not a Windows antivirus test.Validation passed through Crabbox on Blacksmith Testbox
tbx_01m3re6te4c7mneqajsnq7007m(backend run):bun run ci:static,bun run ci:unit(7,250 passed, 3 skipped),bun run ci:packages(461 CLI source tests and 22 artifact tests),bun run ci:types-build, andbun run ci:e2e-http. Independent Codex autoreview is scoped-clean through P2.AI-assisted completion; the code, regression and runtime proof have been reviewed. No release or project version bump.