diff --git a/ROADMAP.md b/ROADMAP.md index f0aff58..46e215a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -9,9 +9,10 @@ ## 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. +1. [Read the skill registry from the published source](plans/sjskills-remote-registry.md) + is proposed so registry changes stop requiring an sjskills release. + Implementation and docs are complete on `dev`; the integration PR and + release remain. ## Tasks @@ -19,6 +20,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/cmd/sjskills/default_status_test.go b/cmd/sjskills/default_status_test.go index dd1463f..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) @@ -405,3 +413,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, + registries: registryFunc(func() (sjskills.PublishedRegistry, error) { + loads++ + return fixtureRegistry() + }), + } + 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..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,7 +93,10 @@ func (c *restoreCommand) Run(ctx *commandContext) error { } type application struct { - loadRegistry func() (sjskills.Registry, error) + registries registryLoader + registryLoaded bool + registryValue sjskills.PublishedRegistry + registryErr error noStatusCheck bool statusSnapshot *commandStatusSnapshot cliStatusService *sjskills.CLIStatusService @@ -144,11 +147,69 @@ func productionMaterialize(ctx context.Context, skills []sjskills.DesiredSkill) return sjskills.NewMaterializer(sjskills.MaterializerConfig{}).Materialize(ctx, skills) } -func (a *application) registry() (sjskills.Registry, error) { - if a.loadRegistry != nil { - return a.loadRegistry() +// 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. +// 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 { + registries := a.registries + if registries == nil { + registries = defaultRegistries + } + if registries == nil { + a.registryErr = errors.New("skill registry source is not configured") + } else { + if ctx == nil { + ctx = context.Background() + } + a.registryValue, a.registryErr = load(registries, ctx) + } + a.registryLoaded = true } - return sjskills.EmbeddedRegistry() + 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 { @@ -181,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) @@ -195,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"}) } @@ -247,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 @@ -266,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 { @@ -301,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 @@ -448,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"}) @@ -468,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 } @@ -656,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", @@ -1228,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..a747c6a 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) } @@ -513,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) } @@ -613,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) } @@ -1813,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) { @@ -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..e351af3 --- /dev/null +++ b/cmd/sjskills/registry_fixture_test.go @@ -0,0 +1,75 @@ +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/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. +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) } + +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) { + 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/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/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/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/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/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.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/registry_source.go b/internal/sjskills/registry_source.go new file mode 100644 index 0000000..58becd6 --- /dev/null +++ b/internal/sjskills/registry_source.go @@ -0,0 +1,497 @@ +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 + "/" + registryTreeMain = "https://github.com/" + registryRepository + "/tree/main" + + maxRegistryBytes = 1 << 20 + maxRefAdvertisement = 4 << 20 + registryCacheSchema = 1 + registryLatestFile = "latest.json" + registryLatestLockFile = "latest.lock" +) + +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 +// 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 + // 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 + 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 != "" } + +// 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() + } + 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 +} + +// 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) { + 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 + } + 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 + } + 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 { + 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, _ = registryEndpoints() + } + 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 = 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 +} + +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, "", httpStatusError{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 +} + +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) { + 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) { + if s.NoCache { + return "", errors.New("registry cache disabled") + } + 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..584750a --- /dev/null +++ b/internal/sjskills/registry_source_test.go @@ -0,0 +1,365 @@ +package sjskills + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "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) + } + // 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() != 2 { + 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) + } + // 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/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/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-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 diff --git a/plans/sjskills-remote-registry.md b/plans/sjskills-remote-registry.md new file mode 100644 index 0000000..6d0b1a4 --- /dev/null +++ b/plans/sjskills-remote-registry.md @@ -0,0 +1,159 @@ +# 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. 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 +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 +`.../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 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 + 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 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.~~ 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.~~ + 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.~~ 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.~~ 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. + +## 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 may use `git ls-remote` +or the equivalent smart-HTTP ref advertisement; step 2 chose the latter. + +## 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 + +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/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 }), []); }); 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} 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.