From 0b235a8c35275c6bc4ab0c8b4e83d390531080e4 Mon Sep 17 00:00:00 2001 From: Rakshith Date: Sat, 13 Jun 2026 16:44:47 -0400 Subject: [PATCH 1/5] =?UTF-8?q?feat(skill):=20vendor=20mode=20=E2=80=94=20?= =?UTF-8?q?materialize=20installed=20skills=20as=20committed=20files?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `qvr add --vendor` and `qvr add --local --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). --- cmd/add.go | 19 ++- cmd/edit.go | 5 + internal/model/lockfile.go | 32 ++++- internal/registry/path.go | 8 ++ internal/skill/eject.go | 177 ++++++++++++++++++++---- internal/skill/installer.go | 157 +++++++++++++++------ internal/skill/linker.go | 9 ++ internal/skill/materialize.go | 84 ++++++++++++ internal/skill/reconciler.go | 87 +++++++----- internal/skill/vendor_test.go | 248 ++++++++++++++++++++++++++++++++++ 10 files changed, 721 insertions(+), 105 deletions(-) create mode 100644 internal/skill/vendor_test.go diff --git a/cmd/add.go b/cmd/add.go index 1111010..b5dcd7d 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -29,6 +29,7 @@ var ( addAs string addAll bool addLocal string + addVendor bool ) var addCmd = &cobra.Command{ @@ -102,7 +103,9 @@ func init() { addCmd.Flags().BoolVar(&addAll, "all", false, "install every skill the registry exposes (requires --registry; do not name a skill)") addCmd.Flags().StringVar(&addLocal, "local", "", - "install an immutable copy of a skill from a local folder (no registry; `qvr edit` to make it mutable)") + "install an immutable copy of a skill from a local folder (no registry; `qvr edit` to make it mutable, or add --vendor to commit it into the repo)") + addCmd.Flags().BoolVar(&addVendor, "vendor", false, + "commit the skill as real files in the repo (no symlink into the store) so it travels with a `git clone` — no store, registry, or qvr needed to read it. Composes with --local.") rootCmd.AddCommand(addCmd) } @@ -333,6 +336,7 @@ func addInstallItem(cmd *cobra.Command, cfg *config.Config, installer *skill.Ins Registry: item.registry, SkillPath: item.skillPath, As: addAs, + Vendor: addVendor, RequireSigned: cfg.Security.RequireSigned, TrustedAuthors: trustedAuthorsForRegistry(cfg, item.registry), TrustedAuthorsByRegistry: trustedAuthorsByRegistry(cfg), @@ -702,6 +706,12 @@ func validateAddModes() error { return fmt.Errorf("--frozen cannot be combined with --local") } } + if addVendor && addFrozen { + // --frozen pins to the recorded store hash and forbids drift; vendoring + // rewrites how the skill lands (real in-repo files), so the two are + // contradictory. + return fmt.Errorf("--vendor cannot be combined with --frozen") + } return nil } @@ -763,6 +773,7 @@ func runAddLocal(cmd *cobra.Command, cfg *config.Config, installer *skill.Instal ProjectRoot: projectRoot, LockPath: lockPath, Force: addForce, + Vendor: addVendor, }) if ierr != nil { printer.Error(fmt.Sprintf("add --local %s: %v", resolved, ierr)) @@ -1272,6 +1283,12 @@ func skillDirForEntry(result *skill.InstallResult, lock *model.LockFile) string return result.Worktree } worktreePath := skill.EntryWorktreePath(entry) + if worktreePath == "" { + // No store worktree (vendor/edit/link): the content lives where + // result.Worktree points — already at the skill root, so don't join + // entry.Path. result.Worktree is EffectiveTarget for these modes. + return result.Worktree + } if entry.Path == "" || entry.Path == "." { return worktreePath } diff --git a/cmd/edit.go b/cmd/edit.go index ec5cd7e..5febb71 100644 --- a/cmd/edit.go +++ b/cmd/edit.go @@ -123,6 +123,11 @@ func ejectUnderLock(name, projectRoot, lockPath, projPath string, result **skill if entry.IsLink() { return fmt.Errorf("%s is a link install at %s — edit the source path directly", name, entry.Source) } + // Vendored skills are already real, writable files committed in the repo; + // there's nothing to eject — point the user at the in-repo dir. + if entry.IsVendor() { + return fmt.Errorf("%s is vendored at %s — its files are real and editable in the repo; edit them directly", name, entry.VendorPath) + } // Already ejected: no-op, return current state. if entry.IsEdit() { *idempotent = true diff --git a/internal/model/lockfile.go b/internal/model/lockfile.go index bd93454..699e9fc 100644 --- a/internal/model/lockfile.go +++ b/internal/model/lockfile.go @@ -80,10 +80,11 @@ var ( // Install mode constants. Empty string means "shared" (default add semantics) // for backward compatibility — older v5 locks predate this field. const ( - ModeShared = "" // symlink → ~/.quiver/worktrees/.../ (default for `qvr add`) - ModeEdit = "edit" // canonical real dir at EditPath (set by `qvr edit`) - ModeLink = "link" // absolute path in Source (legacy `qvr link`; read-only compat) - ModeLocal = "local" // immutable copy of a local folder (set by `qvr add --local`) + ModeShared = "" // symlink → ~/.quiver/worktrees/.../ (default for `qvr add`) + ModeEdit = "edit" // canonical real dir at EditPath (set by `qvr edit`) + ModeLink = "link" // absolute path in Source (legacy `qvr link`; read-only compat) + ModeLocal = "local" // immutable copy of a local folder (set by `qvr add --local`) + ModeVendor = "vendor" // real files committed into the repo at VendorPath (set by `qvr add --vendor`) ) // LocalRegistry is the reserved registry name stamped on `qvr add --local` @@ -180,6 +181,15 @@ type LockEntry struct { // target carry relative symlinks to it. Empty when Mode != "edit". EditPath string `json:"editPath,omitempty" toml:"editPath,omitempty"` + // VendorPath is the project-relative path of the canonical vendored + // copy when Mode == "vendor" (e.g. ".claude/skills/auth"). Like + // EditPath it is a real in-repo directory the canonical target IS, + // with sibling targets carrying relative symlinks to it — but the + // bytes are tracked by the outer repo (no nested .git), so a teammate + // who clones the repo gets the skill files with no store, registry, or + // qvr required. Empty when Mode != "vendor". + VendorPath string `json:"vendorPath,omitempty" toml:"vendorPath,omitempty"` + // Targets is the list of agent dirs the skill is symlinked into. Targets []string `json:"targets" toml:"targets"` @@ -289,6 +299,20 @@ func (e *LockEntry) IsEdit() bool { return e.Mode == ModeEdit } +// IsVendor reports whether this entry is vendored: real skill files committed +// into the repo at VendorPath via `qvr add --vendor`. The canonical agent +// target dir at VendorPath is itself a real directory (sibling targets repoint +// at it), and the bytes are version-controlled by the outer repo rather than +// the qvr store — so the skill travels with a `git clone` without needing a +// store worktree or a reachable registry. Like edit installs, the shared store +// worktree is not load-bearing for a vendored skill. +func (e *LockEntry) IsVendor() bool { + if e == nil { + return false + } + return e.Mode == ModeVendor +} + // LockFile is the on-disk record of installed skills. // // The lockfile is qvr's pillar of portability, governance, and reproducibility: diff --git a/internal/registry/path.go b/internal/registry/path.go index adc8144..4c5be4a 100644 --- a/internal/registry/path.go +++ b/internal/registry/path.go @@ -233,6 +233,14 @@ func WorktreePathForEntry(e *model.LockEntry) string { if e.IsLink() { return e.Source } + // Vendored skills live in-repo at VendorPath, not in the store. They claim + // no store worktree, so the transient one materialized at install time + // becomes orphan and is reclaimed by `qvr cache prune` (the content is + // content-addressed and shared, so prune only drops it when no other + // project references it). Returning a path here would wrongly pin it. + if e.IsVendor() { + return "" + } key := e.InstallCommit if key == "" { key = e.Commit diff --git a/internal/skill/eject.go b/internal/skill/eject.go index 54fdec6..a53a774 100644 --- a/internal/skill/eject.go +++ b/internal/skill/eject.go @@ -28,6 +28,13 @@ var ( // ErrNoTargets is returned by Eject when the lock entry records no // installed targets, leaving no agent dir to eject the copy into. ErrNoTargets = errors.New("entry has no installed targets to eject into") + // ErrCannotVendorLink is returned by VendorIntoRepo for a link install: + // its bytes live at an external path with no store worktree to copy in. + ErrCannotVendorLink = errors.New("cannot vendor a link install") + // ErrCannotEjectVendor is returned by EjectToTarget for a vendored entry: + // its files are already real and writable in the repo, so there is nothing + // to eject. + ErrCannotEjectVendor = errors.New("cannot eject a vendored install — its files are already editable in the repo") ) // EjectRequest drives a `qvr edit` eject. @@ -79,6 +86,12 @@ func EjectToTarget(req EjectRequest) (*EjectResult, error) { if e.IsLink() { return nil, ErrCannotEjectLink } + if e.IsVendor() { + // A vendored skill is already real, writable, in-repo files — there is + // no store worktree to eject and the canonical target is a real dir, so + // the eject rename would refuse to clobber it. Reject explicitly. + return nil, ErrCannotEjectVendor + } if len(e.Targets) == 0 { return nil, ErrNoTargets } @@ -128,47 +141,66 @@ func EjectToTarget(req EjectRequest) (*EjectResult, error) { // materializeEjectDir resolves the shared-worktree source, copies the skill // subtree into a staging sibling dir, restores write bits (the immutable→editable // hinge), inits a fresh git history, then atomically renames the staging dir onto -// the canonical slot — refusing to clobber a real (non-symlink) dir there. The -// staging-then-rename avoids leaving half-copied state if any step fails midway. +// the canonical slot. Thin wrapper over copyTreeToCanonical with the edit-mode +// settings (nested git history seeded). func materializeEjectDir(e *model.LockEntry, req EjectRequest, canonicalAbs string) error { - // Resolve the shared worktree source: where we'll copy the skill files - // from. Falls back to LoadFromPath when the worktree isn't on disk so a - // user who deleted ~/.quiver/worktrees/ before invoking edit gets a clean - // error instead of an empty copy. - sourceDir := EffectiveTarget(e, req.ProjectRoot) + return copyTreeToCanonical(e, req.ProjectRoot, canonicalAbs, ".ejecting", true, req.Author, req.AuthorEmail) +} + +// copyTreeToCanonical resolves the entry's current effective source (the shared +// store worktree or the local copy), copies it into a staging sibling of +// canonicalAbs, restores write bits, optionally seeds a fresh nested git history, +// then atomically renames the staging dir onto the canonical agent-dir slot — +// refusing to clobber a real (non-symlink) dir there. The staging-then-rename +// avoids leaving half-copied state if any step fails midway. +// +// Shared by `qvr edit` (initNestedRepo=true: the eject dir is a standalone fork +// that `qvr publish` later pushes) and `qvr add --vendor` (initNestedRepo=false: +// the bytes are tracked by the OUTER project repo, so a nested .git would be both +// redundant and a committed-repo-inside-a-repo footgun). +func copyTreeToCanonical(e *model.LockEntry, projectRoot, canonicalAbs, stagingSuffix string, initNestedRepo bool, author, authorEmail string) error { + // Resolve the source: where we'll copy the skill files from. Returns "" + // when the worktree isn't on disk so a user who deleted ~/.quiver/worktrees/ + // gets a clean error instead of an empty copy. + sourceDir := EffectiveTarget(e, projectRoot) if sourceDir == "" { - return fmt.Errorf("eject %s: no source worktree to copy from — run `qvr sync` first", e.Name) + return fmt.Errorf("%s: no source worktree to copy from — run `qvr sync` first", e.Name) } if _, err := os.Stat(filepath.Join(sourceDir, "SKILL.md")); err != nil { - return fmt.Errorf("eject %s: source %s does not contain SKILL.md: %w", e.Name, sourceDir, err) + return fmt.Errorf("%s: source %s does not contain SKILL.md: %w", e.Name, sourceDir, err) } // Stage to a sibling tmp dir, then rename onto canonical. Avoids leaving // half-copied state if the copy / git init fails midway. - stagingDir := canonicalAbs + ".ejecting" + stagingDir := canonicalAbs + stagingSuffix _ = os.RemoveAll(stagingDir) if err := copyDir(sourceDir, stagingDir); err != nil { _ = os.RemoveAll(stagingDir) return fmt.Errorf("copy skill tree: %w", err) } - // The shared worktree is frozen read-only and copyDir preserves source - // modes, so the freshly-copied edit tree would be read-only. Restore write - // bits: this copy is the editable working dir, and initEjectRepo is about - // to `git init` and write into it. This is the immutable→editable hinge. + // The source worktree is frozen read-only and copyDir preserves source + // modes, so the freshly-copied tree would be read-only. Restore write bits: + // this copy is the working dir the user (and any `git init` below) writes to. setSubtreeWritable(stagingDir) - if err := initEjectRepo(stagingDir, e, req.Author, req.AuthorEmail); err != nil { - _ = os.RemoveAll(stagingDir) - return fmt.Errorf("init edit repo: %w", err) + if initNestedRepo { + if err := initEjectRepo(stagingDir, e, author, authorEmail); err != nil { + _ = os.RemoveAll(stagingDir) + return fmt.Errorf("init edit repo: %w", err) + } } + return promoteStagingOntoCanonical(e, stagingDir, canonicalAbs) +} - // Remove the existing canonical symlink (or no-op if it isn't there) so - // the rename below lands on a clean slot. CreateSymlink earlier may have - // pointed it at the shared worktree; if a real dir is there already we - // refuse to clobber user content. +// promoteStagingOntoCanonical removes the existing canonical symlink (or no-op +// if absent) so the rename lands on a clean slot, then atomically renames the +// staging dir onto it. CreateSymlink earlier may have pointed the canonical at +// the shared worktree; if a real dir is there already we refuse to clobber user +// content. On any failure the staging dir is removed so no half-state lingers. +func promoteStagingOntoCanonical(e *model.LockEntry, stagingDir, canonicalAbs string) error { if existing, err := os.Lstat(canonicalAbs); err == nil { if existing.Mode()&os.ModeSymlink == 0 { _ = os.RemoveAll(stagingDir) - return fmt.Errorf("eject %s: %s exists and is not a symlink — refuse to overwrite", e.Name, canonicalAbs) + return fmt.Errorf("%s: %s exists and is not a symlink — refuse to overwrite", e.Name, canonicalAbs) } if err := os.Remove(canonicalAbs); err != nil { _ = os.RemoveAll(stagingDir) @@ -181,11 +213,110 @@ func materializeEjectDir(e *model.LockEntry, req EjectRequest, canonicalAbs stri } if err := os.Rename(stagingDir, canonicalAbs); err != nil { _ = os.RemoveAll(stagingDir) - return fmt.Errorf("finalize edit dir: %w", err) + return fmt.Errorf("finalize canonical dir: %w", err) } return nil } +// VendorRequest drives a `qvr add --vendor` post-install vendoring step. +type VendorRequest struct { + Entry *model.LockEntry // the freshly-installed lock entry (mutated in place on success) + ProjectRoot string // absolute project root + // Global, when true, vendors into the user-global agent dir and writes + // VendorPath as an absolute path; project scope (default) writes a + // project-relative VendorPath so the lockfile stays portable across clones. + Global bool +} + +// VendorResult summarises a vendoring for the caller. +type VendorResult struct { + Skill string `json:"skill"` + CanonicalTarget string `json:"canonical_target"` + VendorPath string `json:"vendor_path"` // project-relative (or absolute when Global) + SiblingLinks []string `json:"sibling_links"` // absolute paths of repointed sibling symlinks +} + +// VendorIntoRepo promotes a freshly-installed skill's store worktree into a real +// directory committed into the repo at the alphabetical-first installed target, +// and repoints any other installed targets at it via relative symlinks. The lock +// entry is mutated in place: Mode flips to "vendor", VendorPath records the +// project-relative canonical path, and SubtreeHash is re-sealed against the +// in-repo dir. +// +// Unlike EjectToTarget it seeds NO nested git history — the vendored bytes are +// tracked by the OUTER project repo, which is exactly what lets the skill travel +// with a `git clone` (no store, no registry, no qvr needed to read it). It is the +// `--vendor` counterpart to a normal symlink-into-store install. +// +// Idempotent: a second call when Mode is already "vendor" at the same path is a +// no-op. Refuses link installs (no store worktree to copy from). +func VendorIntoRepo(req VendorRequest) (*VendorResult, error) { + e := req.Entry + if e == nil { + return nil, errors.New("vendor: nil entry") + } + if req.ProjectRoot == "" { + return nil, errors.New("vendor: project root is required") + } + if e.IsLink() { + return nil, ErrCannotVendorLink + } + if len(e.Targets) == 0 { + return nil, ErrNoTargets + } + + canonicalTarget := pickCanonicalTarget(e.Targets) + t, ok := model.LookupTarget(canonicalTarget) + if !ok { + return nil, fmt.Errorf("%w: %s", ErrUnknownTarget, canonicalTarget) + } + + // Reuse the eject path-derivation: --global → absolute canonical/VendorPath, + // project → project-relative. The EjectRequest carries only scope here. + scope := EjectRequest{ProjectRoot: req.ProjectRoot, Global: req.Global} + canonicalAbs, vendorPathForLock, err := ejectCanonicalPaths(t, e, scope) + if err != nil { + return nil, err + } + + // Idempotent no-op when the entry already records this exact vendoring. + if e.IsVendor() && e.VendorPath == vendorPathForLock { + return &VendorResult{Skill: e.Name, CanonicalTarget: canonicalTarget, VendorPath: vendorPathForLock}, nil + } + + if err := copyTreeToCanonical(e, req.ProjectRoot, canonicalAbs, ".vendoring", false, "", ""); err != nil { + return nil, err + } + + siblingLinks, err := repointSiblingTargets(e, scope, canonicalTarget, canonicalAbs) + if err != nil { + return nil, err + } + + finalizeVendoredEntry(e, vendorPathForLock, canonicalAbs) + + return &VendorResult{ + Skill: e.Name, + CanonicalTarget: canonicalTarget, + VendorPath: vendorPathForLock, + SiblingLinks: siblingLinks, + }, nil +} + +// finalizeVendoredEntry mutates the entry on success: flips it to mode:vendor, +// records the VendorPath, and re-seals SubtreeHash against the in-repo dir so +// drift detection (`qvr lock verify`) has a current baseline. The copy is +// byte-identical to the store worktree, so the hash normally matches what the +// install already recorded; re-hashing is defensive and matches eject's pattern. +// HashSubtreeFromDisk excludes .git/, so the seal agrees with a later verify. +func finalizeVendoredEntry(e *model.LockEntry, vendorPathForLock, canonicalAbs string) { + e.Mode = model.ModeVendor + e.VendorPath = vendorPathForLock + if h, err := canonical.HashSubtreeFromDisk(canonicalAbs); err == nil { + e.SubtreeHash = h + } +} + // ejectCanonicalPaths computes the canonical eject dir and the EditPath recorded // in the lock, scoped per issue #82: // diff --git a/internal/skill/installer.go b/internal/skill/installer.go index 75061e4..12bfbb8 100644 --- a/internal/skill/installer.go +++ b/internal/skill/installer.go @@ -113,6 +113,15 @@ type InstallRequest struct { // to the by-name full-index lookup, so this is a pure speedup, never a // correctness change. SkillPath string + // Vendor materializes the skill as real files committed into the repo at + // VendorPath instead of a symlink into the shared store (`qvr add --vendor`). + // After the normal resolve + scan + store materialization, the content is + // promoted into the canonical agent dir as a real directory (sibling targets + // repoint at it via relative symlinks) and the lock entry flips to + // mode:vendor — so the skill travels with a `git clone` without needing the + // store or a reachable registry. Composes with any source (registry or + // --local). + Vendor bool } // InstallResult holds the outcome for a single skill install. @@ -292,11 +301,24 @@ func (in *Installer) InstallInto(req InstallRequest, lock *model.LockFile) (*Ins } lock.Put(entry) + worktree := finalPath + // --vendor: promote the just-materialized store copy into real files + // committed in the repo, flipping the entry to mode:vendor. The entry is the + // pointer held in the lock map, so mutating it in place is reflected on the + // caller's lock.Write(). The store worktree stays on disk (shared, content- + // addressed) until `qvr cache prune` reclaims it as an orphan. + if req.Vendor { + if _, verr := VendorIntoRepo(VendorRequest{Entry: entry, ProjectRoot: req.ProjectRoot, Global: req.Global}); verr != nil { + return nil, fmt.Errorf("vendor %s: %w", localName, verr) + } + worktree = EffectiveTarget(entry, req.ProjectRoot) + } + result := &InstallResult{ Name: localName, Registry: loc.RegistryName, Version: version, - Worktree: finalPath, + Worktree: worktree, Targets: targets, Commit: commit, } @@ -790,21 +812,27 @@ func (in *Installer) RemoveFrom(name string, req InstallRequest, lock *model.Loc } entryGlobal := lock.IsGlobal(quiverHome()) - canonicalEditAbs := "" + // canonicalDirAbs is the in-repo real directory to rm -rf for the modes + // whose canonical target IS a directory rather than a store symlink: + // edit (EditPath) and vendor (VendorPath). + canonicalDirAbs := "" if entry.IsEdit() { if !req.Force { return fmt.Errorf("remove %s: skill is in edit mode — pass --force to delete the eject dir at %s", name, entry.EditPath) } - canonicalEditAbs = entry.EditPath - if canonicalEditAbs != "" && !filepath.IsAbs(canonicalEditAbs) { - canonicalEditAbs = filepath.Join(req.ProjectRoot, canonicalEditAbs) - } + canonicalDirAbs = absInProject(entry.EditPath, req.ProjectRoot) + } + if entry.IsVendor() { + // Vendored files are reproducible (re-run `qvr add --vendor`) and + // tracked by the outer repo, so — unlike an edit dir's hand-authored + // changes — removal needs no --force; `git checkout` restores them. + canonicalDirAbs = absInProject(entry.VendorPath, req.ProjectRoot) } - // Pass 1: drop target symlinks (and the canonical edit dir, when in - // edit mode). Bail without touching the lock if any step fails so the - // user can recover rather than be left with an orphan lock entry. - if err := removeTargetLinks(name, entry, req, entryGlobal, canonicalEditAbs); err != nil { + // Pass 1: drop target symlinks (and the canonical in-repo dir, for edit/ + // vendor). Bail without touching the lock if any step fails so the user can + // recover rather than be left with an orphan lock entry. + if err := removeTargetLinks(name, entry, req, entryGlobal, canonicalDirAbs); err != nil { return err } @@ -821,24 +849,34 @@ func (in *Installer) RemoveFrom(name string, req InstallRequest, lock *model.Loc return nil } -// removeTargetLinks drops every target symlink for the entry, and for a -// mode:edit canonical target rm -rf's the eject dir (siblings are symlinks +// absInProject resolves a lock-recorded path (EditPath/VendorPath) to absolute: +// project-relative paths join projectRoot, absolute paths (e.g. --global) and +// empty pass through unchanged. +func absInProject(p, projectRoot string) string { + if p == "" || filepath.IsAbs(p) { + return p + } + return filepath.Join(projectRoot, p) +} + +// removeTargetLinks drops every target symlink for the entry, and for an +// edit/vendor canonical target rm -rf's the in-repo dir (siblings are symlinks // pointing at canonical and use RemoveSymlink). Bails on the first failure so // the caller can leave the lock untouched for recovery. -func removeTargetLinks(name string, entry *model.LockEntry, req InstallRequest, entryGlobal bool, canonicalEditAbs string) error { +func removeTargetLinks(name string, entry *model.LockEntry, req InstallRequest, entryGlobal bool, canonicalDirAbs string) error { for _, t := range entry.Targets { linkPath, err := ResolveTargetPath(t, name, req.ProjectRoot, entryGlobal) if err != nil { return fmt.Errorf("remove %s: resolve target %s: %w", name, t, err) } - // For mode:edit canonical target: rm -rf the eject dir. Siblings + // For an edit/vendor canonical target: rm -rf the real dir. Siblings // are symlinks pointing at canonical and use RemoveSymlink. - if entry.IsEdit() && canonicalEditAbs != "" { - canonicalAbs, _ := filepath.Abs(canonicalEditAbs) + if (entry.IsEdit() || entry.IsVendor()) && canonicalDirAbs != "" { + canonicalAbs, _ := filepath.Abs(canonicalDirAbs) absLink, _ := filepath.Abs(linkPath) if canonicalAbs == absLink { if err := os.RemoveAll(linkPath); err != nil { - return fmt.Errorf("remove %s: rm eject dir %s: %w", name, linkPath, err) + return fmt.Errorf("remove %s: rm in-repo dir %s: %w", name, linkPath, err) } continue } @@ -859,7 +897,11 @@ func removeTargetLinks(name string, entry *model.LockEntry, req InstallRequest, // entries never had a shared worktree to clean; link installs point at // user-owned dirs we don't touch. func (in *Installer) removeSharedWorktree(name string, entry *model.LockEntry, req InstallRequest, lock *model.LockFile) error { - if entry.IsLink() || entry.IsEdit() { + // Edit/vendor entries' content is the in-repo dir (removed in pass 1); link + // installs point at user-owned dirs. None claim a store worktree to drop — + // the transient one a vendored install left behind is reclaimed by `qvr + // cache prune` once no other project references it. + if entry.IsLink() || entry.IsEdit() || entry.IsVendor() { return nil } worktreePath := EntryWorktreePath(entry) @@ -958,7 +1000,7 @@ func (in *Installer) InstallLocal(localPath string, req InstallRequest) (*Instal return nil, err } - if err := materializeLocalCopy(abs, finalPath); err != nil { + if err := in.materializeLocalCopy(abs, finalPath); err != nil { return nil, err } // Freeze: same immutability contract as a shared install (see immutable.go) @@ -967,21 +1009,12 @@ func (in *Installer) InstallLocal(localPath string, req InstallRequest) (*Instal // `qvr lock verify` and healed by `qvr sync` (re-copy from Source). setSubtreeReadOnly(finalPath) - var created []string - for _, t := range req.Targets { - linkPath, err := ResolveTargetPath(t, name, req.ProjectRoot, req.Global) - if err != nil { - rollbackLinks(created) - return nil, err - } - if err := CreateSymlink(linkPath, finalPath); err != nil { - rollbackLinks(created) - return nil, fmt.Errorf("symlink %s: %w", t, err) - } - created = append(created, linkPath) + created, err := linkLocalTargets(name, finalPath, req) + if err != nil { + return nil, err } - lock.Put(&model.LockEntry{ + entry := &model.LockEntry{ Name: name, Registry: model.LocalRegistry, Source: abs, @@ -992,7 +1025,22 @@ func (in *Installer) InstallLocal(localPath string, req InstallRequest) (*Instal SubtreeHash: hash, Targets: req.Targets, InstalledAt: time.Now().UTC(), - }) + } + lock.Put(entry) + + worktree := finalPath + // --vendor: promote the just-materialized store copy into real files + // committed in the repo, flipping the entry to mode:vendor. Mutates entry + // in place (the same pointer held in the lock map), so the Put above already + // reflects it. On failure the links we created are rolled back. + if req.Vendor { + if _, verr := VendorIntoRepo(VendorRequest{Entry: entry, ProjectRoot: req.ProjectRoot, Global: req.Global}); verr != nil { + rollbackLinks(created) + return nil, fmt.Errorf("vendor %s: %w", name, verr) + } + worktree = EffectiveTarget(entry, req.ProjectRoot) + } + if err := lock.Write(); err != nil { rollbackLinks(created) return nil, fmt.Errorf("write lock file: %w", err) @@ -1001,12 +1049,34 @@ func (in *Installer) InstallLocal(localPath string, req InstallRequest) (*Instal Name: name, Registry: model.LocalRegistry, Version: "local", - Worktree: finalPath, + Worktree: worktree, Commit: commitKey, Targets: req.Targets, }, nil } +// linkLocalTargets symlinks every requested target at finalPath (the local +// install's immutable store copy), rolling back any links created so far if one +// fails so the filesystem is left consistent. Mirrors linkInstallTargets for the +// registry path, minus the legacy root-layout agent-view handling a copied local +// folder never needs. +func linkLocalTargets(name, finalPath string, req InstallRequest) ([]string, error) { + var created []string + for _, t := range req.Targets { + linkPath, err := ResolveTargetPath(t, name, req.ProjectRoot, req.Global) + if err != nil { + rollbackLinks(created) + return nil, err + } + if err := CreateSymlink(linkPath, finalPath); err != nil { + rollbackLinks(created) + return nil, fmt.Errorf("symlink %s: %w", t, err) + } + created = append(created, linkPath) + } + return created, nil +} + // checkLocalConflict refuses to silently replace an existing entry of the same // name pointing at a different source. Idempotent when the source matches; // --force needed to re-point. Mirrors the remote-install conflict rule. @@ -1024,11 +1094,15 @@ func checkLocalConflict(lock *model.LockFile, name, abs string, force bool) erro return nil } -// materializeLocalCopy copies the local skill folder into the immutable worktree -// at finalPath if it isn't already on disk. It stages in a sibling dir and -// atomically renames so a crashed copy never leaves a half-written worktree -// behind. copyDir skips .git/ and preserves exec bits. -func materializeLocalCopy(abs, finalPath string) error { +// materializeLocalCopy materializes the local skill folder into the immutable +// worktree at finalPath if it isn't already on disk. It stages in a sibling dir +// and atomically renames so a crashed copy never leaves a half-written worktree +// behind. The copy flows through the SAME Materializer seam as a registry install +// (Materializer.MaterializeFromDisk → BlobMaterializer reflink/content-store, +// #205) rather than a bespoke recursive copy, and skips exactly what the canonical +// hasher excludes (.git/, signature wrappers) so the materialized copy hash-agrees +// with the source. +func (in *Installer) materializeLocalCopy(abs, finalPath string) error { if _, statErr := os.Stat(finalPath); statErr == nil { return nil } @@ -1037,9 +1111,10 @@ func materializeLocalCopy(abs, finalPath string) error { if err := os.MkdirAll(filepath.Dir(finalPath), 0o755); err != nil { return fmt.Errorf("create worktrees dir: %w", err) } - if err := copyDir(abs, stagingPath); err != nil { + mat := &Materializer{Blob: in.Blob} + if err := mat.MaterializeFromDisk(abs, stagingPath); err != nil { _ = os.RemoveAll(stagingPath) - return fmt.Errorf("copy local skill: %w", err) + return fmt.Errorf("materialize local skill: %w", err) } if err := os.Rename(stagingPath, finalPath); err != nil { // Lost the race to a concurrent install of the same content: the diff --git a/internal/skill/linker.go b/internal/skill/linker.go index 37d16f2..b9d5451 100644 --- a/internal/skill/linker.go +++ b/internal/skill/linker.go @@ -149,6 +149,15 @@ func EffectiveTarget(entry *model.LockEntry, projectRoot string) string { } return entry.EditPath } + // Vendored skills, like edit installs, live as a real in-repo directory at + // VendorPath rather than a store worktree — the canonical agent dir IS the + // content. Resolve project-relative paths against projectRoot. + if entry.IsVendor() && entry.VendorPath != "" { + if projectRoot != "" && !filepath.IsAbs(entry.VendorPath) { + return filepath.Join(projectRoot, entry.VendorPath) + } + return entry.VendorPath + } if entry.IsLink() { return entry.Source } diff --git a/internal/skill/materialize.go b/internal/skill/materialize.go index 1f2d596..f9121e1 100644 --- a/internal/skill/materialize.go +++ b/internal/skill/materialize.go @@ -13,6 +13,7 @@ import ( "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/astra-sh/qvr/internal/canonical" "github.com/astra-sh/qvr/internal/model" ) @@ -110,6 +111,89 @@ func (m *Materializer) MaterializeSubtree(repoPath, commitish, subpath string, r return hash.String(), nil } +// MaterializeFromDisk copies a skill's content from a local source directory +// into dest, mirroring MaterializeSubtree's on-disk layout and file-mode rules +// so canonical.HashSubtreeFromDisk over dest equals the same hash over srcRoot. +// It walks the filesystem (not git objects) and skips exactly what the canonical +// hasher excludes (.git/, signature wrappers — canonical.IsExcluded), so a +// plain-folder `qvr add --local` materializes through the SAME BlobMaterializer +// seam (reflink/content-store, #205) as a registry install rather than a bespoke +// recursive copy. +// +// Symlinks are recreated as links with the same escaping-target refusal as +// writeFile; regular/executable files preserve their exec bit. Empty dirs are +// not created (git doesn't track them and the disk hasher ignores them). +func (m *Materializer) MaterializeFromDisk(srcRoot, dest string) error { + info, err := os.Stat(srcRoot) + if err != nil { + return fmt.Errorf("stat local source: %w", err) + } + if !info.IsDir() { + return fmt.Errorf("local source is not a directory: %s", srcRoot) + } + return filepath.WalkDir(srcRoot, func(path string, d os.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + rel, relErr := filepath.Rel(srcRoot, path) + if relErr != nil { + return fmt.Errorf("relativize %s: %w", path, relErr) + } + rel = filepath.ToSlash(rel) + if d.IsDir() { + // Don't descend into excluded dirs (.git/ above all) — both saves + // the walk and matches the hasher, which never counts their files. + if rel != "." && canonical.IsExcluded(rel) { + return filepath.SkipDir + } + return nil + } + if canonical.IsExcluded(rel) { + return nil + } + fi, statErr := d.Info() + if statErr != nil { + return fmt.Errorf("stat %s: %w", rel, statErr) + } + return m.writeDiskEntry(path, filepath.Join(dest, filepath.FromSlash(rel)), fi) + }) +} + +// writeDiskEntry materializes one filesystem entry at destAbs, preserving the +// git-relevant mode (symlink / executable / regular) so the disk hash agrees. +// Regular files flow through the BlobMaterializer seam (reflink) when configured. +func (m *Materializer) writeDiskEntry(srcPath, destAbs string, fi os.FileInfo) error { + if err := os.MkdirAll(filepath.Dir(destAbs), 0o755); err != nil { + return fmt.Errorf("create dir for %s: %w", destAbs, err) + } + if fi.Mode()&os.ModeSymlink != 0 { + target, err := os.Readlink(srcPath) + if err != nil { + return fmt.Errorf("read symlink %s: %w", srcPath, err) + } + if t := filepath.ToSlash(target); filepath.IsAbs(target) || strings.HasPrefix(filepath.ToSlash(filepath.Clean(target)), "../") { + return fmt.Errorf("refusing symlink %s with escaping target %q", srcPath, t) + } + return os.Symlink(target, destAbs) + } + mode := os.FileMode(0o644) + if fi.Mode().Perm()&0o111 != 0 { + mode = 0o755 + } + read := func() (io.ReadCloser, error) { return os.Open(srcPath) } + if m.Blob != nil { + if err := m.Blob.WriteBlob(destAbs, mode, read); err != nil { + return err + } + } else if err := copyBlobToFile(destAbs, mode, read); err != nil { + return err + } + if mode == 0o755 { + return os.Chmod(destAbs, 0o755) + } + return nil +} + // resolveCommitish turns a SHA or ref label into a commit hash. A full 40-hex // string is taken verbatim (the common, already-resolved case); anything else // is resolved through go-git's revision resolver, which handles branches, tags, diff --git a/internal/skill/reconciler.go b/internal/skill/reconciler.go index a2d4b4e..cdb947a 100644 --- a/internal/skill/reconciler.go +++ b/internal/skill/reconciler.go @@ -106,22 +106,27 @@ func (r *Reconciler) restoreFromLock(lock *model.LockFile, projectRoot string, g continue } if entry.IsEdit() { - // Edit-mode entries live at EditPath inside the project, not in - // the shared cache. The shared worktree is no longer load-bearing, - // so we don't try to (re-)install — just verify the edit dir is - // present and refresh sibling symlinks. If EditPath is missing - // after e.g. a teammate cloned the repo without the dir, - // surface a clear error rather than silently overwriting with - // shared-mode contents. - editDir := EffectiveTarget(entry, projectRoot) - if _, err := os.Stat(editDir); err != nil { - res.Errors = append(res.Errors, fmt.Sprintf("%s: edit dir %s missing — re-run `qvr edit %s` to re-materialize from %s@%s", - entry.Name, editDir, entry.Name, entry.UpstreamSource(), shortHashOrFull(entry.InstallCommit))) - continue - } - if err := r.fixSymlinks(entry, projectRoot, global, opts, res); err != nil { - res.Errors = append(res.Errors, fmt.Sprintf("%s: %v", entry.Name, err)) - } + // Edit-mode entries live at EditPath inside the project, not in the + // shared cache. The shared worktree is no longer load-bearing, so we + // don't (re-)install — just verify the dir is present and refresh + // sibling symlinks. + r.reconcileInRepoEntry(entry, projectRoot, global, opts, res, func(dir string) string { + return fmt.Sprintf("%s: edit dir %s missing — re-run `qvr edit %s` to re-materialize from %s@%s", + entry.Name, dir, entry.Name, entry.UpstreamSource(), shortHashOrFull(entry.InstallCommit)) + }) + continue + } + + if entry.IsVendor() { + // Vendored entries live as real files committed in the repo at + // VendorPath — the source of truth is the project tree, not the + // store. A teammate gets them with the clone; a missing dir means + // the checkout dropped them, so surface a clear error rather than + // silently overwriting. + r.reconcileInRepoEntry(entry, projectRoot, global, opts, res, func(dir string) string { + return fmt.Sprintf("%s: vendored dir %s missing — it is tracked by the repo; restore it from version control or re-run `qvr add --vendor`", + entry.Name, dir) + }) continue } @@ -149,6 +154,22 @@ func (r *Reconciler) restoreFromLock(lock *model.LockFile, projectRoot string, g return nil } +// reconcileInRepoEntry handles entries whose content lives as a real directory +// inside the project (edit + vendor modes): the store worktree is not load- +// bearing, so reconcile only verifies the in-repo dir exists and refreshes +// sibling symlinks — never a re-install. missingMsg builds the error when the +// dir is absent (each mode points the user at its own repair path). +func (r *Reconciler) reconcileInRepoEntry(entry *model.LockEntry, projectRoot string, global bool, opts ReconcileOptions, res *ReconcileResult, missingMsg func(dir string) string) { + dir := EffectiveTarget(entry, projectRoot) + if _, err := os.Stat(dir); err != nil { + res.Errors = append(res.Errors, missingMsg(dir)) + return + } + if err := r.fixSymlinks(entry, projectRoot, global, opts, res); err != nil { + res.Errors = append(res.Errors, fmt.Sprintf("%s: %v", entry.Name, err)) + } +} + // restoreShared restores a shared (registry) entry whose worktree is missing, // re-installing it pinned to the lock's recorded commit (the uv reproducibility // contract). Returns the entry to link (refreshed from the lock after a real @@ -245,22 +266,15 @@ func (r *Reconciler) restoreLocal(entry *model.LockEntry, projectRoot string, gl if worktreePath == "" { return fmt.Errorf("cannot derive worktree path for local copy — re-run `qvr add --local %s`", entry.Source) } - staging := fmt.Sprintf("%s.staging.%d", worktreePath, os.Getpid()) - _ = os.RemoveAll(staging) - if err := os.MkdirAll(filepath.Dir(worktreePath), 0o755); err != nil { - return fmt.Errorf("create worktrees dir: %w", err) - } - if err := copyDir(entry.Source, staging); err != nil { - _ = os.RemoveAll(staging) - return fmt.Errorf("re-copy local skill from %s: %w", entry.Source, err) - } - if err := os.Rename(staging, worktreePath); err != nil { - if _, e := os.Stat(worktreePath); e == nil { - _ = os.RemoveAll(staging) - } else { - _ = os.RemoveAll(staging) - return fmt.Errorf("finalize local worktree: %w", err) - } + if r.Installer == nil { + return fmt.Errorf("cannot re-materialize local copy %s: no installer configured", entry.Name) + } + // Re-materialize through the same seam as the initial install + // (MaterializeFromDisk → reflink), not a bespoke copy — the installer's + // staging + atomic-rename handles the missing-worktree case we just + // confirmed. + if err := r.Installer.materializeLocalCopy(entry.Source, worktreePath); err != nil { + return fmt.Errorf("re-materialize local skill from %s: %w", entry.Source, err) } setSubtreeReadOnly(worktreePath) res.Installed = append(res.Installed, entry.Name) @@ -290,11 +304,12 @@ func (r *Reconciler) fixSymlinks(entry *model.LockEntry, projectRoot string, glo return nil } } - // For edit-mode entries the canonical EditPath is itself a real directory - // that must not be replaced with a self-referential symlink. Identify the - // canonical absolute path so we can skip it in the per-target loop. + // For edit- and vendor-mode entries the canonical EditPath/VendorPath is + // itself a real directory that must not be replaced with a self-referential + // symlink. Identify the canonical absolute path so we can skip it in the + // per-target loop (siblings still get repointed at it). var canonicalAbs string - if entry.IsEdit() { + if entry.IsEdit() || entry.IsVendor() { if abs, err := filepath.Abs(target); err == nil { canonicalAbs = normalize(abs) } diff --git a/internal/skill/vendor_test.go b/internal/skill/vendor_test.go new file mode 100644 index 0000000..de9ebe7 --- /dev/null +++ b/internal/skill/vendor_test.go @@ -0,0 +1,248 @@ +package skill_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/astra-sh/qvr/internal/canonical" + "github.com/astra-sh/qvr/internal/model" + "github.com/astra-sh/qvr/internal/registry" + "github.com/astra-sh/qvr/internal/skill" +) + +// assertRealDirWithSkill asserts path is a real directory (not a symlink) +// holding a SKILL.md — the vendoring contract for the canonical target. +func assertRealDirWithSkill(t *testing.T, path string) { + t.Helper() + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("lstat %s: %v", path, err) + } + if info.Mode()&os.ModeSymlink != 0 { + t.Fatalf("%s is a symlink; vendoring must produce a real directory", path) + } + if !info.IsDir() { + t.Fatalf("%s is not a directory", path) + } + if _, err := os.Stat(filepath.Join(path, "SKILL.md")); err != nil { + t.Errorf("vendored dir missing SKILL.md: %v", err) + } +} + +// TestInstallLocal_Vendor vendors a local folder: the canonical target becomes a +// real in-repo directory (not a store symlink), the lock entry is mode:vendor +// with VendorPath set, and the bytes are a snapshot independent of the source. +func TestInstallLocal_Vendor(t *testing.T) { + h := newHarness(t) + src := writeLocalSkill(t, "my-skill", "local dev skill") + + result, err := h.installer.InstallLocal(src, skill.InstallRequest{ + Targets: []string{"claude"}, + ProjectRoot: h.project, + Vendor: true, + }) + if err != nil { + t.Fatalf("InstallLocal --vendor: %v", err) + } + + vendorDir := filepath.Join(h.project, ".claude/skills/my-skill") + assertRealDirWithSkill(t, vendorDir) + if result.Worktree != vendorDir { + t.Errorf("result.Worktree = %s, want vendored dir %s", result.Worktree, vendorDir) + } + + lock, err := model.ReadLockFile(filepath.Join(h.project, model.LockFileName)) + if err != nil { + t.Fatalf("read lock: %v", err) + } + entry, err := lock.Get("my-skill") + if err != nil { + t.Fatalf("lock get: %v", err) + } + if !entry.IsVendor() { + t.Errorf("entry.IsVendor() = false; mode = %q", entry.Mode) + } + if entry.VendorPath != ".claude/skills/my-skill" { + t.Errorf("VendorPath = %q, want .claude/skills/my-skill", entry.VendorPath) + } + if entry.SubtreeHash == "" { + t.Error("vendored entry has empty SubtreeHash") + } + + // Snapshot, not a live link: editing the source must not change the vendored + // copy (its bytes were materialized into the repo). + if err := os.WriteFile(filepath.Join(src, "SKILL.md"), []byte("mutated"), 0o644); err != nil { + t.Fatalf("rewrite source: %v", err) + } + got, _ := os.ReadFile(filepath.Join(vendorDir, "SKILL.md")) + if string(got) == "mutated" { + t.Error("vendored copy changed when source was edited — not a snapshot") + } +} + +// TestInstall_VendorRegistry vendors a registry skill: real in-repo dir with the +// skill at its root, mode:vendor, and provenance preserved from the install. +func TestInstall_VendorRegistry(t *testing.T) { + h := newHarness(t) + remote := seedRemote(t, map[string]string{"code-review": codeReviewSkill}) + h.addRegistry(t, "acme", remote) + + if _, err := h.installer.Install(skill.InstallRequest{ + Skill: "code-review", + Targets: []string{"claude"}, + ProjectRoot: h.project, + Vendor: true, + }); err != nil { + t.Fatalf("Install --vendor: %v", err) + } + + vendorDir := filepath.Join(h.project, ".claude/skills/code-review") + assertRealDirWithSkill(t, vendorDir) + + lock, _ := model.ReadLockFile(filepath.Join(h.project, model.LockFileName)) + entry, err := lock.Get("code-review") + if err != nil { + t.Fatalf("lock get: %v", err) + } + if !entry.IsVendor() { + t.Errorf("entry.IsVendor() = false; mode = %q", entry.Mode) + } + if entry.AuthorIdentity() != "Test " { + t.Errorf("vendored entry lost provenance: commitAuthor = %q", entry.AuthorIdentity()) + } + // The re-sealed hash must match a fresh recomputation over the in-repo dir. + got, err := canonical.HashSubtreeFromDisk(vendorDir) + if err != nil { + t.Fatalf("hash vendored dir: %v", err) + } + if got != entry.SubtreeHash { + t.Errorf("SubtreeHash %q != disk hash %q", entry.SubtreeHash, got) + } +} + +// TestVendor_MultiTargetSiblingSymlinks: the alphabetical-first target is the +// real dir; other targets are RELATIVE symlinks to it (so the layout survives a +// git clone to a different absolute path). +func TestVendor_MultiTargetSiblingSymlinks(t *testing.T) { + h := newHarness(t) + src := writeLocalSkill(t, "my-skill", "local dev skill") + + if _, err := h.installer.InstallLocal(src, skill.InstallRequest{ + Targets: []string{"claude", "cursor"}, + ProjectRoot: h.project, + Vendor: true, + }); err != nil { + t.Fatalf("InstallLocal --vendor multi-target: %v", err) + } + + // claude sorts first → canonical real dir. + assertRealDirWithSkill(t, filepath.Join(h.project, ".claude/skills/my-skill")) + + // cursor (.agents/skills) → relative symlink to the canonical. + sibling := filepath.Join(h.project, ".agents/skills/my-skill") + info, err := os.Lstat(sibling) + if err != nil { + t.Fatalf("lstat sibling: %v", err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("sibling %s should be a symlink", sibling) + } + target, _ := os.Readlink(sibling) + if filepath.IsAbs(target) { + t.Errorf("sibling symlink target %q is absolute; want relative for clone-portability", target) + } + // It must resolve to the canonical real dir. + if _, err := os.Stat(sibling); err != nil { + t.Errorf("sibling symlink does not resolve: %v", err) + } +} + +// TestVendor_TravelsWithoutStore proves the whole point of vendoring: with the +// store wiped (as on a teammate's fresh clone), the skill still reads and +// `qvr sync` reconciles without error. +func TestVendor_TravelsWithoutStore(t *testing.T) { + h := newHarness(t) + src := writeLocalSkill(t, "my-skill", "local dev skill") + + if _, err := h.installer.InstallLocal(src, skill.InstallRequest{ + Targets: []string{"claude"}, + ProjectRoot: h.project, + Vendor: true, + }); err != nil { + t.Fatalf("InstallLocal --vendor: %v", err) + } + + // Nuke the entire store — a teammate cloning the repo has none of it. + if err := os.RemoveAll(registry.WorktreesRoot()); err != nil { + t.Fatalf("wipe store: %v", err) + } + + vendorDir := filepath.Join(h.project, ".claude/skills/my-skill") + assertRealDirWithSkill(t, vendorDir) // still there — it lives in the repo + + lock, _ := model.ReadLockFile(filepath.Join(h.project, model.LockFileName)) + rec := skill.NewReconciler(h.installer) + res, err := rec.Reconcile(lock, h.project, h.home, skill.ReconcileOptions{}) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(res.Errors) != 0 { + t.Errorf("reconcile reported errors with store gone: %v", res.Errors) + } + assertRealDirWithSkill(t, vendorDir) +} + +// TestVendor_Remove tears a vendored skill down without --force: the in-repo dir, +// symlinks, and lock entry all go away (vendored files are reproducible). +func TestVendor_Remove(t *testing.T) { + h := newHarness(t) + src := writeLocalSkill(t, "my-skill", "local dev skill") + + if _, err := h.installer.InstallLocal(src, skill.InstallRequest{ + Targets: []string{"claude", "cursor"}, + ProjectRoot: h.project, + Vendor: true, + }); err != nil { + t.Fatalf("InstallLocal --vendor: %v", err) + } + + vendorDir := filepath.Join(h.project, ".claude/skills/my-skill") + sibling := filepath.Join(h.project, ".agents/skills/my-skill") + + if err := h.installer.Remove("my-skill", skill.InstallRequest{ProjectRoot: h.project}); err != nil { + t.Fatalf("Remove vendored (no --force): %v", err) + } + if _, err := os.Lstat(vendorDir); !os.IsNotExist(err) { + t.Errorf("vendored dir still present after remove: %v", err) + } + if _, err := os.Lstat(sibling); !os.IsNotExist(err) { + t.Errorf("sibling symlink still present after remove: %v", err) + } + lock, _ := model.ReadLockFile(filepath.Join(h.project, model.LockFileName)) + if _, err := lock.Get("my-skill"); err == nil { + t.Error("lock entry still present after remove") + } +} + +// TestEdit_RefusesVendored: a vendored skill is already editable real files, so +// `qvr edit` (EjectToTarget) refuses it rather than producing a confusing +// clobber error. +func TestEdit_RefusesVendored(t *testing.T) { + h := newHarness(t) + src := writeLocalSkill(t, "my-skill", "local dev skill") + + if _, err := h.installer.InstallLocal(src, skill.InstallRequest{ + Targets: []string{"claude"}, + ProjectRoot: h.project, + Vendor: true, + }); err != nil { + t.Fatalf("InstallLocal --vendor: %v", err) + } + + lock, _ := model.ReadLockFile(filepath.Join(h.project, model.LockFileName)) + entry, _ := lock.Get("my-skill") + if _, err := skill.EjectToTarget(skill.EjectRequest{Entry: entry, ProjectRoot: h.project}); err == nil { + t.Fatal("EjectToTarget on a vendored entry should error") + } +} From bb21393c2d0b566a8500b4ed1471423eba56234e Mon Sep 17 00:00:00 2001 From: Rakshith Date: Sat, 13 Jun 2026 17:09:34 -0400 Subject: [PATCH 2/5] fix(skill): roll back symlinks on vendor failure; reject bare ".." symlink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/skill/installer.go | 14 ++++++++------ internal/skill/materialize.go | 8 ++++++-- internal/skill/vendor_test.go | 24 ++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/internal/skill/installer.go b/internal/skill/installer.go index 12bfbb8..9b3c516 100644 --- a/internal/skill/installer.go +++ b/internal/skill/installer.go @@ -289,7 +289,8 @@ func (in *Installer) InstallInto(req InstallRequest, lock *model.LockFile) (*Ins setSubtreeReadOnly(skillDir) } - if err := in.linkInstallTargets(req, plan, skillDir); err != nil { + created, err := in.linkInstallTargets(req, plan, skillDir) + if err != nil { return nil, err } @@ -309,6 +310,7 @@ func (in *Installer) InstallInto(req InstallRequest, lock *model.LockFile) (*Ins // addressed) until `qvr cache prune` reclaims it as an orphan. if req.Vendor { if _, verr := VendorIntoRepo(VendorRequest{Entry: entry, ProjectRoot: req.ProjectRoot, Global: req.Global}); verr != nil { + rollbackLinks(created) return nil, fmt.Errorf("vendor %s: %w", localName, verr) } worktree = EffectiveTarget(entry, req.ProjectRoot) @@ -484,7 +486,7 @@ func (in *Installer) gateInstallTrust(req InstallRequest, plan *installPlan, ski // a sanitized agent view for a legacy root-layout worktree with a live .git/ — // issue #154) and creates a symlink for every requested target, rolling back any // links created so far if one fails. -func (in *Installer) linkInstallTargets(req InstallRequest, plan *installPlan, skillDir string) error { +func (in *Installer) linkInstallTargets(req InstallRequest, plan *installPlan, skillDir string) ([]string, error) { loc := plan.loc finalPath := plan.finalPath localName := plan.localName @@ -499,7 +501,7 @@ func (in *Installer) linkInstallTargets(req InstallRequest, plan *installPlan, s if IsRootLayoutPath(loc.Entry.Path) && HasGitDir(finalPath) { view, verr := buildAgentViewAt(finalPath) if verr != nil { - return fmt.Errorf("agent view: %w", verr) + return nil, fmt.Errorf("agent view: %w", verr) } linkTarget = view } @@ -511,15 +513,15 @@ func (in *Installer) linkInstallTargets(req InstallRequest, plan *installPlan, s linkPath, err := ResolveTargetPath(t, localName, req.ProjectRoot, req.Global) if err != nil { rollbackLinks(created) - return err + return nil, err } if err := CreateSymlink(linkPath, linkTarget); err != nil { rollbackLinks(created) - return fmt.Errorf("symlink %s: %w", t, err) + return nil, fmt.Errorf("symlink %s: %w", t, err) } created = append(created, linkPath) } - return nil + return created, nil } // buildLockEntry assembles the in-memory lock entry for the install: it merges diff --git a/internal/skill/materialize.go b/internal/skill/materialize.go index f9121e1..d723723 100644 --- a/internal/skill/materialize.go +++ b/internal/skill/materialize.go @@ -171,8 +171,12 @@ func (m *Materializer) writeDiskEntry(srcPath, destAbs string, fi os.FileInfo) e if err != nil { return fmt.Errorf("read symlink %s: %w", srcPath, err) } - if t := filepath.ToSlash(target); filepath.IsAbs(target) || strings.HasPrefix(filepath.ToSlash(filepath.Clean(target)), "../") { - return fmt.Errorf("refusing symlink %s with escaping target %q", srcPath, t) + // Reject any target that escapes the skill dir: absolute, a bare ".." + // (cleans to ".." with no trailing slash), or anything that cleans to a + // "../"-prefixed path. + cleaned := filepath.ToSlash(filepath.Clean(target)) + if filepath.IsAbs(target) || cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return fmt.Errorf("refusing symlink %s with escaping target %q", srcPath, filepath.ToSlash(target)) } return os.Symlink(target, destAbs) } diff --git a/internal/skill/vendor_test.go b/internal/skill/vendor_test.go index de9ebe7..b8a0db3 100644 --- a/internal/skill/vendor_test.go +++ b/internal/skill/vendor_test.go @@ -3,6 +3,7 @@ package skill_test import ( "os" "path/filepath" + "strings" "testing" "github.com/astra-sh/qvr/internal/canonical" @@ -246,3 +247,26 @@ func TestEdit_RefusesVendored(t *testing.T) { t.Fatal("EjectToTarget on a vendored entry should error") } } + +// TestMaterializeFromDisk_RefusesEscapingSymlink guards the --local disk +// materialization escape check, including a BARE ".." target (which cleans to +// ".." with no "../" prefix and previously slipped through). +func TestMaterializeFromDisk_RefusesEscapingSymlink(t *testing.T) { + for _, target := range []string{"..", "../outside", "/etc/passwd"} { + t.Run(target, func(t *testing.T) { + src := t.TempDir() + if err := os.WriteFile(filepath.Join(src, "SKILL.md"), + []byte("---\nname: x\ndescription: y\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(src, "escape")); err != nil { + t.Fatal(err) + } + dest := filepath.Join(t.TempDir(), "out") + err := (&skill.Materializer{}).MaterializeFromDisk(src, dest) + if err == nil || !strings.Contains(err.Error(), "escaping target") { + t.Errorf("target %q: want escaping-target rejection, got %v", target, err) + } + }) + } +} From 2472958feb705e27fb0cbfa5ce807496d80fb42b Mon Sep 17 00:00:00 2001 From: Rakshith Date: Sat, 13 Jun 2026 17:18:11 -0400 Subject: [PATCH 3/5] fix(skill): remove canonical dir if sibling relink fails during vendor/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). --- internal/skill/eject.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/skill/eject.go b/internal/skill/eject.go index a53a774..ffcfde1 100644 --- a/internal/skill/eject.go +++ b/internal/skill/eject.go @@ -125,6 +125,12 @@ func EjectToTarget(req EjectRequest) (*EjectResult, error) { siblingLinks, err := repointSiblingTargets(e, req, canonicalTarget, canonicalAbs) if err != nil { + // materializeEjectDir already created the canonical real directory; a + // sibling-relink failure must not leave it behind. rollbackLinks (in the + // caller) only removes symlinks, so the real dir would orphan and block + // future installs of this skill. Remove it here to keep eject/vendor + // atomic — the caller still rolls back the target symlinks. + _ = os.RemoveAll(canonicalAbs) return nil, err } From a025467b0521df8002f5b2af4dd5f3b1f87c06a1 Mon Sep 17 00:00:00 2001 From: Rakshith Date: Sat, 13 Jun 2026 17:25:08 -0400 Subject: [PATCH 4/5] fix(skill): VendorIntoRepo also removes canonical dir on relink failure 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. --- internal/skill/eject.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/skill/eject.go b/internal/skill/eject.go index ffcfde1..1a509ae 100644 --- a/internal/skill/eject.go +++ b/internal/skill/eject.go @@ -296,6 +296,12 @@ func VendorIntoRepo(req VendorRequest) (*VendorResult, error) { siblingLinks, err := repointSiblingTargets(e, scope, canonicalTarget, canonicalAbs) if err != nil { + // copyTreeToCanonical already materialized the canonical real directory; + // a sibling-relink failure must not leave it behind. The caller's + // rollbackLinks only removes symlinks, so the real dir would orphan and + // block future installs of this skill at that path. Remove it so vendor + // stays atomic (matching EjectToTarget). + _ = os.RemoveAll(canonicalAbs) return nil, err } From f945c8df01ddcf1ab29e3939a942dd0ca1dee1a5 Mon Sep 17 00:00:00 2001 From: Rakshith Date: Sat, 13 Jun 2026 17:35:33 -0400 Subject: [PATCH 5/5] fix(skill): clean up vendored dir when lock.Write fails 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.) --- internal/skill/installer.go | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/skill/installer.go b/internal/skill/installer.go index 9b3c516..d3dcad5 100644 --- a/internal/skill/installer.go +++ b/internal/skill/installer.go @@ -215,11 +215,31 @@ func (in *Installer) Install(req InstallRequest) (*InstallResult, error) { return nil, err } if err := lock.Write(); err != nil { + in.rollbackInstallResult(result, req) return nil, fmt.Errorf("write lock file: %w", err) } return result, nil } +// rollbackInstallResult undoes a completed InstallInto when the single-install +// caller's lock.Write fails: it removes the target symlinks and, for a vendored +// install, the real canonical directory VendorIntoRepo materialized (a plain +// symlink rollback can't, since it's a real dir — a retry would otherwise hit +// "exists and is not a symlink"). Best-effort. +func (in *Installer) rollbackInstallResult(result *InstallResult, req InstallRequest) { + if result == nil { + return + } + for _, t := range result.Targets { + if p, perr := ResolveTargetPath(t, result.Name, req.ProjectRoot, req.Global); perr == nil { + _ = RemoveSymlink(p) + } + } + if req.Vendor && result.Worktree != "" { + _ = os.RemoveAll(result.Worktree) + } +} + // InstallInto runs the full install flow against an already-loaded, in-memory // lock instead of reading and writing the lockfile itself. The caller owns the // lock's lifecycle: it must hold the project file lock (model.WithLock), pass a @@ -1044,7 +1064,7 @@ func (in *Installer) InstallLocal(localPath string, req InstallRequest) (*Instal } if err := lock.Write(); err != nil { - rollbackLinks(created) + rollbackVendoredInstall(created, req.Vendor, worktree) return nil, fmt.Errorf("write lock file: %w", err) } return &InstallResult{ @@ -1158,6 +1178,17 @@ func rollbackLinks(paths []string) { } } +// rollbackVendoredInstall removes the created target symlinks and, for a +// vendored install, the real canonical directory VendorIntoRepo materialized — +// which rollbackLinks alone can't, since it's a real dir (a retry would +// otherwise hit "exists and is not a symlink"). Best-effort. +func rollbackVendoredInstall(created []string, vendor bool, vendorDir string) { + rollbackLinks(created) + if vendor && vendorDir != "" { + _ = os.RemoveAll(vendorDir) + } +} + func mergeTargets(existing, add []string) []string { set := make(map[string]struct{}) for _, t := range existing {