From c8351fdd78bbe92f50af28d9848e5927798fcf13 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Tue, 6 Oct 2026 09:42:30 +0900 Subject: [PATCH 1/2] feat(sjskills): support local-path sources in project [[direct]] entries A project that develops its own skill in-repo can now declare it with `source = "./skills/"` (or `../`, `/abs`, `C:/abs`). Relative paths resolve against the directory containing sjskills.toml, not the process working directory, so plan/apply behave the same from any subdirectory. Design: - Resolution records the absolute LocalPath on the desired skill while Source keeps the committed spelling. Provenance uses `local:` plus the tree hash, so recloning or moving the project keeps ownership and editing the source yields an ordinary verified update rather than local-modification. - The materializer copies the local tree into the same staging layout a Skills CLI fetch produces, after any remote fetch, and only preflights bunx when a remote source is selected. Placement, quarantine, rollback, and restore therefore need no separate path. The staged copy must hash equal to the inspected source, so mid-copy edits are reported, not adopted. - Local sources must be real directories holding only regular files and directories, with a SKILL.md whose frontmatter name matches the entry. Sources inside generated .agents/skills, .claude/skills, or .sjskills directories, project roots, and parent-only paths are rejected. Paths are forward-slash only; `~`, backslashes, access, and full_depth are rejected. Absolute or project-escaping paths work but warn as machine-specific. - A missing or invalid source fails plan/apply as invalid input (exit 65) with an actionable message instead of looking like an outage. - Status never caches local hashes: the cache key and refresh cover only remote skills and local sources are hashed on every check. - The registry and fixed global baseline stay remote-only, and global provenance never trusts a `local:` identity. ResolveProject now takes the project root explicitly. Provenance with `local:` identities requires a build with this change. Closes #28 Co-Authored-By: Claude Opus 5.5 --- cmd/sjskills/local_source_test.go | 102 +++++ cmd/sjskills/main.go | 14 + docs/sjskills-releases.md | 3 +- docs/skill-registry.md | 26 +- internal/sjskills/access_test.go | 8 +- internal/sjskills/classify.go | 9 +- internal/sjskills/contracts_test.go | 14 +- internal/sjskills/global_inventory.go | 4 +- internal/sjskills/global_plan.go | 2 +- internal/sjskills/local_source.go | 418 ++++++++++++++++++ internal/sjskills/local_source_test.go | 341 ++++++++++++++ internal/sjskills/materialize.go | 53 ++- internal/sjskills/resolve.go | 24 +- internal/sjskills/status.go | 67 ++- internal/sjskills/status_cache.go | 2 +- .../sjskills/testdata/invalid/source.toml | 2 +- internal/sjskills/types.go | 20 +- internal/sjskills/validate.go | 20 +- skills/sjskills/SKILL.md | 4 +- .../sjskills/references/project-selection.md | 17 +- 20 files changed, 1095 insertions(+), 55 deletions(-) create mode 100644 cmd/sjskills/local_source_test.go create mode 100644 internal/sjskills/local_source.go create mode 100644 internal/sjskills/local_source_test.go diff --git a/cmd/sjskills/local_source_test.go b/cmd/sjskills/local_source_test.go new file mode 100644 index 0000000..3ebb22b --- /dev/null +++ b/cmd/sjskills/local_source_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeLocalFixtureSkill(t *testing.T, path, name, body string) { + t.Helper() + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(path, "SKILL.md"), []byte("---\nname: "+name+"\ndescription: Fixture\n---\n"+body), 0o644); err != nil { + t.Fatal(err) + } +} + +func localPlanActions(t *testing.T, directory string, overrides map[string]string) (map[string]string, sjskillsEnvelope) { + t.Helper() + code, stdout, stderr := runCLIWithEnvironment(t, directory, overrides, "--json", "plan") + envelope := decodeEnvelope(t, stdout) + if code != 0 { + return nil, envelope + } + if stderr != "" { + t.Fatalf("plan stderr=%q", stderr) + } + actions := map[string]string{} + for _, operation := range envelope.Plan.Operations { + actions[operation.Skill+"@"+operation.Target] = operation.Action + "/" + operation.Reason + } + return actions, envelope +} + +func TestExternalLocalDirectSourceLifecycle(t *testing.T) { + project := t.TempDir() + source := filepath.Join(project, "skills", "team-tool") + writeLocalFixtureSkill(t, source, "team-tool", "# v1\n") + if err := os.WriteFile(filepath.Join(project, "sjskills.toml"), []byte("version = 1\n\n[[direct]]\nname = \"team-tool\"\nsource = \"./skills/team-tool\"\n"), 0o644); err != nil { + t.Fatal(err) + } + overrides := isolatedExternalHomes(t) + // Run from a nested directory: relative sources resolve against the + // manifest, never the process working directory. + directory := filepath.Join(project, "skills") + + expectActions := func(want string) { + t.Helper() + actions, envelope := localPlanActions(t, directory, overrides) + if actions == nil { + t.Fatalf("plan failed: %#v", envelope.Error) + } + if len(actions) != 2 || actions["team-tool@.agents"] != want || actions["team-tool@.claude"] != want { + t.Fatalf("plan actions = %#v, want %s for both targets", actions, want) + } + } + apply := func() { + t.Helper() + if code, stdout, stderr := runCLIWithEnvironment(t, directory, overrides, "--json", "apply", "--yes"); code != 0 { + t.Fatalf("apply code=%d stdout=%s stderr=%s", code, stdout, stderr) + } + } + + expectActions("install/expected-entry-absent") + apply() + expectActions("unchanged/verified-exact") + for _, target := range []string{".agents", ".claude"} { + data, err := os.ReadFile(filepath.Join(project, target, "skills", "team-tool", "SKILL.md")) + if err != nil || !strings.HasSuffix(string(data), "# v1\n") { + t.Fatalf("%s placement = %q err=%v", target, data, err) + } + } + var state struct { + Records []struct { + SourceIdentity string `json:"sourceIdentity"` + } `json:"records"` + } + data, err := os.ReadFile(filepath.Join(project, ".sjskills", "state.json")) + if err != nil || json.Unmarshal(data, &state) != nil || len(state.Records) != 2 || state.Records[0].SourceIdentity != "local:./skills/team-tool" { + t.Fatalf("provenance = %s err=%v", data, err) + } + + writeLocalFixtureSkill(t, source, "team-tool", "# v2\n") + expectActions("update/verified-update") + apply() + expectActions("unchanged/verified-exact") + + if err := os.RemoveAll(source); err != nil { + t.Fatal(err) + } + actions, envelope := localPlanActions(t, directory, overrides) + if actions != nil || envelope.Result != "invalid" || envelope.Error == nil || envelope.Error.Code != "invalid_source" || + !strings.Contains(envelope.Error.Message, `local source "./skills/team-tool" does not exist`) || !strings.Contains(envelope.Error.Message, "[[direct]]") { + t.Fatalf("missing source plan = %#v error=%#v", envelope, envelope.Error) + } + if _, err := os.Stat(filepath.Join(project, ".claude", "skills", "team-tool", "SKILL.md")); err != nil { + t.Fatalf("blocked plan changed the placement: %v", err) + } +} diff --git a/cmd/sjskills/main.go b/cmd/sjskills/main.go index 5d86e17..bb0412b 100644 --- a/cmd/sjskills/main.go +++ b/cmd/sjskills/main.go @@ -367,6 +367,7 @@ func (a *application) prepare(ctx context.Context, global bool, operation sjskil return nil, a.invalid(operation, readErr) } request.Manifest = &manifest + request.ProjectRoot = discovered.Root project = &discovered envelope.Path = discovered.ManifestPath } @@ -385,6 +386,19 @@ func (a *application) prepare(ctx context.Context, global bool, operation sjskil ctx = context.Background() } materialized, materializeErr := materialize(ctx, plan.Desired.Skills) + var localErr *sjskills.LocalSourceError + if errors.As(materializeErr, &localErr) { + // A missing or malformed local source is a manifest/working-tree + // problem, not an outage: report it as invalid input so it is fixed + // rather than retried. + if materialized != nil { + _ = a.materializationCleanup()(materialized) + } + envelope := prepared.envelope + envelope.Result = sjskills.ResultInvalid + envelope.Error = &sjskills.Issue{Code: sjskills.IssueInvalidSource, Path: "manifest.direct." + localErr.Skill + ".source", Message: localErr.Error()} + return nil, envelope + } if materializeErr != nil { if materialized != nil { prepared.materialized = materialized diff --git a/docs/sjskills-releases.md b/docs/sjskills-releases.md index 1bd1dc1..2af6a7d 100644 --- a/docs/sjskills-releases.md +++ b/docs/sjskills-releases.md @@ -28,7 +28,8 @@ The installers require no checkout, Go, or GitHub CLI. macOS uses system shell, `curl`, `tar`, and `shasum`; Windows uses Windows PowerShell 5.1 or newer and .NET. `plan` and `apply` require network access to the published registry, Bun (`bunx`) for the pinned Skills CLI, and Git, which fetches agent-scripts -skills at the registry's commit. Help and version perform no status checks. Profiles and manifest +skills at the registry's commit; a selection made only of local direct sources +needs neither. Help and version perform no status checks. Profiles and manifest initialization retain their primary behavior without Bun; their incidental notices may report unavailable upstream evidence. Bare/named `status` also succeeds with explicit unavailable skill evidence when no cached evidence or Bun diff --git a/docs/skill-registry.md b/docs/skill-registry.md index b4ffd04..e90c8a2 100644 --- a/docs/skill-registry.md +++ b/docs/skill-registry.md @@ -94,6 +94,29 @@ boundary. Skills CLI-managed sources must be Git shorthand credentials, URL query strings, npm specifiers, and other schemes are rejected for that manager. +A project manifest's `[[direct]]` entry may instead name a local skill +directory, so a project can install a skill it develops in-repo. The registry +and the fixed global baseline stay remote-only. Local sources use forward +slashes and are either relative to the directory containing `sjskills.toml` +(starting `./` or `../`) or absolute (`/path` or `C:/path`); home-relative and +backslash spellings are rejected, as are `access` other than public and +`full_depth`. Paths outside the project resolve but warn +(`machine-specific-source`), because other checkouts need the same path. +`sjskills` copies the directory into staging itself, without Skills CLI, and +then treats it like any fetched tree. Provenance records +`local:` with the tree hash, so editing the source +produces an ordinary update. The directory must be a real directory with a +`SKILL.md` whose frontmatter `name` matches the entry, contain only regular +files and directories, not lie inside a generated `.agents/skills`, +`.claude/skills`, or `.sjskills` directory, and be neither a project root nor +contain generated skill roots. A missing or invalid source makes `plan` and +`apply` fail as invalid input (exit 65) with a message naming it; status +reports the project scope unavailable with the same message. Switching an +installed skill between remote and local sources, or respelling its path, +changes its provenance identity and blocks as `provenance-source-mismatch`: +remove the entry and apply, then re-add it and apply. Provenance containing +`local:` identities requires an `sjskills` build that supports local sources. + Public selections use the pinned Skills CLI's remote-fetch path and do not invoke gh. Skills CLI cannot clone a commit, so public GitHub sources pinned to a full commit, including every agent-scripts skill, are fetched anonymously @@ -233,7 +256,8 @@ reused. “No drift detected” does not verify externally provisioned manual or workflow tools. Protected locations retain their existing ownership boundaries. Disposable complete expected-hash snapshots live under the platform user-cache -directory in `sjskills/status/`. Identity includes the sorted Skills CLI-managed +directory in `sjskills/status/`. Local direct sources are never cached; each +check hashes them afresh. Identity includes the sorted Skills CLI-managed selection's names, exact source strings, effective access, copy mode, full-depth options, and the Skills CLI, tree-hash, and cache format versions. Matching selections share evidence across roots and scopes. Placement targets, profile names, and unrelated diff --git a/internal/sjskills/access_test.go b/internal/sjskills/access_test.go index c9af383..1638e1f 100644 --- a/internal/sjskills/access_test.go +++ b/internal/sjskills/access_test.go @@ -8,7 +8,7 @@ import ( func TestAccessProfileSelectionAndPublicCompatibility(t *testing.T) { r := fixtureRegistry(t) - public, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa"}}) + public, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa"}}, "") if err != nil { t.Fatal(err) } @@ -20,7 +20,7 @@ func TestAccessProfileSelectionAndPublicCompatibility(t *testing.T) { t.Fatal(s) } } - mixed, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa", "kicpa-private"}}) + mixed, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa", "kicpa-private"}}, "") if err != nil { t.Fatal(err) } @@ -47,7 +47,7 @@ func TestAccessProfileSelectionAndPublicCompatibility(t *testing.T) { r.Profiles[name] = profile } } - omitted, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa"}}) + omitted, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa"}}, "") if err != nil { t.Fatal(err) } @@ -76,7 +76,7 @@ func TestAccessDecodingRejectsMalformedAndPreservesDirect(t *testing.T) { if err != nil { t.Fatal(err) } - desired, err := ResolveProject(fixtureRegistry(t), m) + desired, err := ResolveProject(fixtureRegistry(t), m, "") if err != nil { t.Fatal(err) } diff --git a/internal/sjskills/classify.go b/internal/sjskills/classify.go index 01b78fe..9f68d60 100644 --- a/internal/sjskills/classify.go +++ b/internal/sjskills/classify.go @@ -342,7 +342,7 @@ func validateProjectClassificationInputs(desired DesiredState, expected map[stri issues = append(issues, Issue{Code: IssueInvalidMode, Path: path + ".mode", Message: "skills-cli installation must use copy mode"}) } if _, ok := canonicalProjectSourceIdentity(skill.Source); !ok { - issues = append(issues, Issue{Code: IssueInvalidSource, Path: path + ".source", Message: "must be a credential-free remote source"}) + issues = append(issues, Issue{Code: IssueInvalidSource, Path: path + ".source", Message: "must be a credential-free remote source or local path"}) } case ManagerManual, ManagerWorkflow: case ManagerNone: @@ -485,10 +485,14 @@ func detachProjectClassification(classification ProjectClassification) ProjectCl // compare as ownership evidence. GitHub URLs intentionally use repository // identity like the legacy reconciler; shorthand catalog suffixes remain part // of the identity so two shorthand paths cannot silently claim one another. +// Local sources use their normalized manifest spelling under `local:`. func canonicalProjectSourceIdentity(source string) (string, bool) { if source == "" || strings.TrimSpace(source) != source || strings.ContainsRune(source, 0) { return "", false } + if IsLocalSource(source) { + return localSourceIdentity(source) + } if !strings.Contains(source, "://") { parts := strings.Split(source, "/") if len(parts) < 2 { @@ -579,6 +583,9 @@ func canonicalProjectSourceIdentity(source string) (string, bool) { } func isCanonicalProjectSourceIdentity(identity string) bool { + if strings.HasPrefix(identity, localSourceIdentityPrefix) { + return isCanonicalLocalSourceIdentity(identity) + } if strings.HasPrefix(identity, "github:") { rest := strings.TrimPrefix(identity, "github:") if !projectCanonicalIdentityPattern.MatchString(rest) || strings.ToLower(rest) != rest { diff --git a/internal/sjskills/contracts_test.go b/internal/sjskills/contracts_test.go index ddadc6c..8c32518 100644 --- a/internal/sjskills/contracts_test.go +++ b/internal/sjskills/contracts_test.go @@ -80,7 +80,7 @@ func TestCanonicalRegistryAndProfiles(t *testing.T) { func TestProjectResolutionDevGoAndDirect(t *testing.T) { registry := fixtureRegistry(t) manifest := fixtureManifest(t, "dev-go.toml") - state, err := ResolveProject(registry, manifest) + state, err := ResolveProject(registry, manifest, "") if err != nil { t.Fatal(err) } @@ -94,7 +94,7 @@ func TestProjectResolutionDevGoAndDirect(t *testing.T) { } manifest = fixtureManifest(t, "direct-third-party.toml") - state, err = ResolveProject(registry, manifest) + state, err = ResolveProject(registry, manifest, "") if err != nil { t.Fatal(err) } @@ -118,7 +118,7 @@ func TestCanonicalManifestIncludesDirectSourceIdentity(t *testing.T) { if len(manifest.Profiles) != 2 || len(manifest.Direct) != 1 { t.Fatalf("manifest = %#v", manifest) } - state, err := ResolveProject(fixtureRegistry(t), manifest) + state, err := ResolveProject(fixtureRegistry(t), manifest, "") if err != nil { t.Fatal(err) } @@ -161,7 +161,7 @@ func TestBuildPlanKeepsWarningsAndEvidenceAtPureBoundary(t *testing.T) { } func TestKicpaAndManagerBoundaries(t *testing.T) { - state, err := ResolveProject(fixtureRegistry(t), fixtureManifest(t, "kicpa.toml")) + state, err := ResolveProject(fixtureRegistry(t), fixtureManifest(t, "kicpa.toml"), "") if err != nil { t.Fatal(err) } @@ -190,7 +190,7 @@ func TestKicpaAndManagerBoundaries(t *testing.T) { registry.Profiles["dev"] = Profile{Skills: append(registry.Profiles["dev"].Skills, "context7-mcp")} slices.Sort(registry.Profiles["dev"].Skills) dev := fixtureManifest(t, "dev-go.toml") - state, err = ResolveProject(registry, dev) + state, err = ResolveProject(registry, dev, "") if err != nil { t.Fatal(err) } @@ -233,7 +233,7 @@ func TestManifestAndRegistryInvalidCases(t *testing.T) { if err != nil { t.Fatal(err) } - if _, err := ResolveProject(registry, manifest); err == nil || !issueCode(err, IssueCollision) { + if _, err := ResolveProject(registry, manifest, ""); err == nil || !issueCode(err, IssueCollision) { t.Fatalf("%s resolution error = %v, want collision", name, err) } } @@ -343,7 +343,7 @@ func TestSelectionScopedCollisionsAndUnselectedDirectCatalog(t *testing.T) { if err := ValidateManifest(registry, manifest); err != nil { t.Fatalf("unselected catalog direct should be allowed: %v", err) } - state, err := ResolveProject(registry, manifest) + state, err := ResolveProject(registry, manifest, "") if err != nil { t.Fatal(err) } diff --git a/internal/sjskills/global_inventory.go b/internal/sjskills/global_inventory.go index dfc7c08..85d25bf 100644 --- a/internal/sjskills/global_inventory.go +++ b/internal/sjskills/global_inventory.go @@ -305,7 +305,7 @@ func decodeGlobalProvenanceState(data []byte) (GlobalProvenanceState, GlobalProv } identity, ok := canonicalProjectSourceIdentity(record.Source) key := projectPlacementKey(target, record.Skill) - if !ok || !isPortableName(record.Skill) || record.HashAlgorithm != TreeHashAlgorithmSHA256V2 || !lowercaseDigestPattern.MatchString(record.Hash) || record.RecordedAt.IsZero() { + if !ok || IsLocalSource(record.Source) || !isPortableName(record.Skill) || record.HashAlgorithm != TreeHashAlgorithmSHA256V2 || !lowercaseDigestPattern.MatchString(record.Hash) || record.RecordedAt.IsZero() { return GlobalProvenanceState{}, "", false } if _, duplicate := seen[key]; duplicate { @@ -343,7 +343,7 @@ func validGlobalProvenanceRecords(records []ProvenanceRecord) bool { key := projectPlacementKey(record.Target, record.Skill) if record.Scope != ScopeGlobal || !isPortableName(record.Skill) || (record.Target != TargetAgents && record.Target != TargetClaude) || - !isCanonicalProjectSourceIdentity(record.SourceIdentity) || + !isCanonicalProjectSourceIdentity(record.SourceIdentity) || isCanonicalLocalSourceIdentity(record.SourceIdentity) || record.TreeHashAlgorithm != TreeHashAlgorithmSHA256V2 || !lowercaseDigestPattern.MatchString(record.TreeHash) || record.RecordedAt.IsZero() { return false diff --git a/internal/sjskills/global_plan.go b/internal/sjskills/global_plan.go index b634c10..860fe5f 100644 --- a/internal/sjskills/global_plan.go +++ b/internal/sjskills/global_plan.go @@ -126,7 +126,7 @@ func validateGlobalClassificationInputs(registry Registry, desired DesiredState, if skill.Mode != ModeCopy { issues = append(issues, Issue{Code: IssueInvalidMode, Path: path + ".mode", Message: "skills-cli installation must use copy mode"}) } - if _, ok := canonicalProjectSourceIdentity(skill.Source); !ok { + if _, ok := canonicalProjectSourceIdentity(skill.Source); !ok || IsLocalSource(skill.Source) { issues = append(issues, Issue{Code: IssueInvalidSource, Path: path + ".source", Message: "must be a credential-free remote source"}) } } else if skill.Manager != ManagerManual && skill.Manager != ManagerWorkflow { diff --git a/internal/sjskills/local_source.go b/internal/sjskills/local_source.go new file mode 100644 index 0000000..9d0a49a --- /dev/null +++ b/internal/sjskills/local_source.go @@ -0,0 +1,418 @@ +package sjskills + +// Local direct sources let a project install a skill it develops in-repo. +// They are deliberately confined to project manifests: the global baseline is +// machine-independent, and registry sources stay remote. A local source is +// copied into the same verified staging snapshot that a remote fetch produces, +// so placement, provenance, quarantine, and rollback need no separate path. + +import ( + "bufio" + "errors" + "fmt" + "io" + "os" + "path" + "path/filepath" + "strings" + "unicode" +) + +const localSourceIdentityPrefix = "local:" + +// LocalSourceError is a configuration problem with one local direct source: +// the user fixes sjskills.toml or the source directory, and retrying cannot +// help. Its message names only the user's own manifest text, so it bypasses +// the credential redaction applied to fetched-source diagnostics. +type LocalSourceError struct { + Skill string + Source string + Problem string +} + +func (e *LocalSourceError) Error() string { + return fmt.Sprintf("%s: local source %q %s", e.Skill, e.Source, e.Problem) +} + +// managedDirectoryPairs are generated skill roots. A source inside one of them +// would be an installed copy rather than the skill's own source, in this or +// any other project or home. +var managedDirectoryPairs = [][2]string{{string(TargetAgents), ManagedSkillsDirectoryName}, {string(TargetClaude), ManagedSkillsDirectoryName}} + +// IsLocalSource reports whether source is written as a filesystem path rather +// than a remote location. It is syntactic, so it classifies invalid local +// spellings (for example `~/x` or a backslash path) for a precise diagnostic. +func IsLocalSource(source string) bool { + return strings.HasPrefix(source, ".") || strings.HasPrefix(source, "/") || strings.HasPrefix(source, "~") || + strings.Contains(source, `\`) || hasDriveLetter(source) +} + +func hasDriveLetter(value string) bool { + return len(value) >= 2 && ((value[0] >= 'A' && value[0] <= 'Z') || (value[0] >= 'a' && value[0] <= 'z')) && value[1] == ':' +} + +// LocalSourceProblem validates the committed spelling of a local direct +// source. Paths use forward slashes on every platform so one manifest works +// everywhere; relative paths must be explicit (`./` or `../`). +func LocalSourceProblem(source string) string { + switch { + case source == "": + return "source is empty" + case strings.TrimSpace(source) != source || strings.IndexFunc(source, unicode.IsControl) >= 0: + return "local path contains invalid characters" + case strings.HasPrefix(source, "~"): + return "home-relative paths are not supported; use a path relative to sjskills.toml" + case strings.Contains(source, `\`): + return "local paths must use forward slashes" + } + if hasDriveLetter(source) { + if len(source) < 3 || source[2] != '/' { + return "drive paths must be absolute, such as C:/path" + } + } else if !strings.HasPrefix(source, "/") && !strings.HasPrefix(source, "./") && !strings.HasPrefix(source, "../") { + return "relative local paths must start with ./ or ../" + } + canonical, ok := canonicalLocalSource(source) + if !ok || canonical == "./" || strings.Trim(strings.ReplaceAll(canonical, "..", ""), "/") == "" { + return "local path must name a skill directory, not the project root or its parent" + } + return "" +} + +// canonicalLocalSource normalizes a validated spelling into the stable form +// used in provenance. Relative sources keep their manifest-relative form, so +// moving or recloning the project does not change ownership. +func canonicalLocalSource(source string) (string, bool) { + if source == "" || strings.ContainsRune(source, 0) || strings.Contains(source, `\`) || strings.HasPrefix(source, "~") { + return "", false + } + if hasDriveLetter(source) { + if len(source) < 3 || source[2] != '/' { + return "", false + } + return strings.ToUpper(source[:1]) + ":" + path.Clean(source[2:]), true + } + if strings.HasPrefix(source, "/") { + return path.Clean(source), true + } + if !strings.HasPrefix(source, "./") && !strings.HasPrefix(source, "../") { + return "", false + } + cleaned := path.Clean(source) + if cleaned == "." { + return "./", true + } + if cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return cleaned, true + } + return "./" + cleaned, true +} + +func localSourceIdentity(source string) (string, bool) { + if LocalSourceProblem(source) != "" { + return "", false + } + canonical, ok := canonicalLocalSource(source) + if !ok { + return "", false + } + return localSourceIdentityPrefix + canonical, true +} + +func isCanonicalLocalSourceIdentity(identity string) bool { + rest, ok := strings.CutPrefix(identity, localSourceIdentityPrefix) + if !ok { + return false + } + canonical, valid := localSourceIdentity(rest) + return valid && canonical == identity +} + +// isPortableLocalSource reports whether a validated source is written +// relative to the project and stays inside it, so the committed manifest +// works in every checkout. +func isPortableLocalSource(source string) bool { + canonical, ok := canonicalLocalSource(source) + return ok && strings.HasPrefix(canonical, "./") +} + +// resolveLocalSourcePath maps a validated source onto this machine. Relative +// paths resolve against the directory containing sjskills.toml, never the +// process working directory. +func resolveLocalSourcePath(projectRoot, source string) (string, error) { + canonical, ok := canonicalLocalSource(source) + if !ok { + return "", errors.New("local source is invalid") + } + if strings.HasPrefix(canonical, "./") || strings.HasPrefix(canonical, "../") { + if projectRoot == "" || !filepath.IsAbs(projectRoot) { + return "", errors.New("relative local source requires the absolute project root") + } + resolved := filepath.Join(projectRoot, filepath.FromSlash(canonical)) + if pathWithin(resolved, projectRoot) { + return "", errors.New("local source must not contain the project root") + } + return resolved, nil + } + resolved := filepath.Clean(filepath.FromSlash(canonical)) + if !filepath.IsAbs(resolved) { + return "", errors.New("absolute local source is not absolute on this platform") + } + if projectRoot != "" && pathWithin(resolved, projectRoot) { + return "", errors.New("local source must not contain the project root") + } + return resolved, nil +} + +// inspectLocalSkillSource proves that a local source is a real skill +// directory that the copy-mode placement contract can represent, and returns +// its tree hash. Every failure names the manifest source so the user can fix +// or remove the declaration. +func inspectLocalSkillSource(skill DesiredSkill, limits MaterializerLimits) (TreeHash, error) { + fail := func(message string, cause error) (TreeHash, error) { + if cause != nil { + message += ": " + cause.Error() + } + return TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: message} + } + if skill.LocalPath == "" || !filepath.IsAbs(skill.LocalPath) { + return fail("has no resolved absolute path", nil) + } + info, err := os.Lstat(skill.LocalPath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return fail("does not exist; restore it or remove its [[direct]] entry from sjskills.toml", nil) + } + return fail("cannot be inspected", nil) + } + if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return fail("must be a real directory, not a file or symlink", nil) + } + canonical, err := filepath.EvalSymlinks(skill.LocalPath) + if err != nil { + return fail("cannot be resolved", nil) + } + if insideManagedDirectory(canonical) { + return fail("is inside a generated .agents/skills, .claude/skills, or .sjskills directory; point it at the skill's own source", nil) + } + if _, err := os.Lstat(filepath.Join(canonical, ManifestFileName)); err == nil { + return fail("is a project root containing sjskills.toml, not a skill directory", nil) + } + if err := validateLocalSkillTree(canonical, limits); err != nil { + return fail("cannot be installed", err) + } + declared, err := localSkillDeclaredName(filepath.Join(skill.LocalPath, "SKILL.md")) + if err != nil { + return fail("is not a skill", err) + } + if declared != skill.Name { + return fail(fmt.Sprintf("declares SKILL.md name %q, not %q", declared, skill.Name), nil) + } + digest, err := hashSkillTree(skill.LocalPath, limits) + if err != nil { + return fail("cannot be hashed", err) + } + return digest, nil +} + +func insideManagedDirectory(canonical string) bool { + parts := strings.Split(filepath.ToSlash(filepath.Clean(canonical)), "/") + for index, part := range parts { + if strings.EqualFold(part, DerivedDirectoryName) { + return true + } + if index == 0 { + continue + } + for _, pair := range managedDirectoryPairs { + if strings.EqualFold(parts[index-1], pair[0]) && strings.EqualFold(part, pair[1]) { + return true + } + } + } + return false +} + +// validateLocalSkillTree enforces the copy contract before any byte is copied: +// project placements contain only real directories and regular files. +func validateLocalSkillTree(root string, limits MaterializerLimits) error { + limits = normalizedLimits(limits) + entries := 0 + var walk func(string, int) error + walk = func(directory string, depth int) error { + if depth > limits.MaxTreeDepth { + return errors.New("directory depth exceeded its bound") + } + children, err := os.ReadDir(directory) + if err != nil { + return errors.New("a directory is unreadable") + } + for _, child := range children { + entries++ + if entries > limits.MaxTreeEntries { + return errors.New("entry count exceeded its bound") + } + childPath := filepath.Join(directory, child.Name()) + info, err := os.Lstat(childPath) + if err != nil { + return errors.New("an entry changed during inspection") + } + relative, _ := filepath.Rel(root, childPath) + switch { + case info.Mode()&os.ModeSymlink != 0: + return fmt.Errorf("%s is a symlink; copy-mode skills contain only regular files and directories", filepath.ToSlash(relative)) + case info.IsDir(): + if insideManagedDirectory(childPath) { + return fmt.Errorf("%s is a generated skills or reconciler directory", filepath.ToSlash(relative)) + } + if err := walk(childPath, depth+1); err != nil { + return err + } + case !info.Mode().IsRegular(): + return fmt.Errorf("%s is not a regular file", filepath.ToSlash(relative)) + } + } + return nil + } + return walk(root, 0) +} + +// localSkillDeclaredName reads the `name` field from SKILL.md frontmatter. +// Only the leading `---` block is read, and only a top-level scalar `name`. +func localSkillDeclaredName(skillFile string) (string, error) { + info, err := os.Lstat(skillFile) + if err != nil || !info.Mode().IsRegular() { + return "", errors.New("SKILL.md is missing") + } + file, err := os.Open(skillFile) + if err != nil { + return "", errors.New("SKILL.md is unreadable") + } + defer file.Close() + scanner := bufio.NewScanner(io.LimitReader(file, 64*1024)) + if !scanner.Scan() || strings.TrimRightFunc(strings.TrimPrefix(scanner.Text(), "\uFEFF"), unicode.IsSpace) != "---" { + return "", errors.New("SKILL.md has no frontmatter") + } + for scanner.Scan() { + line := strings.TrimRightFunc(scanner.Text(), unicode.IsSpace) + if line == "---" { + break + } + key, value, found := strings.Cut(line, ":") + if !found || key != "name" { + continue + } + value = strings.TrimSpace(value) + if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') && value[len(value)-1] == value[0] { + value = value[1 : len(value)-1] + } else if before, _, found := strings.Cut(value, " #"); found { + value = strings.TrimSpace(before) + } + if value == "" { + break + } + return value, nil + } + return "", errors.New("SKILL.md frontmatter does not declare a name") +} + +// stageLocalSkill copies a verified local source into the plan's staging root +// at the same path a Skills CLI install would use, then proves the copy equals +// the source as inspected. A source edited mid-copy is reported, not adopted. +func stageLocalSkill(root string, skill DesiredSkill, limits MaterializerLimits) (string, TreeHash, error) { + sourceHash, err := inspectLocalSkillSource(skill, limits) + if err != nil { + return "", TreeHash{}, err + } + destination := filepath.Join(root, ".agents", "skills", skill.Name) + if !pathWithin(root, destination) { + return "", TreeHash{}, materializationError("staging", "skill path escapes staging root", nil) + } + if err := os.Mkdir(destination, 0o700); err != nil { + return "", TreeHash{}, materializationError(safeSkillName(skill.Name), "local source staging directory could not be created", err) + } + remaining := normalizedLimits(limits).MaxTreeBytes + if err := copyLocalSkillTree(skill.LocalPath, destination, &remaining); err != nil { + return "", TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: "could not be copied: " + err.Error()} + } + staged, err := locateStagedSkill(root, skill.Name) + if err != nil { + return "", TreeHash{}, err + } + stagedHash, err := hashSkillTree(staged, limits) + if err != nil { + return "", TreeHash{}, err + } + if stagedHash != sourceHash { + return "", TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: "changed while it was copied; retry"} + } + return staged, stagedHash, nil +} + +// copyLocalSkillTree copies at most *remaining bytes; the caller's post-copy +// hash comparison rejects any other concurrent change to the source. +func copyLocalSkillTree(source, destination string, remaining *int64) error { + info, err := os.Lstat(source) + if err != nil || !info.IsDir() || info.Mode()&os.ModeSymlink != 0 { + return errors.New("source directory changed during copy") + } + if err := os.Chmod(destination, info.Mode().Perm()|0o700); err != nil { + return err + } + entries, err := os.ReadDir(source) + if err != nil { + return err + } + for _, entry := range entries { + sourcePath := filepath.Join(source, entry.Name()) + destinationPath := filepath.Join(destination, entry.Name()) + entryInfo, err := os.Lstat(sourcePath) + if err != nil { + return errors.New("source changed during copy") + } + switch { + case entryInfo.Mode()&os.ModeSymlink != 0: + return errors.New("source gained a symlink during copy") + case entryInfo.IsDir(): + if err := os.Mkdir(destinationPath, 0o700); err != nil { + return err + } + if err := copyLocalSkillTree(sourcePath, destinationPath, remaining); err != nil { + return err + } + case entryInfo.Mode().IsRegular(): + if err := copyLocalSkillFile(sourcePath, destinationPath, entryInfo, remaining); err != nil { + return err + } + default: + return errors.New("source gained a special file during copy") + } + } + return nil +} + +// copyLocalSkillFile preserves the executable bits the tree hash records; the +// owner read/write bits only keep the staging copy removable. +func copyLocalSkillFile(source, destination string, expected os.FileInfo, remaining *int64) error { + input, err := os.Open(source) + if err != nil { + return err + } + defer input.Close() + opened, err := input.Stat() + if err != nil || !os.SameFile(expected, opened) { + return errors.New("source changed during copy") + } + output, err := os.OpenFile(destination, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + return err + } + written, copyErr := io.Copy(output, io.LimitReader(input, *remaining+1)) + *remaining -= written + if copyErr == nil && *remaining < 0 { + copyErr = errors.New("source exceeded its size bound during copy") + } + chmodErr := output.Chmod(expected.Mode().Perm() | 0o600) + closeErr := output.Close() + return errors.Join(copyErr, chmodErr, closeErr) +} diff --git a/internal/sjskills/local_source_test.go b/internal/sjskills/local_source_test.go new file mode 100644 index 0000000..25c067f --- /dev/null +++ b/internal/sjskills/local_source_test.go @@ -0,0 +1,341 @@ +package sjskills + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "sync/atomic" + "testing" + "time" +) + +func TestLocalSourceSpellingAndIdentity(t *testing.T) { + valid := map[string]string{ + "./skills/team-tool": "local:./skills/team-tool", + "./skills//team-tool/": "local:./skills/team-tool", + "../shared/team-tool": "local:../shared/team-tool", + "/opt/skills/a/../b": "local:/opt/skills/b", + "c:/skills/team-tool": "local:C:/skills/team-tool", + } + for source, want := range valid { + if problem := LocalSourceProblem(source); problem != "" { + t.Fatalf("LocalSourceProblem(%q) = %q", source, problem) + } + identity, ok := canonicalProjectSourceIdentity(source) + if !ok || identity != want || !isCanonicalProjectSourceIdentity(identity) { + t.Fatalf("identity(%q) = %q %v, want canonical %q", source, identity, ok, want) + } + } + for _, source := range []string{"~/skills/x", `.\skills\x`, "C:\\skills\\x", "C:skills", ".", "./", "./skills/..", "skills/x/."} { + if IsLocalSource(source) && LocalSourceProblem(source) == "" { + t.Fatalf("LocalSourceProblem(%q) accepted an invalid local source", source) + } + if _, ok := canonicalProjectSourceIdentity(source); ok && IsLocalSource(source) { + t.Fatalf("invalid local source %q received an identity", source) + } + } + for _, identity := range []string{"local:skills/x", "local:./", "local:./a/../b", "local:c:/x", "local:~/x"} { + if isCanonicalProjectSourceIdentity(identity) { + t.Fatalf("non-canonical identity %q was accepted", identity) + } + } + if IsLocalSource("owner/repo/skills/x") || IsLocalSource("https://github.com/o/r") { + t.Fatal("remote sources were classified as local") + } +} + +func TestManifestLocalSourceValidation(t *testing.T) { + manifest, err := ParseManifest([]byte("version = 1\n[[direct]]\nname = \"team-tool\"\nsource = \"./skills/team-tool\"\n")) + if err != nil || manifest.Direct[0].Source != "./skills/team-tool" { + t.Fatalf("local manifest = %#v err=%v", manifest, err) + } + for _, extra := range []string{"access = \"github-authenticated\"\n", "full_depth = true\n"} { + _, err := ParseManifest([]byte("version = 1\n[[direct]]\nname = \"team-tool\"\nsource = \"./skills/team-tool\"\n" + extra)) + if err == nil || !issueCode(err, IssueInvalidSource) { + t.Fatalf("local source with %q error = %v, want invalid source", extra, err) + } + } + if err := ValidateRegistry(func() Registry { + registry := fixtureRegistry(t) + for id, source := range registry.Sources { + if source.Kind == SourceRepository { + source.Location = "./skills" + registry.Sources[id] = source + } + } + return registry + }()); err == nil || !issueCode(err, IssueInvalidSource) { + t.Fatalf("registry local source error = %v, want invalid source", err) + } +} + +func TestResolveLocalSourceAgainstProjectRoot(t *testing.T) { + root := t.TempDir() + registry := fixtureRegistry(t) + manifest := Manifest{Version: 1, Direct: []DirectSkill{{Name: "team-tool", Source: "./skills/team-tool"}}} + plan, err := BuildPlan(ResolveRequest{Registry: registry, Manifest: &manifest, ProjectRoot: root}) + if err != nil { + t.Fatal(err) + } + skill := plan.Desired.Skills[0] + if skill.LocalPath != filepath.Join(root, "skills", "team-tool") || skill.Source != "./skills/team-tool" || skill.Manager != ManagerSkillsCLI || skill.Mode != ModeCopy { + t.Fatalf("resolved local skill = %#v", skill) + } + if len(plan.Warnings) != 0 { + t.Fatalf("portable source warnings = %#v", plan.Warnings) + } + if _, err := ResolveProject(registry, manifest, ""); err == nil || !issueCode(err, IssueInvalidSource) { + t.Fatalf("missing root error = %v", err) + } + outside := Manifest{Version: 1, Direct: []DirectSkill{{Name: "team-tool", Source: "../shared/team-tool"}}} + plan, err = BuildPlan(ResolveRequest{Registry: registry, Manifest: &outside, ProjectRoot: root}) + if err != nil || len(plan.Warnings) != 1 || plan.Warnings[0].Code != "machine-specific-source" { + t.Fatalf("outside source plan warnings = %#v err=%v", plan.Warnings, err) + } + containing := Manifest{Version: 1, Direct: []DirectSkill{{Name: "team-tool", Source: filepath.ToSlash(filepath.Dir(root))}}} + if _, err := ResolveProject(registry, containing, root); err == nil || !strings.Contains(err.Error(), "must not contain the project root") { + t.Fatalf("containing source error = %v", err) + } +} + +func writeLocalSkill(t *testing.T, path, name string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(path, "scripts"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(path, "SKILL.md"), []byte("---\nname: "+name+"\ndescription: \"Fixture\"\n---\n# Fixture\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(path, "scripts", "run.sh"), []byte("#!/bin/sh\necho ok\n"), 0o755); err != nil { + t.Fatal(err) + } +} + +func localDesiredSkill(path string) DesiredSkill { + return DesiredSkill{Access: AccessPublic, Name: "team-tool", Source: "./skills/team-tool", LocalPath: path, Scope: ScopeProject, + Origin: "direct", Manager: ManagerSkillsCLI, Mode: ModeCopy, Targets: defaultTargets()} +} + +func TestMaterializeLocalSourceWithoutSkillsCLI(t *testing.T) { + source := filepath.Join(t.TempDir(), "skills", "team-tool") + writeLocalSkill(t, source, "team-tool") + var calls atomic.Int32 + materializer := NewMaterializer(MaterializerConfig{Runner: runnerFunc(func(context.Context, string, []string, []string) (ProcessResult, error) { + calls.Add(1) + return ProcessResult{}, nil + })}) + plan, err := materializer.Materialize(context.Background(), []DesiredSkill{localDesiredSkill(source)}) + if err != nil { + t.Fatal(err) + } + defer plan.Cleanup() + if calls.Load() != 0 { + t.Fatalf("local-only materialization ran %d processes", calls.Load()) + } + snapshot, ok := plan.SnapshotFor("team-tool") + if !ok { + t.Fatal("missing local snapshot") + } + want, err := HashSkillTree(source) + if err != nil || snapshot.Hash != want { + t.Fatalf("snapshot hash = %#v, source hash = %#v err=%v", snapshot.Hash, want, err) + } + if err := plan.Verify(); err != nil { + t.Fatal(err) + } + if err := validateApplyCopyTree(snapshot.Path); err != nil { + t.Fatalf("staged copy violates the placement contract: %v", err) + } +} + +func TestMaterializeLocalSourceBlocksInvalidSources(t *testing.T) { + cases := map[string]struct { + prepare func(t *testing.T, path string) string + want string + }{ + "missing": {func(t *testing.T, path string) string { return path }, "does not exist; restore it or remove its [[direct]] entry"}, + "no skill file": {func(t *testing.T, path string) string { + writeLocalSkill(t, path, "team-tool") + if err := os.Remove(filepath.Join(path, "SKILL.md")); err != nil { + t.Fatal(err) + } + return path + }, "SKILL.md is missing"}, + "name mismatch": {func(t *testing.T, path string) string { + writeLocalSkill(t, path, "other-tool") + return path + }, `declares SKILL.md name "other-tool", not "team-tool"`}, + "managed root": {func(t *testing.T, path string) string { + managed := filepath.Join(filepath.Dir(path), ".claude", "skills", "team-tool") + writeLocalSkill(t, managed, "team-tool") + return managed + }, "is inside a generated"}, + } + for name, test := range cases { + t.Run(name, func(t *testing.T) { + path := test.prepare(t, filepath.Join(t.TempDir(), "team-tool")) + plan, err := NewMaterializer(MaterializerConfig{Runner: runnerFunc(func(context.Context, string, []string, []string) (ProcessResult, error) { + t.Fatal("local source invoked a process") + return ProcessResult{}, nil + })}).Materialize(context.Background(), []DesiredSkill{localDesiredSkill(path)}) + if err == nil || !strings.Contains(err.Error(), test.want) || !strings.Contains(err.Error(), "./skills/team-tool") { + t.Fatalf("plan=%v err=%v, want %q", plan, err, test.want) + } + }) + } +} + +func TestMaterializeLocalSourceRejectsSymlinks(t *testing.T) { + source := filepath.Join(t.TempDir(), "team-tool") + writeLocalSkill(t, source, "team-tool") + if err := os.Symlink("SKILL.md", filepath.Join(source, "alias.md")); err != nil { + if runtime.GOOS == "windows" { + t.Skipf("symlink creation unavailable: %v", err) + } + t.Fatal(err) + } + _, err := NewMaterializer(MaterializerConfig{}).Materialize(context.Background(), []DesiredSkill{localDesiredSkill(source)}) + if err == nil || !strings.Contains(err.Error(), "alias.md is a symlink") { + t.Fatalf("symlink error = %v", err) + } +} + +func TestStatusHashesLocalSourcesOnEveryCheck(t *testing.T) { + root := canonicalTempHome(t) + writeLocalSkill(t, filepath.Join(root, "skills", "team-tool"), "team-tool") + if err := os.WriteFile(filepath.Join(root, ManifestFileName), []byte("version = 1\n[[direct]]\nname = \"team-tool\"\nsource = \"./skills/team-tool\"\n"), 0o644); err != nil { + t.Fatal(err) + } + scope, err := ResolveStatusScope(root, minimalGlobalRegistry(t), false) + if err != nil { + t.Fatal(err) + } + now := time.Date(2026, 10, 6, 1, 0, 0, 0, time.UTC) + service := StatusService{CacheRoot: filepath.Join(t.TempDir(), "status"), Now: func() time.Time { return now }} + service.Refresh = func(_ context.Context, desired []DesiredSkill) (StatusSnapshot, error) { + if len(desired) != 0 { + t.Fatalf("local sources reached the upstream refresh: %#v", desired) + } + return StatusSnapshot{map[string]TreeHash{}, now}, nil + } + advisory := service.Check(context.Background(), scope, nil) + if advisory.Error != "" || len(advisory.Findings) != 2 { + t.Fatalf("advisory = %+v", advisory) + } + requireStatusFinding(t, advisory, AdvisoryMissing, "team-tool", TargetAgents, string(ProjectStateReasonExpectedEntryAbsent)) + + if err := os.RemoveAll(filepath.Join(root, "skills", "team-tool")); err != nil { + t.Fatal(err) + } + advisory = service.Check(context.Background(), scope, nil) + if advisory.Freshness != AdvisoryUnavailable || !strings.Contains(advisory.Error, "does not exist") || validateAdvisories([]Advisory{advisory}) != nil { + t.Fatalf("missing-source advisory = %+v", advisory) + } +} + +type runnerFunc func(context.Context, string, []string, []string) (ProcessResult, error) + +func (f runnerFunc) Run(ctx context.Context, command string, args, env []string) (ProcessResult, error) { + return f(ctx, command, args, env) +} + +func TestLocalSourceRejectsParentOnlyAndProjectRoots(t *testing.T) { + for _, source := range []string{"../", "../..", "./skills/../..", "/"} { + if LocalSourceProblem(source) == "" { + t.Fatalf("LocalSourceProblem(%q) accepted a parent-only path", source) + } + } + project := filepath.Join(t.TempDir(), "nested") + writeLocalSkill(t, project, "team-tool") + if err := os.WriteFile(filepath.Join(project, ManifestFileName), []byte("version = 1\n"), 0o644); err != nil { + t.Fatal(err) + } + _, err := inspectLocalSkillSource(localDesiredSkill(project), MaterializerLimits{}) + if err == nil || !strings.Contains(err.Error(), "is a project root") { + t.Fatalf("project-root source error = %v", err) + } + if err := os.Remove(filepath.Join(project, ManifestFileName)); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(project, ".claude", "skills"), 0o755); err != nil { + t.Fatal(err) + } + _, err = inspectLocalSkillSource(localDesiredSkill(project), MaterializerLimits{}) + if err == nil || !strings.Contains(err.Error(), ".claude/skills is a generated") { + t.Fatalf("source containing a managed root error = %v", err) + } +} + +func TestLocalSkillDeclaredNameToleratesYAMLDetails(t *testing.T) { + path := filepath.Join(t.TempDir(), "SKILL.md") + if err := os.WriteFile(path, []byte("---\r\nname: team-tool # owner: platform\r\n--- \r\nname: body\r\n"), 0o644); err != nil { + t.Fatal(err) + } + if name, err := localSkillDeclaredName(path); err != nil || name != "team-tool" { + t.Fatalf("declared name = %q err=%v", name, err) + } +} + +func TestMaterializeMixedLocalAndRemoteSources(t *testing.T) { + source := filepath.Join(t.TempDir(), "team-tool") + writeLocalSkill(t, source, "team-tool") + runner := defaultMaterializeRunner() + materializer, _ := testMaterializer(t, runner, MaterializerLimits{}) + plan, err := materializer.Materialize(context.Background(), []DesiredSkill{localDesiredSkill(source), desiredMaterializeSkill("remote-tool", "example/remote-catalog")}) + if err != nil { + t.Fatal(err) + } + defer plan.Cleanup() + if len(plan.Snapshots()) != 2 { + t.Fatalf("snapshots = %d, want 2", len(plan.Snapshots())) + } + for _, call := range runner.calls { + for _, arg := range call.args { + if strings.Contains(arg, "team-tool") { + t.Fatalf("local skill reached Skills CLI: %q", call.args) + } + } + } + if len(runner.calls) != 3 { + t.Fatalf("calls = %d, want preflight twice and one add", len(runner.calls)) + } +} + +func TestStatusReportsLocalSourceEditAsUpdate(t *testing.T) { + root := canonicalTempHome(t) + source := filepath.Join(root, "skills", "team-tool") + writeLocalSkill(t, source, "team-tool") + if err := os.WriteFile(filepath.Join(root, ManifestFileName), []byte("version = 1\n[[direct]]\nname = \"team-tool\"\nsource = \"./skills/team-tool\"\n"), 0o644); err != nil { + t.Fatal(err) + } + scope, err := ResolveStatusScope(root, minimalGlobalRegistry(t), false) + if err != nil { + t.Fatal(err) + } + layout, err := LayoutForProject(scope.Root) + if err != nil { + t.Fatal(err) + } + plan, err := NewMaterializer(MaterializerConfig{}).Materialize(context.Background(), scope.Plan.Desired.Skills) + if err != nil { + t.Fatal(err) + } + defer plan.Cleanup() + session := &ProjectApplySession{Layout: layout, Desired: scope.Plan.Desired, Expected: map[string]TreeHash{"team-tool": plan.snapshots["team-tool"].Hash}, Materialized: plan} + if session.Plan, err = InspectStatusPlan(scope, session.Expected); err != nil { + t.Fatal(err) + } + if _, err := ApplyProjectChanges(context.Background(), session, ApplyDeps{}); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(source, "SKILL.md"), []byte("---\nname: team-tool\n---\n# edited\n"), 0o644); err != nil { + t.Fatal(err) + } + now := time.Date(2026, 10, 6, 1, 0, 0, 0, time.UTC) + service := StatusService{CacheRoot: filepath.Join(t.TempDir(), "status"), Now: func() time.Time { return now }, + Refresh: func(context.Context, []DesiredSkill) (StatusSnapshot, error) { return StatusSnapshot{map[string]TreeHash{}, now}, nil }} + advisory := service.Check(context.Background(), scope, nil) + requireStatusFinding(t, advisory, AdvisoryUpdate, "team-tool", TargetClaude, string(ProjectStateReasonVerifiedUpdate)) +} diff --git a/internal/sjskills/materialize.go b/internal/sjskills/materialize.go index d7a27e5..39a5945 100644 --- a/internal/sjskills/materialize.go +++ b/internal/sjskills/materialize.go @@ -281,8 +281,11 @@ func skillsCLIAddArgs(skill DesiredSkill) ([]string, error) { // Materialize runs one plan. Installable skills are keyed by identity (name, // source, and full-depth behavior). Skills sharing a source and discovery // options share one fetch; every selected skill still needs its own verified -// snapshot. Manual and workflow entries -// are returned as skipped and never reported as materialized successes. +// snapshot. Local sources are copied into the same staging layout without +// Skills CLI, after any remote fetch so Skills CLI never sees them; it is +// preflighted only when a remote source is selected. +// Manual and workflow entries are returned as skipped and never reported as +// materialized successes. func (m *Materializer) Materialize(ctx context.Context, skills []DesiredSkill) (*MaterializationPlan, error) { if ctx == nil { ctx = context.Background() @@ -306,14 +309,24 @@ func (m *Materializer) Materialize(ctx context.Context, skills []DesiredSkill) ( plan.root = root plan.cleanupRoot = root plan.limits = m.limits - if err := m.preflightIn(ctx, root); err != nil { - if !errors.Is(err, errProcessTreeActive) { - _ = plan.Cleanup() + remote, local := make([]DesiredSkill, 0, len(installable)), make([]DesiredSkill, 0) + for _, skill := range installable { + if skill.LocalPath == "" { + remote = append(remote, skill) + } else { + local = append(local, skill) + } + } + if len(remote) > 0 { + if err := m.preflightIn(ctx, root); err != nil { + if !errors.Is(err, errProcessTreeActive) { + _ = plan.Cleanup() + } + return nil, err } - return nil, err } - for _, batch := range materializationBatches(installable) { + for _, batch := range materializationBatches(remote) { skill := batch[0] args, err := skillsCLIAddArgs(skill) if err == nil && len(batch) > 1 { @@ -391,6 +404,22 @@ func (m *Materializer) Materialize(ctx context.Context, skills []DesiredSkill) ( plan.snapshots[skill.Name] = snapshot } } + for _, skill := range local { + if err := ensureContext(ctx); err != nil { + _ = plan.Cleanup() + return nil, err + } + path, digest, err := stageLocalSkill(root, skill, m.limits) + if err != nil { + _ = plan.Cleanup() + var localErr *LocalSourceError + if errors.As(err, &localErr) { + return nil, localErr + } + return nil, m.sanitizeError(err, root) + } + plan.snapshots[skill.Name] = &SkillSnapshot{Skill: skill, Path: path, Hash: digest, plan: plan, stageRoot: root, limits: m.limits} + } return plan, nil } @@ -457,14 +486,18 @@ func classifyMaterializationSkills(skills []DesiredSkill) ([]DesiredSkill, []Des return nil, nil, materializationError(safeSkillName(skill.Name), "skills-cli installation must use copy mode", nil) } if previous, ok := seen[skill.Name]; ok { - if previous.Manager != ManagerSkillsCLI || previous.Source != skill.Source || previous.FullDepth != skill.FullDepth || previous.Access.Effective() != skill.Access.Effective() { + if previous.Manager != ManagerSkillsCLI || previous.Source != skill.Source || previous.LocalPath != skill.LocalPath || previous.FullDepth != skill.FullDepth || previous.Access.Effective() != skill.Access.Effective() { return nil, nil, materializationError("classify", "skill identity has contradictory source or options", nil) } // The exact identity was already scheduled. Targets differ at // placement time, not at materialization time. continue } - if problem := SkillsCLIPathProblem(skill.Source); problem != "" { + if skill.LocalPath != "" || IsLocalSource(skill.Source) { + if problem := LocalSourceProblem(skill.Source); problem != "" || !filepath.IsAbs(skill.LocalPath) || skill.Access != AccessPublic || skill.FullDepth { + return nil, nil, materializationError(safeSkillName(skill.Name), "local source requires a valid public path resolved against the project root", nil) + } + } else if problem := SkillsCLIPathProblem(skill.Source); problem != "" { return nil, nil, materializationError(safeSkillName(skill.Name), "source is not installable by Skills CLI: "+problem, nil) } seen[skill.Name] = skill @@ -477,7 +510,7 @@ func classifyMaterializationSkills(skills []DesiredSkill) ([]DesiredSkill, []Des } func sameDesiredSkill(left, right DesiredSkill) bool { - if left.Access.Effective() != right.Access.Effective() || left.Name != right.Name || left.SourceID != right.SourceID || left.Source != right.Source || + if left.Access.Effective() != right.Access.Effective() || left.Name != right.Name || left.SourceID != right.SourceID || left.Source != right.Source || left.LocalPath != right.LocalPath || left.Scope != right.Scope || left.Origin != right.Origin || left.Manager != right.Manager || left.Mode != right.Mode || left.Workflow != right.Workflow || left.FullDepth != right.FullDepth || len(left.Targets) != len(right.Targets) { diff --git a/internal/sjskills/resolve.go b/internal/sjskills/resolve.go index 61f62e6..c4194b0 100644 --- a/internal/sjskills/resolve.go +++ b/internal/sjskills/resolve.go @@ -16,7 +16,7 @@ func Resolve(request ResolveRequest) (DesiredState, error) { Code: IssueEmptySelection, Path: "manifest", Message: "project resolution requires a manifest", }}} } - return ResolveProject(request.Registry, *request.Manifest) + return ResolveProject(request.Registry, *request.Manifest, request.ProjectRoot) } // BuildPlan derives stable warnings/evidence without reading files, invoking @@ -35,6 +35,9 @@ func BuildPlan(request ResolveRequest) (Plan, error) { }, } for _, skill := range desired.Skills { + if skill.LocalPath != "" && !isPortableLocalSource(skill.Source) { + plan.Warnings = append(plan.Warnings, Warning{Code: "machine-specific-source", Message: fmt.Sprintf("%s uses local source %s, which is absolute or outside the project; other checkouts need the same path", skill.Name, skill.Source)}) + } switch skill.Manager { case ManagerManual: plan.Warnings = append(plan.Warnings, Warning{Code: "manual-action", Message: fmt.Sprintf("%s requires its recorded manual provisioning procedure", skill.Name)}) @@ -67,8 +70,9 @@ func ResolveGlobal(registry Registry) (DesiredState, error) { // ResolveProject unions selected profile sets and direct entries. Every name // is checked before a desired result is returned, so contradictory/duplicate -// sources cannot reach a later materialization adapter. -func ResolveProject(registry Registry, manifest Manifest) (DesiredState, error) { +// sources cannot reach a later materialization adapter. projectRoot is the +// absolute manifest directory that anchors relative local direct sources. +func ResolveProject(registry Registry, manifest Manifest, projectRoot string) (DesiredState, error) { if err := ValidateManifest(registry, manifest); err != nil { return DesiredState{}, err } @@ -90,12 +94,22 @@ func ResolveProject(registry Registry, manifest Manifest) (DesiredState, error) state.Skills = append(state.Skills, skill) } } - for _, direct := range manifest.Direct { + for index, direct := range manifest.Direct { if previous, exists := selected[direct.Name]; exists { return DesiredState{}, collisionError("direct", direct.Name, previous) } selected[direct.Name] = "direct" - state.Skills = append(state.Skills, resolveDirectSkill(registry, direct)) + skill := resolveDirectSkill(registry, direct) + if IsLocalSource(direct.Source) { + localPath, err := resolveLocalSourcePath(projectRoot, direct.Source) + if err != nil { + return DesiredState{}, &ValidationErrors{Issues: []Issue{{ + Code: IssueInvalidSource, Path: fmt.Sprintf("manifest.direct[%d].source", index), Message: err.Error(), + }}} + } + skill.LocalPath = localPath + } + state.Skills = append(state.Skills, skill) } sortDesiredSkills(state.Skills) return state, nil diff --git a/internal/sjskills/status.go b/internal/sjskills/status.go index eece3c4..c0a2c17 100644 --- a/internal/sjskills/status.go +++ b/internal/sjskills/status.go @@ -147,6 +147,7 @@ func ResolveStatusScope(directory string, registry Registry, global bool) (Statu return StatusScope{}, err } request.Manifest = &manifest + request.ProjectRoot = project.Root root = project.Root } canonical, err := filepath.EvalSymlinks(root) @@ -191,7 +192,8 @@ func (s StatusScope) identity() string { func (s StatusScope) cacheKey() string { // Only upstream materialization inputs belong here. Roots, placement targets, // profile names, and registry metadata affect local classification, which is - // always recomputed. Keep Matches stricter for command-produced snapshots. + // always recomputed. Local sources are hashed on every check, never cached. + // Keep Matches stricter for command-produced snapshots. type input struct { Name, Source string Mode InstallMode @@ -200,7 +202,7 @@ func (s StatusScope) cacheKey() string { } inputs := make([]input, 0, len(s.Plan.Desired.Skills)) for _, skill := range s.Plan.Desired.Skills { - if skill.Manager == ManagerSkillsCLI { + if skill.Manager == ManagerSkillsCLI && skill.LocalPath == "" { inputs = append(inputs, input{skill.Name, skill.Source, skill.Mode, skill.FullDepth, skill.Access.Effective()}) } } @@ -297,6 +299,49 @@ func RefreshStatusSnapshot(ctx context.Context, skills []DesiredSkill) (StatusSn } return StatusSnapshot{expected, time.Now().UTC()}, nil } + +// statusCachedDesired is the part of a scope whose expected content comes +// from the upstream cache. Local sources change with the working tree, so +// Check hashes them afresh instead. +func statusCachedDesired(desired DesiredState) DesiredState { + cached := DesiredState{Scope: desired.Scope, Skills: make([]DesiredSkill, 0, len(desired.Skills))} + for _, skill := range desired.Skills { + if skill.LocalPath == "" { + cached.Skills = append(cached.Skills, skill) + } + } + return cached +} + +func cachedStatusExpected(cached DesiredState, expected map[string]TreeHash) map[string]TreeHash { + result := make(map[string]TreeHash, len(cached.Skills)) + for _, skill := range cached.Skills { + if hash, ok := expected[skill.Name]; ok && skill.Manager == ManagerSkillsCLI { + result[skill.Name] = hash + } + } + return result +} + +// withLocalStatusExpected adds the current tree hash of every local source. +func withLocalStatusExpected(desired DesiredState, cached map[string]TreeHash) (map[string]TreeHash, error) { + expected := make(map[string]TreeHash, len(desired.Skills)) + for name, hash := range cached { + expected[name] = hash + } + for _, skill := range desired.Skills { + if skill.Manager != ManagerSkillsCLI || skill.LocalPath == "" { + continue + } + hash, err := inspectLocalSkillSource(skill, MaterializerLimits{}) + if err != nil { + return nil, err + } + expected[skill.Name] = hash + } + return expected, nil +} + func validStatusExpected(desired DesiredState, expected map[string]TreeHash) bool { count := 0 for _, skill := range desired.Skills { @@ -313,6 +358,7 @@ func validStatusExpected(desired DesiredState, expected map[string]TreeHash) boo } func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *StatusSnapshot) Advisory { result := newAdvisory(scope.Plan.Desired.Scope) + cachedDesired := statusCachedDesired(scope.Plan.Desired) now := s.now() entry, err := s.read(scope) if err != nil { @@ -320,7 +366,7 @@ func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *S } result.Cached = true if reusable != nil && validStatusExpected(scope.Plan.Desired, reusable.Expected) && !reusable.ObservedAt.IsZero() && !reusable.ObservedAt.After(now) { - entry = statusCacheEntry{Version: statusCacheVersion, Identity: scope.cacheKey(), Expected: reusable.Expected, ObservedAt: reusable.ObservedAt} + entry = statusCacheEntry{Version: statusCacheVersion, Identity: scope.cacheKey(), Expected: cachedStatusExpected(cachedDesired, reusable.Expected), ObservedAt: reusable.ObservedAt} result.Cached = false if err := s.publish(scope, entry); err != nil { result.Error = "status cache could not be written" @@ -344,9 +390,9 @@ func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *S if refresh == nil { refresh = RefreshStatusSnapshot } - snapshot, refreshErr := refresh(ctx, scope.Plan.Desired.Skills) + snapshot, refreshErr := refresh(ctx, cachedDesired.Skills) finished := s.now() - if refreshErr == nil && ctx.Err() == nil && validStatusExpected(scope.Plan.Desired, snapshot.Expected) && !snapshot.ObservedAt.IsZero() && !snapshot.ObservedAt.After(finished) { + if refreshErr == nil && ctx.Err() == nil && validStatusExpected(cachedDesired, snapshot.Expected) && !snapshot.ObservedAt.IsZero() && !snapshot.ObservedAt.After(finished) { entry = statusCacheEntry{Version: statusCacheVersion, Identity: scope.cacheKey(), Expected: snapshot.Expected, ObservedAt: snapshot.ObservedAt} result.Cached = false } else { @@ -368,7 +414,7 @@ func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *S } else if !entry.fresh(now) { result.Error = "upstream status could not be refreshed; retry cooldown active" } - if entry.ObservedAt.IsZero() || !validStatusExpected(scope.Plan.Desired, entry.Expected) { + if entry.ObservedAt.IsZero() || !validStatusExpected(cachedDesired, entry.Expected) { result.Cached = false if result.Error == "" { result.Error = "upstream status unavailable" @@ -381,7 +427,14 @@ func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *S result.Freshness = AdvisoryStale result.Cached = true } - plan, err := InspectStatusPlan(scope, entry.Expected) + expected, err := withLocalStatusExpected(scope.Plan.Desired, entry.Expected) + if err != nil { + result.Freshness = AdvisoryUnavailable + result.ObservedAt = nil + result.Error = err.Error() + return result + } + plan, err := InspectStatusPlan(scope, expected) if err != nil { result.Freshness = AdvisoryUnavailable result.ObservedAt = nil diff --git a/internal/sjskills/status_cache.go b/internal/sjskills/status_cache.go index 7db38e0..3dbea97 100644 --- a/internal/sjskills/status_cache.go +++ b/internal/sjskills/status_cache.go @@ -136,7 +136,7 @@ func (s StatusService) read(scope StatusScope) (statusCacheEntry, error) { return statusCacheEntry{}, errors.New("incompatible status cache") } // A failed cold refresh has no snapshot but retains its bounded cooldown. - if !entry.ObservedAt.IsZero() && !validStatusExpected(scope.Plan.Desired, entry.Expected) { + if !entry.ObservedAt.IsZero() && !validStatusExpected(statusCachedDesired(scope.Plan.Desired), entry.Expected) { return statusCacheEntry{}, errors.New("incomplete status cache") } if entry.ObservedAt.IsZero() && len(entry.Expected) != 0 { diff --git a/internal/sjskills/testdata/invalid/source.toml b/internal/sjskills/testdata/invalid/source.toml index 63f5bc3..f52a422 100644 --- a/internal/sjskills/testdata/invalid/source.toml +++ b/internal/sjskills/testdata/invalid/source.toml @@ -2,4 +2,4 @@ version = 1 [[direct]] name = "local-skill" -source = "../local-skill" +source = "~/local-skill" diff --git a/internal/sjskills/types.go b/internal/sjskills/types.go index b08d163..49e6ef8 100644 --- a/internal/sjskills/types.go +++ b/internal/sjskills/types.go @@ -109,9 +109,9 @@ type SkillDeclaration struct { FullDepth bool `json:"fullDepth,omitempty"` } -// DirectSkill is a project-local third-party declaration. Version 1 fixes its -// ownership to Skills CLI, its placement to copy mode, and its targets to the -// registry defaults; the manifest carries only a portable name, installable +// DirectSkill is a project-local declaration. Version 1 fixes its ownership +// to Skills CLI, its placement to copy mode, and its targets to the registry +// defaults; the manifest carries only a portable name, a remote or local // source, access policy, and optional full-depth hint. type DirectSkill struct { Access Access `toml:"access,omitempty" json:"access,omitempty"` @@ -143,6 +143,9 @@ type DesiredSkill struct { Workflow string `json:"workflow,omitempty"` FullDepth bool `json:"fullDepth"` Targets []Target `json:"targets"` + // LocalPath is the absolute directory a local direct source resolves to on + // this machine. Source keeps the committed spelling for provenance. + LocalPath string `json:"localPath,omitempty"` } type DesiredState struct { @@ -152,11 +155,14 @@ type DesiredState struct { // ResolveRequest is the pure boundary consumed by planning adapters. Global // requests ignore Manifest and always select the fixed baseline; project -// requests require it and select only its profile/direct union. +// requests require it and select only its profile/direct union. ProjectRoot, +// the absolute directory containing the manifest, anchors relative local +// direct sources. type ResolveRequest struct { - Registry Registry - Manifest *Manifest - Global bool + Registry Registry + Manifest *Manifest + ProjectRoot string + Global bool } // PlanAction is the stable action vocabulary shared by pure resolution, diff --git a/internal/sjskills/validate.go b/internal/sjskills/validate.go index 82e88ef..c91c8c2 100644 --- a/internal/sjskills/validate.go +++ b/internal/sjskills/validate.go @@ -310,10 +310,24 @@ func ValidateManifestShape(manifest Manifest) error { if index > 0 && direct.Name < manifest.Direct[index-1].Name { addIssue(&issues, IssueInvalidName, "manifest.direct", "must be sorted by name") } - if direct.Source == "" { + switch { + case direct.Source == "": addIssue(&issues, IssueInvalidSource, path+".source", "direct skill must record an installable source identity") - } else if problem := SkillsCLIPathProblem(direct.Source); problem != "" { - addIssue(&issues, IssueInvalidSource, path+".source", "direct skill source must be a git shorthand or credential-free https source: %s", problem) + case IsLocalSource(direct.Source): + if problem := LocalSourceProblem(direct.Source); problem != "" { + addIssue(&issues, IssueInvalidSource, path+".source", "direct skill local source is invalid: %s", problem) + } + if direct.Access.Effective() != AccessPublic { + addIssue(&issues, IssueInvalidSource, path+".access", "local sources are read from disk; omit access or use public") + } + if direct.FullDepth { + addIssue(&issues, IssueInvalidSource, path+".full_depth", "local sources name the skill directory and must not set full_depth") + } + continue + default: + if problem := SkillsCLIPathProblem(direct.Source); problem != "" { + addIssue(&issues, IssueInvalidSource, path+".source", "direct skill source must be a git shorthand, credential-free https source, or local path: %s", problem) + } } validateAccess(path, direct.Access, direct.Source, &issues) // Direct entries intentionally have no manager, mode, target, or diff --git a/skills/sjskills/SKILL.md b/skills/sjskills/SKILL.md index 2357b10..5842d98 100644 --- a/skills/sjskills/SKILL.md +++ b/skills/sjskills/SKILL.md @@ -126,7 +126,9 @@ independent unblocked scope unless the user required an all-or-nothing result. When intended content or the registry changed, reconciliation can only consume the published remote content. Do not apply a local-only change; a -registry change needs no new `sjskills` release once it reaches `main`. +registry change needs no new `sjskills` release once it reaches `main`. The +exception is a project `[[direct]]` entry with a local path source, which +installs the working tree's current content. If publication is requested, follow the source repository's instructions for validation, review, commit, and publication; those mechanics are outside this skill. Before reconciling, verify that the commit named by the plan's diff --git a/skills/sjskills/references/project-selection.md b/skills/sjskills/references/project-selection.md index 7a65923..5700a6e 100644 --- a/skills/sjskills/references/project-selection.md +++ b/skills/sjskills/references/project-selection.md @@ -37,13 +37,24 @@ source = "your-org/team-skills" Use the source's actual skill name, not an alias. Repeat `[[direct]]` for more skills, sorting entries by name and profile names alphabetically. Direct names must be unique and cannot overlap selected profiles or the fixed global -baseline. Sources accept Git shorthand (`owner/repo[/path]`) or credential-free -HTTPS; local paths, embedded credentials, URL queries, npm specifiers, and other -schemes are unsupported. Set optional `full_depth = true` only when deeper +baseline. Sources accept Git shorthand (`owner/repo[/path]`), credential-free +HTTPS, or a local skill directory; embedded credentials, URL queries, npm +specifiers, and other schemes are unsupported. Set optional `full_depth = true` only when deeper source discovery is needed. Direct skills use copy mode and the registry's default targets, currently project `.agents/skills` and `.claude/skills`; per-entry manager, mode, target, and workflow fields are unsupported. +For a skill the project develops itself, point `source` at its directory with +a forward-slash path relative to `sjskills.toml`, such as +`source = "./skills/team-tool"`, and use its `SKILL.md` name. `apply` copies +the current tree; after editing it, `plan` reports an update. Prefer in-repo +paths: `../` and absolute paths work but warn as machine-specific. Do not point +at a generated `.agents/skills` or `.claude/skills` copy, and do not set +`access` or `full_depth`. A missing source or mismatched name makes `plan` +fail as invalid input; restore the directory or remove the entry. Switching an +installed skill between a remote and local source blocks as +`provenance-source-mismatch`: remove the entry and apply, then re-add it. + For a direct-only project, omit `profiles` or use `profiles = []`, and include at least one `[[direct]]` entry. Create that manifest directly when adoption is requested: `sjskills init` requires a profile and cannot initialize this case. From 49ccf61de6625b834f4ac4c668c4ba8e65521351 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Tue, 6 Oct 2026 10:18:47 +0900 Subject: [PATCH 2/2] fix(sjskills): harden local-source validation per review feedback - Inspect, hash, and copy a local source through the one symlink-resolved path, so a swapped ancestor symlink cannot make staging read a different tree than validation saw. The tree walk now rejects any sjskills.toml, which also catches sources that contain the project root through a symlinked ancestor without needing the root at materialization time. - Inspect every local source before Bun preflight or remote fetches, so a broken manifest entry reports invalid_source instead of an unrelated tooling or network outage. - Report edits racing the staging copy as retryable materialization errors (unavailable) rather than invalid configuration. - Parse quoted SKILL.md names followed by a YAML comment. - Serve local-only status without touching the upstream cache or its lock. Co-Authored-By: Claude Opus 5.5 --- docs/skill-registry.md | 5 +- internal/sjskills/local_source.go | 48 ++++++++++++------- internal/sjskills/local_source_test.go | 64 ++++++++++++++++++++++++-- internal/sjskills/materialize.go | 8 ++++ internal/sjskills/status.go | 19 +++++++- 5 files changed, 119 insertions(+), 25 deletions(-) diff --git a/docs/skill-registry.md b/docs/skill-registry.md index e90c8a2..16d3e79 100644 --- a/docs/skill-registry.md +++ b/docs/skill-registry.md @@ -108,8 +108,9 @@ then treats it like any fetched tree. Provenance records produces an ordinary update. The directory must be a real directory with a `SKILL.md` whose frontmatter `name` matches the entry, contain only regular files and directories, not lie inside a generated `.agents/skills`, -`.claude/skills`, or `.sjskills` directory, and be neither a project root nor -contain generated skill roots. A missing or invalid source makes `plan` and +`.claude/skills`, or `.sjskills` directory, and neither contain generated +skill roots nor be or contain a project (any `sjskills.toml`). Checks use the +symlink-resolved path, and the copy is read from that same path. A missing or invalid source makes `plan` and `apply` fail as invalid input (exit 65) with a message naming it; status reports the project scope unavailable with the same message. Switching an installed skill between remote and local sources, or respelling its path, diff --git a/internal/sjskills/local_source.go b/internal/sjskills/local_source.go index 9d0a49a..de8cb13 100644 --- a/internal/sjskills/local_source.go +++ b/internal/sjskills/local_source.go @@ -166,14 +166,16 @@ func resolveLocalSourcePath(projectRoot, source string) (string, error) { // inspectLocalSkillSource proves that a local source is a real skill // directory that the copy-mode placement contract can represent, and returns -// its tree hash. Every failure names the manifest source so the user can fix -// or remove the declaration. -func inspectLocalSkillSource(skill DesiredSkill, limits MaterializerLimits) (TreeHash, error) { - fail := func(message string, cause error) (TreeHash, error) { +// its canonical path and tree hash. Every read after symlink resolution uses +// that one canonical path, so validation, hashing, and staging see the same +// tree. Every failure names the manifest source so the user can fix or remove +// the declaration. +func inspectLocalSkillSource(skill DesiredSkill, limits MaterializerLimits) (string, TreeHash, error) { + fail := func(message string, cause error) (string, TreeHash, error) { if cause != nil { message += ": " + cause.Error() } - return TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: message} + return "", TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: message} } if skill.LocalPath == "" || !filepath.IsAbs(skill.LocalPath) { return fail("has no resolved absolute path", nil) @@ -195,24 +197,23 @@ func inspectLocalSkillSource(skill DesiredSkill, limits MaterializerLimits) (Tre if insideManagedDirectory(canonical) { return fail("is inside a generated .agents/skills, .claude/skills, or .sjskills directory; point it at the skill's own source", nil) } - if _, err := os.Lstat(filepath.Join(canonical, ManifestFileName)); err == nil { - return fail("is a project root containing sjskills.toml, not a skill directory", nil) - } + // The walk also rejects any sjskills.toml, which is how a source that + // contains this (or any) project root is caught after symlink resolution. if err := validateLocalSkillTree(canonical, limits); err != nil { return fail("cannot be installed", err) } - declared, err := localSkillDeclaredName(filepath.Join(skill.LocalPath, "SKILL.md")) + declared, err := localSkillDeclaredName(filepath.Join(canonical, "SKILL.md")) if err != nil { return fail("is not a skill", err) } if declared != skill.Name { return fail(fmt.Sprintf("declares SKILL.md name %q, not %q", declared, skill.Name), nil) } - digest, err := hashSkillTree(skill.LocalPath, limits) + digest, err := hashSkillTree(canonical, limits) if err != nil { return fail("cannot be hashed", err) } - return digest, nil + return canonical, digest, nil } func insideManagedDirectory(canonical string) bool { @@ -270,6 +271,8 @@ func validateLocalSkillTree(root string, limits MaterializerLimits) error { } case !info.Mode().IsRegular(): return fmt.Errorf("%s is not a regular file", filepath.ToSlash(relative)) + case child.Name() == ManifestFileName: + return fmt.Errorf("%s is a project manifest; a skill source must not be or contain a project", filepath.ToSlash(relative)) } } return nil @@ -303,8 +306,17 @@ func localSkillDeclaredName(skillFile string) (string, error) { continue } value = strings.TrimSpace(value) - if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') && value[len(value)-1] == value[0] { - value = value[1 : len(value)-1] + if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') { + // A quoted scalar ends at its closing quote; only a comment may follow. + end := strings.IndexByte(value[1:], value[0]) + if end < 0 { + break + } + rest := strings.TrimSpace(value[end+2:]) + if rest != "" && !strings.HasPrefix(rest, "#") { + break + } + value = value[1 : end+1] } else if before, _, found := strings.Cut(value, " #"); found { value = strings.TrimSpace(before) } @@ -320,7 +332,7 @@ func localSkillDeclaredName(skillFile string) (string, error) { // at the same path a Skills CLI install would use, then proves the copy equals // the source as inspected. A source edited mid-copy is reported, not adopted. func stageLocalSkill(root string, skill DesiredSkill, limits MaterializerLimits) (string, TreeHash, error) { - sourceHash, err := inspectLocalSkillSource(skill, limits) + sourcePath, sourceHash, err := inspectLocalSkillSource(skill, limits) if err != nil { return "", TreeHash{}, err } @@ -332,8 +344,10 @@ func stageLocalSkill(root string, skill DesiredSkill, limits MaterializerLimits) return "", TreeHash{}, materializationError(safeSkillName(skill.Name), "local source staging directory could not be created", err) } remaining := normalizedLimits(limits).MaxTreeBytes - if err := copyLocalSkillTree(skill.LocalPath, destination, &remaining); err != nil { - return "", TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: "could not be copied: " + err.Error()} + // Failures from here on are races with concurrent edits, not configuration + // problems, so they stay ordinary (retryable) materialization errors. + if err := copyLocalSkillTree(sourcePath, destination, &remaining); err != nil { + return "", TreeHash{}, materializationError(safeSkillName(skill.Name), "local source could not be copied; retry", err) } staged, err := locateStagedSkill(root, skill.Name) if err != nil { @@ -344,7 +358,7 @@ func stageLocalSkill(root string, skill DesiredSkill, limits MaterializerLimits) return "", TreeHash{}, err } if stagedHash != sourceHash { - return "", TreeHash{}, &LocalSourceError{Skill: safeSkillName(skill.Name), Source: skill.Source, Problem: "changed while it was copied; retry"} + return "", TreeHash{}, materializationError(safeSkillName(skill.Name), "local source changed while it was copied; retry", nil) } return staged, stagedHash, nil } diff --git a/internal/sjskills/local_source_test.go b/internal/sjskills/local_source_test.go index 25c067f..491f1fb 100644 --- a/internal/sjskills/local_source_test.go +++ b/internal/sjskills/local_source_test.go @@ -2,6 +2,7 @@ package sjskills import ( "context" + "errors" "os" "path/filepath" "runtime" @@ -252,8 +253,8 @@ func TestLocalSourceRejectsParentOnlyAndProjectRoots(t *testing.T) { if err := os.WriteFile(filepath.Join(project, ManifestFileName), []byte("version = 1\n"), 0o644); err != nil { t.Fatal(err) } - _, err := inspectLocalSkillSource(localDesiredSkill(project), MaterializerLimits{}) - if err == nil || !strings.Contains(err.Error(), "is a project root") { + _, _, err := inspectLocalSkillSource(localDesiredSkill(project), MaterializerLimits{}) + if err == nil || !strings.Contains(err.Error(), "sjskills.toml is a project manifest") { t.Fatalf("project-root source error = %v", err) } if err := os.Remove(filepath.Join(project, ManifestFileName)); err != nil { @@ -262,7 +263,7 @@ func TestLocalSourceRejectsParentOnlyAndProjectRoots(t *testing.T) { if err := os.MkdirAll(filepath.Join(project, ".claude", "skills"), 0o755); err != nil { t.Fatal(err) } - _, err = inspectLocalSkillSource(localDesiredSkill(project), MaterializerLimits{}) + _, _, err = inspectLocalSkillSource(localDesiredSkill(project), MaterializerLimits{}) if err == nil || !strings.Contains(err.Error(), ".claude/skills is a generated") { t.Fatalf("source containing a managed root error = %v", err) } @@ -335,7 +336,62 @@ func TestStatusReportsLocalSourceEditAsUpdate(t *testing.T) { } now := time.Date(2026, 10, 6, 1, 0, 0, 0, time.UTC) service := StatusService{CacheRoot: filepath.Join(t.TempDir(), "status"), Now: func() time.Time { return now }, - Refresh: func(context.Context, []DesiredSkill) (StatusSnapshot, error) { return StatusSnapshot{map[string]TreeHash{}, now}, nil }} + Refresh: func(context.Context, []DesiredSkill) (StatusSnapshot, error) { + return StatusSnapshot{map[string]TreeHash{}, now}, nil + }} advisory := service.Check(context.Background(), scope, nil) requireStatusFinding(t, advisory, AdvisoryUpdate, "team-tool", TargetClaude, string(ProjectStateReasonVerifiedUpdate)) } + +func TestLocalSkillDeclaredNameStripsCommentAfterQuotedValue(t *testing.T) { + path := filepath.Join(t.TempDir(), "SKILL.md") + for content, want := range map[string]string{ + "---\nname: \"team-tool\" # maintained locally\n---\n": "team-tool", + "---\nname: 'team-tool'\n---\n": "team-tool", + } { + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + if name, err := localSkillDeclaredName(path); err != nil || name != want { + t.Fatalf("%q: name = %q err=%v", content, name, err) + } + } +} + +func TestMaterializeReportsBrokenLocalSourceBeforeRemoteTooling(t *testing.T) { + missing := filepath.Join(t.TempDir(), "team-tool") + runner := &materializeRunner{invoke: func(context.Context, string, []string, []string) (ProcessResult, error) { + return ProcessResult{}, errors.New("bunx unavailable") + }} + materializer, _ := testMaterializer(t, runner, MaterializerLimits{}) + _, err := materializer.Materialize(context.Background(), []DesiredSkill{localDesiredSkill(missing), desiredMaterializeSkill("remote-tool", "example/remote-catalog")}) + var localErr *LocalSourceError + if !errors.As(err, &localErr) || len(runner.calls) != 0 { + t.Fatalf("error = %v, calls = %d; want a local source error before any process", err, len(runner.calls)) + } +} + +func TestLocalOnlyStatusDoesNotDependOnUpstreamCache(t *testing.T) { + root := canonicalTempHome(t) + writeLocalSkill(t, filepath.Join(root, "skills", "team-tool"), "team-tool") + if err := os.WriteFile(filepath.Join(root, ManifestFileName), []byte("version = 1\n[[direct]]\nname = \"team-tool\"\nsource = \"./skills/team-tool\"\n"), 0o644); err != nil { + t.Fatal(err) + } + scope, err := ResolveStatusScope(root, minimalGlobalRegistry(t), false) + if err != nil { + t.Fatal(err) + } + blocked := filepath.Join(t.TempDir(), "not-a-directory") + if err := os.WriteFile(blocked, nil, 0o644); err != nil { + t.Fatal(err) + } + service := StatusService{CacheRoot: blocked, Refresh: func(context.Context, []DesiredSkill) (StatusSnapshot, error) { + t.Fatal("local-only status refreshed upstream evidence") + return StatusSnapshot{}, nil + }} + advisory := service.Check(context.Background(), scope, nil) + if advisory.Freshness != AdvisoryFresh || advisory.Error != "" || validateAdvisories([]Advisory{advisory}) != nil { + t.Fatalf("advisory = %+v", advisory) + } + requireStatusFinding(t, advisory, AdvisoryMissing, "team-tool", TargetAgents, string(ProjectStateReasonExpectedEntryAbsent)) +} diff --git a/internal/sjskills/materialize.go b/internal/sjskills/materialize.go index 39a5945..8eff156 100644 --- a/internal/sjskills/materialize.go +++ b/internal/sjskills/materialize.go @@ -317,6 +317,14 @@ func (m *Materializer) Materialize(ctx context.Context, skills []DesiredSkill) ( local = append(local, skill) } } + // Report a broken local source before any tool or network dependency can + // fail first and hide the deterministic configuration error. + for _, skill := range local { + if _, _, err := inspectLocalSkillSource(skill, m.limits); err != nil { + _ = plan.Cleanup() + return nil, err + } + } if len(remote) > 0 { if err := m.preflightIn(ctx, root); err != nil { if !errors.Is(err, errProcessTreeActive) { diff --git a/internal/sjskills/status.go b/internal/sjskills/status.go index c0a2c17..174ab7f 100644 --- a/internal/sjskills/status.go +++ b/internal/sjskills/status.go @@ -333,7 +333,7 @@ func withLocalStatusExpected(desired DesiredState, cached map[string]TreeHash) ( if skill.Manager != ManagerSkillsCLI || skill.LocalPath == "" { continue } - hash, err := inspectLocalSkillSource(skill, MaterializerLimits{}) + _, hash, err := inspectLocalSkillSource(skill, MaterializerLimits{}) if err != nil { return nil, err } @@ -360,6 +360,11 @@ func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *S result := newAdvisory(scope.Plan.Desired.Scope) cachedDesired := statusCachedDesired(scope.Plan.Desired) now := s.now() + if !hasSkillsCLIPlacement(cachedDesired) { + // No upstream content is needed, so the cache and its lock cannot + // make live local status unavailable. + return s.inspectStatus(result, scope, map[string]TreeHash{}, now) + } entry, err := s.read(scope) if err != nil { entry = statusCacheEntry{Version: statusCacheVersion, Identity: scope.cacheKey()} @@ -427,7 +432,17 @@ func (s StatusService) Check(ctx context.Context, scope StatusScope, reusable *S result.Freshness = AdvisoryStale result.Cached = true } - expected, err := withLocalStatusExpected(scope.Plan.Desired, entry.Expected) + return s.inspectStatus(result, scope, entry.Expected, entry.ObservedAt) +} + +// inspectStatus adds live local hashes to cached upstream evidence observed at +// observedAt and classifies the current inventory. +func (s StatusService) inspectStatus(result Advisory, scope StatusScope, cached map[string]TreeHash, observedAt time.Time) Advisory { + if result.ObservedAt == nil { + result.ObservedAt = &observedAt + result.Freshness = AdvisoryFresh + } + expected, err := withLocalStatusExpected(scope.Plan.Desired, cached) if err != nil { result.Freshness = AdvisoryUnavailable result.ObservedAt = nil