Skip to content

fix(install): pre-validate provides.files + rollback on hook gate fail#96

Merged
jcfischer merged 2 commits intomainfrom
fix/install-rollback
Apr 25, 2026
Merged

fix(install): pre-validate provides.files + rollback on hook gate fail#96
jcfischer merged 2 commits intomainfrom
fix/install-rollback

Conversation

@jcfischer
Copy link
Copy Markdown
Contributor

Summary

Closes #89.

`install` previously left orphan symlinks under `~/.claude/{skills,agents,bin,...}` when validation failed mid-flow. Two distinct partial-state shapes:

  1. `provides.files` entry N's source missing → entries 1..N-1 already linked, per-type primary symlinks already in place.
  2. Hook gate trips after symlinks landed → symlinks orphaned, `settings.json` untouched, DB unaware.

User had no clean recovery path: `arc remove` couldn't drive cleanup (no DB entry), so manual rm was the only option.

Changes

  • `src/lib/artifact-installer.ts` — implement both fixes from the issue:

    1. Pre-pass validation (option 1). Before the per-type switch, walk every `provides.files` entry and assert each `source` exists in the package. If any missing, return immediately with `filesMissingSource` populated and zero filesystem mutations.
    2. Rollback tracking (option 2). Every symlink (per-type primary, CLI bins, `provides.files` target) and shim now goes through `linkTracked` / `shimTracked` helpers that record the path on an `ArtifactSymlinkRecord`. New `rollbackArtifactSymlinks()` helper unlinks them best-effort.
  • `src/commands/install.ts` — call `rollbackArtifactSymlinks(symlinkResult.record)` on the hook-gate failure path. Missing hook target now tears down everything that was placed before the gate ran.

Tests (test/commands/install.test.ts, +2)

  • Pre-validation: manifest with two `provides.files`, second source missing. Install fails with `provides.files` diagnostic. The FIRST entry's target is also absent (pre-validation prevented it being created), and the per-type skill symlink is absent.
  • Hook-gate rollback: `provides.files` succeeds but a separate hook command references an unlanded file. Install fails with hook diagnostic. Skill symlink AND `provides.files` target both rolled back.

Suite: 624/624 pass, tsc clean.

🤖 Generated with Claude Code

jcfischer and others added 2 commits April 25, 2026 15:36
Issue #89: install previously left orphan symlinks under
~/.claude/{skills,agents,bin,...} when validation failed mid-flow.
Two distinct partial-state shapes:

1. provides.files entry N's source missing → entries 1..N-1 already
   linked, per-type primary symlinks already in place.
2. Hook gate trips after symlinks landed → symlinks orphaned,
   settings.json untouched, DB unaware.

Implement both fixes from the issue:

- **Pre-pass validation (option 1):** in createArtifactSymlinks, walk
  every provides.files entry up front and assert each source exists
  in the package. If any missing, return immediately with
  filesMissingSource populated and zero filesystem mutations. The
  most common failure mode (manifest typo, repo drift) is now caught
  before any symlink lands.

- **Rollback tracking (option 2):** every symlink (per-type primary,
  CLI bins, provides.files target) and shim is recorded in an
  ArtifactSymlinkRecord returned alongside filesCreated. New
  rollbackArtifactSymlinks() helper unlinks them best-effort.
  install.ts calls it on the hook-gate failure path, so a missing
  hook target tears down everything that was placed before the gate
  ran.

Tests (test/commands/install.test.ts, +2):
- Manifest with two provides.files, second source missing → install
  fails with provides.files diagnostic, the FIRST entry's target is
  also absent (pre-validation prevented it being created), and the
  per-type skill symlink is absent.
- Manifest where provides.files succeeds but a separate hook command
  references an unlanded file → install fails with hook diagnostic,
  skill symlink AND provides.files target both rolled back.

Closes #89.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Luna's feedback on PR #96.

- Tighten rollbackArtifactSymlinks JSDoc: was overselling postinstall
  coverage but install() never calls rollback on postinstall failure.
  Postinstall recovery is a strictly larger fix (also requires
  unregistering hooks from settings.json + DB cleanup); track
  separately. Doc now reflects what actually happens.
- Surface non-ENOENT errors during rollback via console.warn instead
  of silent .catch. ENOENT still tolerated (best-effort across all
  entries); permission-denied or other unexpected failures now leave
  the user aware of orphans they need to inspect manually.
- Tighten hook-gate rollback test: add provides.cli to the fixture so
  the bin symlink + PATH shim are actually created and the test
  asserts both are torn down. Previously the fixture had no CLI
  declaration, so the shim-rollback assertion would have been
  vacuously true.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jcfischer
Copy link
Copy Markdown
Contributor Author

Addressed Luna's review in commit 514bd47.

  • JSDoc tightened on rollbackArtifactSymlinks — no longer claims postinstall coverage. Now reads: "Currently called by install when the hook-validation gate fails. Postinstall-script failure also leaves partial state but is a strictly larger fix; tracked separately."
  • Non-ENOENT errors surfaced via console.warn instead of silent .catch. ENOENT still tolerated (best-effort across all entries); permission-denied etc. now leave the user aware of orphans.
  • Hook-gate test tightened — added provides.cli to the fixture so bin symlink + PATH shim are actually created. Test now asserts both are torn down (was vacuous before).
  • Filed install: rollback symlinks + unregister hooks on postinstall script failure #97install: rollback symlinks + unregister hooks on postinstall script failure. Labeled bug, next. Body cites Luna's review, lists the three rollback components needed (symlinks, hooks unregister, possible DB cleanup), and proposes the call-site changes.

Suite stable at 624/624. Re-pinging.

@jcfischer jcfischer merged commit 1d77676 into main Apr 25, 2026
@jcfischer jcfischer deleted the fix/install-rollback branch April 25, 2026 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install: rollback partial symlinks when provides.files validation fails

1 participant