Skip to content

feat(skill): vendor mode — materialize installed skills as committed files#263

Merged
raks097 merged 5 commits into
mainfrom
feat/vendor
Jun 13, 2026
Merged

feat(skill): vendor mode — materialize installed skills as committed files#263
raks097 merged 5 commits into
mainfrom
feat/vendor

Conversation

@raks097

@raks097 raks097 commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

Adds a --vendor install mode and unifies --local through the Materializer seam. Targeted for v0.28.0 (must land before the release bump #262).

What

  • --vendor (qvr add <skill> --vendor, qvr add --local <path> --vendor): materializes the skill as real, committed files — a real directory at the canonical target, sibling targets as relative symlinks — instead of a symlink into the global store. This is the only mode where a skill travels with a git clone: no store, no registry, no qvr needed to read it. Verified end-to-end (wiped the entire store; vendored files persisted; qvr sync reconciled cleanly).
  • --local via the Materializer seam: the bespoke copyDir in both the install and sync-restore paths now flows through Materializer.MaterializeFromDisk → the reflink/content-store BlobMaterializer, skipping exactly what the canonical hasher excludes so the copy hash-agrees. qvr lock verify --strict passes on a plain --local install and after a store-wipe + sync.
  • ModeVendor mirrors ModeEdit across reconcile / remove / cache-prune / edit-guard; vendoring reuses the qvr edit eject machinery (shared copyTreeToCanonical with an initNestedRepo flag).

Verification

  • New internal/skill/vendor_test.go: local vendor, registry vendor (provenance + hash preserved), multi-target relative siblings, travels-without-store, remove-without-force, edit-refuses-vendored — all pass.
  • make all green (fmt + lint + test + build); gocyclo clean (≤15 — extracted linkLocalTargets to keep InstallLocal under); golangci-lint 0 issues; modernize clean.
  • Real-binary smoke tests of every path.

Review-only — please merge manually (and before #262 so v0.28.0 ships with it).

…files

`qvr add <skill> --vendor` and `qvr add --local <path> --vendor` now write the
skill as real files in the repo (a real directory at the canonical target,
sibling targets as relative symlinks) instead of a symlink into the global store.
This is the only mode where a skill travels with a git clone — no store, no
registry, no qvr needed to read it. Verified end-to-end: wiping the store leaves
the vendored files intact and `qvr sync` reconciles cleanly.

Also routes `--local` installs (and sync-restore) through the Materializer seam:
the bespoke copyDir now flows through Materializer.MaterializeFromDisk → the
reflink/content-store BlobMaterializer, skipping exactly what the canonical
hasher excludes so the copy hash-agrees. `qvr lock verify --strict` passes on a
plain `--local` install and after a store-wipe + sync.

ModeVendor mirrors ModeEdit across reconcile/remove/cache-prune/edit-guard, and
vendoring reuses the `qvr edit` eject machinery (shared copyTreeToCanonical).
@raks097 raks097 closed this Jun 13, 2026
@raks097 raks097 reopened this Jun 13, 2026
@greptile-apps

greptile-apps Bot commented Jun 13, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

All four rollback holes flagged in prior rounds are correctly plugged; the feature logic is well-tested and the existing suite passes.

The vendor flow — materialize, promote, repoint siblings, finalize entry, write lock — handles every failure mode with the appropriate rollback or RemoveAll guard, matching the prior EjectToTarget pattern. The MaterializeFromDisk path correctly unifies exec-bit handling and .git/signature exclusion with the canonical hasher. The single finding is a stale 'eject' prefix in a shared error message, which has no functional impact.

No files require special attention beyond the minor error-message nit in internal/skill/eject.go.

Important Files Changed

Filename Overview
internal/skill/eject.go Adds VendorIntoRepo and refactors materializeEjectDir into the shared copyTreeToCanonical/promoteStagingOntoCanonical helpers; rollback on repointSiblingTargets failure mirrors EjectToTarget. One style nit: the 'eject …' prefix in repointSiblingTargets's error message is now stale now that the function is shared with the vendor path.
internal/skill/installer.go Adds Vendor field to InstallRequest, wires VendorIntoRepo post-install in both InstallInto and InstallLocal, and adds rollbackInstallResult/rollbackVendoredInstall to clean up the vendor dir on lock.Write() failure. All four previously-flagged rollback holes are now plugged.
internal/skill/materialize.go Adds MaterializeFromDisk and writeDiskEntry to the Materializer, replacing bespoke copyDir calls for local installs. Symlink escape checks correctly handle the bare '..' case. Exec-bit and exclusion logic mirrors the canonical hasher.
internal/skill/reconciler.go Adds ModeVendor handling via the new reconcileInRepoEntry helper, reused for both edit and vendor modes. restoreLocal now delegates to Installer.materializeLocalCopy through the Materializer seam; the nil-Installer guard is safe given NewReconciler is the only non-test constructor.
internal/model/lockfile.go Adds ModeVendor constant and VendorPath field to LockEntry, with IsVendor() accessor mirroring IsEdit(). Backward-compatible via omitempty.
internal/skill/linker.go Adds IsVendor() branch to EffectiveTarget, correctly resolving project-relative VendorPath against projectRoot and passing through absolute paths for global scope.
internal/skill/vendor_test.go New test file covering local vendor, registry vendor, multi-target relative siblings, travels-without-store, remove, and edit-refusal; also covers the symlink-escape fix via TestMaterializeFromDisk_RefusesEscapingSymlink.
cmd/add.go Adds --vendor flag, wires it into addInstallItem and runAddLocal, and adds a --vendor && --frozen validation guard. skillDirForEntry is patched to handle the empty-worktree case for vendor/edit/link modes.
cmd/edit.go Adds an early guard in ejectUnderLock that returns a clear user-facing error when qvr edit is run on a vendored skill instead of producing a confusing clobber failure.
internal/registry/path.go Returns empty string from WorktreePathForEntry for vendor entries, letting qvr cache prune treat the transient store worktree as orphanable. Correctly documented with rationale.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["qvr add skill --vendor"] --> B[InstallInto / InstallLocal]
    B --> C[Resolve + scan + store materialize]
    C --> D[linkInstallTargets\ncreates symlinks]
    D --> E[lock.Put entry]
    E --> F{req.Vendor?}
    F -->|No| G[lock.Write\nmode:shared]
    F -->|Yes| H[VendorIntoRepo]
    H --> I[copyTreeToCanonical\ninitNestedRepo=false]
    I --> J[copyDir source to staging]
    J --> K[setSubtreeWritable staging]
    K --> L[promoteStagingOntoCanonical\nrename to canonical real dir]
    L --> M{repointSiblingTargets}
    M -->|success| N[finalizeVendoredEntry\nmode:vendor, VendorPath, SubtreeHash]
    M -->|failure| O[os.RemoveAll canonicalAbs\nreturn error]
    O --> P[rollbackLinks created\nreturn error]
    N --> Q[lock.Write\nmode:vendor]
    Q -->|success| R[InstallResult\nWorktree=vendorDir]
    Q -->|failure| S[rollbackVendoredInstall\nRemoveAll vendorDir + symlinks]
Loading

Fix All in Claude Code

Reviews (5): Last reviewed commit: "fix(skill): clean up vendored dir when l..." | Re-trigger Greptile

Comment thread internal/skill/installer.go
Comment thread internal/skill/materialize.go Outdated
…mlink

Addresses Greptile review of #263:
- InstallInto (registry --vendor path): linkInstallTargets now returns the
  created symlink paths, and a VendorIntoRepo failure rolls them back — matching
  InstallLocal, so a failed vendor no longer leaves orphan target symlinks.
- writeDiskEntry escape guard: a bare ".." target cleans to ".." (no "../"
  prefix) and previously slipped through; now rejected alongside absolute and
  "../"-prefixed targets. Regression test covers "..", "../outside", and absolute.
@raks097 raks097 closed this Jun 13, 2026
@raks097 raks097 reopened this Jun 13, 2026
@greptile-apps

greptile-apps Bot commented Jun 13, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

…r/eject

Greptile #263 (4/5): a multi-target vendor install that hits a filesystem error
during sibling relinking left the canonical agent dir as an orphaned real
directory, blocking future installs of the same skill. The caller's rollbackLinks
only removes symlinks, not the materialized real dir. The shared eject
orchestrator now os.RemoveAll(canonicalAbs) on a repointSiblingTargets failure,
keeping eject/vendor atomic (the caller still rolls back the target symlinks).
@raks097

raks097 commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the 4/5 finding (multi-target sibling relink) in 2472958: the shared eject/vendor orchestrator now os.RemoveAll(canonicalAbs) when repointSiblingTargets fails, so a partial multi-target vendor no longer orphans the canonical real directory (which previously blocked future installs). The caller still rolls back the target symlinks, so eject/vendor is atomic.

@raks097 raks097 closed this Jun 13, 2026
@raks097 raks097 reopened this Jun 13, 2026
Comment thread internal/skill/eject.go
The previous fix covered EjectToTarget; VendorIntoRepo has its own
copyTreeToCanonical + repointSiblingTargets path that still orphaned the
canonical real directory on a sibling-relink failure. Mirror the cleanup so a
partial multi-target vendor no longer blocks future installs at that path.
@raks097 raks097 closed this Jun 13, 2026
@raks097 raks097 reopened this Jun 13, 2026
Comment thread internal/skill/installer.go
A lock.Write failure after a successful VendorIntoRepo left the canonical real
directory behind (symlink rollback can't remove a real dir), so a retry hit
'exists and is not a symlink'. Both single-install paths now clean it up:
- InstallLocal: rollbackVendoredInstall removes links + the vendored dir
- Install (registry wrapper): rollbackInstallResult undoes InstallInto's links
  and the vendored dir from the returned result
(extracted rollbackVendoredInstall to keep InstallLocal under the cyclo limit.)
@raks097

raks097 commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the 4/5 finding (lock.Write failure orphaning the vendored dir) in f945c8d: both single-install paths now clean up the canonical real directory on a post-vendor lock.Write failure — InstallLocal via rollbackVendoredInstall (links + dir), and the Install registry wrapper via rollbackInstallResult (undoes the InstallInto links + the vendored dir from the returned result). A retry no longer hits "exists and is not a symlink".

@raks097 raks097 closed this Jun 13, 2026
@raks097 raks097 reopened this Jun 13, 2026
@raks097 raks097 merged commit 6647e39 into main Jun 13, 2026
3 of 6 checks passed
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.

1 participant