[SebTardif]

A completed skill replacement could be rolled back merely because deleting its old backup failed with EBUSY or EPERM. Backup removal now happens after the swap’s rollback boundary, so the new files remain installed and the caller records the matching version in the lockfile. Failures before the replacement completes retain the existing rollback behavior.

Fixes #3733. Preserves @SebTardif’s fix and regression, with a built-CLI filesystem regression, CLI documentation, and an Unreleased entry.

Validation uses the repository’s Blacksmith Testbox backend through one Crabbox lease. The new artifact test runs the real built CLI against a local synthetic HTTP registry, extracts a ZIP into real directories, and injects only backup-removal EBUSY in the CLI child process. It failed on unchanged main and passes with this fix, verifying new content, the lockfile version, and the retained old backup. It does not claim a real Windows antivirus-lock test.

Independent Codex autoreview: scoped-clean through P2. AI-assisted completion; the implementation and tests have been reviewed.

Remote gates passed: bun run ci:static, bun run ci:unit (7,170 passed; 3 skipped), bun run ci:packages (including schema build, CLI typecheck, and built artifact tests), and bun run ci:e2e-http. Provider: Blacksmith Testbox; lease tbx_01m33nwh6z4rcm0er9wakvcwkd; backend run.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[vercel]

@SebTardif is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed September 29, 2026, 5:36 AM ET / 09:36 UTC (Revision 4).

ClawSweeper review

What this changes

The ClawHub CLI keeps a completed skill replacement installed when deleting its old backup fails, with unit and built-CLI regressions, CLI documentation, and a changelog entry.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

✅ Ready for maintainer review

The defect remains on current main, and this PR is the linked repair for the open issue. The changed rollback boundary matches the reported failure, the built-CLI regression exercises the completed replacement with a real filesystem and injected cleanup error, and I found no blocking patch defect.

Priority: P2
Reviewed head: 52a74c957813038bd1a8f0a8696afe5de004130a

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A small, coherent repair has focused unit and built-CLI fault coverage with reported before-and-after behavior.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The changed CLI helper is exercised through the built CLI against a local registry and real directories with backup-removal EBUSY injected; the reported after-fix result retains the new skill, records version 1.0.0, and leaves the old backup. The test and reported before-and-after run support this fault scenario, though they do not claim a Windows antivirus run. No stored-data format changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The changed CLI helper is exercised through the built CLI against a local registry and real directories with backup-removal EBUSY injected; the reported after-fix result retains the new skill, records version 1.0.0, and leaves the old backup. The test and reported before-and-after run support this fault scenario, though they do not claim a Windows antivirus run. No stored-data format changes.
Evidence reviewed	7 items	Current-main failure path: On current main, backup removal is inside the swap try block; a cleanup error enters the catch and removes the newly installed target. Proposed rollback boundary: The PR restores the backup only before the staged directory becomes the target and makes later backup removal best-effort. Production-path regression coverage: The added artifact test launches the built CLI against a local registry, injects EBUSY only for backup removal, and checks the new files, lockfile version, and retained old backup. The PR body reports a failing unchanged-main run and a passing fixed run; current packages checks are reported successful.
Findings	None	None.
Security	None	None.

How this fits together

The ClawHub CLI downloads a skill into a staging directory, replaces the installed files, then records the installed version. The changed helper decides whether a failure should restore the old files or leave the completed replacement in place.

flowchart LR
  A[Install or update command] --> B[Download skill into staging]
  B --> C[Move old skill to backup]
  C --> D[Move new skill into place]
  D --> E[Remove old backup]
  D --> F[Write origin and lockfile]
  C --> G[Restore old skill on swap failure]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch size	+101/-3 across 5 files	The production change is small and has focused regression coverage.
Production and test lines	production +3/-2; tests +94/-1	The production growth is justified by moving cleanup outside the rollback boundary.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #3733
Summary: This PR is the open candidate fix for the linked skill replacement rollback issue; the browser-fixture CI work is separate.

Members:

• canonical: Skill --force install rolls back after leftover backup delete fails #3733 - It reports the exact completed-swap backup-cleanup failure addressed by this PR.
• adjacent_distinct: ci: use a seeded fixture for skill hero layout checks #3795 - It concerns a seeded browser-test fixture, not CLI replacement rollback.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Land the shared-helper repair after ordinary review so completed installs survive backup cleanup errors while failures before replacement still restore the previous skill.

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

Yes. Current-main source shows that making backup deletion reject after both renames sends a completed replacement into rollback; the contributor also reports a before-and-after built-CLI run.

Is this the best way to solve the issue?

Yes. Moving backup cleanup beyond the rollback boundary is the narrowest fix in the helper shared by install and update; retrying deletion alone would still permit a later cleanup failure to undo the replacement.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

No label changes.

Label justifications:

• P2: A backup cleanup error can undo a completed CLI skill replacement, with a limited install and update scope.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed CLI helper is exercised through the built CLI against a local registry and real directories with backup-removal EBUSY injected; the reported after-fix result retains the new skill, records version 1.0.0, and leaves the old backup. The test and reported before-and-after run support this fault scenario, though they do not claim a Windows antivirus run. No stored-data format changes.
• proof: sufficient: Contributor real behavior proof is sufficient. The changed CLI helper is exercised through the built CLI against a local registry and real directories with backup-removal EBUSY injected; the reported after-fix result retains the new skill, records version 1.0.0, and leaves the old backup. The test and reported before-and-after run support this fault scenario, though they do not claim a Windows antivirus run. No stored-data format changes.

Evidence

What I checked:

• Current-main failure path: On current main, backup removal is inside the swap try block; a cleanup error enters the catch and removes the newly installed target. (packages/clawhub/src/cli/commands/skills.ts:1848, 71681ad03bc3)
• Proposed rollback boundary: The PR restores the backup only before the staged directory becomes the target and makes later backup removal best-effort. (packages/clawhub/src/cli/commands/skills.ts:1849, 52a74c957813)
• Production-path regression coverage: The added artifact test launches the built CLI against a local registry, injects EBUSY only for backup removal, and checks the new files, lockfile version, and retained old backup. The PR body reports a failing unchanged-main run and a passing fixed run; current packages checks are reported successful. (packages/clawhub/test-artifact/cli.artifact.test.ts:671, 52a74c957813)
• Existing feature provenance: The recorded patch for commit 7d69b33 introduced the shared staging and backup helper used by install and update; its recorded author and committer are vincentkoc. Local historical blame could not complete because the partial checkout could not fetch a required blob, so no source-line introduction pointer is asserted. (packages/clawhub/src/cli/commands/skills.ts, 7d69b337c7a7)
• Previous review continuity: The changed CLI source and regression tests are identical to the earlier reviewed head; that completed review listed no findings. (packages/clawhub/src/cli/commands/skills.ts, 52a74c957813)
• Linked issue and separate CI work: The linked issue describes this same backup-cleanup rollback. The discussion identifies a separate browser-fixture CI problem tracked by ci: use a seeded fixture for skill hero layout checks #3795; that PR remains open and is not a replacement for this CLI fix.

Likely related people:

• vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• 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.

History

Review history (3 earlier review cycles)

• reviewed 2026-09-16T05:17:27.642Z sha 0c198eb :: needs real behavior proof before merge. :: none
• reviewed 2026-09-22T04:55:41.593Z sha 37d3485 :: needs maintainer review before merge. :: none
• reviewed 2026-09-22T05:15:52.674Z sha 37d3485 :: needs maintainer review before merge. :: none

[steipete]

The updated head 37d3485cc646a029305f7b541cf0dae92e2d2c53 passes static, unit, packages, types/build and HTTP e2e. Its public browser smoke failure is the same pre-existing missing skills.sh fixture failure seen on main, confirmed in run 35688489751.

#3795 fixes that CI root cause by moving the existing layout regression to a seeded local backend. Its exact head has green static, unit, package, type/build, HTTP, public-browser and local-auth checks, and the matching remote gates passed. It is awaiting the requested owner review. This install fix remains open until the CI repair can land and the required checks are green.

[steipete]

Completed and landed in #3853 (d26329b) on a maintainer-owned branch after the changelog conflict with current main. Your fix, regression tests, Co-authored-by credit, and Unreleased thanks are preserved; this fork branch is unchanged.

The built-CLI EBUSY regression was reproduced before the fix and passes afterward. All required remote gates and exact-head CI passed. Thank you, @SebTardif.