From 24495f400c0f8f702da0571df4b4093fbeeebe65 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 13:30:48 +0900 Subject: [PATCH 1/7] Plan reading the sjskills registry from the published source The executable embeds the registry while installing skill contents from main at run time, so catalog changes need a release and the two can drift unnoticed. The plan fetches skill-registry.json at the same commit as the contents, records that commit in plan evidence, pins global apply to the reviewed commit, caches registries for offline status, and removes the embedded copy. A release is then needed only for schema or CLI changes. Decided with the user on 2026-09-30; implementation starts with a spike confirming Skills CLI 1.5.23 installs commit-pinned public sources. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 3 + plans/sjskills-remote-registry.md | 101 ++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 plans/sjskills-remote-registry.md diff --git a/ROADMAP.md b/ROADMAP.md index f0aff58..9315b71 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -12,6 +12,9 @@ 1. [Roll out the fixed global skill baseline](plans/sjskills-global-rollout.md) is proposed and requires separate evidence-bound authorization for each machine. Exact-content approval binding is delivered. +2. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) + is proposed so registry changes stop requiring an sjskills release; the + Skills CLI commit-pinning spike is next. ## Tasks diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md new file mode 100644 index 0000000..320b667 --- /dev/null +++ b/plans/sjskills-remote-registry.md @@ -0,0 +1,101 @@ +# Read the skill registry from the published source + +## Outcome + +`sjskills` reads `skill-registry.json` from the published agent-scripts source +at the same commit as the skill contents it installs. Changing profiles, the +global baseline, targets, or skill entries takes effect once it reaches `main`, +without a new `sjskills` release. A release is needed only when the registry +schema or the CLI itself changes. + +## Current state + +Proposed on 2026-09-30. Not started. + +The executable embeds `internal/sjskills/data/registry-v4.json` +(`internal/sjskills/registry.go`) and reads no other registry, while skill +contents come from `.../tree/main/skills` at run time. The two can drift +unnoticed: the CLI release notice compares version numbers only, and nothing +records which commit supplied the contents. `PROGRESS.md` records that syncing +a registry change on 2026-09-10 needed a temporary binary until v1.2.0 shipped it. + +## Decisions + +- **Source of truth.** The binary embeds the registry's location (repository, + branch `main`, path `skill-registry.json`), not its contents. Remove the + embedded registry data and the Node test that keeps it equal to the root + file. Tests use fixtures through the existing `loadRegistry` seam. +- **One commit per invocation.** Resolve `main` to a commit once, fetch the + registry at that commit, and pin the agent-scripts source for every skill to + `.../tree//skills`. Load the registry once per command and share it + across `prepare`, status, and notices; today `plan`/`apply` load it three times. +- **Evidence.** Replace the `registry: embedded version 4` evidence with the + registry commit and SHA-256. It then flows into the reviewed-plan semantic + comparison unchanged. Global apply uses the commit recorded in the reviewed + plan instead of re-resolving `main`, so a push between plan and apply cannot + change the reviewed desired state. Project apply resolves fresh, as it has no + artifact. +- **Caching and offline use.** Cache verified registries by commit in a new + `registry/` namespace, reusing the status cache's bounded read, lock, and + atomic-write helpers. `status`, `profiles`, and `init` may use the last + verified registry when resolution fails and must label it stale with its + commit and age. Status refreshes follow the existing 24-hour interval and + 15-minute cooldown. `plan` and `apply` always require a fresh resolution; + they already need the network. With no cache and no network, status reports + the registry unavailable, which is an existing supported state. +- **Schema compatibility.** Keep exact `RegistryVersion` equality. A fetched + registry with a different version fails with a message that names the + required `sjskills` update; never partially interpret it. +- **Resolution method.** The implementer chooses, subject to no required + credentials, bounded time and size, and tolerance of unauthenticated GitHub + API rate limits, since the startup hook runs status often. The hook's + resolve-then-fetch in `plugins/sjskills-maintenance/scripts/observation.cjs` + is a precedent. +- **No local override.** Reconciliation consumes only published content + (`AGENTS.md`), so the CLI gets no flag to read a working-tree registry. + Development validates the root file with `scripts/validate-skills` and the + Node registry tests. +- **Unchanged.** Other external and authenticated sources keep their own refs. + Provenance identity stays `github:owner/repo`, so pinning the ref does not + change ownership of installed copies. + +## Sequence + +1. Spike: confirm Skills CLI 1.5.23 installs a public `.../tree//skills` + source correctly (only the authenticated path handles commits today). If it + does not, materialize the agent-scripts source through the existing + commit-fetch path and record that choice here. +2. Add registry resolution, fetch, validation, and cache with the fixed + location; load once per invocation. +3. Pin agent-scripts sources to the resolved commit and add registry + commit/digest evidence; make global apply reuse the reviewed commit. +4. Remove the embedded registry data and equality test; move Go tests to + fixtures. +5. Update `docs/skill-registry.md`, `docs/sjskills-releases.md`, + `skills/sjskills/` (including its global-rollout reference), and comments + that describe the embedded registry. +6. Release a new minor version and install it on each machine under the + existing release process. + +## Acceptance and validation + +- Changing a profile on `main` changes the next plan's desired state with the + same binary. +- A reviewed global plan applies against its recorded commit even after `main` + moves, and a tampered commit or digest fails the reviewed-plan check. +- Offline `status`, `profiles`, and `init` use a cached registry with a stale + label; with no cache they report the registry unavailable without failing. +- A registry with an unsupported version fails closed with an update message. +- Contents and registry in one plan come from the same commit. +- Full Go suite, Node tests, `scripts/validate-skills`, and native release + checks on every release target pass. + +## Out of scope + +Registry schema changes beyond evidence, CLI self-update, changes to external +or private sources, and rollout of the new binary beyond the machines the +release step authorizes. + +## Next action + +Run the step 1 spike. From d123bbb2b80879570249dc1e3d6c408b388c74f8 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 13:34:05 +0900 Subject: [PATCH 2/7] Record the commit-pinned source spike for the remote registry plan Skills CLI 1.5.23 cannot install `.../tree//skills`: it clones with `--branch `, which rejects a commit hash. An anonymous shallow Git fetch of the commit followed by a local-path `skills add` installs a tree identical to that commit, so public agent-scripts materialization will reuse the authenticated path's commit fetch and gain a Git prerequisite. `git ls-remote` resolves main without API rate limits, and the raw registry at a commit matches `git show`. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 ++-- plans/sjskills-remote-registry.md | 33 +++++++++++++++++++++++++------ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 9315b71..b6ce921 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -13,8 +13,8 @@ is proposed and requires separate evidence-bound authorization for each machine. Exact-content approval binding is delivered. 2. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) - is proposed so registry changes stop requiring an sjskills release; the - Skills CLI commit-pinning spike is next. + is proposed so registry changes stop requiring an sjskills release. The + spike chose a Git commit fetch; registry resolution and caching are next. ## Tasks diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md index 320b667..f53b55f 100644 --- a/plans/sjskills-remote-registry.md +++ b/plans/sjskills-remote-registry.md @@ -10,7 +10,7 @@ schema or the CLI itself changes. ## Current state -Proposed on 2026-09-30. Not started. +Proposed on 2026-09-30. The step 1 spike is complete; implementation has not started. The executable embeds `internal/sjskills/data/registry-v4.json` (`internal/sjskills/registry.go`) and reads no other registry, while skill @@ -61,10 +61,11 @@ a registry change on 2026-09-10 needed a temporary binary until v1.2.0 shipped i ## Sequence -1. Spike: confirm Skills CLI 1.5.23 installs a public `.../tree//skills` - source correctly (only the authenticated path handles commits today). If it - does not, materialize the agent-scripts source through the existing - commit-fetch path and record that choice here. +1. ~~Spike: confirm Skills CLI 1.5.23 installs a commit-pinned public source.~~ + Done 2026-09-30; see [Spike result](#spike-result). Materialize the + agent-scripts source by fetching the commit with Git and passing the local + `skills/` path to Skills CLI, generalizing the authenticated path's + commit fetch to anonymous public sources. 2. Add registry resolution, fetch, validation, and cache with the fixed location; load once per invocation. 3. Pin agent-scripts sources to the resolved commit and add registry @@ -77,6 +78,26 @@ a registry change on 2026-09-10 needed a temporary binary until v1.2.0 shipped i 6. Release a new minor version and install it on each machine under the existing release process. +## Spike result + +Run on Windows on 2026-09-30 with the materializer's isolated environment and +Skills CLI 1.5.23, installing `next-goal` from `bbae6d1`, whose tree differs +from `main`: + +- `.../tree/<40-hex commit>/skills` fails: Skills CLI clones with + `--branch `, and Git reports the remote branch not found. A nonexistent + commit fails the same way. `.../tree/main/skills` installs correctly. +- Anonymous `git init`, `fetch --depth=1 --no-tags `, and + `checkout --detach FETCH_HEAD`, then `skills add /skills`, installs + a tree identical to `git archive bbae6d1 skills/next-goal`. +- Anonymous `git ls-remote refs/heads/main` resolves `main` without API + rate limits, and `raw.githubusercontent.com////skill-registry.json` + returns the registry at that commit, matching `git show`. + +Consequence: public agent-scripts materialization gains a Git prerequisite that +today applies only to authenticated sources. Resolution uses `git ls-remote`; +the registry fetch may use the raw URL or the same shallow checkout. + ## Acceptance and validation - Changing a profile on `main` changes the next plan's desired state with the @@ -98,4 +119,4 @@ release step authorizes. ## Next action -Run the step 1 spike. +Implement step 2: registry resolution, fetch, validation, and cache. From 4fe85e075336afde33a400fb80d374e94223ff6b Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 13:36:06 +0900 Subject: [PATCH 3/7] Mark the global rollout plan complete The roadmap still described the global rollout as proposed and awaiting per-machine authorization, but its plan records the evidence binding as delivered and assigns per-machine outcomes to their execution artifacts. Today's configured global syncs ran under that contract, so the plan now states its outcome and completion and moves to the completed list. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 8 ++++---- plans/sjskills-global-rollout.md | 19 +++++++++++++++++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index b6ce921..ccfe061 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -9,10 +9,7 @@ ## Plans -1. [Roll out the fixed global skill baseline](plans/sjskills-global-rollout.md) - is proposed and requires separate evidence-bound authorization for each - machine. Exact-content approval binding is delivered. -2. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) +1. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) is proposed so registry changes stop requiring an sjskills release. The spike chose a Git commit fetch; registry resolution and caching are next. @@ -22,6 +19,9 @@ _None._ ## Completed +- [Roll out the fixed global skill baseline](plans/sjskills-global-rollout.md): + evidence-bound configured sync is delivered; per-machine runs keep their own + execution evidence. - [Install private GitHub skills through authenticated profiles](plans/sjskills-private-github-sources.md) is recorded as shipped in v1.3.0; live private-skill installation remains separate. - [Prepare and spawn the next goal](tasks/next-goal-preparation-spawn.md) diff --git a/plans/sjskills-global-rollout.md b/plans/sjskills-global-rollout.md index 0df86ed..0213b67 100644 --- a/plans/sjskills-global-rollout.md +++ b/plans/sjskills-global-rollout.md @@ -1,7 +1,22 @@ # Global reconciliation rollout -Status: evidence binding implemented; configured sync uses request authority. -Per-machine outcomes belong with their retained execution artifacts. +## Outcome + +Global skill reconciliation runs safely on any machine from a configured-sync +request, bound to agent-reviewed plan evidence, without a separate human +approval ceremony. + +## Current state + +Complete. Evidence binding is implemented and configured sync uses request +authority. On 2026-09-30, two configured global syncs on `DESKTOP-V33SCUA` followed +this contract with verified sjskills v1.3.0 and ended with exact final global +plans. Per-machine outcomes belong with their retained execution artifacts, not +with this plan. + +## Next action + +None — complete. ## Delivered contract From 428871a32fbc58566c2424fc7254be7c076c844b Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 14:01:38 +0900 Subject: [PATCH 4/7] Resolve and cache the published skill registry Step 2 of the remote-registry plan. RegistrySource resolves refs/heads/main from Git's anonymous smart-HTTP ref advertisement, fetches skill-registry.json from the raw URL at that commit, validates it, and records its SHA-256. The ref advertisement has the same protocol and rate-limit profile as `git ls-remote` but keeps the frequent status path free of child processes. Verified registries are cached by commit in a `registry/` namespace that reuses the status cache's bounded reads, lock, and atomic writes. Status reuses a resolution for 24 hours and honors the 15-minute retry cooldown; selection commands always try to refresh. Both fall back to the last verified registry marked stale, while an unsupported or invalid published registry fails closed so a cached copy cannot hide a needed update. The CLI now loads its registry once per invocation instead of three times for plan and apply. Production still reads the embedded registry until step 3 switches loading, pins sources, and records commit evidence together. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 5 +- cmd/sjskills/default_status_test.go | 27 ++ cmd/sjskills/main.go | 18 +- internal/sjskills/registry_source.go | 435 ++++++++++++++++++++++ internal/sjskills/registry_source_test.go | 289 ++++++++++++++ plans/sjskills-remote-registry.md | 25 +- 6 files changed, 787 insertions(+), 12 deletions(-) create mode 100644 internal/sjskills/registry_source.go create mode 100644 internal/sjskills/registry_source_test.go diff --git a/ROADMAP.md b/ROADMAP.md index ccfe061..f6d257a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -10,8 +10,9 @@ ## Plans 1. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) - is proposed so registry changes stop requiring an sjskills release. The - spike chose a Git commit fetch; registry resolution and caching are next. + is proposed so registry changes stop requiring an sjskills release. + Registry resolution and caching are implemented; switching production + loading, commit pinning, and evidence are next. ## Tasks diff --git a/cmd/sjskills/default_status_test.go b/cmd/sjskills/default_status_test.go index dd1463f..d912bc4 100644 --- a/cmd/sjskills/default_status_test.go +++ b/cmd/sjskills/default_status_test.go @@ -405,3 +405,30 @@ func TestDefaultStatusAlreadyCancelled(t *testing.T) { t.Fatalf("cancel %d %q %q", code, out.String(), errout.String()) } } + +func TestRegistryLoadsOncePerInvocation(t *testing.T) { + f := newStatusCLIFixture(t, "") + materializer, _ := testInjectedMaterializer(t) + service := sjskills.StatusService{CacheRoot: f.cache, Refresh: func(context.Context, []sjskills.DesiredSkill) (sjskills.StatusSnapshot, error) { + return sjskills.StatusSnapshot{}, errors.New("test upstream unavailable") + }} + loads := 0 + app := &application{ + directory: f.project, + homeDirectory: func() (string, error) { return f.home, nil }, + statusService: &service, + cliStatusService: f.cliService(), + materialize: materializer.Materialize, + loadRegistry: func() (sjskills.Registry, error) { + loads++ + return sjskills.EmbeddedRegistry() + }, + } + if envelope := app.plan(context.Background(), true); envelope.Result != sjskills.ResultSuccess { + t.Fatalf("plan %+v", envelope.Error) + } + app.collectStatus(context.Background()) + if loads != 1 { + t.Fatalf("registry loaded %d times", loads) + } +} diff --git a/cmd/sjskills/main.go b/cmd/sjskills/main.go index c35c819..540d276 100644 --- a/cmd/sjskills/main.go +++ b/cmd/sjskills/main.go @@ -94,7 +94,10 @@ func (c *restoreCommand) Run(ctx *commandContext) error { type application struct { loadRegistry func() (sjskills.Registry, error) - noStatusCheck bool + registryLoaded bool + registryValue sjskills.Registry + registryErr error + noStatusCheck bool statusSnapshot *commandStatusSnapshot cliStatusService *sjskills.CLIStatusService statusService *sjskills.StatusService @@ -144,11 +147,18 @@ func productionMaterialize(ctx context.Context, skills []sjskills.DesiredSkill) return sjskills.NewMaterializer(sjskills.MaterializerConfig{}).Materialize(ctx, skills) } +// registry loads the registry at most once per invocation, so preparation, +// the status snapshot, and incidental notices all read the same desired state. func (a *application) registry() (sjskills.Registry, error) { - if a.loadRegistry != nil { - return a.loadRegistry() + if !a.registryLoaded { + if a.loadRegistry != nil { + a.registryValue, a.registryErr = a.loadRegistry() + } else { + a.registryValue, a.registryErr = sjskills.EmbeddedRegistry() + } + a.registryLoaded = true } - return sjskills.EmbeddedRegistry() + return a.registryValue, a.registryErr } func (a *application) base(operation sjskills.CommandOperation) sjskills.Envelope { diff --git a/internal/sjskills/registry_source.go b/internal/sjskills/registry_source.go new file mode 100644 index 0000000..aa5c21e --- /dev/null +++ b/internal/sjskills/registry_source.go @@ -0,0 +1,435 @@ +package sjskills + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "time" +) + +// The binary embeds where the registry is published, never its contents. +// Reconciliation consumes only published content, so there is no local override. +const ( + registryRepository = releaseRepository + registryBranchRef = "refs/heads/main" + registryFile = "skill-registry.json" + registryRefsURL = "https://github.com/" + registryRepository + ".git/info/refs?service=git-upload-pack" + registryRawBase = "https://raw.githubusercontent.com/" + registryRepository + "/" + + maxRegistryBytes = 1 << 20 + maxRefAdvertisement = 4 << 20 + registryCacheSchema = 1 + registryLatestFile = "latest.json" + registryLatestLockFile = "latest.lock" +) + +var registryCommitPattern = regexp.MustCompile(`^[0-9a-f]{40}$`) + +// PublishedRegistry is a validated registry together with the commit that +// supplied it and the SHA-256 of its exact published bytes. ResolvedAt is when +// the branch was last observed at Commit; it is zero for a registry loaded at an +// explicitly reviewed commit. Stale marks a cached registry used because a +// refresh failed or is cooling down; StaleReason says why. +type PublishedRegistry struct { + Registry Registry + Commit string + SHA256 string + ResolvedAt time.Time + Stale bool + StaleReason string +} + +// RegistrySource resolves the published branch, fetches the registry at the +// resolved commit, and caches verified registries by commit. Resolution uses +// Git's anonymous smart-HTTP ref advertisement, which needs no credentials, +// Git executable, or GitHub API quota. +type RegistrySource struct { + CacheRoot string + Now func() time.Time + Client *http.Client + + // Tests replace the published endpoints; production uses the fixed location. + refsURL string + rawBase string +} + +type registryLatest struct { + Schema int `json:"schema"` + Source string `json:"source"` + Ref string `json:"ref"` + Commit string `json:"commit"` + SHA256 string `json:"sha256"` + ResolvedAt time.Time `json:"resolvedAt"` + RetryAt time.Time `json:"retryAt"` +} + +func (e registryLatest) resolved() bool { return e.Commit != "" } + +func (s RegistrySource) now() time.Time { + if s.Now != nil { + return s.Now().UTC() + } + return time.Now().UTC() +} + +func (s RegistrySource) client() *http.Client { + if s.Client != nil { + return s.Client + } + return &http.Client{Timeout: StatusRefreshBudget, CheckRedirect: func(*http.Request, []*http.Request) error { return errors.New("registry redirect refused") }} +} + +// Resolve observes the published branch now and never falls back to a cached +// registry. Plan and apply use it because they need current desired state. +func (s RegistrySource) Resolve(ctx context.Context) (PublishedRegistry, error) { + directory, cacheErr := s.cacheDirectory() + commit, err := s.resolveCommit(ctx) + if err != nil { + return PublishedRegistry{}, err + } + published, err := s.load(ctx, directory, cacheErr, commit) + if err != nil { + return PublishedRegistry{}, err + } + published.ResolvedAt = s.now() + if cacheErr == nil { + // The cache only accelerates later status; a write failure never + // invalidates evidence that was just verified. + _ = s.recordLatest(directory, registryLatest{Commit: commit, SHA256: published.SHA256, ResolvedAt: published.ResolvedAt}) + } + return published, nil +} + +// At loads the registry at an explicitly reviewed commit without consulting +// the branch, so a push after review cannot change the reviewed desired state. +func (s RegistrySource) At(ctx context.Context, commit string) (PublishedRegistry, error) { + if !registryCommitPattern.MatchString(commit) { + return PublishedRegistry{}, &Issue{Code: IssueMalformedInput, Path: "registry.commit", Message: "registry commit must be 40 lowercase hexadecimal characters"} + } + directory, cacheErr := s.cacheDirectory() + return s.load(ctx, directory, cacheErr, commit) +} + +// ForStatus reuses a registry resolved within the status refresh interval and +// otherwise refreshes, honoring the status retry cooldown. When refreshing is +// not possible it returns the last verified registry marked stale. +func (s RegistrySource) ForStatus(ctx context.Context) (PublishedRegistry, error) { + return s.recent(ctx, statusRefreshInterval, true) +} + +// ForSelection always attempts a refresh for commands that list or validate +// profiles, falling back to the last verified registry marked stale. +func (s RegistrySource) ForSelection(ctx context.Context) (PublishedRegistry, error) { + return s.recent(ctx, 0, false) +} + +func (s RegistrySource) recent(ctx context.Context, maxAge time.Duration, cooldown bool) (PublishedRegistry, error) { + directory, err := s.cacheDirectory() + if err != nil { + return s.Resolve(ctx) + } + latest, _ := s.readLatest(directory) + now := s.now() + if latest.resolved() && maxAge > 0 && !latest.ResolvedAt.After(now) && now.Sub(latest.ResolvedAt) < maxAge { + if cached, err := s.cachedAt(directory, latest); err == nil { + return cached, nil + } + } + if cooldown && !(statusCacheEntry{RetryAt: latest.RetryAt}).retryDue(now) { + return s.staleOrError(directory, latest, errors.New("registry refresh cooldown active")) + } + published, resolveErr := s.Resolve(ctx) + if resolveErr == nil { + return published, nil + } + var issue *Issue + var validation *ValidationErrors + if errors.As(resolveErr, &issue) || errors.As(resolveErr, &validation) { + // An unsupported or invalid published registry fails closed; an older + // cached registry must not mask that the binary or catalog needs work. + return PublishedRegistry{}, resolveErr + } + latest.RetryAt = now.Add(statusRetryInterval) + _ = s.recordLatest(directory, latest) + return s.staleOrError(directory, latest, resolveErr) +} + +func (s RegistrySource) staleOrError(directory string, latest registryLatest, cause error) (PublishedRegistry, error) { + if latest.resolved() { + if cached, err := s.cachedAt(directory, latest); err == nil { + cached.Stale = true + cached.StaleReason = cause.Error() + return cached, nil + } + } + return PublishedRegistry{}, fmt.Errorf("skill registry unavailable: %w", cause) +} + +func (s RegistrySource) cachedAt(directory string, latest registryLatest) (PublishedRegistry, error) { + data, err := readStatusFile(filepath.Join(directory, latest.Commit+".json")) + if err != nil { + return PublishedRegistry{}, err + } + published, err := parsePublishedRegistry(latest.Commit, data) + if err != nil { + return PublishedRegistry{}, err + } + if published.SHA256 != latest.SHA256 { + return PublishedRegistry{}, errors.New("cached registry digest mismatch") + } + published.ResolvedAt = latest.ResolvedAt + return published, nil +} + +func (s RegistrySource) load(ctx context.Context, directory string, cacheErr error, commit string) (PublishedRegistry, error) { + path := filepath.Join(directory, commit+".json") + if cacheErr == nil { + if data, err := readStatusFile(path); err == nil { + if published, err := parsePublishedRegistry(commit, data); err == nil { + return published, nil + } + } + } + data, err := s.fetch(ctx, commit) + if err != nil { + return PublishedRegistry{}, err + } + published, err := parsePublishedRegistry(commit, data) + if err != nil { + return PublishedRegistry{}, err + } + if cacheErr == nil { + _ = s.writeCommit(directory, commit, data) + } + return published, nil +} + +func parsePublishedRegistry(commit string, data []byte) (PublishedRegistry, error) { + if err := checkRegistryVersion(data); err != nil { + return PublishedRegistry{}, err + } + registry, err := ParseRegistry(data) + if err != nil { + return PublishedRegistry{}, err + } + sum := sha256.Sum256(data) + return PublishedRegistry{Registry: registry, Commit: commit, SHA256: hex.EncodeToString(sum[:])}, nil +} + +// checkRegistryVersion names the required action for a schema this binary +// cannot interpret, before strict decoding reports unrelated unknown fields. +func checkRegistryVersion(data []byte) error { + var header struct { + Version *int `json:"version"` + } + if err := json.Unmarshal(data, &header); err != nil || header.Version == nil || *header.Version == RegistryVersion { + return nil + } + return &Issue{Code: IssueRegistryVersion, Path: "registry.version", Message: fmt.Sprintf( + "published registry version %d is not supported by sjskills %s, which reads version %d; install the sjskills release that supports it", + *header.Version, ToolVersion, RegistryVersion)} +} + +func (s RegistrySource) resolveCommit(ctx context.Context) (string, error) { + url := s.refsURL + if url == "" { + url = registryRefsURL + } + data, contentType, err := s.get(ctx, url, maxRefAdvertisement, "registry branch resolution") + if err != nil { + return "", err + } + if contentType != "application/x-git-upload-pack-advertisement" { + return "", errors.New("registry branch resolution returned an unexpected content type") + } + return parseRefAdvertisement(data, registryBranchRef) +} + +func (s RegistrySource) fetch(ctx context.Context, commit string) ([]byte, error) { + base := s.rawBase + if base == "" { + base = registryRawBase + } + data, _, err := s.get(ctx, base+commit+"/"+registryFile, maxRegistryBytes, "registry fetch") + return data, err +} + +func (s RegistrySource) get(ctx context.Context, url string, limit int64, operation string) ([]byte, string, error) { + if ctx == nil { + ctx = context.Background() + } + request, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, "", err + } + request.Header.Set("User-Agent", "sjskills-registry") + response, err := s.client().Do(request) + if err != nil { + return nil, "", fmt.Errorf("%s failed (network or deadline)", operation) + } + defer response.Body.Close() + if response.StatusCode != http.StatusOK { + return nil, "", fmt.Errorf("%s returned HTTP %d", operation, response.StatusCode) + } + data, err := io.ReadAll(io.LimitReader(response.Body, limit+1)) + if err != nil || int64(len(data)) > limit { + return nil, "", fmt.Errorf("%s response unreadable or oversized", operation) + } + return data, strings.TrimSpace(strings.Split(response.Header.Get("Content-Type"), ";")[0]), nil +} + +// parseRefAdvertisement reads Git's pkt-line ref advertisement and returns the +// commit for ref. Any framing error rejects the whole response. +func parseRefAdvertisement(data []byte, ref string) (string, error) { + invalid := errors.New("registry branch resolution returned a malformed ref advertisement") + for len(data) > 0 { + if len(data) < 4 { + return "", invalid + } + length, err := strconv.ParseUint(string(data[:4]), 16, 16) + if err != nil { + return "", invalid + } + if length == 0 { + data = data[4:] + continue + } + if length < 4 || int(length) > len(data) { + return "", invalid + } + line := bytes.TrimSuffix(data[4:length], []byte("\n")) + data = data[length:] + if bytes.HasPrefix(line, []byte("#")) { + continue + } + line, _, _ = bytes.Cut(line, []byte{0}) + hash, name, ok := strings.Cut(string(line), " ") + if !ok || name != ref { + continue + } + if !registryCommitPattern.MatchString(hash) { + return "", invalid + } + return hash, nil + } + return "", fmt.Errorf("registry branch %s is not advertised", ref) +} + +func (s RegistrySource) cacheDirectory() (string, error) { + return disposableCacheDirectory(s.CacheRoot, "registry") +} + +func (s RegistrySource) readLatest(directory string) (registryLatest, error) { + data, err := readStatusFile(filepath.Join(directory, registryLatestFile)) + if err != nil { + return registryLatest{}, err + } + var entry registryLatest + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&entry); err != nil { + return registryLatest{}, err + } + if decoder.Decode(new(any)) != io.EOF || entry.Schema != registryCacheSchema || entry.Source != registryRepository || entry.Ref != registryBranchRef { + return registryLatest{}, errors.New("incompatible registry cache") + } + resolved := registryCommitPattern.MatchString(entry.Commit) && lowercaseDigestPattern.MatchString(entry.SHA256) && !entry.ResolvedAt.IsZero() + unresolved := entry.Commit == "" && entry.SHA256 == "" && entry.ResolvedAt.IsZero() + if !resolved && !unresolved { + return registryLatest{}, errors.New("invalid registry cache") + } + return entry, nil +} + +// recordLatest publishes the branch observation under the namespace lock and +// keeps the newest successful observation when refreshes race. +func (s RegistrySource) recordLatest(directory string, entry registryLatest) error { + entry.Schema, entry.Source, entry.Ref = registryCacheSchema, registryRepository, registryBranchRef + unlock, err := lockStatusPath(filepath.Join(directory, registryLatestLockFile)) + if err != nil { + return err + } + defer unlock() + if current, err := s.readLatest(directory); err == nil && current.ResolvedAt.After(entry.ResolvedAt) && !current.ResolvedAt.After(s.now()) { + if entry.resolved() { + return nil + } + // A failed refresh keeps the newer success and records only its cooldown. + current.RetryAt = entry.RetryAt + entry = current + } + data, err := json.Marshal(entry) + if err != nil { + return err + } + if err := writeRegistryCacheFile(directory, registryLatestFile, data); err != nil { + return err + } + s.prune(directory, entry.Commit) + return nil +} + +func (s RegistrySource) writeCommit(directory, commit string, data []byte) error { + unlock, err := lockStatusPath(filepath.Join(directory, registryLatestLockFile)) + if err != nil { + return err + } + defer unlock() + return writeRegistryCacheFile(directory, commit+".json", data) +} + +func writeRegistryCacheFile(directory, name string, data []byte) error { + path := filepath.Join(directory, name) + if info, err := os.Lstat(path); err == nil && !info.Mode().IsRegular() { + return errors.New("unsafe registry cache destination") + } + file, err := os.CreateTemp(directory, name+".tmp-") + if err != nil { + return err + } + temp := file.Name() + defer os.Remove(temp) + if _, err := file.Write(data); err != nil { + file.Close() + return err + } + if err := file.Close(); err != nil { + return err + } + return os.Rename(temp, path) +} + +// prune runs under the namespace lock. It bounds directory work and removes +// expired commit registries and abandoned temporary files, keeping the latest. +func (s RegistrySource) prune(directory, keep string) { + dir, err := os.Open(directory) + if err != nil { + return + } + defer dir.Close() + entries, _ := dir.ReadDir(256) + for _, entry := range entries { + name := entry.Name() + commit, isRegistry := strings.CutSuffix(name, ".json") + isRegistry = isRegistry && registryCommitPattern.MatchString(commit) + if (!isRegistry || commit == keep) && !strings.Contains(name, ".tmp-") { + continue + } + path := filepath.Join(directory, name) + info, err := os.Lstat(path) + if err == nil && info.Mode().IsRegular() && s.now().Sub(info.ModTime()) >= statusRetention { + _ = os.Remove(path) + } + } +} diff --git a/internal/sjskills/registry_source_test.go b/internal/sjskills/registry_source_test.go new file mode 100644 index 0000000..319a8e7 --- /dev/null +++ b/internal/sjskills/registry_source_test.go @@ -0,0 +1,289 @@ +package sjskills + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +const ( + testCommitA = "1111111111111111111111111111111111111111" + testCommitB = "2222222222222222222222222222222222222222" +) + +func pktLine(value string) string { return fmt.Sprintf("%04x%s", len(value)+4, value) } + +func refAdvertisement(commit string) string { + return pktLine("# service=git-upload-pack\n") + "0000" + + pktLine(commit+" HEAD\x00multi_ack symref=HEAD:refs/heads/main\n") + + pktLine("3333333333333333333333333333333333333333 refs/heads/dev\n") + + pktLine(commit+" refs/heads/main\n") + "0000" +} + +type registryServer struct { + commit atomic.Value + registries map[string]string + refs atomic.Int32 + raw atomic.Int32 + offline atomic.Bool + server *httptest.Server +} + +func newRegistryServer(t *testing.T, commit string, registries map[string]string) *registryServer { + t.Helper() + s := ®istryServer{registries: registries} + s.commit.Store(commit) + s.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/refs" { + s.refs.Add(1) + } else { + s.raw.Add(1) + } + if s.offline.Load() { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + if r.URL.Path == "/refs" { + w.Header().Set("Content-Type", "application/x-git-upload-pack-advertisement") + fmt.Fprint(w, refAdvertisement(s.commit.Load().(string))) + return + } + commit, file, _ := strings.Cut(strings.TrimPrefix(r.URL.Path, "/raw/"), "/") + content, ok := s.registries[commit] + if !ok || file != registryFile { + http.NotFound(w, r) + return + } + fmt.Fprint(w, content) + })) + t.Cleanup(s.server.Close) + return s +} + +func (s *registryServer) source(t *testing.T, cacheRoot string, now *time.Time) RegistrySource { + return RegistrySource{ + CacheRoot: cacheRoot, + Now: func() time.Time { return *now }, + Client: s.server.Client(), + refsURL: s.server.URL + "/refs", + rawBase: s.server.URL + "/raw/", + } +} + +func digest(content string) string { + sum := sha256.Sum256([]byte(content)) + return hex.EncodeToString(sum[:]) +} + +func TestParseRefAdvertisementFindsMain(t *testing.T) { + commit, err := parseRefAdvertisement([]byte(refAdvertisement(testCommitA)), registryBranchRef) + if err != nil || commit != testCommitA { + t.Fatalf("commit=%q err=%v", commit, err) + } + for name, input := range map[string]string{ + "truncated": refAdvertisement(testCommitA)[:30], + "bad length": "zzzz" + testCommitA, + "short hash": pktLine("abc refs/heads/main\n"), + "upper hash": pktLine(strings.ToUpper("abcdef1234abcdef1234abcdef1234abcdef1234") + " refs/heads/main\n"), + "missing": pktLine(testCommitA+" refs/heads/dev\n") + "0000", + } { + if _, err := parseRefAdvertisement([]byte(input), registryBranchRef); err == nil { + t.Fatalf("%s: accepted", name) + } + } +} + +func TestRegistrySourceResolveFetchesAtResolvedCommitAndCaches(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + + published, err := source.Resolve(context.Background()) + if err != nil { + t.Fatal(err) + } + if published.Commit != testCommitA || published.SHA256 != digest(string(registryV4JSON)) || !published.ResolvedAt.Equal(now) || published.Stale { + t.Fatalf("published = %+v", published) + } + if published.Registry.Version != RegistryVersion { + t.Fatalf("registry version = %d", published.Registry.Version) + } + // The commit cache avoids refetching content; the branch is always resolved. + if _, err := source.Resolve(context.Background()); err != nil { + t.Fatal(err) + } + if server.refs.Load() != 2 || server.raw.Load() != 1 { + t.Fatalf("refs=%d raw=%d", server.refs.Load(), server.raw.Load()) + } +} + +func TestRegistrySourceResolveFollowsBranchMoves(t *testing.T) { + changed := strings.Replace(string(registryV4JSON), `"Version 4 desired-state registry for sjskills."`, `"Changed on main."`, 1) + if changed == string(registryV4JSON) { + t.Fatal("fixture description not found") + } + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON), testCommitB: changed}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + if _, err := source.Resolve(context.Background()); err != nil { + t.Fatal(err) + } + server.commit.Store(testCommitB) + published, err := source.Resolve(context.Background()) + if err != nil { + t.Fatal(err) + } + if published.Commit != testCommitB || published.Registry.Description != "Changed on main." { + t.Fatalf("published = %s %q", published.Commit, published.Registry.Description) + } + pinned, err := source.At(context.Background(), testCommitA) + if err != nil || pinned.Registry.Description == "Changed on main." || !pinned.ResolvedAt.IsZero() { + t.Fatalf("pinned = %+v err=%v", pinned, err) + } +} + +func TestRegistrySourceForStatusUsesCacheWithinInterval(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + if _, err := source.Resolve(context.Background()); err != nil { + t.Fatal(err) + } + server.offline.Store(true) + now = now.Add(statusRefreshInterval - time.Minute) + published, err := source.ForStatus(context.Background()) + if err != nil || published.Stale || published.Commit != testCommitA { + t.Fatalf("published = %+v err=%v", published, err) + } + if server.refs.Load() != 1 { + t.Fatalf("status refreshed within interval: refs=%d", server.refs.Load()) + } +} + +func TestRegistrySourceStaleFallbackAndCooldown(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + resolvedAt := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + now := resolvedAt + source := server.source(t, t.TempDir(), &now) + if _, err := source.Resolve(context.Background()); err != nil { + t.Fatal(err) + } + server.offline.Store(true) + now = now.Add(statusRefreshInterval + time.Hour) + + published, err := source.ForStatus(context.Background()) + if err != nil || !published.Stale || published.Commit != testCommitA || !published.ResolvedAt.Equal(resolvedAt) || !strings.Contains(published.StaleReason, "HTTP 503") { + t.Fatalf("published = %+v err=%v", published, err) + } + attempts := server.refs.Load() + now = now.Add(statusRetryInterval / 2) + published, err = source.ForStatus(context.Background()) + if err != nil || !published.Stale || !strings.Contains(published.StaleReason, "cooldown") { + t.Fatalf("cooldown published = %+v err=%v", published, err) + } + if server.refs.Load() != attempts { + t.Fatal("status refreshed during cooldown") + } + // Selection commands are explicit and always attempt a refresh. + if published, err = source.ForSelection(context.Background()); err != nil || !published.Stale { + t.Fatalf("selection published = %+v err=%v", published, err) + } + if server.refs.Load() != attempts+1 { + t.Fatal("selection did not attempt a refresh") + } + + server.offline.Store(false) + now = now.Add(statusRetryInterval) + if published, err = source.ForStatus(context.Background()); err != nil || published.Stale || !published.ResolvedAt.Equal(now) { + t.Fatalf("recovered published = %+v err=%v", published, err) + } +} + +func TestRegistrySourceUnavailableWithoutCache(t *testing.T) { + server := newRegistryServer(t, testCommitA, nil) + server.offline.Store(true) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + for name, load := range map[string]func(context.Context) (PublishedRegistry, error){ + "status": source.ForStatus, "selection": source.ForSelection, "resolve": source.Resolve, + } { + if _, err := load(context.Background()); err == nil { + t.Fatalf("%s: loaded without network or cache", name) + } + } +} + +func TestRegistrySourceUnsupportedVersionFailsClosed(t *testing.T) { + future := strings.Replace(string(registryV4JSON), `"version": 4`, `"version": 5`, 1) + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON), testCommitB: future}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + if _, err := source.Resolve(context.Background()); err != nil { + t.Fatal(err) + } + server.commit.Store(testCommitB) + now = now.Add(statusRefreshInterval + time.Hour) + for name, load := range map[string]func(context.Context) (PublishedRegistry, error){ + "status": source.ForStatus, "selection": source.ForSelection, "resolve": source.Resolve, + } { + _, err := load(context.Background()) + var issue *Issue + if !errors.As(err, &issue) || issue.Code != IssueRegistryVersion || !strings.Contains(issue.Message, "version 5") || !strings.Contains(issue.Message, "install the sjskills release") { + t.Fatalf("%s: err = %v", name, err) + } + } +} + +func TestRegistrySourceRejectsInvalidInputs(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: `{"version": 4}`}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + var validation *ValidationErrors + if _, err := source.Resolve(context.Background()); !errors.As(err, &validation) { + t.Fatalf("invalid registry err = %v", err) + } + if _, err := source.At(context.Background(), strings.ToUpper(testCommitA)); err == nil { + t.Fatal("accepted non-canonical commit") + } + + html := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + fmt.Fprint(w, refAdvertisement(testCommitA)) + })) + t.Cleanup(html.Close) + source.refsURL = html.URL + if _, err := source.Resolve(context.Background()); err == nil || !strings.Contains(err.Error(), "content type") { + t.Fatalf("content type err = %v", err) + } +} + +func TestRegistrySourceIgnoresTamperedCommitCache(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + cacheRoot := t.TempDir() + source := server.source(t, cacheRoot, &now) + if _, err := source.Resolve(context.Background()); err != nil { + t.Fatal(err) + } + directory, err := source.cacheDirectory() + if err != nil { + t.Fatal(err) + } + tampered := strings.Replace(string(registryV4JSON), `"Version 4 desired-state registry for sjskills."`, `"Tampered."`, 1) + if err := writeRegistryCacheFile(directory, testCommitA+".json", []byte(tampered)); err != nil { + t.Fatal(err) + } + server.offline.Store(true) + // A digest mismatch makes the cached registry unusable rather than trusted. + if _, err := source.ForStatus(context.Background()); err == nil { + t.Fatal("status used a tampered cached registry") + } +} diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md index f53b55f..8705701 100644 --- a/plans/sjskills-remote-registry.md +++ b/plans/sjskills-remote-registry.md @@ -10,7 +10,10 @@ schema or the CLI itself changes. ## Current state -Proposed on 2026-09-30. The step 1 spike is complete; implementation has not started. +Proposed on 2026-09-30. Steps 1 and 2 are complete: `RegistrySource` +(`internal/sjskills/registry_source.go`) resolves, fetches, validates, and +caches the published registry, and the CLI loads its registry once per +invocation. Production still reads the embedded registry until step 3. The executable embeds `internal/sjskills/data/registry-v4.json` (`internal/sjskills/registry.go`) and reads no other registry, while skill @@ -66,8 +69,13 @@ a registry change on 2026-09-10 needed a temporary binary until v1.2.0 shipped i agent-scripts source by fetching the commit with Git and passing the local `skills/` path to Skills CLI, generalizing the authenticated path's commit fetch to anonymous public sources. -2. Add registry resolution, fetch, validation, and cache with the fixed - location; load once per invocation. +2. ~~Add registry resolution, fetch, validation, and cache with the fixed + location; load once per invocation.~~ Done 2026-09-30. Resolution reads + Git's anonymous smart-HTTP ref advertisement for `refs/heads/main` instead + of running `git ls-remote`: same protocol and rate-limit profile, but no + process on the status path. The registry comes from the raw URL at the + resolved commit; a published version other than 4 fails closed with an + update message and never falls back to a cached registry. 3. Pin agent-scripts sources to the resolved commit and add registry commit/digest evidence; make global apply reuse the reviewed commit. 4. Remove the embedded registry data and equality test; move Go tests to @@ -95,8 +103,8 @@ from `main`: returns the registry at that commit, matching `git show`. Consequence: public agent-scripts materialization gains a Git prerequisite that -today applies only to authenticated sources. Resolution uses `git ls-remote`; -the registry fetch may use the raw URL or the same shallow checkout. +today applies only to authenticated sources. Resolution may use `git ls-remote` +or the equivalent smart-HTTP ref advertisement; step 2 chose the latter. ## Acceptance and validation @@ -119,4 +127,9 @@ release step authorizes. ## Next action -Implement step 2: registry resolution, fetch, validation, and cache. +Implement step 3: switch production loading to `RegistrySource` (`Resolve` +for plan/apply, `At` for reviewed global apply, `ForStatus`/`ForSelection` +with stale labels elsewhere), pin agent-scripts sources to the resolved +commit, and replace the `embedded version 4` evidence with commit and digest. +Binary-level CLI tests route HTTPS through an unreachable proxy, so they will +need a fake registry endpoint. From ecb9e1a051092985cfa8a13327f96ccca6acc5c1 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 14:30:40 +0900 Subject: [PATCH 5/7] Read the skill registry from the published source Step 3 of the remote-registry plan. The CLI now loads skill-registry.json through RegistrySource once per invocation: plan and apply resolve main afresh, global apply reloads the registry at the commit recorded in the reviewed plan so a push after review cannot change the applied state, status reuses a resolution for 24 hours, and profiles/init fall back to a cached registry with a registry-stale warning. Plan evidence records `sjunepark/agent-scripts@ sha256:`; plans from embedded-registry releases must be reviewed again. Agent-scripts sources are pinned to the registry's commit. Skills CLI cannot clone a commit, so the materializer fetches public /tree/ sources anonymously with Git and installs from the checkout. Git runs in Skills CLI's own isolated environment so system settings such as core.autocrlf still apply: disabling them checked out LF files on Windows and reported every CRLF-installed skill as an update. Inherited GIT_*, GH_*, askpass, and token variables are stripped so a Git hook environment cannot redirect the forced checkout into the caller's repository. Plan and apply always fetch registry bytes rather than trusting an unverifiable cached file, and a reviewed commit that no longer exists is a conflict. `--no-status-check` also skips the registry cache. CLI integration tests build with the sjskillstest tag, which alone lets the binary read a fixture registry server; release.py clears GOFLAGS so the tag cannot reach a release. A live read-only `plan --global` matches the previous binary's expected-content hashes exactly. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +- cmd/sjskills/default_status_test.go | 20 ++- cmd/sjskills/main.go | 121 ++++++++++++--- cmd/sjskills/main_test.go | 26 +++- cmd/sjskills/registry_fixture_test.go | 70 +++++++++ cmd/sjskills/registry_test.go | 139 ++++++++++++++++++ cmd/sjskills/status.go | 16 +- cmd/sjskills/status_test.go | 8 + cmd/sjskills/testdata/fakebunx/main.go | 3 +- cmd/sjskills/testdata/fakegithub/main.go | 37 +++++ internal/sjskills/github_auth.go | 72 +++++++-- internal/sjskills/materialize.go | 10 +- internal/sjskills/materialize_test.go | 65 ++++++++ internal/sjskills/registry_source.go | 88 +++++++++-- internal/sjskills/registry_source_test.go | 80 +++++++++- internal/sjskills/registry_source_testhook.go | 13 ++ internal/sjskills/reviewed_plan.go | 6 + plans/sjskills-remote-registry.md | 56 ++++--- scripts/release.py | 4 +- 19 files changed, 750 insertions(+), 88 deletions(-) create mode 100644 cmd/sjskills/registry_fixture_test.go create mode 100644 cmd/sjskills/registry_test.go create mode 100644 internal/sjskills/registry_source_testhook.go diff --git a/ROADMAP.md b/ROADMAP.md index f6d257a..3178ed4 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -11,8 +11,8 @@ 1. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) is proposed so registry changes stop requiring an sjskills release. - Registry resolution and caching are implemented; switching production - loading, commit pinning, and evidence are next. + The CLI reads the published registry at a pinned commit on `dev`; removing + the embedded data, docs, and the release remain. ## Tasks diff --git a/cmd/sjskills/default_status_test.go b/cmd/sjskills/default_status_test.go index d912bc4..a023f7a 100644 --- a/cmd/sjskills/default_status_test.go +++ b/cmd/sjskills/default_status_test.go @@ -269,7 +269,9 @@ func TestDefaultStatusDiscoveryFailuresAndOptOut(t *testing.T) { case "home": app.homeDirectory = func() (string, error) { return "", errors.New("no home") } case "registry", "registry-missing", "registry-malformed": - app.loadRegistry = func() (sjskills.Registry, error) { return sjskills.Registry{}, errors.New("no registry") } + app.registries = registryFunc(func() (sjskills.PublishedRegistry, error) { + return sjskills.PublishedRegistry{}, errors.New("no registry") + }) if kind == "registry-missing" { _ = os.Remove(filepath.Join(f.project, "sjskills.toml")) } @@ -280,7 +282,10 @@ func TestDefaultStatusDiscoveryFailuresAndOptOut(t *testing.T) { app.noStatusCheck = true app.directory = "invalid" app.homeDirectory = func() (string, error) { t.Fatal("home discovery during opt-out"); return "", nil } - app.loadRegistry = func() (sjskills.Registry, error) { t.Fatal("registry during opt-out"); return sjskills.Registry{}, nil } + app.registries = registryFunc(func() (sjskills.PublishedRegistry, error) { + t.Fatal("registry during opt-out") + return sjskills.PublishedRegistry{}, nil + }) } e := app.status(context.Background()) if e.ExitStatus() != sjskills.ExitSuccess { @@ -397,7 +402,10 @@ func TestDefaultStatusPresentation(t *testing.T) { func TestDefaultStatusAlreadyCancelled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - app := &application{loadRegistry: func() (sjskills.Registry, error) { t.Fatal("work after cancellation"); return sjskills.Registry{}, nil }} + app := &application{registries: registryFunc(func() (sjskills.PublishedRegistry, error) { + t.Fatal("work after cancellation") + return sjskills.PublishedRegistry{}, nil + })} e := app.status(ctx) var out, errout bytes.Buffer code := emitEnvelope(&out, &errout, false, e) @@ -419,10 +427,10 @@ func TestRegistryLoadsOncePerInvocation(t *testing.T) { statusService: &service, cliStatusService: f.cliService(), materialize: materializer.Materialize, - loadRegistry: func() (sjskills.Registry, error) { + registries: registryFunc(func() (sjskills.PublishedRegistry, error) { loads++ - return sjskills.EmbeddedRegistry() - }, + return fixtureRegistry() + }), } if envelope := app.plan(context.Background(), true); envelope.Result != sjskills.ResultSuccess { t.Fatalf("plan %+v", envelope.Error) diff --git a/cmd/sjskills/main.go b/cmd/sjskills/main.go index 540d276..5d86e17 100644 --- a/cmd/sjskills/main.go +++ b/cmd/sjskills/main.go @@ -68,12 +68,12 @@ type commandContext struct { } func (c *initCommand) Run(ctx *commandContext) error { - ctx.application.envelope = ctx.application.init(c.Profiles) + ctx.application.envelope = ctx.application.init(ctx.context, c.Profiles) return nil } func (c *profilesCommand) Run(ctx *commandContext) error { - ctx.application.envelope = ctx.application.profiles() + ctx.application.envelope = ctx.application.profiles(ctx.context) return nil } @@ -93,11 +93,11 @@ func (c *restoreCommand) Run(ctx *commandContext) error { } type application struct { - loadRegistry func() (sjskills.Registry, error) + registries registryLoader registryLoaded bool - registryValue sjskills.Registry + registryValue sjskills.PublishedRegistry registryErr error - noStatusCheck bool + noStatusCheck bool statusSnapshot *commandStatusSnapshot cliStatusService *sjskills.CLIStatusService statusService *sjskills.StatusService @@ -147,20 +147,71 @@ func productionMaterialize(ctx context.Context, skills []sjskills.DesiredSkill) return sjskills.NewMaterializer(sjskills.MaterializerConfig{}).Materialize(ctx, skills) } +// registryLoader is implemented by sjskills.RegistrySource. Each command +// chooses its freshness policy; see the RegistrySource methods. +type registryLoader interface { + Resolve(context.Context) (sjskills.PublishedRegistry, error) + At(context.Context, string) (sjskills.PublishedRegistry, error) + ForStatus(context.Context) (sjskills.PublishedRegistry, error) + ForSelection(context.Context) (sjskills.PublishedRegistry, error) +} + +// defaultRegistries serves applications constructed directly by tests. +// Production always injects a RegistrySource in executeWithInput. +var defaultRegistries registryLoader + // registry loads the registry at most once per invocation, so preparation, // the status snapshot, and incidental notices all read the same desired state. -func (a *application) registry() (sjskills.Registry, error) { +// The first caller's load policy wins. +func (a *application) registry(ctx context.Context, load func(registryLoader, context.Context) (sjskills.PublishedRegistry, error)) (sjskills.PublishedRegistry, error) { if !a.registryLoaded { - if a.loadRegistry != nil { - a.registryValue, a.registryErr = a.loadRegistry() + registries := a.registries + if registries == nil { + registries = defaultRegistries + } + if registries == nil { + a.registryErr = errors.New("skill registry source is not configured") } else { - a.registryValue, a.registryErr = sjskills.EmbeddedRegistry() + if ctx == nil { + ctx = context.Background() + } + a.registryValue, a.registryErr = load(registries, ctx) } a.registryLoaded = true } return a.registryValue, a.registryErr } +func registryAt(commit string) func(registryLoader, context.Context) (sjskills.PublishedRegistry, error) { + return func(registries registryLoader, ctx context.Context) (sjskills.PublishedRegistry, error) { + return registries.At(ctx, commit) + } +} + +// registryFailure keeps an invalid or unsupported published registry distinct +// from a registry that could not be reached. +func (a *application) registryFailure(operation sjskills.CommandOperation, err error) sjskills.Envelope { + var issue *sjskills.Issue + var validation *sjskills.ValidationErrors + if errors.As(err, &issue) && issue.Code == sjskills.IssueReconciliationConflict { + return a.conflict(operation, err) + } + if errors.As(err, &issue) || errors.As(err, &validation) { + return a.invalid(operation, err) + } + return a.unavailable(operation, err) +} + +// registryWarnings labels a cached registry used after a failed refresh. +func registryWarnings(published sjskills.PublishedRegistry, now time.Time) []sjskills.Warning { + if !published.Stale { + return nil + } + return []sjskills.Warning{{Code: "registry-stale", Message: fmt.Sprintf( + "using cached skill registry at %s resolved %s ago; refresh failed: %s", + published.Commit[:12], statusAge(now.Sub(published.ResolvedAt)), published.StaleReason)}} +} + func (a *application) base(operation sjskills.CommandOperation) sjskills.Envelope { return sjskills.Envelope{ Operation: operation, @@ -191,12 +242,13 @@ func (a *application) conflict(operation sjskills.CommandOperation, err error) s return envelope } -func (a *application) profiles() sjskills.Envelope { +func (a *application) profiles(ctx context.Context) sjskills.Envelope { envelope := a.base(sjskills.CommandOperationProfiles) - registry, err := a.registry() + published, err := a.registry(ctx, registryLoader.ForSelection) if err != nil { - return a.invalid(sjskills.CommandOperationProfiles, err) + return a.registryFailure(sjskills.CommandOperationProfiles, err) } + registry := published.Registry profiles := make([]string, 0, len(registry.Profiles)) for name := range registry.Profiles { profiles = append(profiles, name) @@ -205,16 +257,18 @@ func (a *application) profiles() sjskills.Envelope { for _, name := range profiles { envelope.Profiles = append(envelope.Profiles, sjskills.ProfileInfo{Name: name, Access: registry.Profiles[name].Access.Effective(), Count: len(registry.Profiles[name].Skills)}) } - envelope.Evidence = append(envelope.Evidence, sjskills.Evidence{Kind: "registry", Detail: "embedded version 4"}) + envelope.Evidence = append(envelope.Evidence, published.Evidence()) + envelope.Warnings = append(envelope.Warnings, registryWarnings(published, time.Now())...) return envelope } -func (a *application) init(profileNames []string) sjskills.Envelope { +func (a *application) init(ctx context.Context, profileNames []string) sjskills.Envelope { envelope := a.base(sjskills.CommandOperationInit) - registry, err := a.registry() + published, err := a.registry(ctx, registryLoader.ForSelection) if err != nil { - return a.invalid(sjskills.CommandOperationInit, err) + return a.registryFailure(sjskills.CommandOperationInit, err) } + registry := published.Registry if len(profileNames) == 0 { return a.invalid(sjskills.CommandOperationInit, &sjskills.Issue{Code: sjskills.IssueEmptySelection, Path: "init.profile", Message: "at least one profile is required"}) } @@ -257,11 +311,13 @@ func (a *application) init(profileNames []string) sjskills.Envelope { return a.unavailable(sjskills.CommandOperationInit, fmt.Errorf("close manifest: %w", closeErr)) } envelope.Path = manifestPath - envelope.Evidence = append(envelope.Evidence, sjskills.Evidence{Kind: "manifest", Detail: fmt.Sprintf("created %s", manifestPath)}) + envelope.Evidence = append(envelope.Evidence, published.Evidence(), sjskills.Evidence{Kind: "manifest", Detail: fmt.Sprintf("created %s", manifestPath)}) + envelope.Warnings = append(envelope.Warnings, registryWarnings(published, time.Now())...) return envelope } type preparedPlan struct { + registry sjskills.Evidence observedAt time.Time envelope sjskills.Envelope plan sjskills.Plan @@ -276,19 +332,26 @@ type preparedPlan struct { } func (a *application) plan(ctx context.Context, global bool) sjskills.Envelope { - prepared, envelope := a.prepare(ctx, global, sjskills.CommandOperationPlan) + prepared, envelope := a.prepare(ctx, global, sjskills.CommandOperationPlan, "") if prepared == nil { return envelope } return a.finishPrepared(prepared, envelope, "cleanup") } -func (a *application) prepare(ctx context.Context, global bool, operation sjskills.CommandOperation) (*preparedPlan, sjskills.Envelope) { +// prepare resolves the published registry afresh, or loads it at +// registryCommit when a reviewed plan recorded one. +func (a *application) prepare(ctx context.Context, global bool, operation sjskills.CommandOperation, registryCommit string) (*preparedPlan, sjskills.Envelope) { envelope := a.base(operation) - registry, err := a.registry() + load := registryLoader.Resolve + if registryCommit != "" { + load = registryAt(registryCommit) + } + published, err := a.registry(ctx, load) if err != nil { - return nil, a.invalid(operation, err) + return nil, a.registryFailure(operation, err) } + registry := published.Registry request := sjskills.ResolveRequest{Registry: registry, Global: global} var project *sjskills.ProjectRoot if !global { @@ -311,7 +374,7 @@ func (a *application) prepare(ctx context.Context, global bool, operation sjskil if err != nil { return nil, a.invalid(operation, err) } - prepared := &preparedPlan{envelope: envelope, plan: plan, expected: map[string]sjskills.TreeHash{}} + prepared := &preparedPlan{registry: published.Evidence(), envelope: envelope, plan: plan, expected: map[string]sjskills.TreeHash{}} prepared.syncPlan(plan) materialize := a.materialize @@ -458,6 +521,7 @@ func (a *application) applyApproved(ctx context.Context, global, yes bool, appro return a.invalid(sjskills.CommandOperationApply, &sjskills.Issue{Code: sjskills.IssueMalformedInput, Path: "apply.approvedPlan", Message: "approved plan evidence is available only for global apply"}) } var reviewed sjskills.ReviewedPlan + registryCommit := "" if global { if approvedPlanPath == "" || approvedPlanSHA256 == "" { return a.invalid(sjskills.CommandOperationApply, &sjskills.Issue{Code: sjskills.IssueMalformedInput, Path: "apply.approvedPlan", Message: "global apply requires --approved-plan and --approved-plan-sha256"}) @@ -478,8 +542,16 @@ func (a *application) applyApproved(ctx context.Context, global, yes bool, appro } return a.unavailable(sjskills.CommandOperationApply, errors.New("approved plan artifact could not be read")) } + // Apply the registry the operator reviewed, even if main moved since. + var ok bool + if registryCommit, ok = reviewed.RegistryCommit(); !ok { + envelope := a.base(sjskills.CommandOperationApply) + envelope.Result = sjskills.ResultConflict + envelope.Error = &sjskills.Issue{Code: sjskills.IssueReconciliationConflict, Path: "apply.approvedPlan", Message: "approved plan does not record a published registry commit; review a new plan"} + return envelope + } } - prepared, envelope := a.prepare(ctx, global, sjskills.CommandOperationApply) + prepared, envelope := a.prepare(ctx, global, sjskills.CommandOperationApply, registryCommit) if prepared == nil { return envelope } @@ -666,7 +738,7 @@ func (p *preparedPlan) syncPlan(plan sjskills.Plan) { p.plan = plan p.envelope.Plan = &p.plan p.envelope.Warnings = append([]sjskills.Warning{}, plan.Warnings...) - p.envelope.Evidence = append([]sjskills.Evidence{{Kind: "registry", Detail: "embedded version 4"}}, plan.Evidence...) + p.envelope.Evidence = append([]sjskills.Evidence{p.registry}, plan.Evidence...) for _, snapshot := range p.snapshots { p.envelope.Evidence = append(p.envelope.Evidence, sjskills.Evidence{ Kind: "expected-content", @@ -1238,6 +1310,7 @@ func executeWithInput(ctx context.Context, args []string, stdin io.Reader, stdou input: stdin, promptOutput: stderr, materialize: productionMaterialize, + registries: sjskills.RegistrySource{NoCache: commands.NoStatusCheck}, } if err := parsed.Run(&commandContext{application: app, context: ctx}); err != nil { if jsonMode { diff --git a/cmd/sjskills/main_test.go b/cmd/sjskills/main_test.go index 4a16bee..5f645b9 100644 --- a/cmd/sjskills/main_test.go +++ b/cmd/sjskills/main_test.go @@ -33,7 +33,8 @@ func TestMain(m *testing.M) { if runtime.GOOS == "windows" { testBinary += ".exe" } - build := exec.Command("go", "build", "-o", testBinary, ".") + // The sjskillstest tag lets the binary read the fixture registry server. + build := exec.Command("go", "build", "-tags", "sjskillstest", "-o", testBinary, ".") build.Stdout = os.Stdout build.Stderr = os.Stderr if err := build.Run(); err != nil { @@ -58,7 +59,26 @@ func TestMain(m *testing.M) { if err := buildGitHub.Run(); err != nil { panic(fmt.Sprintf("build fake github tools: %v", err)) } + // Commit-pinned agent-scripts sources are fetched with Git; the fake's + // public mode supplies a checkout whose skills fake bunx generates. + fakeGit := filepath.Join(directory, "git") + if runtime.GOOS == "windows" { + fakeGit += ".exe" + } + buildGit := exec.Command("go", "build", "-o", fakeGit, "./testdata/fakegithub") + buildGit.Stdout, buildGit.Stderr = os.Stdout, os.Stderr + if err := buildGit.Run(); err != nil { + panic(fmt.Sprintf("build fake git: %v", err)) + } + registryServer, err := startFixtureRegistryServer() + if err != nil { + panic(fmt.Sprintf("start fixture registry: %v", err)) + } + if err := os.Setenv("SJSKILLS_TEST_REGISTRY_URL", registryServer.URL); err != nil { + panic(err) + } code := m.Run() + registryServer.Close() _ = os.RemoveAll(directory) os.Exit(code) } @@ -2084,10 +2104,10 @@ func TestApplicationMaterializationFailuresAndLifecycle(t *testing.T) { calls++ return nil, errors.New("unexpected materialization") }} - if envelope := app.profiles(); envelope.Result != sjskills.ResultSuccess { + if envelope := app.profiles(context.Background()); envelope.Result != sjskills.ResultSuccess { t.Fatalf("profiles=%#v", envelope) } - if envelope := app.init([]string{"dev"}); envelope.Result != sjskills.ResultSuccess { + if envelope := app.init(context.Background(), []string{"dev"}); envelope.Result != sjskills.ResultSuccess { t.Fatalf("init=%#v", envelope) } if envelope := app.restore(context.Background(), "0123456789abcdef0123456789abcdef", true); envelope.Result != sjskills.ResultConflict { diff --git a/cmd/sjskills/registry_fixture_test.go b/cmd/sjskills/registry_fixture_test.go new file mode 100644 index 0000000..5ec235e --- /dev/null +++ b/cmd/sjskills/registry_fixture_test.go @@ -0,0 +1,70 @@ +package main + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "net/http" + "net/http/httptest" + "os" + + "github.com/sjunepark/agent-scripts/internal/sjskills" +) + +// fixtureRegistryCommit is the commit the fake published source advertises. +const fixtureRegistryCommit = "0123456789abcdef0123456789abcdef01234567" + +const fixtureRegistryPath = "../../internal/sjskills/data/registry-v4.json" + +// registryFunc adapts one load result to every registry policy for +// in-process tests; the reviewed commit passed to At is ignored. +type registryFunc func() (sjskills.PublishedRegistry, error) + +func (f registryFunc) Resolve(context.Context) (sjskills.PublishedRegistry, error) { return f() } +func (f registryFunc) At(context.Context, string) (sjskills.PublishedRegistry, error) { + return f() +} +func (f registryFunc) ForStatus(context.Context) (sjskills.PublishedRegistry, error) { return f() } +func (f registryFunc) ForSelection(context.Context) (sjskills.PublishedRegistry, error) { + return f() +} + +// fixtureRegistry is the unpinned fixture registry used by directly +// constructed applications, whose materializers ignore source locations. +func fixtureRegistry() (sjskills.PublishedRegistry, error) { + data, err := os.ReadFile(fixtureRegistryPath) + if err != nil { + return sjskills.PublishedRegistry{}, err + } + registry, err := sjskills.ParseRegistry(data) + if err != nil { + return sjskills.PublishedRegistry{}, err + } + sum := sha256.Sum256(data) + return sjskills.PublishedRegistry{Registry: registry, Commit: fixtureRegistryCommit, SHA256: hex.EncodeToString(sum[:])}, nil +} + +func init() { defaultRegistries = registryFunc(fixtureRegistry) } + +// startFixtureRegistryServer serves the fixture registry as the published +// source for CLI binaries built with the sjskillstest tag. +func startFixtureRegistryServer() (*httptest.Server, error) { + data, err := os.ReadFile(fixtureRegistryPath) + if err != nil { + return nil, err + } + pkt := func(value string) string { return fmt.Sprintf("%04x%s", len(value)+4, value) } + refs := pkt("# service=git-upload-pack\n") + "0000" + pkt(fixtureRegistryCommit+" refs/heads/main\n") + "0000" + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/refs": + w.Header().Set("Content-Type", "application/x-git-upload-pack-advertisement") + fmt.Fprint(w, refs) + case r.URL.Path == "/raw/"+fixtureRegistryCommit+"/skill-registry.json": + _, _ = w.Write(data) + default: + http.NotFound(w, r) + } + })), nil +} diff --git a/cmd/sjskills/registry_test.go b/cmd/sjskills/registry_test.go new file mode 100644 index 0000000..9752fd2 --- /dev/null +++ b/cmd/sjskills/registry_test.go @@ -0,0 +1,139 @@ +package main + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/sjunepark/agent-scripts/internal/sjskills" +) + +// movingRegistries models main moving after a global plan was reviewed. +type movingRegistries struct { + reviewed, moved sjskills.PublishedRegistry + resolved int + at []string +} + +func (m *movingRegistries) Resolve(context.Context) (sjskills.PublishedRegistry, error) { + m.resolved++ + return m.moved, nil +} +func (m *movingRegistries) At(_ context.Context, commit string) (sjskills.PublishedRegistry, error) { + m.at = append(m.at, commit) + if commit == m.reviewed.Commit { + return m.reviewed, nil + } + return m.moved, nil +} +func (m *movingRegistries) ForStatus(ctx context.Context) (sjskills.PublishedRegistry, error) { + return m.Resolve(ctx) +} +func (m *movingRegistries) ForSelection(ctx context.Context) (sjskills.PublishedRegistry, error) { + return m.Resolve(ctx) +} + +func TestGlobalApplyUsesReviewedRegistryCommit(t *testing.T) { + reviewed, err := fixtureRegistry() + if err != nil { + t.Fatal(err) + } + moved := reviewed + moved.Commit, moved.SHA256 = strings.Repeat("b", 40), strings.Repeat("c", 64) + newApp := func(registries registryLoader) *application { + materializer, _ := testInjectedMaterializer(t) + home := t.TempDir() + return &application{directory: t.TempDir(), homeDirectory: func() (string, error) { return home, nil }, materialize: materializer.Materialize, registries: registries} + } + + planned := newApp(registryFunc(func() (sjskills.PublishedRegistry, error) { return reviewed, nil })).plan(context.Background(), true) + if planned.Result != sjskills.ResultSuccess || planned.Evidence[0] != reviewed.Evidence() { + t.Fatalf("plan %+v evidence=%+v", planned.Error, planned.Evidence) + } + approvedPlan, approvedSHA256 := writeReviewedPlanEnvelope(t, planned) + + registries := &movingRegistries{reviewed: reviewed, moved: moved} + applied := newApp(registries).applyApproved(context.Background(), true, true, approvedPlan, approvedSHA256) + if applied.Result != sjskills.ResultSuccess || registries.resolved != 0 || len(registries.at) != 1 || registries.at[0] != reviewed.Commit { + t.Fatalf("apply %+v resolved=%d at=%v", applied.Error, registries.resolved, registries.at) + } + + // A recorded commit whose registry differs from the reviewed digest fails. + tampered := planned + tampered.Evidence = append([]sjskills.Evidence(nil), planned.Evidence...) + tampered.Evidence[0] = moved.Evidence() + tampered.Evidence[0].Detail = strings.Replace(tampered.Evidence[0].Detail, moved.SHA256, reviewed.SHA256, 1) + approvedPlan, approvedSHA256 = writeReviewedPlanEnvelope(t, tampered) + registries = &movingRegistries{reviewed: reviewed, moved: moved} + applied = newApp(registries).applyApproved(context.Background(), true, true, approvedPlan, approvedSHA256) + if applied.Result != sjskills.ResultConflict || !strings.Contains(applied.Error.Message, "does not match the approved plan") { + t.Fatalf("tampered apply %+v", applied) + } + + // Plans from embedded-registry releases cannot be applied. + legacy := planned + legacy.Evidence = append([]sjskills.Evidence{{Kind: "registry", Detail: "embedded version 4"}}, planned.Evidence[1:]...) + approvedPlan, approvedSHA256 = writeReviewedPlanEnvelope(t, legacy) + registries = &movingRegistries{reviewed: reviewed, moved: moved} + applied = newApp(registries).applyApproved(context.Background(), true, true, approvedPlan, approvedSHA256) + if applied.Result != sjskills.ResultConflict || !strings.Contains(applied.Error.Message, "published registry commit") || len(registries.at)+registries.resolved != 0 { + t.Fatalf("legacy apply %+v", applied) + } +} + +func TestRegistryFailuresAndStaleWarnings(t *testing.T) { + stale, err := fixtureRegistry() + if err != nil { + t.Fatal(err) + } + stale.Stale, stale.StaleReason, stale.ResolvedAt = true, "registry branch resolution returned HTTP 503", time.Now().Add(-50*time.Hour) + app := &application{directory: t.TempDir(), registries: registryFunc(func() (sjskills.PublishedRegistry, error) { return stale, nil })} + envelope := app.profiles(context.Background()) + if envelope.Result != sjskills.ResultSuccess || len(envelope.Warnings) != 1 || envelope.Warnings[0].Code != "registry-stale" { + t.Fatalf("stale profiles %+v", envelope) + } + var out, errout bytes.Buffer + emitEnvelope(&out, &errout, false, envelope) + if !strings.Contains(errout.String(), "using cached skill registry at "+fixtureRegistryCommit[:12]+" resolved 2d ago; refresh failed: registry branch resolution returned HTTP 503") { + t.Fatalf("stale rendering %q", errout.String()) + } + + unsupported := &sjskills.Issue{Code: sjskills.IssueRegistryVersion, Path: "registry.version", Message: "published registry version 5 is not supported"} + for name, test := range map[string]struct { + err error + result sjskills.Result + }{ + "unreachable": {errors.New("skill registry unavailable: offline"), sjskills.ResultUnavailable}, + "unsupported": {unsupported, sjskills.ResultInvalid}, + } { + app := &application{directory: t.TempDir(), registries: registryFunc(func() (sjskills.PublishedRegistry, error) { return sjskills.PublishedRegistry{}, test.err })} + if envelope := app.plan(context.Background(), true); envelope.Result != test.result { + t.Fatalf("%s plan %+v", name, envelope) + } + } +} + +func TestExternalPlanPinsPublishedSourcesToRegistryCommit(t *testing.T) { + directory := t.TempDir() + code, stdout, stderr := runCLI(t, directory, "--json", "plan", "--global") + if code != 0 || stderr != "" { + t.Fatalf("plan code=%d stdout=%q stderr=%q", code, stdout, stderr) + } + envelope := decodeStatusEnvelope(t, stdout) + if envelope.Evidence[0].Kind != "registry" || !strings.HasPrefix(envelope.Evidence[0].Detail, "sjunepark/agent-scripts@"+fixtureRegistryCommit+" sha256:") { + t.Fatalf("registry evidence %+v", envelope.Evidence) + } + for _, skill := range envelope.Plan.Desired.Skills { + if skill.SourceID == "agent-scripts" && skill.Source != "https://github.com/sjunepark/agent-scripts/tree/"+fixtureRegistryCommit+"/skills" { + t.Fatalf("unpinned source %+v", skill) + } + } + + code, stdout, _ = runCLIWithEnvironment(t, directory, map[string]string{"SJSKILLS_FAKE_PUBLIC_FETCH_FAIL": "1"}, "--json", "plan", "--global") + if code == 0 || !strings.Contains(stdout, "public Git fetch failed") { + t.Fatalf("failed fetch code=%d stdout=%q", code, stdout) + } +} diff --git a/cmd/sjskills/status.go b/cmd/sjskills/status.go index 22fd7aa..3a4c593 100644 --- a/cmd/sjskills/status.go +++ b/cmd/sjskills/status.go @@ -23,8 +23,9 @@ type commandStatusSnapshot struct { func (a *application) finishPrepared(p *preparedPlan, envelope sjskills.Envelope, stage string) sjskills.Envelope { envelope = p.finish(envelope, stage) if envelope.Result == sjskills.ResultSuccess && p.verified && p.cleaned { - registry, err := a.registry() + published, err := a.registry(nil, registryLoader.Resolve) if err == nil { + registry := published.Registry root := "" if p.project != nil { root = p.project.Layout.Root @@ -57,6 +58,9 @@ func (a *application) status(ctx context.Context) sjskills.Envelope { envelope.Status = &report.result envelope.Advisories = report.advisories envelope.CLIAdvisory = report.cli + if a.registryErr == nil { + envelope.Warnings = append(envelope.Warnings, registryWarnings(a.registryValue, time.Now())...) + } } if ctx.Err() != nil { envelope.Result = sjskills.ResultUnavailable @@ -97,7 +101,13 @@ func (a *application) collectStatus(ctx context.Context) statusCollection { ctx, cancel := context.WithTimeout(ctx, sjskills.StatusRefreshBudget) defer cancel() report := statusCollection{result: discoverStatusProject(a.directory)} - registry, registryErr := a.registry() + published, registryErr := a.registry(ctx, registryLoader.ForStatus) + registry := published.Registry + registryUnavailable := "skill registry unavailable" + var registryIssue *sjskills.Issue + if errors.As(registryErr, ®istryIssue) && registryIssue.Code == sjskills.IssueRegistryVersion { + registryUnavailable = registryIssue.Message + } results := make([]*sjskills.Advisory, 2) var group sync.WaitGroup // CLI evidence and both skill scopes share one foreground deadline. @@ -142,7 +152,7 @@ func (a *application) collectStatus(ctx context.Context) statusCollection { if !global { report.result.ProjectConfiguration = sjskills.ProjectUnavailable } - unavailable("skill registry unavailable") + unavailable(registryUnavailable) return } scope, resolveErr := sjskills.ResolveStatusScope(directory, registry, global) diff --git a/cmd/sjskills/status_test.go b/cmd/sjskills/status_test.go index 42b27d1..53ab13d 100644 --- a/cmd/sjskills/status_test.go +++ b/cmd/sjskills/status_test.go @@ -197,7 +197,15 @@ func TestStatusCLIEligibility(t *testing.T) { if runtime.GOOS == "darwin" { cache = filepath.Join(f.home, "Library", "Caches") } + // Commands that load the registry may cache it; no status + // namespace may be written. entries, _ := os.ReadDir(cache) + if len(entries) == 1 && entries[0].Name() == "sjskills" { + entries, _ = os.ReadDir(filepath.Join(cache, "sjskills")) + if len(entries) == 1 && entries[0].Name() == "registry" { + entries = nil + } + } if len(entries) != 0 { t.Fatalf("ineligible cache writes: %v", entries) } diff --git a/cmd/sjskills/testdata/fakebunx/main.go b/cmd/sjskills/testdata/fakebunx/main.go index 894f86e..50e6f29 100644 --- a/cmd/sjskills/testdata/fakebunx/main.go +++ b/cmd/sjskills/testdata/fakebunx/main.go @@ -66,7 +66,8 @@ func main() { if local == "fixture/private/skills" && os.Getenv("SJSKILLS_GIT_FIXTURE") != "" { local = filepath.Join(os.Getenv("SJSKILLS_GIT_FIXTURE"), "skills") } - if filepath.IsAbs(local) { + _, publicErr := os.Stat(filepath.Join(local, ".fake-public")) + if filepath.IsAbs(local) && publicErr != nil { data, err := os.ReadFile(filepath.Join(local, skill, "SKILL.md")) if err != nil { os.Exit(7) diff --git a/cmd/sjskills/testdata/fakegithub/main.go b/cmd/sjskills/testdata/fakegithub/main.go index f74516c..0a5da0e 100644 --- a/cmd/sjskills/testdata/fakegithub/main.go +++ b/cmd/sjskills/testdata/fakegithub/main.go @@ -71,6 +71,10 @@ func main() { if index < 0 || len(args) < 2 || os.Getenv("GIT_ALLOW_PROTOCOL") != "https" || os.Getenv("GIT_CONFIG_COUNT") != "" || os.Getenv("GIT_TRACE") != "" { fail() } + if !strings.Contains(strings.Join(args[:index], " "), "credential.helper=!") { + publicGit(args, index) + return + } joined := strings.Join(args[:index], " ") for _, want := range []string{"credential.helper=", "credential.useHttpPath=true", "http.followRedirects=false", "submodule.recurse=false", "core.hooksPath="} { if !strings.Contains(joined, want) { @@ -129,3 +133,36 @@ func main() { fail() } } + +// publicGit models an anonymous, commit-pinned fetch of the published +// agent-scripts source. The checkout holds only a marker; fake bunx generates +// the skill content it would otherwise download. +func publicGit(args []string, index int) { + joined := strings.Join(args[:index], " ") + for _, want := range []string{"credential.helper=", "http.followRedirects=false", "submodule.recurse=false", "core.hooksPath="} { + if !strings.Contains(joined, want) { + fail() + } + } + switch args[index] { + case "init": + if err := os.MkdirAll(args[len(args)-1], 0o700); err != nil { + fail() + } + case "fetch": + clone, remote, commit := args[index-1], args[len(args)-2], args[len(args)-1] + if remote != "https://github.com/sjunepark/agent-scripts.git" || len(commit) != 40 || os.Getenv("SJSKILLS_FAKE_PUBLIC_FETCH_FAIL") == "1" { + fail() + } + skills := filepath.Join(clone, "skills") + if err := os.MkdirAll(skills, 0o700); err != nil { + fail() + } + if err := os.WriteFile(filepath.Join(skills, ".fake-public"), []byte(commit), 0o600); err != nil { + fail() + } + case "checkout": + default: + fail() + } +} diff --git a/internal/sjskills/github_auth.go b/internal/sjskills/github_auth.go index 4e590a9..5558834 100644 --- a/internal/sjskills/github_auth.go +++ b/internal/sjskills/github_auth.go @@ -105,6 +105,51 @@ func gitShellQuote(value string) string { return "'" + strings.ReplaceAll(filepath.ToSlash(value), "'", "'\\''") + "'" } +// pinnedPublicGitHubSource reports a public GitHub /tree// source. +// Skills CLI clones refs with --branch, which rejects commits, so these +// sources are fetched with Git and installed from the local checkout. +func pinnedPublicGitHubSource(skill DesiredSkill) bool { + if skill.Access == AccessGitHubAuthenticated || !strings.HasPrefix(skill.Source, "https://github.com/") { + return false + } + if _, err := githubRepository(skill.Source); err != nil { + return false + } + ref, _ := githubSourcePath(skill.Source) + return fullGitCommit.MatchString(ref) +} + +// publicSource fetches a commit-pinned public GitHub source anonymously. Every +// credential helper is disabled; only the pinned commit is fetched. Git runs in +// the environment Skills CLI uses for its own remote clones, keeping system +// configuration such as core.autocrlf, so installed bytes and expected-content +// hashes match those of a branch install on the same machine. +func (m *Materializer) publicSource(ctx context.Context, root, source string) (string, error) { + repo, err := githubRepository(source) + if err != nil { + return "", err + } + git, err := m.lookPath("git") + if err != nil { + return "", errors.New("git is unavailable; commit-pinned GitHub skills are fetched with Git") + } + options := []string{"-c", "credential.helper=", "-c", "http.followRedirects=false", "-c", "submodule.recurse=false", "-c", "core.hooksPath=" + filepath.Join(root, ".empty-hooks")} + // Inherited GIT_* variables such as GIT_DIR or GIT_WORK_TREE (set inside Git + // hooks) could redirect the forced checkout into a caller's repository, and + // askpass or token variables could make the fetch non-anonymous. + base := make([]string, 0, len(m.baseEnv)) + for _, entry := range m.baseEnv { + key, _, _ := strings.Cut(entry, "=") + upper := strings.ToUpper(key) + if !strings.HasPrefix(upper, "GIT_") && !strings.HasPrefix(upper, "GH_") && upper != "SSH_ASKPASS" && upper != "GITHUB_TOKEN" { + base = append(base, entry) + } + } + env := append(isolatedEnvironment(base, root, m.platform), "GIT_TERMINAL_PROMPT=0", "GIT_ALLOW_PROTOCOL=https") + return m.gitCheckout(ctx, root, git, repo, source, options, env, + "public Git fetch failed; verify the network and that the pinned commit exists") +} + func (m *Materializer) authenticatedSource(ctx context.Context, root, source string) (string, error) { repo, err := githubRepository(source) if err != nil { @@ -144,13 +189,6 @@ func (m *Materializer) authenticatedSource(ctx context.Context, root, source str if err != nil || !filepath.IsAbs(helper) { return "", errors.New("cannot locate sjskills credential helper") } - clone, err := os.MkdirTemp(root, "github-") - if err != nil { - return "", errors.New("cannot prepare private Git staging") - } - // Git clone expects an absent or empty destination. Each fetch has a distinct - // directory; its lifetime and cleanup belong to the materialization plan. - ref, subpath := githubSourcePath(source) env := credentialEnvironment(m.baseEnv, root, m.platform) if token != "" { env = append(env, "GH_TOKEN="+token) @@ -158,6 +196,20 @@ func (m *Materializer) authenticatedSource(ctx context.Context, root, source str env = append(env, "SJSKILLS_GH_EXECUTABLE="+gh, "SJSKILLS_GH_CONFIG_DIR="+config, "SJSKILLS_GH_REPOSITORY="+repo) helperCommand := "!" + gitShellQuote(helper) + " " + GitHubCredentialCommand options := []string{"-c", "credential.helper=", "-c", "credential.helper=" + helperCommand, "-c", "credential.useHttpPath=true", "-c", "http.followRedirects=false", "-c", "submodule.recurse=false", "-c", "core.hooksPath=" + filepath.Join(root, ".empty-hooks")} + return m.gitCheckout(ctx, root, git, repo, source, options, env, + "authenticated Git fetch failed; verify gh login, repository access, ref, and network") +} + +// gitCheckout fetches source's ref into a fresh staging directory and returns +// the verified local subpath. Process output is never used as diagnostics. +func (m *Materializer) gitCheckout(ctx context.Context, root, git, repo, source string, options, env []string, failure string) (string, error) { + clone, err := os.MkdirTemp(root, "github-") + if err != nil { + return "", errors.New("cannot prepare Git staging") + } + // Git clone expects an absent or empty destination. Each fetch has a distinct + // directory; its lifetime and cleanup belong to the materialization plan. + ref, subpath := githubSourcePath(source) remote := "https://github.com/" + repo + ".git" commands := [][]string{} if fullGitCommit.MatchString(ref) { @@ -178,16 +230,16 @@ func (m *Materializer) authenticatedSource(ctx context.Context, root, source str args := append(append([]string{}, options...), command...) result, err := m.runCommand(ctx, git, args, env) if err != nil || result.ExitCode != 0 { - return "", privateProcessError("authenticated Git fetch failed; verify gh login, repository access, ref, and network", err) + return "", privateProcessError(failure, err) } } local := filepath.Join(clone, filepath.FromSlash(subpath)) if err := validateSymlinkParents(clone, local); err != nil { - return "", errors.New("authenticated source subpath is missing or unsafe") + return "", errors.New("Git source subpath is missing or unsafe") } if err := checkRealDirectory(local); err != nil { - return "", errors.New("authenticated source subpath is not a real directory") + return "", errors.New("Git source subpath is not a real directory") } return local, nil } diff --git a/internal/sjskills/materialize.go b/internal/sjskills/materialize.go index 1e3394b..d7a27e5 100644 --- a/internal/sjskills/materialize.go +++ b/internal/sjskills/materialize.go @@ -327,14 +327,18 @@ func (m *Materializer) Materialize(ctx context.Context, skills []DesiredSkill) ( _ = plan.Cleanup() return nil, m.errorAt(root, safeSkillName(skill.Name), "build Skills CLI command", err) } - if skill.Access == AccessGitHubAuthenticated { - local, fetchErr := m.authenticatedSource(ctx, root, skill.Source) + if skill.Access == AccessGitHubAuthenticated || pinnedPublicGitHubSource(skill) { + fetch, operation := m.publicSource, "fetch pinned source" + if skill.Access == AccessGitHubAuthenticated { + fetch, operation = m.authenticatedSource, "fetch authenticated source" + } + local, fetchErr := fetch(ctx, root, skill.Source) if fetchErr != nil { if errors.Is(fetchErr, errProcessTreeActive) { return nil, errProcessTreeActive } _ = plan.Cleanup() - return nil, m.errorAt(root, safeSkillName(skill.Name), "fetch authenticated source", fetchErr) + return nil, m.errorAt(root, safeSkillName(skill.Name), operation, fetchErr) } // Only this internally verified staging path may bypass remote input // validation; DesiredSkill and provenance retain the original URL. diff --git a/internal/sjskills/materialize_test.go b/internal/sjskills/materialize_test.go index c1a6593..46bd52b 100644 --- a/internal/sjskills/materialize_test.go +++ b/internal/sjskills/materialize_test.go @@ -988,3 +988,68 @@ func TestMaterializeBatchesSourceAndDiscoveryOptionsWithoutPartialSuccess(t *tes t.Fatalf("partial batch accepted: plan=%v err=%v stage=%s", plan, err, stage()) } } + +func TestMaterializeFetchesCommitPinnedPublicSourceWithGit(t *testing.T) { + const commit = "0123456789abcdef0123456789abcdef01234567" + runner := defaultMaterializeRunner() + skillsAdd := runner.invoke + runner.invoke = func(ctx context.Context, command string, args []string, env []string) (ProcessResult, error) { + if command != "git" { + return skillsAdd(ctx, command, args, env) + } + index := 0 + for index < len(args) && args[index] == "-c" { + index += 2 + } + switch operation := args[index:]; { + case operation[0] == "init": + return ProcessResult{}, os.MkdirAll(operation[len(operation)-1], 0o700) + case operation[0] == "-C" && operation[2] == "fetch": + return ProcessResult{}, os.MkdirAll(filepath.Join(operation[1], "skills"), 0o700) + case operation[0] == "-C" && operation[2] == "checkout": + return ProcessResult{}, nil + } + return ProcessResult{}, fmt.Errorf("unexpected git args: %q", args) + } + m, _ := testMaterializer(t, runner, MaterializerLimits{}) + m.baseEnv = append(m.baseEnv, "GIT_DIR=/caller/repo/.git", "git_work_tree=/caller/repo", "SSH_ASKPASS=/askpass", "GH_TOKEN=token") + source := "https://github.com/sjunepark/agent-scripts/tree/" + commit + "/skills" + plan, err := m.Materialize(context.Background(), []DesiredSkill{desiredMaterializeSkill("demo", source), desiredMaterializeSkill("other", source)}) + if err != nil { + t.Fatal(err) + } + defer plan.Cleanup() + var git []materializeCall + var add materializeCall + for _, call := range runner.calls { + if call.command == "git" { + git = append(git, call) + } else if len(call.args) > 1 && call.args[1] == "add" { + add = call + } + } + if len(git) != 3 { + t.Fatalf("git calls = %d, want one init/fetch/checkout for the shared source", len(git)) + } + fetch := strings.Join(git[1].args, " ") + if !strings.Contains(fetch, "fetch --depth=1 --no-tags --no-recurse-submodules -- https://github.com/sjunepark/agent-scripts.git "+commit) { + t.Fatalf("fetch = %q", fetch) + } + for _, call := range git { + joined := strings.Join(call.args, " ") + if strings.Contains(joined, "credential.helper=!") || !strings.Contains(joined, "-c credential.helper= ") || envValue(call.env, "GIT_TERMINAL_PROMPT") != "0" { + t.Fatalf("git call is not anonymous: %q", joined) + } + // System configuration (core.autocrlf) must match Skills CLI's own + // clones, while a caller's repository variables must never leak in. + if envHas(call.env, "GIT_CONFIG_NOSYSTEM") || envHas(call.env, "GIT_DIR") || envHas(call.env, "git_work_tree") || envHas(call.env, "SSH_ASKPASS") || envHas(call.env, "GH_TOKEN") || envValue(call.env, "HOME") == "" { + t.Fatalf("git environment = %q", call.env) + } + } + if !filepath.IsAbs(add.args[2]) || filepath.Base(add.args[2]) != "skills" { + t.Fatalf("skills add source = %q, want the fetched checkout", add.args[2]) + } + if plan.Snapshots()[0].Skill.Source != source { + t.Fatal("snapshot lost the pinned source URL") + } +} diff --git a/internal/sjskills/registry_source.go b/internal/sjskills/registry_source.go index aa5c21e..58becd6 100644 --- a/internal/sjskills/registry_source.go +++ b/internal/sjskills/registry_source.go @@ -26,6 +26,7 @@ const ( registryFile = "skill-registry.json" registryRefsURL = "https://github.com/" + registryRepository + ".git/info/refs?service=git-upload-pack" registryRawBase = "https://raw.githubusercontent.com/" + registryRepository + "/" + registryTreeMain = "https://github.com/" + registryRepository + "/tree/main" maxRegistryBytes = 1 << 20 maxRefAdvertisement = 4 << 20 @@ -35,6 +36,12 @@ const ( ) var registryCommitPattern = regexp.MustCompile(`^[0-9a-f]{40}$`) +var errRegistryCommitMissing = errors.New("registry commit not found") +var registryEvidencePattern = regexp.MustCompile(`^` + regexp.QuoteMeta(registryRepository) + `@([0-9a-f]{40}) sha256:[0-9a-f]{64}$`) + +// registryEndpoints is replaced only by the sjskillstest build tag, so CLI +// integration tests can serve a fixture registry. Release builds never read it. +var registryEndpoints = func() (refs, raw string) { return registryRefsURL, registryRawBase } // PublishedRegistry is a validated registry together with the commit that // supplied it and the SHA-256 of its exact published bytes. ResolvedAt is when @@ -56,8 +63,11 @@ type PublishedRegistry struct { // Git executable, or GitHub API quota. type RegistrySource struct { CacheRoot string - Now func() time.Time - Client *http.Client + // NoCache neither reads nor writes the registry cache, for invocations that + // opted out of cache writes. Every load then resolves the branch afresh. + NoCache bool + Now func() time.Time + Client *http.Client // Tests replace the published endpoints; production uses the fixed location. refsURL string @@ -76,6 +86,28 @@ type registryLatest struct { func (e registryLatest) resolved() bool { return e.Commit != "" } +// Evidence identifies the exact registry bytes behind a plan. Reviewed global +// plans carry it, and apply reloads the registry at its commit. +func (p PublishedRegistry) Evidence() Evidence { + return Evidence{Kind: "registry", Detail: registryRepository + "@" + p.Commit + " sha256:" + p.SHA256} +} + +// RegistryCommitFromEvidence returns the commit recorded by Evidence. +func RegistryCommitFromEvidence(evidence []Evidence) (string, bool) { + commit := "" + for _, item := range evidence { + if item.Kind != "registry" { + continue + } + match := registryEvidencePattern.FindStringSubmatch(item.Detail) + if match == nil || commit != "" { + return "", false + } + commit = match[1] + } + return commit, commit != "" +} + func (s RegistrySource) now() time.Time { if s.Now != nil { return s.Now().UTC() @@ -192,16 +224,15 @@ func (s RegistrySource) cachedAt(directory string, latest registryLatest) (Publi return published, nil } +// load always fetches: plan and apply need the network anyway, and a cached +// file has no digest to verify against until a branch observation records one. +// The fetched bytes then refresh the cache used by status fallbacks. func (s RegistrySource) load(ctx context.Context, directory string, cacheErr error, commit string) (PublishedRegistry, error) { - path := filepath.Join(directory, commit+".json") - if cacheErr == nil { - if data, err := readStatusFile(path); err == nil { - if published, err := parsePublishedRegistry(commit, data); err == nil { - return published, nil - } - } - } data, err := s.fetch(ctx, commit) + if errors.Is(err, errRegistryCommitMissing) { + return PublishedRegistry{}, &Issue{Code: IssueReconciliationConflict, Path: "registry.commit", Message: fmt.Sprintf( + "published registry commit %s is unavailable from %s; review a new plan", commit, registryRepository)} + } if err != nil { return PublishedRegistry{}, err } @@ -223,10 +254,25 @@ func parsePublishedRegistry(commit string, data []byte) (PublishedRegistry, erro if err != nil { return PublishedRegistry{}, err } + pinPublishedSources(®istry, commit) sum := sha256.Sum256(data) return PublishedRegistry{Registry: registry, Commit: commit, SHA256: hex.EncodeToString(sum[:])}, nil } +// pinPublishedSources makes skill contents come from the registry's own +// commit. Other sources, including other refs of this repository, keep their +// declared locations. Provenance identity ignores the ref, so pinning does not +// change ownership of installed copies. +func pinPublishedSources(registry *Registry, commit string) { + for id, source := range registry.Sources { + rest, ok := strings.CutPrefix(source.Location, registryTreeMain) + if ok && (rest == "" || strings.HasPrefix(rest, "/")) { + source.Location = "https://github.com/" + registryRepository + "/tree/" + commit + rest + registry.Sources[id] = source + } + } +} + // checkRegistryVersion names the required action for a schema this binary // cannot interpret, before strict decoding reports unrelated unknown fields. func checkRegistryVersion(data []byte) error { @@ -244,7 +290,7 @@ func checkRegistryVersion(data []byte) error { func (s RegistrySource) resolveCommit(ctx context.Context) (string, error) { url := s.refsURL if url == "" { - url = registryRefsURL + url, _ = registryEndpoints() } data, contentType, err := s.get(ctx, url, maxRefAdvertisement, "registry branch resolution") if err != nil { @@ -259,9 +305,13 @@ func (s RegistrySource) resolveCommit(ctx context.Context) (string, error) { func (s RegistrySource) fetch(ctx context.Context, commit string) ([]byte, error) { base := s.rawBase if base == "" { - base = registryRawBase + _, base = registryEndpoints() } data, _, err := s.get(ctx, base+commit+"/"+registryFile, maxRegistryBytes, "registry fetch") + var status httpStatusError + if errors.As(err, &status) && status.code == http.StatusNotFound { + return nil, errRegistryCommitMissing + } return data, err } @@ -280,7 +330,7 @@ func (s RegistrySource) get(ctx context.Context, url string, limit int64, operat } defer response.Body.Close() if response.StatusCode != http.StatusOK { - return nil, "", fmt.Errorf("%s returned HTTP %d", operation, response.StatusCode) + return nil, "", httpStatusError{operation, response.StatusCode} } data, err := io.ReadAll(io.LimitReader(response.Body, limit+1)) if err != nil || int64(len(data)) > limit { @@ -289,6 +339,15 @@ func (s RegistrySource) get(ctx context.Context, url string, limit int64, operat return data, strings.TrimSpace(strings.Split(response.Header.Get("Content-Type"), ";")[0]), nil } +type httpStatusError struct { + operation string + code int +} + +func (e httpStatusError) Error() string { + return fmt.Sprintf("%s returned HTTP %d", e.operation, e.code) +} + // parseRefAdvertisement reads Git's pkt-line ref advertisement and returns the // commit for ref. Any framing error rejects the whole response. func parseRefAdvertisement(data []byte, ref string) (string, error) { @@ -327,6 +386,9 @@ func parseRefAdvertisement(data []byte, ref string) (string, error) { } func (s RegistrySource) cacheDirectory() (string, error) { + if s.NoCache { + return "", errors.New("registry cache disabled") + } return disposableCacheDirectory(s.CacheRoot, "registry") } diff --git a/internal/sjskills/registry_source_test.go b/internal/sjskills/registry_source_test.go index 319a8e7..584750a 100644 --- a/internal/sjskills/registry_source_test.go +++ b/internal/sjskills/registry_source_test.go @@ -8,6 +8,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "sync/atomic" "testing" @@ -116,11 +118,12 @@ func TestRegistrySourceResolveFetchesAtResolvedCommitAndCaches(t *testing.T) { if published.Registry.Version != RegistryVersion { t.Fatalf("registry version = %d", published.Registry.Version) } - // The commit cache avoids refetching content; the branch is always resolved. + // Plan and apply never trust an unverifiable cached file: both the branch + // and the registry bytes are fetched again. if _, err := source.Resolve(context.Background()); err != nil { t.Fatal(err) } - if server.refs.Load() != 2 || server.raw.Load() != 1 { + if server.refs.Load() != 2 || server.raw.Load() != 2 { t.Fatalf("refs=%d raw=%d", server.refs.Load(), server.raw.Load()) } } @@ -281,9 +284,82 @@ func TestRegistrySourceIgnoresTamperedCommitCache(t *testing.T) { if err := writeRegistryCacheFile(directory, testCommitA+".json", []byte(tampered)); err != nil { t.Fatal(err) } + // Resolve ignores and repairs a tampered commit file. + if published, err := source.Resolve(context.Background()); err != nil || published.Registry.Description == "Tampered." { + t.Fatalf("resolve used tampered cache: %+v err=%v", published.Registry.Description, err) + } + if err := writeRegistryCacheFile(directory, testCommitA+".json", []byte(tampered)); err != nil { + t.Fatal(err) + } server.offline.Store(true) // A digest mismatch makes the cached registry unusable rather than trusted. if _, err := source.ForStatus(context.Background()); err == nil { t.Fatal("status used a tampered cached registry") } } + +func TestPublishedRegistryPinsOwnSourcesAndRecordsEvidence(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + published, err := source.Resolve(context.Background()) + if err != nil { + t.Fatal(err) + } + if got := published.Registry.Sources["agent-scripts"].Location; got != "https://github.com/sjunepark/agent-scripts/tree/"+testCommitA+"/skills" { + t.Fatalf("agent-scripts location = %q", got) + } + if got := published.Registry.Sources["darty"].Location; got != "https://github.com/cpaikr/darty/tree/main/skill/darty" { + t.Fatalf("external location changed: %q", got) + } + registry := Registry{Sources: map[string]Source{"other-ref": {Location: registryTreeMain + "tenance/skills"}}} + pinPublishedSources(®istry, testCommitA) + if registry.Sources["other-ref"].Location != registryTreeMain+"tenance/skills" { + t.Fatal("pinned a different branch whose name shares the prefix") + } + + evidence := published.Evidence() + if commit, ok := RegistryCommitFromEvidence([]Evidence{{Kind: "resolution"}, evidence}); !ok || commit != testCommitA { + t.Fatalf("commit=%q ok=%v", commit, ok) + } + for name, items := range map[string][]Evidence{ + "legacy": {{Kind: "registry", Detail: "embedded version 4"}}, + "duplicate": {evidence, evidence}, + "missing": {{Kind: "resolution"}}, + "other": {{Kind: "registry", Detail: "example/other@" + testCommitA + " sha256:" + published.SHA256}}, + } { + if _, ok := RegistryCommitFromEvidence(items); ok { + t.Fatalf("%s: accepted", name) + } + } +} + +func TestRegistrySourceNoCacheWritesNothing(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + cacheRoot := filepath.Join(t.TempDir(), "cache") + source := server.source(t, cacheRoot, &now) + source.NoCache = true + for _, load := range []func(context.Context) (PublishedRegistry, error){source.Resolve, source.ForStatus, source.ForSelection} { + if _, err := load(context.Background()); err != nil { + t.Fatal(err) + } + } + if _, err := os.Stat(cacheRoot); !os.IsNotExist(err) { + t.Fatalf("cache written: %v", err) + } + if server.raw.Load() != 3 { + t.Fatalf("raw fetches = %d, want one per load without a cache", server.raw.Load()) + } +} + +func TestRegistrySourceMissingReviewedCommitConflicts(t *testing.T) { + server := newRegistryServer(t, testCommitA, map[string]string{testCommitA: string(registryV4JSON)}) + now := time.Date(2026, 9, 30, 0, 0, 0, 0, time.UTC) + source := server.source(t, t.TempDir(), &now) + _, err := source.At(context.Background(), testCommitB) + var issue *Issue + if !errors.As(err, &issue) || issue.Code != IssueReconciliationConflict || !strings.Contains(issue.Message, "review a new plan") { + t.Fatalf("err = %v", err) + } +} diff --git a/internal/sjskills/registry_source_testhook.go b/internal/sjskills/registry_source_testhook.go new file mode 100644 index 0000000..f8a4f26 --- /dev/null +++ b/internal/sjskills/registry_source_testhook.go @@ -0,0 +1,13 @@ +//go:build sjskillstest + +package sjskills + +import "os" + +// CLI integration tests build with -tags sjskillstest and serve a fixture +// registry at SJSKILLS_TEST_REGISTRY_URL (/refs and /raw//). +func init() { + if base := os.Getenv("SJSKILLS_TEST_REGISTRY_URL"); base != "" { + registryEndpoints = func() (string, string) { return base + "/refs", base + "/raw/" } + } +} diff --git a/internal/sjskills/reviewed_plan.go b/internal/sjskills/reviewed_plan.go index c8bdf00..0ade868 100644 --- a/internal/sjskills/reviewed_plan.go +++ b/internal/sjskills/reviewed_plan.go @@ -27,6 +27,12 @@ type ReviewedPlan struct { // manufacture approval evidence without passing through that loader. func (p ReviewedPlan) SHA256() string { return p.sha256 } +// RegistryCommit returns the published registry commit the reviewed plan +// was built from. Plans from embedded-registry releases record none. +func (p ReviewedPlan) RegistryCommit() (string, bool) { + return RegistryCommitFromEvidence(p.envelope.Evidence) +} + // LoadReviewedPlan reads the approval artifact once, binds it to the supplied // digest, and accepts only a successful global plan emitted by sjskills. func LoadReviewedPlan(path, approvedSHA256 string) (ReviewedPlan, error) { diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md index 8705701..71a61d1 100644 --- a/plans/sjskills-remote-registry.md +++ b/plans/sjskills-remote-registry.md @@ -10,24 +10,25 @@ schema or the CLI itself changes. ## Current state -Proposed on 2026-09-30. Steps 1 and 2 are complete: `RegistrySource` -(`internal/sjskills/registry_source.go`) resolves, fetches, validates, and -caches the published registry, and the CLI loads its registry once per -invocation. Production still reads the embedded registry until step 3. - -The executable embeds `internal/sjskills/data/registry-v4.json` -(`internal/sjskills/registry.go`) and reads no other registry, while skill -contents come from `.../tree/main/skills` at run time. The two can drift -unnoticed: the CLI release notice compares version numbers only, and nothing -records which commit supplied the contents. `PROGRESS.md` records that syncing -a registry change on 2026-09-10 needed a temporary binary until v1.2.0 shipped it. +Proposed on 2026-09-30. Steps 1-3 are complete on `dev`, unreleased. The CLI +reads the published registry through `RegistrySource` +(`internal/sjskills/registry_source.go`), pins agent-scripts sources to the +registry's commit, records commit and digest evidence, and binds global apply +to the reviewed commit. The embedded registry data remains only as the Go test +fixture and for the Node equality test until step 4. + +Before this plan, the executable embedded +`internal/sjskills/data/registry-v4.json` while skill contents came from +`.../tree/main/skills` at run time, so the two could drift unnoticed. +`PROGRESS.md` records that syncing a registry change on 2026-09-10 needed a +temporary binary until v1.2.0 shipped it. ## Decisions - **Source of truth.** The binary embeds the registry's location (repository, branch `main`, path `skill-registry.json`), not its contents. Remove the embedded registry data and the Node test that keeps it equal to the root - file. Tests use fixtures through the existing `loadRegistry` seam. + file. Tests use fixtures through the CLI's `registryLoader` seam. - **One commit per invocation.** Resolve `main` to a commit once, fetch the registry at that commit, and pin the agent-scripts source for every skill to `.../tree//skills`. Load the registry once per command and share it @@ -76,8 +77,25 @@ a registry change on 2026-09-10 needed a temporary binary until v1.2.0 shipped i process on the status path. The registry comes from the raw URL at the resolved commit; a published version other than 4 fails closed with an update message and never falls back to a cached registry. -3. Pin agent-scripts sources to the resolved commit and add registry - commit/digest evidence; make global apply reuse the reviewed commit. +3. ~~Pin agent-scripts sources to the resolved commit and add registry + commit/digest evidence; make global apply reuse the reviewed commit.~~ + Done 2026-09-30. Evidence reads + `sjunepark/agent-scripts@ sha256:`; a reviewed plan without + it (from an embedded-registry release) must be re-reviewed. Public + `/tree/` GitHub sources are fetched with Git in Skills CLI's own + isolated environment, which keeps system configuration such as + `core.autocrlf`: a first attempt that disabled system configuration checked + out LF files on Windows and reported every CRLF-installed skill as an update. + A live read-only `plan --global` now matches the previous binary's + expected-content hashes exactly. The fetch strips inherited `GIT_*`, `GH_*`, + askpass, and token variables so hooks cannot redirect it into a caller's + repository. `Resolve` and `At` always fetch the registry bytes; a reviewed + commit that no longer exists is a conflict requiring a new plan. + `--no-status-check` skips the registry cache entirely, so it has no offline + fallback; step 5 documents this. Authenticated sources still disable system + Git configuration (LF checkouts on Windows), a pre-existing difference from + public sources. CLI integration tests build with the `sjskillstest` tag, which alone + lets the binary read a fixture registry server; release builds never set it. 4. Remove the embedded registry data and equality test; move Go tests to fixtures. 5. Update `docs/skill-registry.md`, `docs/sjskills-releases.md`, @@ -127,9 +145,7 @@ release step authorizes. ## Next action -Implement step 3: switch production loading to `RegistrySource` (`Resolve` -for plan/apply, `At` for reviewed global apply, `ForStatus`/`ForSelection` -with stale labels elsewhere), pin agent-scripts sources to the resolved -commit, and replace the `embedded version 4` evidence with commit and digest. -Binary-level CLI tests route HTTPS through an unreachable proxy, so they will -need a fake registry endpoint. +Implement step 4: move `internal/sjskills/data/registry-v4.json` to a Go test +fixture (`cmd/sjskills/registry_fixture_test.go` and the internal tests read it +today), remove `EmbeddedRegistry` and the Node equality test, and keep +`scripts/validate-skills` validating the root registry. diff --git a/scripts/release.py b/scripts/release.py index 2bfc158..8c2fd8a 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -57,7 +57,9 @@ def build(output, tag=None): for target in TARGETS: executable = 'sjskills.exe' if target['os'] == 'windows' else 'sjskills' binary = Path(directory) / executable - env = dict(os.environ, GOOS=target['os'], GOARCH=target['arch'], CGO_ENABLED='0') + # GOFLAGS could add build tags such as sjskillstest, whose binaries + # read a test registry endpoint; release builds use none. + env = dict(os.environ, GOOS=target['os'], GOARCH=target['arch'], CGO_ENABLED='0', GOFLAGS='') subprocess.run(['go', 'build', '-trimpath', '-buildvcs=true', '-o', str(binary), './cmd/sjskills'], cwd=ROOT, env=env, check=True) metadata = json.dumps({'version': VERSION, 'commit': commit, 'target': f"{target['os']}/{target['arch']}"}, sort_keys=True).encode() + b'\n' contents = {executable: binary.read_bytes(), 'release.json': metadata} From 2fb873cffcd3a4af593474c527ad215f3a7bdb77 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 14:38:23 +0900 Subject: [PATCH 6/7] Stop embedding the skill registry in sjskills Step 4 of the remote-registry plan. The CLI reads the registry from the published source, so the embedded copy and EmbeddedRegistry are removed. The former data file becomes a frozen Go test fixture under internal/sjskills/testdata: tests assert its catalog counts, so catalog changes on main no longer require test edits. The Node test drops its embedded-copy equality check and keeps validating skill-registry.json against the repository's skills. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 ++-- cmd/sjskills/main_test.go | 8 +++---- cmd/sjskills/registry_fixture_test.go | 7 +++++- internal/sjskills/contracts_test.go | 12 +++++++++- internal/sjskills/registry.go | 14 ----------- internal/sjskills/status_benchmark_test.go | 6 ++--- .../{data => testdata}/registry-v4.json | 0 plans/sjskills-remote-registry.md | 23 +++++++++++-------- scripts/lib/skill-registry.test.js | 4 +--- 9 files changed, 41 insertions(+), 37 deletions(-) delete mode 100644 internal/sjskills/registry.go rename internal/sjskills/{data => testdata}/registry-v4.json (100%) diff --git a/ROADMAP.md b/ROADMAP.md index 3178ed4..e3526fa 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -11,8 +11,8 @@ 1. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) is proposed so registry changes stop requiring an sjskills release. - The CLI reads the published registry at a pinned commit on `dev`; removing - the embedded data, docs, and the release remain. + The CLI reads the published registry at a pinned commit and embeds none on + `dev`; docs and the release remain. ## Tasks diff --git a/cmd/sjskills/main_test.go b/cmd/sjskills/main_test.go index 5f645b9..a747c6a 100644 --- a/cmd/sjskills/main_test.go +++ b/cmd/sjskills/main_test.go @@ -533,7 +533,7 @@ func TestExternalProjectApplyInstallsIdempotently(t *testing.T) { t.Fatalf("idempotent global apply code=%d stdout=%q stderr=%q", code, stdout, stderr) } - registry, err := sjskills.EmbeddedRegistry() + registry, err := fixtureRegistryValue() if err != nil { t.Fatal(err) } @@ -633,7 +633,7 @@ func TestExternalGlobalUpdateAndRestoreLifecycle(t *testing.T) { t.Fatalf("occupied global restore code=%d stdout=%q stderr=%q", code, stdout, stderr) } - registry, err := sjskills.EmbeddedRegistry() + registry, err := fixtureRegistryValue() if err != nil { t.Fatal(err) } @@ -1833,12 +1833,12 @@ func TestRenderHumanShowsReviewableOperationsAndRecoveryCommand(t *testing.T) { } func TestApplicationMaterializationFailuresAndLifecycle(t *testing.T) { - registry, err := sjskills.EmbeddedRegistry() + registry, err := fixtureRegistryValue() if err != nil { t.Fatal(err) } if len(registry.Global.Baseline) == 0 { - t.Fatal("embedded registry has no global baseline") + t.Fatal("fixture registry has no global baseline") } t.Run("materialize failure preserves plan", func(t *testing.T) { diff --git a/cmd/sjskills/registry_fixture_test.go b/cmd/sjskills/registry_fixture_test.go index 5ec235e..e351af3 100644 --- a/cmd/sjskills/registry_fixture_test.go +++ b/cmd/sjskills/registry_fixture_test.go @@ -15,7 +15,7 @@ import ( // fixtureRegistryCommit is the commit the fake published source advertises. const fixtureRegistryCommit = "0123456789abcdef0123456789abcdef01234567" -const fixtureRegistryPath = "../../internal/sjskills/data/registry-v4.json" +const fixtureRegistryPath = "../../internal/sjskills/testdata/registry-v4.json" // registryFunc adapts one load result to every registry policy for // in-process tests; the reviewed commit passed to At is ignored. @@ -47,6 +47,11 @@ func fixtureRegistry() (sjskills.PublishedRegistry, error) { func init() { defaultRegistries = registryFunc(fixtureRegistry) } +func fixtureRegistryValue() (sjskills.Registry, error) { + published, err := fixtureRegistry() + return published.Registry, err +} + // startFixtureRegistryServer serves the fixture registry as the published // source for CLI binaries built with the sjskillstest tag. func startFixtureRegistryServer() (*httptest.Server, error) { diff --git a/internal/sjskills/contracts_test.go b/internal/sjskills/contracts_test.go index dbab0b5..ddadc6c 100644 --- a/internal/sjskills/contracts_test.go +++ b/internal/sjskills/contracts_test.go @@ -9,9 +9,19 @@ import ( "testing" ) +// registryV4JSON is a frozen version 4 registry. Tests assert its catalog +// counts, so it deliberately does not follow the published skill-registry.json. +var registryV4JSON = func() []byte { + data, err := os.ReadFile(filepath.Join("testdata", "registry-v4.json")) + if err != nil { + panic(err) + } + return data +}() + func fixtureRegistry(t *testing.T) Registry { t.Helper() - registry, err := EmbeddedRegistry() + registry, err := ParseRegistry(registryV4JSON) if err != nil { t.Fatal(err) } diff --git a/internal/sjskills/registry.go b/internal/sjskills/registry.go deleted file mode 100644 index 343123f..0000000 --- a/internal/sjskills/registry.go +++ /dev/null @@ -1,14 +0,0 @@ -package sjskills - -import ( - _ "embed" -) - -// registryV4JSON is shipped with the command so an installed binary does not -// depend on the repository working tree for the policy catalog. -// -//go:embed data/registry-v4.json -var registryV4JSON []byte - -// EmbeddedRegistry returns the canonical v4 registry used by the CLI. -func EmbeddedRegistry() (Registry, error) { return ParseRegistry(registryV4JSON) } diff --git a/internal/sjskills/status_benchmark_test.go b/internal/sjskills/status_benchmark_test.go index 2593092..4fc6e84 100644 --- a/internal/sjskills/status_benchmark_test.go +++ b/internal/sjskills/status_benchmark_test.go @@ -12,11 +12,11 @@ import ( "time" ) -// BenchmarkStatusWarmDevGoAndGlobal models the CLI's two scopes with the real -// embedded selections, eight 8 KiB files per placement, and trusted provenance. +// BenchmarkStatusWarmDevGoAndGlobal models the CLI's two scopes with the +// fixture registry's selections, eight 8 KiB files per placement, and trusted provenance. // It measures cache lookup plus fresh local hashing/classification, not network. func BenchmarkStatusWarmDevGoAndGlobal(b *testing.B) { - registry, err := EmbeddedRegistry() + registry, err := ParseRegistry(registryV4JSON) if err != nil { b.Fatal(err) } diff --git a/internal/sjskills/data/registry-v4.json b/internal/sjskills/testdata/registry-v4.json similarity index 100% rename from internal/sjskills/data/registry-v4.json rename to internal/sjskills/testdata/registry-v4.json diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md index 71a61d1..d1fed7b 100644 --- a/plans/sjskills-remote-registry.md +++ b/plans/sjskills-remote-registry.md @@ -10,12 +10,12 @@ schema or the CLI itself changes. ## Current state -Proposed on 2026-09-30. Steps 1-3 are complete on `dev`, unreleased. The CLI +Proposed on 2026-09-30. Steps 1-4 are complete on `dev`, unreleased. The CLI reads the published registry through `RegistrySource` (`internal/sjskills/registry_source.go`), pins agent-scripts sources to the registry's commit, records commit and digest evidence, and binds global apply -to the reviewed commit. The embedded registry data remains only as the Go test -fixture and for the Node equality test until step 4. +to the reviewed commit. The binary embeds no registry data; Go tests use the +frozen fixture `internal/sjskills/testdata/registry-v4.json`. Before this plan, the executable embedded `internal/sjskills/data/registry-v4.json` while skill contents came from @@ -96,8 +96,10 @@ temporary binary until v1.2.0 shipped it. Git configuration (LF checkouts on Windows), a pre-existing difference from public sources. CLI integration tests build with the `sjskillstest` tag, which alone lets the binary read a fixture registry server; release builds never set it. -4. Remove the embedded registry data and equality test; move Go tests to - fixtures. +4. ~~Remove the embedded registry data and equality test; move Go tests to + fixtures.~~ Done 2026-09-30. The fixture is frozen: tests assert its catalog + counts, so catalog changes on `main` no longer touch Go tests. The Node test + still validates the root registry against the repository's skills. 5. Update `docs/skill-registry.md`, `docs/sjskills-releases.md`, `skills/sjskills/` (including its global-rollout reference), and comments that describe the embedded registry. @@ -145,7 +147,10 @@ release step authorizes. ## Next action -Implement step 4: move `internal/sjskills/data/registry-v4.json` to a Go test -fixture (`cmd/sjskills/registry_fixture_test.go` and the internal tests read it -today), remove `EmbeddedRegistry` and the Node equality test, and keep -`scripts/validate-skills` validating the root registry. +Implement step 5: update `docs/skill-registry.md`, `docs/sjskills-releases.md`, +`skills/sjskills/` (including its global-rollout reference), and code comments +that still describe the embedded registry (`internal/sjskills/status.go`, +`internal/sjskills/global_layout.go`). Document the Git prerequisite for +agent-scripts skills, registry evidence and re-review of older plans, stale +registry warnings, and that `--no-status-check` has no offline registry +fallback. diff --git a/scripts/lib/skill-registry.test.js b/scripts/lib/skill-registry.test.js index 34121b9..4879d9c 100644 --- a/scripts/lib/skill-registry.test.js +++ b/scripts/lib/skill-registry.test.js @@ -33,15 +33,13 @@ function assertErrorIncludes(errors, expected) { ); } -test("live registry is the validated version 4 contract embedded by sjskills", () => { +test("live registry is the validated version 4 contract published to sjskills", () => { const live = liveRegistry(); - const embedded = readJSON("internal/sjskills/data/registry-v4.json"); const repositorySkillNames = fs .readdirSync(path.join(repositoryRoot, "skills"), { withFileTypes: true }) .filter((entry) => entry.isDirectory()) .map((entry) => entry.name); - assert.deepEqual(live, embedded); assert.deepEqual(validateSkillRegistry(live, { repositorySkillNames }), []); }); From a5891d5bf94de494d8b352650a0731d132eb4be9 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Wed, 30 Sep 2026 14:47:09 +0900 Subject: [PATCH 7/7] Document reading the skill registry from the published source Step 5 of the remote-registry plan. docs/skill-registry.md now owns the publication and loading contract: the registry is read from main at run time, agent-scripts skills install from its commit, plans record commit and digest evidence that global apply reloads, older reviewed plans need re-review, cached registries carry a registry-stale warning, and --no-status-check has no offline fallback. The release guide notes that only schema or CLI changes need a release and that plan/apply need Git. The sjskills skill and its references describe the same boundaries. The installed-archive consumer check runs offline and the binary no longer embeds a registry, so test-release.py seeds the registry cache: status uses it, profiles/init fall back with a stale warning, and plan fails because it never uses a cached registry. It passed on windows/amd64. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 4 +- docs/sjskills-releases.md | 16 +++-- docs/skill-registry.md | 71 +++++++++++++++++--- internal/sjskills/global_layout.go | 2 +- internal/sjskills/status.go | 2 +- plans/sjskills-remote-registry.md | 23 ++++--- scripts/test-release.py | 22 +++++- skills/sjskills/SKILL.md | 16 +++-- skills/sjskills/references/global-rollout.md | 10 +-- skills/sjskills/references/status.md | 9 ++- 10 files changed, 131 insertions(+), 44 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index e3526fa..46e215a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -11,8 +11,8 @@ 1. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) is proposed so registry changes stop requiring an sjskills release. - The CLI reads the published registry at a pinned commit and embeds none on - `dev`; docs and the release remain. + Implementation and docs are complete on `dev`; the integration PR and + release remain. ## Tasks diff --git a/docs/sjskills-releases.md b/docs/sjskills-releases.md index e3bc55d..77c118d 100644 --- a/docs/sjskills-releases.md +++ b/docs/sjskills-releases.md @@ -13,7 +13,11 @@ GitHub Releases are the canonical binary source. `packaging/targets.json` owns both the native verification matrix and installer target selection: macOS Intel, macOS Apple silicon, and Windows x64. Linux reconciliation is unsupported. -`internal/sjskills/VERSION` is embedded in the executable. Tags use +`internal/sjskills/VERSION` is embedded in the executable. The skill registry +is not: `sjskills` reads it from `main` at run time (see +[publication and loading](skill-registry.md#publication-and-loading)), so +registry changes need no release. Release only for a registry schema change or +a CLI change. Tags use `sjskills-vX.Y.Z`; archives use `sjskills_X.Y.Z_OS_ARCH.tar.gz` or `.zip`. Each archive contains only the executable and `release.json`, which records its version, target, and source commit. `SHA256SUMS` covers every archive and both @@ -22,8 +26,9 @@ GitHub HTTPS channel and immutable release identity. 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` still require Bun (`bunx`) and access to the existing pinned Skills CLI and Git -sources. Help and version perform no status checks. Profiles and manifest +.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 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 @@ -107,7 +112,10 @@ exercise help/version, profiles/init, and bare/named/JSON/disabled status from a unrelated directory with an empty PATH and isolated home/cache/staging roots, and verify reinstallation and failure preservation. Status checks cover both unconfigured and configured projects without Bun, plus CLI update comparison -from seeded release metadata with network access blocked. Linux runs source checks and +from seeded release metadata with network access blocked. A seeded registry +cache stands in for the published registry: status uses it, `profiles` and +`init` fall back to it with a stale warning, and `plan` fails because it never +uses a cached registry. Linux runs source checks and cross-builds, not reconciliation tests. Development PRs targeting `dev` and manual source checks run on Linux only. Integration PRs to `main`, merge-queue runs targeting `main`, and releases diff --git a/docs/skill-registry.md b/docs/skill-registry.md index acf9e5c..b4ffd04 100644 --- a/docs/skill-registry.md +++ b/docs/skill-registry.md @@ -5,6 +5,46 @@ recommended external skills without duplicating a skill declaration across global and project policy. +## Publication and loading + +`sjskills` embeds the registry's location, not its contents: it reads +`skill-registry.json` from `sjunepark/agent-scripts` on `main` at run time. +Registry changes take effect once they reach `main`; a new `sjskills` release +is needed only for a registry schema or CLI change. There is no flag to read a +working-tree registry, because reconciliation consumes only published content. +Validate local edits with the commands under +[Validation and consumers](#validation-and-consumers). + +Each invocation resolves `main` to one commit through Git's anonymous smart-HTTP +ref advertisement, which needs no credentials or GitHub API quota, and reads the +registry at that commit from `raw.githubusercontent.com`. Sources declared at +`https://github.com/sjunepark/agent-scripts/tree/main/...` are pinned to the +same commit, so registry and contents cannot drift. Other sources, including +other refs of this repository, keep their declared locations. Provenance +identity ignores the ref, so pinning does not change ownership of installed +copies. The registry is loaded once per invocation and shared by planning, +status, and notices. + +Plans record `registry` evidence as +`sjunepark/agent-scripts@ sha256:`. Global apply reloads the +registry at the reviewed commit instead of resolving `main`, so a push after +review cannot change the reviewed desired state; the recorded digest then +participates in the reviewed-plan comparison. A reviewed commit that no longer +exists is a conflict requiring a new plan. Plans from releases that embedded the +registry lack this evidence and must be reviewed again. + +`plan` and `apply` always resolve and fetch afresh. `status` reuses a +resolution for 24 hours and otherwise follows the status retry cooldown; +`profiles` and `init` always try to refresh. When refreshing fails, those three +commands use the last verified registry, cached by commit under the platform +user-cache directory in `sjskills/registry/`, and warn with +`registry-stale`, naming its commit and age. Without a cached registry, status +reports the registry unavailable and other commands fail. A published registry +whose `version` differs from the binary's supported version fails closed with a +message naming the required `sjskills` update; a cached registry never masks it. +`--no-status-check` neither reads nor writes this cache, so it has no offline +fallback. + ## Desired sets `global.baseline` is one fixed, machine-independent set. It is the only @@ -54,8 +94,14 @@ boundary. Skills CLI-managed sources must be Git shorthand credentials, URL query strings, npm specifiers, and other schemes are rejected for that manager. -Public selections retain the pinned Skills CLI's remote-fetch path and do not -invoke a new gh operation. Authenticated selections require Git, GitHub CLI, +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 +with Git and installed through local-source discovery; these selections require +Git. That fetch disables credential helpers, redirects, and submodules and drops +inherited `GIT_*`, `GH_*`, askpass, and token variables, while keeping system +Git configuration such as `core.autocrlf` so installed bytes match Skills CLI's +own clones on the same machine. Authenticated selections require Git, GitHub CLI, and an existing gh login with repository access. They accept GitHub.com shorthand or credential-free `https://github.com` sources, including supported `/tree/[/]` forms. A ref may be a branch, tag, or full 40-character @@ -90,8 +136,8 @@ helper output or guessing whether a not-found response means denied access. Any selected-source failure prevents reconciliation for that scope; no anonymous retry, optional skipping, or partial successful snapshot is allowed. -Older public manifests and reviewed global plans remain usable; explicit and -omitted public access have the same meaning. Older executables may reject the +Older public manifests remain usable; explicit and omitted public access have +the same meaning. Older executables may reject the new fields/profiles: use a build supporting authenticated access for private selections and the same executable for plan/apply. Source installation support in this checkout does not imply a published binary release; delivery evidence @@ -191,7 +237,8 @@ directory in `sjskills/status/`. 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 -registry metadata do not trigger another fetch; local classification always uses +registry metadata do not trigger another fetch, but source strings pinned to a +new registry commit do; local classification always uses the current scope, registry, files, and provenance. Older cache keys without access are not reused; the first check after upgrading refreshes upstream. Changing access requires new fetch/review evidence but does not change same-repository installed ownership. @@ -270,7 +317,7 @@ apply rejects status envelopes and any `status` field on a plan, including null; only `advisories` and `cliAdvisory` retain the limited semantic-comparison exclusion. `--no-status-check` disables all status discovery, inventory, refresh, and cache -work. Explicit status then prints “Status checks disabled (--no-status-check).” +work, including the registry cache. Explicit status then prints “Status checks disabled (--no-status-check).” or emits `projectConfiguration: "skipped"` with no root, skill advisories, or CLI advisory in JSON. For other commands the flag does not disable primary live verification. Root @@ -289,8 +336,11 @@ go test ./... ``` Run the Go suite on Windows as well as Unix when changing reconciliation. -CLI integration tests build a native fake `bunx` in a temporary directory so -they do not fall through to a real Skills CLI on Windows. Unix permission-bit +CLI integration tests build a native fake `bunx` and fake `git` in a temporary +directory so they do not fall through to a real Skills CLI or network on +Windows. They build the binary with the `sjskillstest` tag, which alone lets it +read a local fixture registry server; release builds never set it. Go tests use +the frozen `internal/sjskills/testdata/registry-v4.json`, not the live registry. Unix permission-bit assertions apply only on Unix; Windows uses native ACLs and synthesized mode bits. All global mutation tests use isolated temporary homes. @@ -307,8 +357,9 @@ mutation arguments are retired. Global apply requires `--approved-plan ` together with `--approved-plan-sha256 `. The command reads the artifact once, verifies -its approved digest and strict successful-global-plan shape, then rematerializes -and recomputes the complete plan. All stable warnings, operations, current and +its approved digest and strict successful-global-plan shape, reloads the +registry at the artifact's recorded commit, then rematerializes and recomputes +the complete plan. All stable warnings, operations, current and expected evidence must match before confirmation or mutation. Apply uses that same still-live verified materialization session, so a remote ref change cannot replace reviewed expected content after the recheck. Missing evidence, artifact diff --git a/internal/sjskills/global_layout.go b/internal/sjskills/global_layout.go index b35ce98..0f8a3f4 100644 --- a/internal/sjskills/global_layout.go +++ b/internal/sjskills/global_layout.go @@ -80,7 +80,7 @@ func LayoutForGlobal(home string) (GlobalLayout, error) { // mutationLayout maps the fixed global boundary onto the shared transaction // engine. The project manifest path is intentionally empty: global intent is -// the embedded registry, while all mutable recovery data stays in the private +// the published registry, while all mutable recovery data stays in the private // derived directory under .agents. func (layout GlobalLayout) mutationLayout() DerivedLayout { return DerivedLayout{ diff --git a/internal/sjskills/status.go b/internal/sjskills/status.go index 5b5db41..eece3c4 100644 --- a/internal/sjskills/status.go +++ b/internal/sjskills/status.go @@ -127,7 +127,7 @@ func summarizeStatus(plan Plan) []AdvisoryFinding { } // StatusScope is resolved afresh before looking up advisory evidence. Its -// identity includes every desired/source/install input and the embedded registry. +// identity includes every desired/source/install input and the loaded registry. type StatusScope struct { Root string Registry Registry diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md index d1fed7b..6d0b1a4 100644 --- a/plans/sjskills-remote-registry.md +++ b/plans/sjskills-remote-registry.md @@ -10,7 +10,7 @@ schema or the CLI itself changes. ## Current state -Proposed on 2026-09-30. Steps 1-4 are complete on `dev`, unreleased. The CLI +Proposed on 2026-09-30. Steps 1-5 are complete on `dev`, unreleased. The CLI reads the published registry through `RegistrySource` (`internal/sjskills/registry_source.go`), pins agent-scripts sources to the registry's commit, records commit and digest evidence, and binds global apply @@ -100,9 +100,13 @@ temporary binary until v1.2.0 shipped it. fixtures.~~ Done 2026-09-30. The fixture is frozen: tests assert its catalog counts, so catalog changes on `main` no longer touch Go tests. The Node test still validates the root registry against the repository's skills. -5. Update `docs/skill-registry.md`, `docs/sjskills-releases.md`, +5. ~~Update `docs/skill-registry.md`, `docs/sjskills-releases.md`, `skills/sjskills/` (including its global-rollout reference), and comments - that describe the embedded registry. + that describe the embedded registry.~~ Done 2026-09-30. + `docs/skill-registry.md` owns the publication and loading contract. The + release consumer check (`scripts/test-release.py`) now seeds the registry + cache, since the installed binary runs offline and embeds no registry; it + passed on windows/amd64. 6. Release a new minor version and install it on each machine under the existing release process. @@ -147,10 +151,9 @@ release step authorizes. ## Next action -Implement step 5: update `docs/skill-registry.md`, `docs/sjskills-releases.md`, -`skills/sjskills/` (including its global-rollout reference), and code comments -that still describe the embedded registry (`internal/sjskills/status.go`, -`internal/sjskills/global_layout.go`). Document the Git prerequisite for -agent-scripts skills, registry evidence and re-review of older plans, stale -registry warnings, and that `--no-status-check` has no offline registry -fallback. +Step 6: merge `dev` into `main` through an integration PR so the native +matrix (macOS Intel, macOS Apple silicon, Windows x64) runs, then release a new +minor version under `docs/sjskills-releases.md` and install it on each machine. +Publishing and per-machine installation need the user's go-ahead. Merging also +publishes the registry the new binary reads, so confirm `skill-registry.json` +on `main` is intended first. diff --git a/scripts/test-release.py b/scripts/test-release.py index 8f2ef8c..7c31f41 100755 --- a/scripts/test-release.py +++ b/scripts/test-release.py @@ -2,6 +2,7 @@ """Exercise built archives through the supported installers, including failures.""" import argparse import importlib.util +import hashlib import json from datetime import datetime, timezone import os @@ -118,6 +119,20 @@ def main(): 'observedAt': datetime.now(timezone.utc).isoformat(), 'retryAt': '0001-01-01T00:00:00Z', })) + # The binary embeds no registry. Seed the published-registry cache as a + # fresh observation so status stays offline and profiles/init fall back + # to it with a stale warning after their refresh attempt fails. + registry_bytes = (release.ROOT / 'skill-registry.json').read_bytes() + registry_commit = '0' * 40 + registry_cache = cache_base / 'sjskills' / 'registry' + registry_cache.mkdir(parents=True) + (registry_cache / f'{registry_commit}.json').write_bytes(registry_bytes) + (registry_cache / 'latest.json').write_text(json.dumps({ + 'schema': 1, 'source': 'sjunepark/agent-scripts', 'ref': 'refs/heads/main', + 'commit': registry_commit, 'sha256': hashlib.sha256(registry_bytes).hexdigest(), + 'resolvedAt': datetime.now(timezone.utc).isoformat(), + 'retryAt': '0001-01-01T00:00:00Z', + })) def cli(*arguments): return subprocess.run([str(binary), *arguments], cwd=consumer, env=env, capture_output=True, text=True, timeout=45) @@ -127,7 +142,9 @@ def cli(*arguments): assert result.returncode == 0 and 'sjskills ' in result.stdout and 'status' in result.stdout, result check_status(cli, consumer, configured=False) result = cli('--json', 'profiles') - assert result.returncode == 0 and json.loads(result.stdout)['result'] == 'success', result + value = json.loads(result.stdout) + assert result.returncode == 0 and value['result'] == 'success', result + assert [w['code'] for w in value['warnings']] == ['registry-stale'], value result = cli('init', 'dev') assert result.returncode == 0, result manifest = consumer / 'sjskills.toml' @@ -136,8 +153,9 @@ def cli(*arguments): assert manifest.read_bytes() == before, 'status changed the project manifest' result = cli('init', 'go') assert result.returncode != 0 and manifest.read_bytes() == before, result + # Plan never uses a cached registry, so it fails offline before Bun. result = cli('--json', 'plan') - assert result.returncode != 0 and 'bunx' in result.stdout, result + assert result.returncode != 0 and 'registry' in result.stdout, result # Reinstallation uses the same supported path. result = invoke(installer, output, destination) assert result.returncode == 0, result.stderr + result.stdout diff --git a/skills/sjskills/SKILL.md b/skills/sjskills/SKILL.md index 5f4fa44..2357b10 100644 --- a/skills/sjskills/SKILL.md +++ b/skills/sjskills/SKILL.md @@ -25,8 +25,9 @@ still blocks reconciliation. Restoring edited bytes does not grant ownership. Keep four states distinct: 1. A source repository's local catalog is editable but not yet published. -2. The registry identifies the published remote content reconciliation can - consume. +2. The registry published on the source repository's `main` identifies the + remote content reconciliation can consume. `sjskills` reads it at run time + and installs that repository's skills from the same commit. 3. A project's committed `sjskills.toml` selects project profiles and direct declarations. 4. The registry defines one fixed, machine-independent global baseline. @@ -123,13 +124,14 @@ independent unblocked scope unless the user required an all-or-nothing result. remain. Retain every reported quarantine identifier through a normal work cycle. -When intended content changed in a registry-backed source, reconciliation can -only consume the published remote content. Do not apply a local-only change. +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`. If publication is requested, follow the source repository's instructions for validation, review, commit, and publication; those mechanics are outside this -skill. Before reconciling, resolve the registry source and verify that its -pinned remote ref contains each intended skill tree. A local or feature-branch -commit is insufficient when the registry points elsewhere. +skill. Before reconciling, verify that the commit named by the plan's +`registry` evidence contains each intended registry entry and skill tree. A +local or feature-branch commit is insufficient. ## Restore diff --git a/skills/sjskills/references/global-rollout.md b/skills/sjskills/references/global-rollout.md index 640fc67..8e2d3e5 100644 --- a/skills/sjskills/references/global-rollout.md +++ b/skills/sjskills/references/global-rollout.md @@ -38,8 +38,8 @@ Create a complete JSON plan: ``` Compute SHA-256 values for the executable and `plan.json`. Review every -operation, warning, current-state fact, expected-content hash, and -materialization result, including quarantine of undeclared skills regardless of +operation, warning, current-state fact, the `registry` commit and digest, +expected-content hash, and materialization result, including quarantine of undeclared skills regardless of ownership or local edits and `local-modification` updates that quarantine edited managed desired copies before replacement. Stop on blocked placements, untrusted provenance, unmanaged desired copies, source mismatches, unsafe filesystem boundaries, @@ -50,8 +50,10 @@ would otherwise quarantine those entries. ## Apply and verify Before mutation, verify the executable and artifact still match the reviewed -hashes. Apply uses the complete reviewed artifact and recomputes the plan before -mutation; `--yes` only skips the redundant interactive prompt: +hashes. Apply uses the complete reviewed artifact, reloads the registry at its +recorded commit even if `main` has moved, and recomputes the plan before +mutation; `--yes` only skips the redundant interactive prompt. A plan without +`registry` commit evidence, from an older release, must be recreated: ```text apply --global --yes \ diff --git a/skills/sjskills/references/status.md b/skills/sjskills/references/status.md index a48b51f..983bdd9 100644 --- a/skills/sjskills/references/status.md +++ b/skills/sjskills/references/status.md @@ -17,6 +17,9 @@ or problems remain silent. `cliAdvisory`; status also includes setup metadata under `status`. None grants apply authority or substitutes for a reviewed plan. Use the same compatible executable for plan and apply; older loaders may reject the new advisory field. -Use `--no-status-check` to skip discovery, inspection, fetching, and cache writes; -status then reports checks disabled, while other commands retain their primary -verification. +A `registry-stale` warning means the published registry could not be refreshed +and a cached one, named by commit and age, was used; treat its findings as +possibly outdated. Use `--no-status-check` to skip discovery, inspection, +fetching, and cache writes; status then reports checks disabled, while other +commands retain their primary verification but lose the cached-registry +fallback when offline.