Skill --force install rolls back after leftover backup delete fails #3733

Activity

1. SebTardif commented on Sep 16, 2026

   

   SebTardif

   on Sep 16, 2026

   ContributorAuthor

   More actions

   Opened PR #3734 for this tracker (Ref only).
2. clawsweeper

   added

   P2Normal backlog priority with limited blast radius.Normal backlog priority with limited blast radius.

   clawsweeper:bulk-filedClawSweeper detected a high recent issue-filing volume from this author.ClawSweeper detected a high recent issue-filing volume from this author.

   clawsweeper:no-new-fix-prClawSweeper does not recommend queueing a new automated fix PR for this issue.ClawSweeper does not recommend queueing a new automated fix PR for this issue.

   clawsweeper:source-reproClawSweeper found a high-confidence source-level issue reproduction.ClawSweeper found a high-confidence source-level issue reproduction.

   impact:ux-frictionUser-facing flow adds avoidable confusion or support burden without fully blocking progress.User-facing flow adds avoidable confusion or support burden without fully blocking progress.

   issue-rating: 🦞 diamond lobsterVery strong issue quality with high-confidence source-level or clear reproduction.Very strong issue quality with high-confidence source-level or clear reproduction.

   on Sep 16, 2026
3. clawsweeper commented on Sep 16, 2026

   

   clawsweeperBot

   on Sep 16, 2026 – with ClawSweeper

   Last edited by clawsweeper

   Contributor

   More actions

   Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Reviewed September 20, 2026, 10:36 AM ET / 14:36 UTC.

   Summary
   The defect remains on current main and in v0.23.3. The reporter’s open fix PR already covers this repair, so the issue remains necessary.

   Reproducibility: yes. from source: make backup deletion reject after both renames succeed, and the catch removes the replacement and restores the old tree. No runtime reproduction was executed.

   Next step
   Review the existing candidate at #3734; the paired PR and bulk-filer signal exclude another automated repair dispatch.

   Review details

   Best possible solution:

   Treat successful replacement as the rollback boundary, make subsequent backup cleanup best-effort, and preserve recovery for failures before replacement completes.

   Do we have a high-confidence way to reproduce the issue?

   Yes, from source: make backup deletion reject after both renames succeed, and the catch removes the replacement and restores the old tree. No runtime reproduction was executed.

   Is this the best way to solve the issue?

   Yes, separating post-swap cleanup from rollback is the narrowest repair in the shared helper; filesystem retries alone would still allow eventual cleanup failure to undo success.

   AGENTS.md: found and applied where relevant.

   Remaining risk / open question:

   • The reported Windows antivirus or file-lock trigger was not exercised in this read-only review.

   Codex review notes: model internal, reasoning medium; reviewed against ff6c118c132a.

   Label changes

   Label justifications:

   • P2: A backup cleanup error can undo a completed skill replacement; the demonstrated scope is an existing-install CLI workflow.
   • impact:ux-friction: Users receive an installation error and retain the old version after the replacement itself succeeded.

   Evidence reviewed

   What I checked:

   • Current source establishes the defect: After the staged directory becomes the target, backup deletion remains inside the swap try block. A deletion rejection enters a catch that removes the new target and attempts to restore the backup, despite stageMoved being true. (packages/clawhub/src/cli/commands/skills.ts:1848, ff6c118c132a)
   • Both public command paths reach the helper: Install invokes the helper before writing origin and lock metadata; update does likewise. Existing tests cover successful staging and replacement, while docs describe overwriting installed skills. (packages/clawhub/src/cli/commands/skills.ts:1122, ff6c118c132a)
   • Published release still contains the failure path: The v0.23.3 source retrieved through GitHub contains the same backup deletion inside the rollback try block. GitHub identifies v0.23.3 as the latest release, published August 4, 2026. (packages/clawhub/src/cli/commands/skills.ts, 87ca030c30f3cfb78ab15c8e66b5ff1469c8f9c8f9c8)
   • Existing repair candidate: The reporter linked fix: skill --force install rolls back when leftover backup delete fails #3734. A live REST check confirms it remains open and unmerged; its patch moves backup cleanup outside rollback handling and adds a cleanup-failure regression test. The supplied discussion also includes a macOS filesystem fault-injection transcript. (0c198ebed809)
   • History and routing: The recorded commit and GitHub patch for the rollback-safe update work provide relevant history. Recent file history also includes Patrick Erichsen’s install-path work. Local blame could not complete because a historical blob was unavailable; GitHub REST supplied the historical patch and release source. (packages/clawhub/src/cli/commands/skills.ts, 7d69b337c7a7)

   Likely related people:

   • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
   • Patrick Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

   How this review workflow works

   • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
   • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
   • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
   • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
   • Maintainers can also comment @clawsweeper review to request a fresh review only.
   • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
   • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
   • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
4. steipete

   closed this as completedin d26329bon Sep 30, 2026
5. steipete commented on Sep 30, 2026

   

   steipete

   on Sep 30, 2026

   Contributor

   More actions

   Fixed on main by #3853 (d26329b), completing @SebTardif's #3734 with contributor credit preserved. Successful replacement is now the rollback boundary; failure to remove the old backup leaves the new files and matching lockfile installed.

   The Linux regression runs the built Node CLI against a synthetic loopback registry with real ZIP extraction and filesystem operations, injecting EBUSY only for backup removal. It failed on unchanged main; afterward all 22 artifact tests pass, including new content, lockfile version, and retained backup assertions. Static, full coverage (7,250 passed, 3 skipped), package, type/build, and HTTP gates passed, as did exact-head CI.

   A CLI release containing this commit is still required.
6. pull

   added a commit that references this issue on Sep 30, 2026

   fix: keep completed skill installs when backup cleanup fails (opencla…

   ...

   This issue will close once commit d26329b is merged into the 'main' branch.

   d26329b
7. github-actions

   locked as resolved and limited conversation to collaborators on Oct 3, 2026