diff --git a/PROGRESS.md b/PROGRESS.md index 804944a..5c4751c 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -1,3 +1,28 @@ +# Authenticated private-source delivery + +PR #23 review follow-up: confirmed comments 3979633085 (commit pins), 3979633091 (unsupported GitHub page URLs), and 3979633101 (manual/workflow validation). Fixed with regression coverage; PR checks own follow-up native validation and the merge gate. + +- PR #23 combines local skill-authoring work, origin main, and the PR #22 + recovery history. Public `kicpa` membership is restored; recovered private + registrations now belong to `kicpa-private`. +- Access validation/resolution, controlled Git/gh staging, evidence separation, + and compatible public-plan loading are implemented. The + [private-source plan](plans/sjskills-private-github-sources.md) owns the design + and acceptance evidence. Original recovery stash remains + `038118178cce4759e3acebf854f19b36723488fc`. +- Full Go/race suites, vet, Node tests, release tests, and skill validation + passed on macOS. Bounded independent review found no implementation defect; + its requested native auth cancellation coverage and token-login acceptance + were added and passed. Operator documentation and the plugin bundle agree. +- Implementation `84ba2e7` passed source checks and hosted native acceptance on + macOS Intel/ARM and Windows in + [run 34482396368](https://github.com/sjunepark/agent-scripts/actions/runs/34482396368). + [PR #23](https://github.com/sjunepark/agent-scripts/pull/23) owns the final merge + status and final check results. CodeRabbit skipped its + requested review because automatic reviews are disabled. +- No new release, actual private-skill installation, or real-machine + reconciliation is included. + # sjskills maintenance plugin delivery - User authorized automatic CLI updates and configured skill sync on every diff --git a/README.md b/README.md index ab66ca3..4a085d3 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,14 @@ sjskills apply ``` `sjskills.toml` may combine named profiles with direct third-party -declarations. In a project that adopts this ownership model, ignore +declarations. `sjskills profiles` reports their access requirements. `kicpa` +stays public; add `kicpa-private` for the enrolled private catalog. A direct +entry can set `access = "github-authenticated"` to use an existing gh login +for a GitHub.com source. See the [access contract](docs/skill-registry.md#skill-and-source-records) +for dependencies, supported configuration, and failure handling. Use a build +that supports these fields; this source change does not publish a new release. + +In a project that adopts this ownership model, ignore `.sjskills/`, `.agents/skills/`, and `.claude/skills/`; do not add those patterns until any preexisting committed content has been reviewed and migrated. Review every plan before apply. Sync removes undeclared skills from diff --git a/cmd/sjskills/github_auth_test.go b/cmd/sjskills/github_auth_test.go new file mode 100644 index 0000000..e05d896 --- /dev/null +++ b/cmd/sjskills/github_auth_test.go @@ -0,0 +1,374 @@ +package main + +import ( + "context" + "encoding/json" + "os" + "os/exec" + "path/filepath" + "reflect" + "runtime" + "strings" + "testing" + "time" + + "github.com/sjunepark/agent-scripts/internal/sjskills" +) + +func githubFixtureGitEnvironment(root string) []string { + env := []string{} + for _, entry := range os.Environ() { + key, _, _ := strings.Cut(entry, "=") + if !strings.HasPrefix(strings.ToUpper(key), "GIT_") { + env = append(env, entry) + } + } + return append(env, "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL="+filepath.Join(root, "no-gitconfig"), "GIT_AUTHOR_NAME=Fixture", "GIT_AUTHOR_EMAIL=fixture@example.invalid", "GIT_COMMITTER_NAME=Fixture", "GIT_COMMITTER_EMAIL=fixture@example.invalid") +} + +func githubFixture(t *testing.T) (string, map[string]string) { + t.Helper() + root := t.TempDir() + project := filepath.Join(root, "project") + config := filepath.Join(root, "original-gh") + bin := filepath.Join(root, "user's 한글 tools") + for _, d := range []string{project, config, bin} { + if e := os.MkdirAll(d, 0700); e != nil { + t.Fatal(e) + } + } + if e := os.WriteFile(filepath.Join(config, "config.yml"), []byte("version: 1\n"), 0600); e != nil { + t.Fatal(e) + } + suffix := "" + if runtime.GOOS == "windows" { + suffix = ".exe" + } + data, e := os.ReadFile(filepath.Join(filepath.Dir(testBinary), "fakegithub"+suffix)) + if e != nil { + t.Fatal(e) + } + for _, name := range []string{"git", "gh"} { + if e := os.WriteFile(filepath.Join(bin, name+suffix), data, 0700); e != nil { + t.Fatal(e) + } + } + realGit, e := exec.LookPath("git") + if e != nil { + t.Fatal(e) + } + repo := filepath.Join(root, "repo") + command := func(args ...string) { + t.Helper() + c := exec.Command(realGit, args...) + c.Env = githubFixtureGitEnvironment(root) + if out, e := c.CombinedOutput(); e != nil { + t.Fatalf("fixture git: %v %s", e, out) + } + } + command("init", repo) + if e := os.MkdirAll(filepath.Join(repo, "skills", "private-fixture"), 0700); e != nil { + t.Fatal(e) + } + if e := os.WriteFile(filepath.Join(repo, "skills", "private-fixture", "SKILL.md"), []byte("---\nname: private-fixture\ndescription: Private fixture\n---\nFixture\n"), 0600); e != nil { + t.Fatal(e) + } + command("-C", repo, "add", ".") + command("-C", repo, "-c", "core.hooksPath="+filepath.Join(root, "no-hooks"), "commit", "-m", "fixture") + env := isolatedExternalHomes(t) + env["PATH"] = bin + string(os.PathListSeparator) + filepath.Dir(testBinary) + string(os.PathListSeparator) + os.Getenv("PATH") + env["GH_CONFIG_DIR"] = config + env["GH_TOKEN"] = "" + env["GITHUB_TOKEN"] = "" + env["SJSKILLS_EXPECT_GH_CONFIG"] = config + env["SJSKILLS_ORIGINAL_HOME"] = env["HOME"] + env["SJSKILLS_GIT_FIXTURE"] = repo + env["SJSKILLS_REAL_GIT"] = realGit + env["SJSKILLS_FAKE_GITHUB_LOG"] = filepath.Join(root, "auth.log") + env["GIT_TRACE"] = "1" + env["GH_DEBUG"] = "api" + env["GIT_CONFIG_COUNT"] = "1" + env["GIT_CONFIG_KEY_0"] = "credential.helper" + env["GIT_CONFIG_VALUE_0"] = "untrusted-helper" + return project, env +} + +func writeAuthenticatedManifest(t *testing.T, project string, private bool) { + t.Helper() + text := "version=1\nprofiles=[]\n" + if private { + text += "\n[[direct]]\nname=\"private-fixture\"\nsource=\"fixture/private/skills\"\naccess=\"github-authenticated\"\n" + } + text += "\n[[direct]]\nname=\"public-fixture\"\nsource=\"fixture/public\"\n" + if e := os.WriteFile(filepath.Join(project, "sjskills.toml"), []byte(text), 0600); e != nil { + t.Fatal(e) + } +} + +func TestExternalAuthenticatedLifecycleAndCredentialBoundary(t *testing.T) { + project, env := githubFixture(t) + writeAuthenticatedManifest(t, project, true) + before := captureFixtureTree(t, env["GH_CONFIG_DIR"]) + run := func(args ...string) sjskills.Envelope { + t.Helper() + code, out, stderr := runCLIWithEnvironment(t, project, env, append([]string{"--json"}, args...)...) + if code != 0 || stderr != "" { + t.Fatalf("%v: code=%d out=%s err=%s", args, code, out, stderr) + } + if strings.Contains(out, "sjskills-sentinel-credential") || strings.Contains(out, "/github-") || strings.Contains(out, `\\github-`) { + t.Fatal("credential/staging leaked") + } + var e sjskills.Envelope + if err := json.Unmarshal([]byte(out), &e); err != nil { + t.Fatal(err) + } + return e + } + plan := run("plan") + if plan.Plan.Desired.Skills[0].Access != sjskills.AccessGitHubAuthenticated || plan.Plan.Desired.Skills[0].Source != "fixture/private/skills" { + t.Fatal(plan) + } + run("apply", "--yes") + for _, target := range []string{".agents", ".claude"} { + if _, e := os.Stat(filepath.Join(project, target, "skills", "private-fixture", "SKILL.md")); e != nil { + t.Fatal(e) + } + } + next := run("apply", "--yes") + for _, op := range next.Plan.Operations { + if op.Action != sjskills.PlanActionUnchanged { + t.Fatal(op) + } + } + env["SJSKILLS_FAKE_CONTENT"] = " updated" + privateFile := filepath.Join(env["SJSKILLS_GIT_FIXTURE"], "skills", "private-fixture", "SKILL.md") + if err := os.WriteFile(privateFile, []byte("# private fixture upstream update\n"), 0600); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "."}, {"-c", "user.name=Fixture", "-c", "user.email=fixture@example.invalid", "-c", "core.hooksPath=" + t.TempDir(), "commit", "-m", "upstream update"}} { + cmd := exec.Command(env["SJSKILLS_REAL_GIT"], append([]string{"-C", env["SJSKILLS_GIT_FIXTURE"]}, args...)...) + cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL="+filepath.Join(t.TempDir(), "empty")) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("update fixture: %v %s", err, out) + } + } + run("apply", "--yes") + installed, err := os.ReadFile(filepath.Join(project, ".agents", "skills", "private-fixture", "SKILL.md")) + if err != nil || string(installed) != "# private fixture upstream update\n" { + t.Fatalf("upstream update missing: %s %v", installed, err) + } + // Access-only changes retain remote ownership; an exact tree remains managed. + manifest, e := os.ReadFile(filepath.Join(project, "sjskills.toml")) + if e != nil { + t.Fatal(e) + } + if e := os.WriteFile(filepath.Join(project, "sjskills.toml"), []byte(strings.ReplaceAll(string(manifest), "github-authenticated", "public")), 0600); e != nil { + t.Fatal(e) + } + public := run("plan") + for _, op := range public.Plan.Operations { + if op.Action != sjskills.PlanActionUnchanged { + t.Fatal(op) + } + } + writeAuthenticatedManifest(t, project, false) + removed := run("apply", "--yes") + id := "" + for _, e := range removed.Evidence { + if e.Kind == "quarantine" { + id = strings.TrimSuffix(strings.TrimPrefix(e.Detail, "id="), " status=committed") + } + } + if id == "" { + t.Fatalf("no quarantine: %+v", removed.Evidence) + } + run("restore", id, "--yes") + if !reflect.DeepEqual(before, captureFixtureTree(t, env["GH_CONFIG_DIR"])) { + t.Fatal("gh configuration modified") + } + log, e := os.ReadFile(env["SJSKILLS_FAKE_GITHUB_LOG"]) + if e != nil { + t.Fatal(e) + } + if strings.Count(string(log), "git\n") != strings.Count(string(log), "gh\n") { + t.Fatalf("wrong-host/path invoked gh: %s", log) + } +} + +func TestExternalAuthenticationFailurePreservesInstallations(t *testing.T) { + for _, mode := range []string{"SJSKILLS_FAKE_AUTH_FAIL", "SJSKILLS_FAKE_AUTH_OVERSIZE"} { + t.Run(mode, func(t *testing.T) { + project, env := githubFixture(t) + writeAuthenticatedManifest(t, project, true) + code, out, stderr := runCLIWithEnvironment(t, project, env, "--json", "apply", "--yes") + if code != 0 { + t.Fatalf("initial %d %s %s", code, out, stderr) + } + before := captureFixtureTree(t, project) + env[mode] = "1" + code, out, stderr = runCLIWithEnvironment(t, project, env, "--json", "apply", "--yes") + if code == 0 || strings.Contains(out+stderr, "sjskills-sentinel-credential") || !strings.Contains(out, "verify gh login") { + t.Fatalf("failure %d %s %s", code, out, stderr) + } + if !reflect.DeepEqual(before, captureFixtureTree(t, project)) { + t.Fatal("failed auth changed managed project") + } + }) + } +} + +func TestExternalPublicSelectionDoesNotUseAuthentication(t *testing.T) { + project, env := githubFixture(t) + writeAuthenticatedManifest(t, project, false) + env["SJSKILLS_FAKE_AUTH_FAIL"] = "1" + code, out, stderr := runCLIWithEnvironment(t, project, env, "--json", "plan") + if code != 0 { + t.Fatalf("public %d %s %s", code, out, stderr) + } + if _, err := os.Stat(env["SJSKILLS_FAKE_GITHUB_LOG"]); !os.IsNotExist(err) { + t.Fatal("public invoked auth tools") + } +} + +func TestAuthenticatedManifestRenderingAndProfiles(t *testing.T) { + m := sjskills.Manifest{Version: 1, Direct: []sjskills.DirectSkill{{Name: "fixture", Source: "fixture/private", Access: sjskills.AccessGitHubAuthenticated}}} + parsed, err := sjskills.ParseManifest([]byte(renderManifest(m))) + if err != nil || parsed.Direct[0].Access != sjskills.AccessGitHubAuthenticated { + t.Fatalf("render %v %+v", err, parsed) + } + code, out, stderr := runCLI(t, t.TempDir(), "profiles") + if code != 0 || !strings.Contains(out, "kicpa-private (7 skills, github-authenticated)") { + t.Fatalf("profiles %d %s %s", code, out, stderr) + } +} + +func TestNativeAuthenticatedDescendantsStopBeforeCleanup(t *testing.T) { + for _, mode := range []string{"cancel", "timeout"} { + t.Run(mode, func(t *testing.T) { + _, env := githubFixture(t) + env["SJSKILLS_FAKE_AUTH_BLOCK"] = "1" + env["SJSKILLS_FAKE_AUTH_HEARTBEAT"] = filepath.Join(t.TempDir(), "heartbeat") + t.Setenv("PATH", env["PATH"]) + base := append([]string{}, os.Environ()...) + for key, value := range env { + setEnvironmentValue(&base, key, value) + } + stages := t.TempDir() + materializer := sjskills.NewMaterializer(sjskills.MaterializerConfig{HelperExecutable: testBinary, BaseEnvironment: base, TempRootFactory: func() (string, error) { return os.MkdirTemp(stages, "stage-") }, Limits: sjskills.MaterializerLimits{CommandTimeout: 3 * time.Second}}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := make(chan error, 1) + go func() { + plan, err := materializer.Materialize(ctx, []sjskills.DesiredSkill{{Name: "private-fixture", Source: "fixture/private/skills", Manager: sjskills.ManagerSkillsCLI, Mode: sjskills.ModeCopy, Access: sjskills.AccessGitHubAuthenticated}}) + if plan != nil { + plan.Cleanup() + } + done <- err + }() + deadline := time.Now().Add(5 * time.Second) + for { + if _, err := os.Stat(env["SJSKILLS_FAKE_AUTH_HEARTBEAT"]); err == nil { + break + } + if time.Now().After(deadline) { + cancel() + t.Fatal("gh descendant never reached blocked fixture") + } + time.Sleep(20 * time.Millisecond) + } + if mode == "cancel" { + cancel() + } + select { + case err := <-done: + if err == nil { + t.Fatal("blocked authentication succeeded") + } + case <-time.After(6 * time.Second): + cancel() + t.Fatal("authentication did not stop") + } + before, err := os.ReadFile(env["SJSKILLS_FAKE_AUTH_HEARTBEAT"]) + if err != nil { + t.Fatal(err) + } + time.Sleep(100 * time.Millisecond) + after, _ := os.ReadFile(env["SJSKILLS_FAKE_AUTH_HEARTBEAT"]) + if string(before) != string(after) { + t.Fatal("gh descendant remained active after materializer returned") + } + entries, err := os.ReadDir(stages) + if err != nil || len(entries) != 0 { + t.Fatalf("staging cleanup %v %v", entries, err) + } + }) + } +} + +func TestExternalTokenAuthenticationNeedsNoOriginalConfig(t *testing.T) { + for _, name := range []string{"GH_TOKEN", "GITHUB_TOKEN"} { + t.Run(name, func(t *testing.T) { + project, env := githubFixture(t) + writeAuthenticatedManifest(t, project, true) + if err := os.WriteFile(filepath.Join(env["GH_CONFIG_DIR"], "config.yml"), []byte("git_protocol: https\n"), 0600); err != nil { + t.Fatal(err) + } + before := captureFixtureTree(t, env["GH_CONFIG_DIR"]) + env[name] = "sjskills-sentinel-credential-never-log" + env["SJSKILLS_EXPECT_TOKEN"] = "1" + code, out, stderr := runCLIWithEnvironment(t, project, env, "--json", "apply", "--yes") + if code != 0 || strings.Contains(out+stderr, env[name]) { + t.Fatalf("token auth %d %s %s", code, out, stderr) + } + for _, content := range captureFixtureTree(t, project).Files { + if strings.Contains(string(content), env[name]) { + t.Fatal("token persisted in project") + } + } + if !reflect.DeepEqual(before, captureFixtureTree(t, env["GH_CONFIG_DIR"])) { + t.Fatal("token login migrated original configuration") + } + }) + } +} + +func TestExternalAuthenticatedCommitPin(t *testing.T) { + project, env := githubFixture(t) + repo := env["SJSKILLS_GIT_FIXTURE"] + c := exec.Command(env["SJSKILLS_REAL_GIT"], "-C", repo, "rev-parse", "HEAD") + c.Env = githubFixtureGitEnvironment(filepath.Dir(repo)) + pin, err := c.Output() + if err != nil { + t.Fatal(err) + } + original, err := os.ReadFile(filepath.Join(repo, "skills", "private-fixture", "SKILL.md")) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "skills", "private-fixture", "SKILL.md"), []byte("newer unpinned content"), 0600); err != nil { + t.Fatal(err) + } + c = exec.Command(env["SJSKILLS_REAL_GIT"], "-C", repo, "-c", "user.name=Fixture", "-c", "user.email=fixture@example.invalid", "-c", "core.hooksPath="+filepath.Join(repo, "no-hooks"), "commit", "-am", "newer commit") + c.Env = githubFixtureGitEnvironment(filepath.Dir(repo)) + if out, err := c.CombinedOutput(); err != nil { + t.Fatalf("%v: %s", err, out) + } + writeAuthenticatedManifest(t, project, true) + manifest := filepath.Join(project, "sjskills.toml") + data, err := os.ReadFile(manifest) + if err != nil { + t.Fatal(err) + } + source := "https://github.com/fixture/private/tree/" + strings.TrimSpace(string(pin)) + "/skills" + if err := os.WriteFile(manifest, []byte(strings.ReplaceAll(string(data), "fixture/private/skills", source)), 0600); err != nil { + t.Fatal(err) + } + code, out, stderr := runCLIWithEnvironment(t, project, env, "--json", "apply", "--yes") + if code != 0 || stderr != "" { + t.Fatalf("code=%d out=%s stderr=%s", code, out, stderr) + } + installed, err := os.ReadFile(filepath.Join(project, ".agents", "skills", "private-fixture", "SKILL.md")) + if err != nil || string(installed) != string(original) { + t.Fatalf("pin mismatch: %q err=%v", installed, err) + } +} diff --git a/cmd/sjskills/main.go b/cmd/sjskills/main.go index acce1d9..c35c819 100644 --- a/cmd/sjskills/main.go +++ b/cmd/sjskills/main.go @@ -193,7 +193,7 @@ func (a *application) profiles() sjskills.Envelope { } sort.Strings(profiles) for _, name := range profiles { - envelope.Profiles = append(envelope.Profiles, sjskills.ProfileInfo{Name: name, Count: len(registry.Profiles[name].Skills)}) + 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"}) return envelope @@ -1044,6 +1044,9 @@ func renderManifest(manifest sjskills.Manifest) string { for _, direct := range manifest.Direct { builder.WriteString("\n[[direct]]\n") fmt.Fprintf(&builder, "name = %q\nsource = %q\n", direct.Name, direct.Source) + if direct.Access.Effective() == sjskills.AccessGitHubAuthenticated { + fmt.Fprintf(&builder, "access = %q\n", direct.Access) + } if direct.FullDepth { builder.WriteString("full_depth = true\n") } @@ -1110,7 +1113,7 @@ func renderHuman(stdout, stderr io.Writer, envelope sjskills.Envelope) { } case sjskills.CommandOperationProfiles: for _, profile := range envelope.Profiles { - fmt.Fprintf(stdout, "%s (%d skills)\n", profile.Name, profile.Count) + fmt.Fprintf(stdout, "%s (%d skills, %s)\n", profile.Name, profile.Count, profile.Access.Effective()) } case sjskills.CommandOperationPlan, sjskills.CommandOperationApply: if envelope.Plan != nil { @@ -1267,6 +1270,12 @@ func isExactVersionRequest(args []string) bool { } func main() { + if len(os.Args) > 1 && os.Args[1] == sjskills.GitHubCredentialCommand { + if err := sjskills.RunGitHubCredential(context.Background(), os.Args[2:], os.Stdin, os.Stdout, os.Environ()); err != nil { + os.Exit(1) + } + return + } directory, err := os.Getwd() if err != nil { fmt.Fprintf(os.Stderr, "sjskills: get working directory: %v\n", err) diff --git a/cmd/sjskills/main_test.go b/cmd/sjskills/main_test.go index 7f9bb91..4a16bee 100644 --- a/cmd/sjskills/main_test.go +++ b/cmd/sjskills/main_test.go @@ -49,6 +49,15 @@ func TestMain(m *testing.M) { if err := buildFake.Run(); err != nil { panic(fmt.Sprintf("build fake bunx: %v", err)) } + fakeGitHub := filepath.Join(directory, "fakegithub") + if runtime.GOOS == "windows" { + fakeGitHub += ".exe" + } + buildGitHub := exec.Command("go", "build", "-o", fakeGitHub, "./testdata/fakegithub") + buildGitHub.Stdout, buildGitHub.Stderr = os.Stdout, os.Stderr + if err := buildGitHub.Run(); err != nil { + panic(fmt.Sprintf("build fake github tools: %v", err)) + } code := m.Run() _ = os.RemoveAll(directory) os.Exit(code) diff --git a/cmd/sjskills/testdata/fakebunx/main.go b/cmd/sjskills/testdata/fakebunx/main.go index 3f3e690..894f86e 100644 --- a/cmd/sjskills/testdata/fakebunx/main.go +++ b/cmd/sjskills/testdata/fakebunx/main.go @@ -62,6 +62,17 @@ func main() { panic(err) } content := fmt.Sprintf("# %s%s\n", skill, os.Getenv("SJSKILLS_FAKE_CONTENT")) + local := args[2] + if local == "fixture/private/skills" && os.Getenv("SJSKILLS_GIT_FIXTURE") != "" { + local = filepath.Join(os.Getenv("SJSKILLS_GIT_FIXTURE"), "skills") + } + if filepath.IsAbs(local) { + data, err := os.ReadFile(filepath.Join(local, skill, "SKILL.md")) + if err != nil { + os.Exit(7) + } + content = string(data) + } if err := os.WriteFile(filepath.Join(target, "SKILL.md"), []byte(content), 0o644); err != nil { panic(err) } diff --git a/cmd/sjskills/testdata/fakegithub/main.go b/cmd/sjskills/testdata/fakegithub/main.go new file mode 100644 index 0000000..f74516c --- /dev/null +++ b/cmd/sjskills/testdata/fakegithub/main.go @@ -0,0 +1,131 @@ +// Controlled native Git/gh boundary for authentication acceptance tests. +// Unknown commands fail; this fixture never forwards network operations. +package main + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "time" +) + +const token = "sjskills-sentinel-credential-never-log" + +func fail() { os.Exit(7) } +func main() { + name := strings.TrimSuffix(filepath.Base(os.Args[0]), ".exe") + if os.Getenv("SJSKILLS_FAKE_GITHUB_LOG") != "" { + f, e := os.OpenFile(os.Getenv("SJSKILLS_FAKE_GITHUB_LOG"), os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) + if e != nil { + fail() + } + fmt.Fprintln(f, name) + f.Close() + } + switch name { + case "gh": + if strings.Join(os.Args[1:], " ") != "auth git-credential get" { + fail() + } + if os.Getenv("SJSKILLS_EXPECT_TOKEN") == "1" { + if os.Getenv("GH_TOKEN") != token || os.Getenv("GH_CONFIG_DIR") != filepath.Join(os.Getenv("HOME"), ".gh-auth") { + fail() + } + } else if os.Getenv("GH_CONFIG_DIR") != os.Getenv("SJSKILLS_EXPECT_GH_CONFIG") { + fail() + } + if os.Getenv("HOME") == os.Getenv("SJSKILLS_ORIGINAL_HOME") { + fail() + } + if os.Getenv("GH_DEBUG") != "" || os.Getenv("GIT_TRACE") != "" { + fail() + } + if os.Getenv("SJSKILLS_FAKE_AUTH_BLOCK") == "1" { + deadline := time.Now().Add(8 * time.Second) + for time.Now().Before(deadline) { + _ = os.WriteFile(os.Getenv("SJSKILLS_FAKE_AUTH_HEARTBEAT"), []byte(fmt.Sprint(time.Now().UnixNano())), 0600) + time.Sleep(20 * time.Millisecond) + } + fail() + } + fmt.Fprintln(os.Stderr, token) // Must never reach any diagnostic channel. + if os.Getenv("SJSKILLS_FAKE_AUTH_FAIL") == "1" { + fail() + } + if os.Getenv("SJSKILLS_FAKE_AUTH_OVERSIZE") == "1" { + fmt.Print(strings.Repeat("x", 128*1024)) + return + } + fmt.Printf("protocol=https\nhost=github.com\nusername=fixture\npassword=%s\n\n", token) + case "git": + args := os.Args[1:] + index := -1 + for i, a := range args { + if a == "clone" || a == "init" || a == "fetch" || a == "checkout" { + index = i + break + } + } + if index < 0 || len(args) < 2 || os.Getenv("GIT_ALLOW_PROTOCOL") != "https" || os.Getenv("GIT_CONFIG_COUNT") != "" || os.Getenv("GIT_TRACE") != "" { + fail() + } + 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) { + fail() + } + } + operation := args[index] + realGit := os.Getenv("SJSKILLS_REAL_GIT") + if !filepath.IsAbs(realGit) { + fail() + } + if operation == "init" || operation == "checkout" { + cmd := exec.Command(realGit, args...) + cmd.Env = os.Environ() + if err := cmd.Run(); err != nil { + fail() + } + return + } + remote := args[len(args)-2] + if remote != "https://github.com/fixture/private.git" { + fail() + } + runCredential := func(host, path string) ([]byte, error) { + ca := append(append([]string{}, args[:index]...), "credential", "fill") + cmd := exec.Command(realGit, ca...) + cmd.Env = os.Environ() + cmd.Stdin = strings.NewReader("protocol=https\nhost=" + host + "\npath=" + path + "\n\n") + return cmd.Output() + } + for _, request := range [][2]string{{"evil.invalid", "fixture/private.git"}, {"github.com", "fixture/other.git"}} { + out, err := runCredential(request[0], request[1]) + if err == nil || strings.Contains(string(out), token) { + fail() + } + } + out, err := runCredential("github.com", "fixture/private.git") + if err != nil || !strings.Contains(string(out), "password="+token) { + fmt.Fprintln(os.Stderr, token) + fail() + } + cloneArgs := append([]string{}, args...) + cloneArgs[len(cloneArgs)-2] = os.Getenv("SJSKILLS_GIT_FIXTURE") + cmd := exec.Command(realGit, cloneArgs...) + env := []string{} + for _, e := range os.Environ() { + if !strings.HasPrefix(e, "GIT_ALLOW_PROTOCOL=") { + env = append(env, e) + } + } + cmd.Env = append(env, "GIT_ALLOW_PROTOCOL=file") + if err := cmd.Run(); err != nil { + fail() + } + default: + fail() + } +} diff --git a/docs/settings-sync.md b/docs/settings-sync.md index b2922dc..345e92f 100644 --- a/docs/settings-sync.md +++ b/docs/settings-sync.md @@ -82,7 +82,9 @@ Repo-managed plugin source includes: - `sjskills-maintenance` requests published-main plugin updates, stable CLI updates, and reviewed skill synchronization at session startup. Its [management guide](sjskills-startup-hook.md) owns installation and update - behavior; [PROGRESS.md](../PROGRESS.md) tracks pending publication and rollout. + behavior; [PROGRESS.md](../PROGRESS.md) tracks publication and rollout. Its + bundled sjskills procedure preserves explicit public/authenticated selections + and stops a failing private scope without starting authentication setup. For `chezmoi-sync`: diff --git a/docs/skill-registry.md b/docs/skill-registry.md index 8d2b6bf..acf9e5c 100644 --- a/docs/skill-registry.md +++ b/docs/skill-registry.md @@ -12,9 +12,18 @@ selection resolved by `sjskills plan --global`; global machine profiles and hostname inference do not exist. `profiles` contains composable project selections such as `dev`, `go`, -`rust`, and `kicpa`. A project's committed `sjskills.toml` selects those -profiles and may add direct third-party declarations. Profile membership does -not make a skill global. +`rust`, `kicpa`, and `kicpa-private`. A project's committed `sjskills.toml` +selects those profiles and may add direct third-party declarations. Profile +membership does not make a skill global. Required profiles remain public; +additional profiles have portable names, nonempty members, and the same +selection and collision rules. `sjskills profiles` reports each access policy. + +Each profile has `access: public` or `access: github-authenticated`; omission +means public. `kicpa` retains its public members, while `kicpa-private` selects +the enrolled private catalog. Select both to combine them. A direct declaration +can use `access = "github-authenticated"` with the same policy. Unknown, empty, +and malformed access values are rejected. Access is a fetching requirement, +not a visibility check; private-source names and URLs in the registry are public. Every selected skill name must be unique across the global baseline, selected project profiles, and direct project declarations. Duplicate names, @@ -40,11 +49,54 @@ Each `skills` entry records: A `repository` source names the published GitHub `skills/` catalog. An `external` source names a deliberately tracked upstream or a manual -boundary. Skills CLI-managed sources must be public Git shorthand +boundary. Skills CLI-managed sources must be Git shorthand (`owner/repo[/path]`) or credential-free HTTPS. Local paths, embedded 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, +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 +commit hash. Other GitHub page URLs (such as `/blob/` or `/issues/`), hosts, +ports, escaped paths, and embedded credentials are rejected before fetching. +Manual and workflow-managed entries do not fetch or validate a GitHub source. + +Authenticated fetching uses bounded HTTPS Git operations in private staging, +then the pinned Skills CLI's local-source discovery. Named refs use shallow +clone; full commit pins use shallow fetch and detached checkout, failing if the +server cannot supply that commit. Redirects, alternative +protocols, submodules, inherited Git credential helpers/configuration, and +tracing are disabled. The internal credential helper permits only the selected +GitHub repository and forwards a bounded credential response directly to Git's +private pipe. No credential or local staging path becomes installed source +identity, reviewed evidence, or diagnostic output. Git/gh descendants share the +existing process containment and cleanup boundary. + +The original gh configuration is located through `GH_CONFIG_DIR`, +`XDG_CONFIG_HOME/gh`, Windows `APPDATA/GitHub CLI`, or `HOME/.config/gh` in that +order. Configuration version 1 is required: gh can migrate older configuration +even during credential lookup, so sjskills refuses it before invoking gh. +Complete login or migration yourself with `gh auth status` (or `gh auth login` +when needed), then retry. Existing `GH_TOKEN` or `GITHUB_TOKEN` authentication +uses isolated version-only configuration; tokens remain transient and are not +written into it. sjskills never starts interactive login, switches accounts, +or copies credentials into staging. + +Missing tooling/configuration is reported directly. Fetch failure asks the +operator to check login, repository access, ref, and network without exposing +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 +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 +is tracked in the [private-source plan](../plans/sjskills-private-github-sources.md). + `manual` entries remain externally owned. `workflow` entries are provisioned by the named project workflow. `none` entries are catalog-only: they are classified for discovery but never selected by a desired set. @@ -57,8 +109,8 @@ additional configuration. `sjskills` invokes the exactly pinned Skills CLI only inside isolated temporary homes, verifies one staged tree per desired skill, and owns final -placement itself. Skills with the same source and `fullDepth` option share one -fetch; every requested tree must still be present and verified. Materializer +placement itself. Skills with the same source, effective access, and +`fullDepth` option share one fetch; every requested tree must still be present and verified. Materializer process groups on Unix and jobs on Windows stop descendants before staging cleanup, including after a parent exits. Byte equality and Skills CLI lock metadata do not grant ownership. A desired placement is updated only when trusted @@ -136,12 +188,13 @@ workflow tools. Protected locations retain their existing ownership boundaries. Disposable complete expected-hash snapshots live under the platform user-cache directory in `sjskills/status/`. Identity includes the sorted Skills CLI-managed -selection's names, exact source strings, copy mode, full-depth options, and the -Skills CLI, tree-hash, and cache format versions. Matching selections share +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 -the current scope, registry, files, and provenance. Older scope-keyed cache entries -are not reused, so the first check after this format change refreshes upstream. +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. Snapshots refresh after 24 hours, with a shared 30-second foreground budget and at most two concurrent skill-scope refreshes alongside one independent CLI diff --git a/go.mod b/go.mod index f2dfb09..eb78a03 100644 --- a/go.mod +++ b/go.mod @@ -5,5 +5,6 @@ go 1.23 require ( github.com/BurntSushi/toml v1.6.0 github.com/alecthomas/kong v1.16.1 + go.yaml.in/yaml/v3 v3.0.4 golang.org/x/sys v0.29.0 ) diff --git a/go.sum b/go.sum index 2c70068..41ce796 100644 --- a/go.sum +++ b/go.sum @@ -8,5 +8,9 @@ github.com/alecthomas/repr v0.5.2 h1:SU73FTI9D1P5UNtvseffFSGmdNci/O6RsqzeXJtP0Qs github.com/alecthomas/repr v0.5.2/go.mod h1:Fr0507jx4eOXV7AlPV6AVZLYrLIuIeSOWtW57eE/O/4= github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM= github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg= +go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= +go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/sys v0.29.0 h1:TPYlXGxvx1MGTn2GiZDhnjPA9wZzZeGKHHmKhHYvgaU= golang.org/x/sys v0.29.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/sjskills/access.go b/internal/sjskills/access.go new file mode 100644 index 0000000..4b3bf8c --- /dev/null +++ b/internal/sjskills/access.go @@ -0,0 +1,89 @@ +package sjskills + +import ( + "encoding/json" + "fmt" + "net/url" + "strings" +) + +// Access is a fetching policy, independent of installed source ownership. +// The zero value retains the public policy of older registries and manifests. +type Access string + +const ( + AccessPublic Access = "public" + AccessGitHubAuthenticated Access = "github-authenticated" +) + +func (a Access) Effective() Access { + if a == "" { + return AccessPublic + } + return a +} + +func (a Access) valid() bool { + return a.Effective() == AccessPublic || a == AccessGitHubAuthenticated +} + +func (a *Access) UnmarshalText(data []byte) error { + value := Access(data) + if value == "" || !value.valid() { + return fmt.Errorf("access must be public or github-authenticated") + } + *a = value + return nil +} + +func (a *Access) UnmarshalJSON(data []byte) error { + var value string + if err := json.Unmarshal(data, &value); err != nil { + return fmt.Errorf("access must be public or github-authenticated") + } + return a.UnmarshalText([]byte(value)) +} + +// githubRepository accepts only source forms whose GitHub repository identity +// is unambiguous before invoking Git or a credential helper. +func githubRepository(source string) (string, error) { + if SkillsCLIPathProblem(source) != "" { + return "", fmt.Errorf("authenticated source must be a credential-free GitHub.com repository") + } + path := source + if strings.HasPrefix(source, "https://") { + u, err := url.Parse(source) + if err != nil || u.Host != "github.com" || u.RawPath != "" || strings.Contains(u.Path, "%") { + return "", fmt.Errorf("authenticated source must use https://github.com without a port or escaped path") + } + path = strings.TrimPrefix(u.Path, "/") + } + path = strings.TrimSuffix(path, "/") + parts := strings.Split(path, "/") + if len(parts) < 2 || !shorthandPattern.MatchString(path) { + return "", fmt.Errorf("authenticated source must identify a GitHub owner and repository") + } + for _, part := range parts { + if part == "." || part == ".." { + return "", fmt.Errorf("authenticated source must not contain traversal") + } + } + if strings.HasPrefix(source, "https://") && len(parts) > 2 && (len(parts) < 4 || parts[2] != "tree") { + return "", fmt.Errorf("authenticated GitHub URL must identify a repository or /tree/ path") + } + repo := strings.TrimSuffix(parts[1], ".git") + if repo == "" || repo == "." || repo == ".." { + return "", fmt.Errorf("authenticated source must identify a repository") + } + return parts[0] + "/" + repo, nil +} + +func validateAccess(path string, access Access, source string, issues *[]Issue) { + if !access.valid() { + addIssue(issues, IssueInvalidSource, path+".access", "must be public or github-authenticated") + } else if access == AccessGitHubAuthenticated { + if _, err := githubRepository(source); err != nil { + addIssue(issues, IssueInvalidSource, path+".source", "%s", err) + } + } +} diff --git a/internal/sjskills/access_test.go b/internal/sjskills/access_test.go new file mode 100644 index 0000000..c9af383 --- /dev/null +++ b/internal/sjskills/access_test.go @@ -0,0 +1,186 @@ +package sjskills + +import ( + "encoding/json" + "strings" + "testing" +) + +func TestAccessProfileSelectionAndPublicCompatibility(t *testing.T) { + r := fixtureRegistry(t) + public, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa"}}) + if err != nil { + t.Fatal(err) + } + if len(public.Skills) != 5 { + t.Fatalf("public profile changed: %+v", public) + } + for _, s := range public.Skills { + if s.Access != AccessPublic { + t.Fatal(s) + } + } + mixed, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa", "kicpa-private"}}) + if err != nil { + t.Fatal(err) + } + if len(mixed.Skills) != 12 { + t.Fatal(mixed) + } + for _, s := range mixed.Skills { + if (s.Access == AccessGitHubAuthenticated) != (s.SourceID == "kicpa") { + t.Fatal(s) + } + } + global, err := ResolveGlobal(r) + if err != nil { + t.Fatal(err) + } + for _, s := range global.Skills { + if s.Access != AccessPublic { + t.Fatal(s) + } + } + for name, profile := range r.Profiles { + if profile.Access == AccessPublic { + profile.Access = "" + r.Profiles[name] = profile + } + } + omitted, err := ResolveProject(r, Manifest{Version: 1, Profiles: []string{"kicpa"}}) + if err != nil { + t.Fatal(err) + } + for i := range omitted.Skills { + if !sameDesiredSkill(omitted.Skills[i], public.Skills[i]) { + t.Fatal("omitted public changed identity") + } + } +} + +func TestAccessDecodingRejectsMalformedAndPreservesDirect(t *testing.T) { + for _, input := range []string{`null`, `""`, `"private"`, `true`, `1`, `[]`, `{}`} { + var profile Profile + if err := json.Unmarshal([]byte(`{"access":`+input+`,"skills":["test"]}`), &profile); err == nil { + t.Fatalf("accepted %s", input) + } + } + for _, value := range []string{`""`, `"private"`, `true`, `1`, `[]`} { + _, err := ParseManifest([]byte("version=1\n[[direct]]\nname=\"fixture\"\nsource=\"owner/repo\"\naccess=" + value + "\n")) + if err == nil { + t.Fatalf("accepted TOML %s", value) + } + } + for _, access := range []string{"", "access=\"public\"\n", "access=\"github-authenticated\"\n"} { + m, err := ParseManifest([]byte("version=1\n[[direct]]\nname=\"fixture\"\nsource=\"owner/repo\"\n" + access)) + if err != nil { + t.Fatal(err) + } + desired, err := ResolveProject(fixtureRegistry(t), m) + if err != nil { + t.Fatal(err) + } + if (desired.Skills[0].Access == AccessGitHubAuthenticated) != strings.Contains(access, "github-authenticated") { + t.Fatal(desired) + } + } +} + +func TestAuthenticatedSourcesAreBoundedToGitHub(t *testing.T) { + for _, source := range []string{"owner/repo", "owner/repo/skills/fixture", "https://github.com/owner/repo.git", "https://github.com/owner/repo/tree/main/skills"} { + if repo, err := githubRepository(source); err != nil || repo != "owner/repo" { + t.Fatalf("%s: %s %v", source, repo, err) + } + } + for _, source := range []string{"https://example.com/o/r", "https://github.com:443/o/r", "https://github.com.evil/o/r", "https://u:secret@github.com/o/r", "https://github.com/o/r?token=secret", "https://github.com/o/r#main", "https://github.com/o/r/%2e%2e/a", "https://github.com/o/r/../a", "https://github.com/o/r/a%2Fb", "https://github.com/o/r\\a", "./o/r", "owner/..", "owner/.git", "https://github.com/o/r/blob/main/skills", "https://github.com/o/r/tree", "https://github.com/o/r/issues"} { + if _, err := githubRepository(source); err == nil { + t.Fatalf("accepted %q", source) + } + } + r := fixtureRegistry(t) + p := r.Profiles["kicpa"] + p.Access = AccessGitHubAuthenticated + r.Profiles["kicpa"] = p + if ValidateRegistry(r) == nil { + t.Fatal("required public profile allowed authentication") + } + r = fixtureRegistry(t) + p = r.Profiles["kicpa-private"] + delete(r.Profiles, "kicpa-private") + r.Profiles["other-private"] = p + if err := ValidateRegistry(r); err != nil { + t.Fatal(err) + } + r.Profiles["bad_name"] = p + if ValidateRegistry(r) == nil { + t.Fatal("accepted malformed additional profile") + } +} + +func TestAccessSeparatesFetchAndReviewEvidenceButNotOwnership(t *testing.T) { + a := DesiredSkill{Name: "fixture", Source: "owner/repo", Manager: ManagerSkillsCLI, Mode: ModeCopy, Access: AccessPublic} + b := a + b.Access = AccessGitHubAuthenticated + if sameDesiredSkill(a, b) { + t.Fatal("access omitted from desired identity") + } + if _, _, err := classifyMaterializationSkills([]DesiredSkill{a, b}); err == nil { + t.Fatal("contradictory fetch policy accepted") + } + b.Name = "other" + if len(materializationBatches([]DesiredSkill{a, b})) != 2 { + t.Fatal("access batches combined") + } + sa := StatusScope{Plan: Plan{Desired: DesiredState{Skills: []DesiredSkill{a}}}} + b.Name = a.Name + sb := StatusScope{Plan: Plan{Desired: DesiredState{Skills: []DesiredSkill{b}}}} + if sa.cacheKey() == sb.cacheKey() { + t.Fatal("authentication shares public cache evidence") + } + legacy := a + legacy.Access = "" + sl := StatusScope{Plan: Plan{Desired: DesiredState{Skills: []DesiredSkill{legacy}}}} + if sa.cacheKey() != sl.cacheKey() { + t.Fatal("omitted public changes cache key") + } + ea := Envelope{Plan: &sa.Plan} + el := Envelope{Plan: &sl.Plan} + na := normalizeReviewedEnvelope(ea) + nl := normalizeReviewedEnvelope(el) + if !sameDesiredSkill(na.Plan.Desired.Skills[0], nl.Plan.Desired.Skills[0]) { + t.Fatal("legacy review not normalized") + } + if el.Plan.Desired.Skills[0].Access != "" { + t.Fatal("normalization mutated input") + } +} + +func TestAccessStatusSnapshotsNormalizePublic(t *testing.T) { + scope, _, _, _ := statusFixture(t, false) + other := scope + other.Registry.Profiles = map[string]Profile{} + for name, profile := range scope.Registry.Profiles { + profile.Access = "" + other.Registry.Profiles[name] = profile + } + other.Plan.Desired = cloneDesiredState(scope.Plan.Desired) + for i := range other.Plan.Desired.Skills { + other.Plan.Desired.Skills[i].Access = "" + } + if !scope.Matches(other) { + t.Fatal("omitted public changed command snapshot identity") + } + other.Plan.Desired.Skills[0].Access = AccessGitHubAuthenticated + if scope.Matches(other) { + t.Fatal("authenticated policy reused command snapshot") + } +} + +func TestAuthenticatedNonInstallableSkillsAreSkipped(t *testing.T) { + for _, manager := range []Manager{ManagerManual, ManagerWorkflow} { + installed, skipped, err := classifyMaterializationSkills([]DesiredSkill{{Name: "manual-fixture", Manager: manager, Access: AccessGitHubAuthenticated}}) + if err != nil || len(installed) != 0 || len(skipped) != 1 { + t.Fatalf("%s: installed=%v skipped=%v err=%v", manager, installed, skipped, err) + } + } +} diff --git a/internal/sjskills/contracts_test.go b/internal/sjskills/contracts_test.go index a0f4886..dcc3a5b 100644 --- a/internal/sjskills/contracts_test.go +++ b/internal/sjskills/contracts_test.go @@ -49,8 +49,8 @@ func TestCanonicalRegistryAndProfiles(t *testing.T) { if registry.Version != RegistryVersion { t.Fatalf("version = %d", registry.Version) } - if len(registry.Skills) != 43 { - t.Fatalf("skills = %d, want 43", len(registry.Skills)) + if len(registry.Skills) != 50 { + t.Fatalf("skills = %d, want 50", len(registry.Skills)) } global, err := ResolveGlobal(registry) if err != nil { @@ -155,7 +155,12 @@ func TestKicpaAndManagerBoundaries(t *testing.T) { if err != nil { t.Fatal(err) } - if len(state.Skills) != 5 || state.Skills[0].Name != "darty" || state.Skills[1].Name != "kasb" || state.Skills[2].Name != "kisnet-ytm" || state.Skills[3].Name != "krx-cli" || state.Skills[4].Name != "windows-cleanup" { + wantNames := []string{"darty", "kasb", "kisnet-ytm", "krx-cli", "windows-cleanup"} + var gotNames []string + for _, skill := range state.Skills { + gotNames = append(gotNames, skill.Name) + } + if !slices.Equal(gotNames, wantNames) { t.Fatalf("kicpa = %#v", state.Skills) } if darty := state.Skills[0]; darty.Source != "https://github.com/cpaikr/darty/tree/main/skill/darty" || darty.Manager != ManagerSkillsCLI || darty.Mode != ModeCopy { diff --git a/internal/sjskills/data/registry-v4.json b/internal/sjskills/data/registry-v4.json index b6aed0a..d50839b 100644 --- a/internal/sjskills/data/registry-v4.json +++ b/internal/sjskills/data/registry-v4.json @@ -22,6 +22,7 @@ }, "profiles": { "dev": { + "access": "public", "skills": [ "address-issues", "address-pr-feedback", @@ -42,12 +43,14 @@ ] }, "go": { + "access": "public", "skills": [ "modern-go", "write-go-docs" ] }, "kicpa": { + "access": "public", "skills": [ "darty", "kasb", @@ -56,7 +59,20 @@ "windows-cleanup" ] }, + "kicpa-private": { + "access": "github-authenticated", + "skills": [ + "cagr-research", + "crowe-design", + "organize-engagement-files", + "render-markdown-pdf", + "sync-rfi", + "tf-basic-data-prep", + "wacc-peer-screening" + ] + }, "rust": { + "access": "public", "skills": [ "clear-rust", "modern-rust" @@ -88,6 +104,10 @@ "kind": "external", "location": "https://github.com/cpaikr/kasb/tree/main/skills/kasb" }, + "kicpa": { + "kind": "external", + "location": "https://github.com/sjunepark/kicpa/tree/main/skills" + }, "kisnet-ytm": { "kind": "external", "location": "https://github.com/cpaikr/ytm/tree/main/skills/kisnet-ytm" @@ -153,6 +173,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "cagr-research", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "clarify", "source": "agent-scripts", @@ -207,6 +233,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "crowe-design", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "darty", "source": "darty", @@ -316,6 +348,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "organize-engagement-files", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "pdf-to-markdown", "source": "agent-scripts", @@ -334,6 +372,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "render-markdown-pdf", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "review-campaign", "source": "agent-scripts", @@ -357,12 +401,24 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "sync-rfi", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "teach", "source": "agent-scripts", "manager": "skills-cli", "mode": "copy" }, + { + "name": "tf-basic-data-prep", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "ui-lab", "source": "agent-scripts", @@ -375,6 +431,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "wacc-peer-screening", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "windows-cleanup", "source": "agent-scripts", diff --git a/internal/sjskills/github_auth.go b/internal/sjskills/github_auth.go new file mode 100644 index 0000000..4e590a9 --- /dev/null +++ b/internal/sjskills/github_auth.go @@ -0,0 +1,316 @@ +package sjskills + +import ( + "bufio" + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" + "time" + + "go.yaml.in/yaml/v3" +) + +const GitHubCredentialCommand = "__github-credential" + +var fullGitCommit = regexp.MustCompile(`^[0-9a-fA-F]{40}$`) + +// ghConfigDirectory resolves only the original gh configuration location. The +// materializer never copies a user's home, Git configuration, or credentials. +func ghConfigDirectory(env []string, platform string) (string, error) { + get := func(name string) string { return environmentValue(env, name, platform) } + directory := get("GH_CONFIG_DIR") + if directory == "" && get("XDG_CONFIG_HOME") != "" { + directory = filepath.Join(get("XDG_CONFIG_HOME"), "gh") + } + if directory == "" && isWindowsPlatform(platform) && get("APPDATA") != "" { + directory = filepath.Join(get("APPDATA"), "GitHub CLI") + } + if directory == "" { + home := get("HOME") + if home == "" && isWindowsPlatform(platform) { + home = get("USERPROFILE") + } + if home != "" { + directory = filepath.Join(home, ".config", "gh") + } + } + if directory == "" || !filepath.IsAbs(directory) { + return "", errors.New("cannot locate absolute gh configuration; set GH_CONFIG_DIR") + } + return directory, nil +} + +func environmentValue(env []string, name, platform string) string { + value := "" + for _, entry := range env { + key, candidate, ok := strings.Cut(entry, "=") + if ok && (key == name || isWindowsPlatform(platform) && strings.EqualFold(key, name)) { + value = candidate + } + } + return value +} + +// gh migrates legacy configuration even for credential-helper invocations. +// Refuse it before starting gh: authentication must never perform that write. +// Decode YAML rather than guessing at quoting, aliases, or duplicate keys. +func checkGHConfig(directory string) error { + data, err := readBoundedFile(filepath.Join(directory, "config.yml"), 1024*1024) + if err != nil { + return errors.New("gh configuration is missing or unreadable; run gh auth status to complete login/configuration setup, then retry") + } + var config map[string]any + decoder := yaml.NewDecoder(bytes.NewReader(data)) + if err := decoder.Decode(&config); err != nil { + return errors.New("gh configuration is malformed; repair it using gh before retrying") + } + var extra any + if err := decoder.Decode(&extra); err != io.EOF { + return errors.New("gh configuration must contain one YAML document") + } + // gh's current multi-account migration has post-version 1. Future schemas + // need a reviewed compatibility change, rather than assuming migration safety. + if config["version"] != "1" && config["version"] != 1 { + return errors.New("gh configuration requires migration; run gh auth status yourself, then retry (supported configuration version: 1)") + } + return nil +} + +// credentialEnvironment excludes inherited Git configuration and diagnostics: +// only our exact repository helper may supply credentials, with no redirects, +// fallback protocols, caller hooks, or persisted Git configuration. +func credentialEnvironment(base []string, root, platform string) []string { + filtered := make([]string, 0, len(base)) + for _, entry := range base { + key, _, _ := strings.Cut(entry, "=") + upper := strings.ToUpper(key) + if strings.HasPrefix(upper, "GIT_") || strings.HasPrefix(upper, "GH_") || strings.HasPrefix(upper, "SJSKILLS_GH_") || upper == "SSH_ASKPASS" || upper == "GITHUB_TOKEN" { + continue + } + filtered = append(filtered, entry) + } + env := isolatedEnvironment(filtered, root, platform) + return append(env, "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL="+filepath.Join(root, ".gitconfig"), "GIT_TERMINAL_PROMPT=0", "GIT_ALLOW_PROTOCOL=https", "GH_PROMPT_DISABLED=1", "GH_NO_UPDATE_NOTIFIER=1", "GH_NO_EXTENSION_UPDATE_NOTIFIER=1", "GH_TELEMETRY_DISABLED=1") +} + +func gitShellQuote(value string) string { + // Git executes ! helpers with its POSIX shell, including Git for Windows. + return "'" + strings.ReplaceAll(filepath.ToSlash(value), "'", "'\\''") + "'" +} + +func (m *Materializer) authenticatedSource(ctx context.Context, root, source string) (string, error) { + repo, err := githubRepository(source) + if err != nil { + return "", err + } + gh, err := m.lookPath("gh") + if err != nil { + return "", errors.New("gh is unavailable; install GitHub CLI and sign in before fetching authenticated skills") + } + git, err := m.lookPath("git") + if err != nil { + return "", errors.New("git is unavailable for authenticated fetching") + } + config, err := ghConfigDirectory(m.baseEnv, m.platform) + token := environmentValue(m.baseEnv, "GH_TOKEN", m.platform) + if token == "" { + token = environmentValue(m.baseEnv, "GITHUB_TOKEN", m.platform) + } + if token != "" { + // Environment-backed gh login needs no persistent config. This temporary + // config contains only a schema version, never the credential itself. + config = filepath.Join(root, ".gh-auth") + if err = os.MkdirAll(config, 0o700); err == nil { + err = os.WriteFile(filepath.Join(config, "config.yml"), []byte("version: 1\n"), 0o600) + } + } + if err != nil { + return "", errors.New("cannot prepare gh configuration; set GH_CONFIG_DIR or sign in with gh") + } + if err = checkGHConfig(config); err != nil { + return "", err + } + helper := m.helperExecutable + if helper == "" { + helper, err = os.Executable() + } + 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) + } + 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")} + remote := "https://github.com/" + repo + ".git" + commands := [][]string{} + if fullGitCommit.MatchString(ref) { + // clone --branch accepts names only. Fetch a full commit explicitly; + // never fall back to the default branch if the server rejects the pin. + commands = append(commands, + []string{"init", "--template=", "--", clone}, + []string{"-C", clone, "fetch", "--depth=1", "--no-tags", "--no-recurse-submodules", "--", remote, ref}, + []string{"-C", clone, "checkout", "--detach", "--force", "FETCH_HEAD", "--"}) + } else { + args := []string{"clone", "--depth=1", "--single-branch", "--no-recurse-submodules"} + if ref != "" { + args = append(args, "--branch", ref) + } + commands = append(commands, append(args, "--", remote, clone)) + } + for _, command := range commands { + 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) + } + } + + local := filepath.Join(clone, filepath.FromSlash(subpath)) + if err := validateSymlinkParents(clone, local); err != nil { + return "", errors.New("authenticated source subpath is missing or unsafe") + } + if err := checkRealDirectory(local); err != nil { + return "", errors.New("authenticated source subpath is not a real directory") + } + return local, nil +} + +// Match the supported subset of pinned Skills CLI source parsing. Shorthand +// suffixes are subpaths; GitHub /tree// URLs select a ref and optional path. +func githubSourcePath(source string) (ref, subpath string) { + path := strings.TrimSuffix(strings.TrimPrefix(source, "https://github.com/"), "/") + parts := strings.Split(path, "/") + if strings.HasPrefix(source, "https://") { + if len(parts) >= 4 && parts[2] == "tree" { + return parts[3], strings.Join(parts[4:], "/") + } + return "", "" + } + return "", strings.Join(parts[2:], "/") +} + +func privateProcessError(message string, err error) error { + // External output is never diagnostic evidence for authenticated processes. + // In particular a failed helper must not echo credentials through Git/CLI. + switch { + case errors.Is(err, errProcessTreeActive): + return fmt.Errorf("%s: %w", message, errProcessTreeActive) + case errors.Is(err, context.DeadlineExceeded): + return fmt.Errorf("%s: %w", message, context.DeadlineExceeded) + case errors.Is(err, context.Canceled): + return fmt.Errorf("%s: %w", message, context.Canceled) + default: + return errors.New(message) + } +} + +// RunGitHubCredential is the private Git helper entry point. The only output +// is Git's credential protocol, written directly into Git's private pipe. +// It intentionally emits no diagnostics and never handles store or erase. +func RunGitHubCredential(ctx context.Context, args []string, input io.Reader, output io.Writer, env []string) error { + if len(args) != 1 { + return errors.New("invalid credential operation") + } + if args[0] == "store" || args[0] == "erase" { + return nil + } + if args[0] != "get" { + return errors.New("invalid credential operation") + } + fields := map[string]string{} + scanner := bufio.NewScanner(io.LimitReader(input, 8193)) + total := 0 + for scanner.Scan() { + line := scanner.Text() + total += len(line) + 1 + if total > 8192 { + return errors.New("credential request exceeds bound") + } + if line == "" { + break + } + key, value, ok := strings.Cut(line, "=") + if !ok { + return errors.New("invalid credential request") + } + if _, exists := fields[key]; exists { + return errors.New("duplicate credential field") + } + fields[key] = value + } + if scanner.Err() != nil { + return errors.New("invalid credential request") + } + get := func(name string) string { return environmentValue(env, name, "windows") } + repo := get("SJSKILLS_GH_REPOSITORY") + valid, err := githubRepository(repo) + if err != nil || valid != repo || fields["protocol"] != "https" || fields["host"] != "github.com" || strings.TrimSuffix(fields["path"], ".git") != repo { + return errors.New("credential request outside selected repository") + } + gh, config := get("SJSKILLS_GH_EXECUTABLE"), get("SJSKILLS_GH_CONFIG_DIR") + if !filepath.IsAbs(gh) || !filepath.IsAbs(config) { + return errors.New("invalid credential helper configuration") + } + if err := checkGHConfig(config); err != nil { + return err + } + helperEnv := make([]string, 0, len(env)) + for _, entry := range env { + key, _, _ := strings.Cut(entry, "=") + upper := strings.ToUpper(key) + if strings.HasPrefix(upper, "GIT_") || strings.HasPrefix(upper, "GH_") || strings.HasPrefix(upper, "SJSKILLS_GH_") { + continue + } + helperEnv = append(helperEnv, entry) + } + if token := get("GH_TOKEN"); token != "" { + helperEnv = append(helperEnv, "GH_TOKEN="+token) + } + helperEnv = append(helperEnv, "GH_CONFIG_DIR="+config, "GH_HOST=github.com", "GH_PROMPT_DISABLED=1", "GH_NO_UPDATE_NOTIFIER=1", "GH_NO_EXTENSION_UPDATE_NOTIFIER=1", "GH_TELEMETRY_DISABLED=1") + helperCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + cmd := exec.CommandContext(helperCtx, gh, "auth", "git-credential", "get") + cmd.Env = helperEnv + cmd.Stdin = strings.NewReader("protocol=https\nhost=github.com\npath=" + repo + ".git\n\n") + cmd.Stdout = &credentialPipeWriter{writer: output, remaining: 64 * 1024} + cmd.Stderr = io.Discard + cmd.WaitDelay = boundedExecWaitDelay + // Stay in the enclosing Git process group/job so outer cancellation and + // cleanup verification cover gh as well as Git and the helper itself. + if err := cmd.Run(); err != nil { + return errors.New("gh credential lookup failed") + } + return nil +} + +// Bound the private pipe without retaining a second copy of credential output. +type credentialPipeWriter struct { + writer io.Writer + remaining int +} + +func (w *credentialPipeWriter) Write(data []byte) (int, error) { + if len(data) > w.remaining { + return 0, errors.New("credential response exceeds bound") + } + n, err := w.writer.Write(data) + w.remaining -= n + return n, err +} diff --git a/internal/sjskills/github_auth_test.go b/internal/sjskills/github_auth_test.go new file mode 100644 index 0000000..e15252d --- /dev/null +++ b/internal/sjskills/github_auth_test.go @@ -0,0 +1,167 @@ +package sjskills + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" +) + +func TestGHConfigReadOnlyMigrationBoundary(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yml") + for _, data := range []string{"version: 1\n", "version: \"1\"\n"} { + if err := os.WriteFile(path, []byte(data), 0600); err != nil { + t.Fatal(err) + } + if err := checkGHConfig(dir); err != nil { + t.Fatal(err) + } + after, _ := os.ReadFile(path) + if string(after) != data { + t.Fatal("config changed") + } + } + for _, data := range []string{"", "git_protocol: https\n", "version: 2\n", "version: 1\nversion: 0\n", "version: []\n", "version: 1\n---\nversion: 0\n", "version: [\nsecret"} { + if err := os.WriteFile(path, []byte(data), 0600); err != nil { + t.Fatal(err) + } + err := checkGHConfig(dir) + if err == nil || strings.Contains(err.Error(), "secret") { + t.Fatalf("config error: %v", err) + } + after, _ := os.ReadFile(path) + if string(after) != data { + t.Fatal("config changed") + } + } +} + +func TestCredentialGateAndBoundsNeverInvokeGH(t *testing.T) { + config := t.TempDir() + if err := os.WriteFile(filepath.Join(config, "config.yml"), []byte("version: 1\n"), 0600); err != nil { + t.Fatal(err) + } + env := []string{"SJSKILLS_GH_REPOSITORY=owner/repo", "SJSKILLS_GH_EXECUTABLE=" + filepath.Join(t.TempDir(), "never-run"), "SJSKILLS_GH_CONFIG_DIR=" + config} + for _, input := range []string{"protocol=http\nhost=github.com\npath=owner/repo.git\n", "protocol=https\nhost=evil.invalid\npath=owner/repo.git\n", "protocol=https\nhost=github.com\npath=owner/other.git\n", "protocol=https\nhost=github.com\nhost=github.com\npath=owner/repo.git\n", "protocol=https\nhost=github.com:443\npath=owner/repo.git\n", strings.Repeat("x", 8193)} { + var out bytes.Buffer + err := RunGitHubCredential(context.Background(), []string{"get"}, strings.NewReader(input), &out, env) + if err == nil || out.Len() != 0 || err.Error() == "gh credential lookup failed" { + t.Fatalf("gate failed %v %q", err, out.String()) + } + } + for _, action := range []string{"store", "erase"} { + if err := RunGitHubCredential(context.Background(), []string{action}, strings.NewReader("password=secret\n"), &bytes.Buffer{}, nil); err != nil { + t.Fatal(err) + } + } + var out bytes.Buffer + writer := credentialPipeWriter{writer: &out, remaining: 4} + if _, err := writer.Write([]byte("12345")); err == nil || out.Len() != 0 { + t.Fatal("pipe bound failed") + } +} + +func TestAuthenticatedFetchFailureCleanup(t *testing.T) { + for _, mode := range []string{"missing-gh", "missing-git", "migration", "denied", "timeout", "cancel", "oversize", "survivor", "missing-skill"} { + t.Run(mode, func(t *testing.T) { + parent := t.TempDir() + config := t.TempDir() + configData := "version: 1\n" + if mode == "migration" { + configData = "git_protocol: https\n" + } + if err := os.WriteFile(filepath.Join(config, "config.yml"), []byte(configData), 0600); err != nil { + t.Fatal(err) + } + runner := defaultMaterializeRunner() + fallback := runner.invoke + runner.invoke = func(ctx context.Context, command string, args, env []string) (ProcessResult, error) { + if command == "git" { + switch mode { + case "denied": + return ProcessResult{Stderr: []byte("sentinel-secret"), ExitCode: 1}, errors.New("sentinel-secret") + case "timeout": + <-ctx.Done() + return ProcessResult{}, ctx.Err() + case "cancel": + return ProcessResult{}, context.Canceled + case "oversize": + return ProcessResult{Stdout: bytes.Repeat([]byte("sentinel-secret"), 100)}, nil + case "survivor": + return ProcessResult{}, errProcessTreeActive + case "missing-skill": + return ProcessResult{}, nil + } + } + if len(args) > 1 && args[1] == "add" && mode == "missing-skill" { + return ProcessResult{}, nil + } + return fallback(ctx, command, args, env) + } + m := NewMaterializer(MaterializerConfig{Runner: runner, BaseEnvironment: []string{"GH_CONFIG_DIR=" + config}, TempRootFactory: func() (string, error) { return os.MkdirTemp(parent, "stage-") }, LookPath: func(name string) (string, error) { + if mode == "missing-"+name { + return "", errors.New("not found") + } + return name, nil + }, Limits: MaterializerLimits{CommandTimeout: 20 * time.Millisecond, MaxStdoutBytes: 100}}) + skill := desiredMaterializeSkill("fixture", "owner/repo") + skill.Access = AccessGitHubAuthenticated + plan, err := m.Materialize(context.Background(), []DesiredSkill{skill}) + if err == nil || plan != nil || strings.Contains(err.Error(), "sentinel-secret") { + t.Fatalf("error %v plan %v", err, plan) + } + entries, _ := os.ReadDir(parent) + if mode == "survivor" { + if !errors.Is(err, errProcessTreeActive) || len(entries) == 0 { + t.Fatal("unverified descendants staging removed") + } + } else if len(entries) != 0 { + t.Fatalf("staging retained after %s", mode) + } + if mode == "missing-gh" || mode == "missing-git" || mode == "migration" { + for _, call := range runner.calls { + if call.command == "git" || call.command == "gh" { + t.Fatal("preflight invoked auth process") + } + } + } + }) + } +} + +func TestGHEnvironmentAndSourceParsing(t *testing.T) { + original := t.TempDir() + config := filepath.Join(original, "gh") + env := []string{"HOME=" + original, "GH_CONFIG_DIR=" + config, "GIT_CONFIG_COUNT=2", "GIT_TRACE=1", "GH_DEBUG=api", "GH_TOKEN=sentinel", "GITHUB_TOKEN=sentinel", "PATH=/bin"} + isolated := credentialEnvironment(env, t.TempDir(), "darwin") + for _, name := range []string{"GIT_TRACE", "GIT_CONFIG_COUNT", "GH_DEBUG", "GH_TOKEN", "GITHUB_TOKEN", "GH_CONFIG_DIR"} { + if environmentValue(isolated, name, "darwin") != "" { + t.Fatalf("leaked %s", name) + } + } + if environmentValue(isolated, "GIT_ALLOW_PROTOCOL", "darwin") != "https" { + t.Fatal("protocol widened") + } + for _, tc := range []struct{ source, ref, path string }{{"owner/repo/skills/a", "", "skills/a"}, {"https://github.com/owner/repo/tree/main/skills", "main", "skills"}, {"https://github.com/owner/repo/tree/v1", "v1", ""}, {"https://github.com/owner/repo.git", "", ""}} { + ref, path := githubSourcePath(tc.source) + if ref != tc.ref || path != tc.path { + t.Fatal(tc, ref, path) + } + } + dir, err := ghConfigDirectory(env, "darwin") + if err != nil || dir != config { + t.Fatal(dir, err) + } + a := DesiredSkill{Name: "a", Source: "o/r", Access: AccessPublic} + b := a + b.Access = "" + if !reflect.DeepEqual(normalizeReviewedEnvelope(Envelope{Plan: &Plan{Desired: DesiredState{Skills: []DesiredSkill{a}}}}), normalizeReviewedEnvelope(Envelope{Plan: &Plan{Desired: DesiredState{Skills: []DesiredSkill{b}}}})) { + t.Fatal("legacy review compatibility") + } +} diff --git a/internal/sjskills/materialize.go b/internal/sjskills/materialize.go index eda01ff..1e3394b 100644 --- a/internal/sjskills/materialize.go +++ b/internal/sjskills/materialize.go @@ -53,10 +53,9 @@ type ProcessResult struct { ExitCode int } -// Runner is the only process dependency of Materializer. command is always -// "bunx" for the production adapter; the versioned Skills CLI is the first -// argument. env is a complete environment, preserving ordinary inherited -// values while replacing every home/config signal used by the CLI. +// Runner bounds Skills CLI and authenticated Git processes. The Git process +// owns its credential-helper descendants. env is complete: Skills CLI uses +// isolated home/config paths; private Git also restricts authentication policy. type Runner interface { Run(context.Context, string, []string, []string) (ProcessResult, error) } @@ -86,7 +85,10 @@ type MaterializerLimits struct { // MaterializerConfig contains only process, environment, staging, and bound // dependencies. It intentionally has no inventory/apply/rollout knobs. type MaterializerConfig struct { - Runner Runner + // HelperExecutable overrides the current sjskills executable only for + // controlled process tests embedding this adapter in another executable. + HelperExecutable string + Runner Runner // TempRootFactory supplies a candidate directory. It is removed after the // session only once it passes the fresh-empty ownership check. Tests inject @@ -114,12 +116,13 @@ type MaterializerConfig struct { // snapshots are never reused across plans because moving remote sources must // be revalidated for each plan. type Materializer struct { - runner Runner - tempRoot TempRootFactory - baseEnv []string - lookPath LookPathFunc - platform string - limits MaterializerLimits + helperExecutable string + runner Runner + tempRoot TempRootFactory + baseEnv []string + lookPath LookPathFunc + platform string + limits MaterializerLimits } // NewMaterializer constructs an isolated Skills CLI adapter. No process, @@ -185,12 +188,13 @@ func NewMaterializer(config MaterializerConfig) *Materializer { } return &Materializer{ - runner: runner, - tempRoot: tempRoot, - baseEnv: baseEnv, - lookPath: lookPath, - platform: platform, - limits: limits, + helperExecutable: config.HelperExecutable, + runner: runner, + tempRoot: tempRoot, + baseEnv: baseEnv, + lookPath: lookPath, + platform: platform, + limits: limits, } } @@ -323,7 +327,32 @@ func (m *Materializer) Materialize(ctx context.Context, skills []DesiredSkill) ( _ = plan.Cleanup() return nil, m.errorAt(root, safeSkillName(skill.Name), "build Skills CLI command", err) } - result, runErr := m.run(ctx, root, args) + if skill.Access == AccessGitHubAuthenticated { + local, fetchErr := m.authenticatedSource(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) + } + // Only this internally verified staging path may bypass remote input + // validation; DesiredSkill and provenance retain the original URL. + args[2] = local + } + var result ProcessResult + var runErr error + if skill.Access == AccessGitHubAuthenticated { + result, runErr = m.runCommand(ctx, SkillsCLICommand, args, credentialEnvironment(m.baseEnv, root, m.platform)) + } else { + result, runErr = m.run(ctx, root, args) + } + if skill.Access == AccessGitHubAuthenticated && (runErr != nil || result.ExitCode != 0) { + if !errors.Is(runErr, errProcessTreeActive) { + _ = plan.Cleanup() + } + return nil, privateProcessError("private skill discovery failed; verify the selected skill exists in the source", runErr) + } if runErr != nil { if !errors.Is(runErr, errProcessTreeActive) { _ = plan.Cleanup() @@ -369,11 +398,12 @@ func materializationBatches(skills []DesiredSkill) [][]DesiredSkill { type key struct { source string fullDepth bool + access Access } positions := map[key]int{} batches := [][]DesiredSkill{} for _, skill := range skills { - k := key{skill.Source, skill.FullDepth} + k := key{skill.Source, skill.FullDepth, skill.Access.Effective()} index, ok := positions[k] if !ok { index = len(batches) @@ -390,6 +420,11 @@ func classifyMaterializationSkills(skills []DesiredSkill) ([]DesiredSkill, []Des skipped := make([]DesiredSkill, 0) seen := make(map[string]DesiredSkill, len(skills)) for _, skill := range skills { + skill.Access = skill.Access.Effective() + if !skill.Access.valid() { + return nil, nil, materializationError("classify", "invalid access policy", nil) + } + if skill.Name == "" || !isPortableName(skill.Name) { return nil, nil, materializationError("classify", "skill name is not portable", nil) } @@ -409,11 +444,16 @@ func classifyMaterializationSkills(skills []DesiredSkill) ([]DesiredSkill, []Des skipped = append(skipped, skill) } case ManagerSkillsCLI: + if skill.Access == AccessGitHubAuthenticated { + if _, err := githubRepository(skill.Source); err != nil { + return nil, nil, materializationError("classify", "invalid authenticated source", err) + } + } if skill.Mode != ModeCopy { return nil, nil, materializationError(safeSkillName(skill.Name), "skills-cli installation must use copy mode", nil) } if previous, ok := seen[skill.Name]; ok { - if previous.Manager != ManagerSkillsCLI || previous.Source != skill.Source || previous.FullDepth != skill.FullDepth { + if previous.Manager != ManagerSkillsCLI || previous.Source != skill.Source || previous.FullDepth != skill.FullDepth || previous.Access.Effective() != skill.Access.Effective() { return nil, nil, materializationError("classify", "skill identity has contradictory source or options", nil) } // The exact identity was already scheduled. Targets differ at @@ -433,7 +473,7 @@ func classifyMaterializationSkills(skills []DesiredSkill) ([]DesiredSkill, []Des } func sameDesiredSkill(left, right DesiredSkill) bool { - if left.Name != right.Name || left.SourceID != right.SourceID || left.Source != right.Source || + if left.Access.Effective() != right.Access.Effective() || left.Name != right.Name || left.SourceID != right.SourceID || left.Source != right.Source || left.Scope != right.Scope || left.Origin != right.Origin || left.Manager != right.Manager || left.Mode != right.Mode || left.Workflow != right.Workflow || left.FullDepth != right.FullDepth || len(left.Targets) != len(right.Targets) { @@ -495,13 +535,16 @@ func validateStagingRoot(root string) error { } func (m *Materializer) run(ctx context.Context, root string, args []string) (ProcessResult, error) { + return m.runCommand(ctx, SkillsCLICommand, args, isolatedEnvironment(m.baseEnv, root, m.platform)) +} + +func (m *Materializer) runCommand(ctx context.Context, command string, args, env []string) (ProcessResult, error) { if err := ensureContext(ctx); err != nil { return ProcessResult{}, err } commandCtx, cancel := context.WithTimeout(ctx, m.limits.CommandTimeout) defer cancel() - env := isolatedEnvironment(m.baseEnv, root, m.platform) - result, err := m.runner.Run(commandCtx, SkillsCLICommand, append([]string(nil), args...), env) + result, err := m.runner.Run(commandCtx, command, append([]string(nil), args...), env) stdoutExceeded := int64(len(result.Stdout)) > m.limits.MaxStdoutBytes stderrExceeded := int64(len(result.Stderr)) > m.limits.MaxStderrBytes if stdoutExceeded { diff --git a/internal/sjskills/parse.go b/internal/sjskills/parse.go index 06c6cef..da3829d 100644 --- a/internal/sjskills/parse.go +++ b/internal/sjskills/parse.go @@ -33,6 +33,10 @@ func ParseRegistry(data []byte) (Registry, error) { if err := ValidateRegistry(registry); err != nil { return Registry{}, err } + for name, profile := range registry.Profiles { + profile.Access = profile.Access.Effective() + registry.Profiles[name] = profile + } return registry, nil } @@ -66,6 +70,9 @@ func ParseManifest(data []byte) (Manifest, error) { if err := ValidateManifestShape(manifest); err != nil { return Manifest{}, err } + for i := range manifest.Direct { + manifest.Direct[i].Access = manifest.Direct[i].Access.Effective() + } return manifest, nil } diff --git a/internal/sjskills/resolve.go b/internal/sjskills/resolve.go index fad39d4..61f62e6 100644 --- a/internal/sjskills/resolve.go +++ b/internal/sjskills/resolve.go @@ -85,7 +85,9 @@ func ResolveProject(registry Registry, manifest Manifest) (DesiredState, error) return DesiredState{}, collisionError(origin, name, previous) } selected[name] = origin - state.Skills = append(state.Skills, resolveCentralSkill(registry, declarations[name], ScopeProject, origin)) + skill := resolveCentralSkill(registry, declarations[name], ScopeProject, origin) + skill.Access = registry.Profiles[profileName].Access.Effective() + state.Skills = append(state.Skills, skill) } } for _, direct := range manifest.Direct { @@ -111,6 +113,7 @@ func declarationByName(registry Registry, name string) SkillDeclaration { func resolveCentralSkill(registry Registry, declaration SkillDeclaration, scope Scope, origin string) DesiredSkill { source := registry.Sources[declaration.Source] return DesiredSkill{ + Access: AccessPublic, Name: declaration.Name, SourceID: declaration.Source, Source: source.Location, @@ -126,6 +129,7 @@ func resolveCentralSkill(registry Registry, declaration SkillDeclaration, scope func resolveDirectSkill(registry Registry, direct DirectSkill) DesiredSkill { return DesiredSkill{ + Access: direct.Access.Effective(), Name: direct.Name, Source: direct.Source, Scope: ScopeProject, diff --git a/internal/sjskills/reviewed_plan.go b/internal/sjskills/reviewed_plan.go index e70ced7..c8bdf00 100644 --- a/internal/sjskills/reviewed_plan.go +++ b/internal/sjskills/reviewed_plan.go @@ -128,6 +128,10 @@ func normalizeReviewedEnvelope(envelope Envelope) Envelope { envelope.Evidence = evidence if envelope.Plan != nil { plan := *envelope.Plan + plan.Desired.Skills = append([]DesiredSkill(nil), plan.Desired.Skills...) + for i := range plan.Desired.Skills { + plan.Desired.Skills[i].Access = plan.Desired.Skills[i].Access.Effective() + } plan.Warnings = nil plan.Evidence = nil envelope.Plan = &plan diff --git a/internal/sjskills/status.go b/internal/sjskills/status.go index c84356a..5b5db41 100644 --- a/internal/sjskills/status.go +++ b/internal/sjskills/status.go @@ -168,13 +168,23 @@ func ResolveStatusScope(directory string, registry Registry, global bool) (Statu func (s StatusScope) Matches(other StatusScope) bool { return s.identity() == other.identity() } func (s StatusScope) identity() string { + registry := s.Registry + registry.Profiles = make(map[string]Profile, len(s.Registry.Profiles)) + for name, profile := range s.Registry.Profiles { + profile.Access = profile.Access.Effective() + registry.Profiles[name] = profile + } + desired := cloneDesiredState(s.Plan.Desired) + for i := range desired.Skills { + desired.Skills[i].Access = desired.Skills[i].Access.Effective() + } data, _ := json.Marshal(struct { Root string Registry Registry Desired DesiredState CLI, Hash string Format int - }{s.Root, s.Registry, s.Plan.Desired, SkillsCLIVersion, TreeHashAlgorithmSHA256V2, statusCacheVersion}) + }{s.Root, registry, desired, SkillsCLIVersion, TreeHashAlgorithmSHA256V2, statusCacheVersion}) sum := sha256.Sum256(data) return hex.EncodeToString(sum[:]) } @@ -186,11 +196,12 @@ func (s StatusScope) cacheKey() string { Name, Source string Mode InstallMode FullDepth bool + Access Access } inputs := make([]input, 0, len(s.Plan.Desired.Skills)) for _, skill := range s.Plan.Desired.Skills { if skill.Manager == ManagerSkillsCLI { - inputs = append(inputs, input{skill.Name, skill.Source, skill.Mode, skill.FullDepth}) + inputs = append(inputs, input{skill.Name, skill.Source, skill.Mode, skill.FullDepth, skill.Access.Effective()}) } } sort.Slice(inputs, func(i, j int) bool { return inputs[i].Name < inputs[j].Name }) diff --git a/internal/sjskills/status_test.go b/internal/sjskills/status_test.go index 8776b06..49d561f 100644 --- a/internal/sjskills/status_test.go +++ b/internal/sjskills/status_test.go @@ -521,3 +521,35 @@ func TestStatusManualWorkflowAndProtectedPathsHaveNoInventedUpdates(t *testing.T t.Fatalf("workflow status %+v", result) } } + +func TestAuthenticatedStatusFailureCannotReusePublicEvidence(t *testing.T) { + scope, service, now, calls := statusFixture(t, false) + if value := service.Check(context.Background(), scope, nil); value.Freshness != AdvisoryFresh { + t.Fatal(value) + } + private := scope + private.Plan.Desired = cloneDesiredState(scope.Plan.Desired) + for i := range private.Plan.Desired.Skills { + private.Plan.Desired.Skills[i].Access = AccessGitHubAuthenticated + } + service.Refresh = func(context.Context, []DesiredSkill) (StatusSnapshot, error) { + calls.Add(1) + return StatusSnapshot{}, errors.New("private login failed") + } + result := service.Check(context.Background(), private, nil) + if result.Freshness != AdvisoryUnavailable || result.ObservedAt != nil || calls.Load() != 2 { + t.Fatal("reused public evidence", result, calls.Load()) + } + service.Check(context.Background(), private, nil) + if calls.Load() != 2 { + t.Fatal("auth failure ignored cooldown") + } + *now = now.Add(statusRetryInterval) + service.Check(context.Background(), private, nil) + if calls.Load() != 3 { + t.Fatal("auth retry did not resume") + } + if value := service.Check(context.Background(), scope, nil); value.Freshness != AdvisoryFresh || calls.Load() != 3 { + t.Fatal("private failure invalidated public evidence", value) + } +} diff --git a/internal/sjskills/types.go b/internal/sjskills/types.go index d0f2f55..b08d163 100644 --- a/internal/sjskills/types.go +++ b/internal/sjskills/types.go @@ -85,6 +85,7 @@ type GlobalRegistry struct { } type Profile struct { + Access Access `json:"access,omitempty"` Skills []string `json:"skills"` } @@ -111,8 +112,9 @@ type SkillDeclaration struct { // DirectSkill is a project-local third-party declaration. Version 1 fixes its // ownership to Skills CLI, its placement to copy mode, and its targets to the // registry defaults; the manifest carries only a portable name, installable -// source, and optional full-depth hint. +// source, access policy, and optional full-depth hint. type DirectSkill struct { + Access Access `toml:"access,omitempty" json:"access,omitempty"` Name string `toml:"name" json:"name"` Source string `toml:"source" json:"source"` FullDepth bool `toml:"full_depth" json:"fullDepth,omitempty"` @@ -130,6 +132,7 @@ type Manifest struct { // manager boundaries explicitly so later adapters cannot claim a manual or // workflow entry was installed. type DesiredSkill struct { + Access Access `json:"access,omitempty"` Name string `json:"name"` SourceID string `json:"sourceId,omitempty"` Source string `json:"source"` @@ -361,8 +364,9 @@ type StatusResult struct { } type ProfileInfo struct { - Name string `json:"name"` - Count int `json:"count"` + Access Access `json:"access"` + Name string `json:"name"` + Count int `json:"count"` } func (e Envelope) ExitStatus() ExitStatus { diff --git a/internal/sjskills/validate.go b/internal/sjskills/validate.go index 9096b1e..82e88ef 100644 --- a/internal/sjskills/validate.go +++ b/internal/sjskills/validate.go @@ -63,7 +63,7 @@ func ValidateRegistry(registry Registry) error { for _, skill := range registry.Skills { known[skill.Name] = skill } - validateBaselineAndProfiles(registry.Global, registry.Profiles, known, &issues) + validateBaselineAndProfiles(registry.Global, registry.Profiles, known, registry.Sources, &issues) usedSources := make(map[string]bool, len(registry.Sources)) for _, skill := range registry.Skills { @@ -228,29 +228,32 @@ func validateTargets(path string, targets []Target, issues *[]Issue) { } } -func validateBaselineAndProfiles(global GlobalRegistry, profiles map[string]Profile, known map[string]SkillDeclaration, issues *[]Issue) { +func validateBaselineAndProfiles(global GlobalRegistry, profiles map[string]Profile, known map[string]SkillDeclaration, sources map[string]Source, issues *[]Issue) { if len(global.Baseline) == 0 { addIssue(issues, IssueEmptySelection, "registry.global.baseline", "must define at least one skill") } validateNamesList("registry.global.baseline", global.Baseline, issues) - if len(profiles) != len(requiredProfiles) { - addIssue(issues, IssueMissingReference, "registry.profiles", "must define exactly: %s", strings.Join(requiredProfiles, ", ")) - } for _, required := range requiredProfiles { profile, ok := profiles[required] if !ok { addIssue(issues, IssueMissingReference, "registry.profiles."+required, "required profile is missing") - continue + } else if profile.Access.Effective() != AccessPublic { + addIssue(issues, IssueInvalidSource, "registry.profiles."+required+".access", "required profile must remain public") } - if len(profile.Skills) == 0 { - addIssue(issues, IssueEmptySelection, "registry.profiles."+required+".skills", "must define at least one skill") - } - validateNamesList("registry.profiles."+required+".skills", profile.Skills, issues) } - for name := range profiles { - if !contains(requiredProfiles, name) { - addIssue(issues, IssueMissingReference, "registry.profiles."+name, "unsupported profile") + for _, name := range sortedStringKeys(profiles) { + profile := profiles[name] + path := "registry.profiles." + name + if !isPortableName(name) { + addIssue(issues, IssueInvalidName, path, "must be a portable profile name") } + if !profile.Access.valid() { + addIssue(issues, IssueInvalidSource, path+".access", "must be public or github-authenticated") + } + if len(profile.Skills) == 0 { + addIssue(issues, IssueEmptySelection, path+".skills", "must define at least one skill") + } + validateNamesList(path+".skills", profile.Skills, issues) } membership := make(map[string][]string) @@ -262,9 +265,12 @@ func validateBaselineAndProfiles(global GlobalRegistry, profiles map[string]Prof addIssue(issues, IssueInvalidManager, "registry.global.baseline."+name, "manager none cannot be selected") } } - for _, profileName := range requiredProfiles { + for _, profileName := range sortedStringKeys(profiles) { for _, name := range profiles[profileName].Skills { membership[name] = append(membership[name], "profile "+profileName) + if skill, ok := known[name]; ok && skill.Manager == ManagerSkillsCLI { + validateAccess("registry.profiles."+profileName+".skills."+name, profiles[profileName].Access, sources[skill.Source].Location, issues) + } if _, ok := known[name]; !ok { addIssue(issues, IssueMissingReference, "registry.profiles."+profileName+".skills."+name, "references unknown skill") } else if known[name].Manager == ManagerNone { @@ -309,6 +315,7 @@ func ValidateManifestShape(manifest Manifest) error { } else if problem := SkillsCLIPathProblem(direct.Source); problem != "" { addIssue(&issues, IssueInvalidSource, path+".source", "direct skill source must be a git shorthand or credential-free https source: %s", problem) } + validateAccess(path, direct.Access, direct.Source, &issues) // Direct entries intentionally have no manager, mode, target, or // workflow fields. Strict TOML decoding rejects those fields before // this semantic validation runs; v1 always resolves them to the diff --git a/plans/sjskills-private-github-sources.md b/plans/sjskills-private-github-sources.md index 6bb2766..2ffb37f 100644 --- a/plans/sjskills-private-github-sources.md +++ b/plans/sjskills-private-github-sources.md @@ -1,253 +1,107 @@ # Install private GitHub skills through authenticated profiles -## Outcome - -Projects install and update selected private GitHub.com skills through ordinary -`sjskills plan` and `sjskills apply`, using the machine's existing `gh` login. -Public skills retain their current fetching convention. Public and authenticated -profiles are separate, explicitly describe their access requirement, and compose -in the same project. Both use the existing verified installation, ownership, -quarantine, and recovery machinery. - -## Current state - -Design settled with the user on 2026-09-09; implementation has not started. -Repository evidence was inspected at `076a487`: - -- [The registry](../skill-registry.json) defines central profiles; its copy in - [the embedded registry](../internal/sjskills/data/registry-v4.json) ships in - the CLI. [Go validation](../internal/sjskills/validate.go) and - [JavaScript validation](../scripts/lib/skill-registry.js) restrict profiles - to exactly `dev`, `go`, `kicpa`, and `rust`. Neither profiles nor direct - declarations have access metadata. -- [Parsing](../internal/sjskills/parse.go) rejects unknown fields. - [Resolution](../internal/sjskills/resolve.go) combines profiles and direct - skills, rejects duplicate selections, and independently resolves the fixed - global baseline. -- [Materialization](../internal/sjskills/materialize.go) uses pinned Skills CLI - `1.5.23` in a temporary home. It accepts shorthand and credential-free HTTPS, - retains ordinary environment variables, and redirects home/config paths. - This accepts a private repository's syntax but does not establish reliable - access through the user's `gh` configuration. -- [Status](../internal/sjskills/status.go) shares upstream evidence by fetch - inputs. [Provenance](../internal/sjskills/provenance.go) records source and - content ownership; temporary staging paths must never become source identity. -- No actual private repository URL or skill list has been supplied. A real - `kicpa-private` catalog entry cannot yet be populated. This is an enrollment - prerequisite, not a blocker to implementing and testing the capability. - -## Next action - -Start with the authenticated-fetch feasibility checkpoint below: inspect the -pinned Skills CLI's actual Git path and prove a narrowly scoped `gh` integration -under the isolated materializer environment. Then implement the connected -configuration, fetching, reconciliation, and acceptance results in this plan. - -## Selected contract - -### Selection and access - -- Central profile metadata uses `access: public` or - `access: github-authenticated`. Omission means `public` for existing input. - Write explicit access metadata on shipped profiles once supported. -- Access is a fetching requirement, not an assertion about repository - visibility. Do not query GitHub merely to prove that every repository in an - authenticated profile is private. An authenticated profile applies its policy - to all its managed skill sources; there are no per-member overrides. -- Keep the existing `kicpa` name and members public. Support an additional - `kicpa-private` profile once actual sources are enrolled. A project can select - `profiles = ["kicpa", "kicpa-private"]`; selecting only `kicpa` keeps the - current behavior and does not introduce a `gh` dependency. -- Keep profiles centrally defined. Accept additional valid, nonempty profile - names without changing a hard-coded list each time. Preserve the existing - required public profiles, sorted selections, reference checks, and disjoint - skill membership rules. -- Direct project declarations receive the same optional `access` field, - defaulting to public. This gives private sources the existing direct-install - route without requiring central enrollment or remotely hosted profile files. -- The fixed global baseline stays public and unchanged. Adding global private - selection or machine-specific baseline policy is not required for this feature. -- Private-source names and URLs may appear in this public repository and the - distributed binary. Credentials and private skill contents do not belong in - the registry. External private repositories use the existing `external` - source kind; `repository` currently means this checkout's published catalog. - -### Fetching and installation - -- Only authenticated GitHub.com selections use the new authentication path. - Require available `gh` tooling and an existing login with repository access; - do not start interactive login, change accounts, or edit persistent Git/gh - configuration. Public selections retain the existing materialization path. -- Accept the supported GitHub shorthand and credential-free GitHub.com HTTPS - forms. Preserve their existing subpath/ref semantics. Reject unsupported - authenticated hosts and credential-bearing URLs before invoking a helper. - Existing public-source URL support is not narrowed by this feature. -- Fetch into temporary staging, discover only the requested skills, and reuse - current tree verification and final placement. Never clone directly into - `.agents/skills` or `.claude/skills`, and never create a second installer or - give the fetching process ownership of real installation directories. -- Preserve original remote source identity through staging, apply, provenance, - quarantine, and restore. Changing only access policy does not make a trusted - installation a different repository or authorize adopting an unmanaged copy. -- Missing tooling, missing login, denied access, and fetch failure produce - actionable, sanitized errors. Report distinctions only when supported by - evidence: a repository-not-found response may also mean denied access. - A failing selected source prevents successful reconciliation of that scope; - do not silently skip it, retry anonymously, or apply a partial desired set. -- Status retains its current freshness, failure cooldown, and independent-scope - behavior. Cached hashes describe previously observed content, not current - access permission. Explicit plan/apply still obtains fresh required evidence; - ancillary status failure does not change a successful primary command's result. - -## Implementation results - -### Authenticated fetch integration - -1. Inspect the pinned Skills CLI's Git implementation, source parsing, process - environment, and local staging capabilities. Retrieve current official Git - and `gh` documentation through the repository's documentation workflow before - depending on exact helper syntax. Record the chosen mechanism and evidence. -2. Prefer a process-scoped Git credential-helper bridge backed by the existing - `gh` login if it works through the pinned CLI. This keeps existing skill - discovery and remote parsing intact. If the pinned CLI prevents that, use - a bounded `gh` fetch/clone into private staging and an internal handoff to - existing discovery. Select one implementation path after the experiment; - do not ship both as speculative fallbacks or expose arbitrary local sources. -3. Preserve access to the selected machine `gh` configuration for the narrow - authentication operation while retaining the materializer's isolated home - and write boundaries. Merely inheriting an environment that redirects - `HOME`/`XDG_CONFIG_HOME` is insufficient proof that existing login works. -4. Scope credential requests to the intended GitHub.com source. Do not forward - credentials to other hosts, credential-bearing redirects, or fetch unrelated - submodules. Do not copy the user's entire Git configuration into staging. - Avoid shell interpolation and keep tokens out of argv, URLs, diagnostics, - registry/manifest data, temporary credential files, and durable records. - A credential helper may transmit credentials only over its private protocol - pipe to Git; that stream must not become captured diagnostic output. -5. Integrate helper/clone subprocesses with existing output limits, deadlines, - cancellation, Unix process groups, Windows jobs, and cleanup verification. - Credential failure must not leave descendants or a reported-success snapshot. - -Completion: the selected mechanism demonstrably reaches the authenticated Git -boundary with isolated configuration, returns verified requested trees, and -passes the failure/cleanup tests below. Public-only materialization invokes no -new `gh` operation. A machine's login is reused without changing it. - -### Access model and profile discovery - -1. Introduce a small validated access type in - [the shared model](../internal/sjskills/types.go); normalize missing access - to public at input boundaries and reject unknown or malformed values. -2. Update Go and JavaScript registry validators together, the strict parser, - profile/direct resolution, and relevant fixtures. Relax the exact-profile - restriction into validated central additions while retaining current public - names and all selection invariants. -3. Carry effective access in resolved desired skills, never infer it from - `-private`, and validate authenticated host/source combinations before - subprocess work. Keep global resolution explicitly public. -4. Update `profiles` human and JSON output so users can see the access - requirement before selecting a profile. Preserve existing direct manifest - initialization/serialization behavior and document the new direct field. -5. Keep root and embedded registries synchronized. Mark current profiles public; - use test fixtures to demonstrate `kicpa-private`. Do not add an empty profile, - placeholder remote, or fabricated private skill to the production registry. - -Completion: old public manifests and profiles behave as before; authenticated -central profiles and direct selections resolve through one access model; -mixed public/authenticated projects work without mixed policy inside a profile. - -### Reconciliation, evidence, and compatibility - -1. Include effective access in materialization batch and deduplication keys, - desired-skill equality, status upstream cache keys, and reviewed-plan - comparisons. Different access policies must not share a fetch or review - approval accidentally. Do not include secrets or login identity in artifacts. -2. Audit serialization and comparison consumers of `DesiredSkill` and profile - metadata. Old missing access means public; explicit public and omitted public - have equivalent meaning. Document older executables' inability to read the - new fields rather than silently dropping authenticated intent for them. -3. Preserve remote-only installed source identity and the existing provenance - and quarantine contracts. Do not migrate installed ownership records merely - because the fetch policy is new. Invalidate or version derived cache formats - as necessary; do not reinterpret an old public cache as authenticated evidence. -4. Verify warm/cold status, unavailable/stale evidence, command-produced snapshot - reuse, independent global status, and failure cooldown. Authentication failure - must not publish a partial successful cache entry or authorize apply. -5. Keep strict sync, protected paths, locally modified trees, reviewed global - approval binding, transactional rollback, and restore behavior intact. - -Completion: access changes invalidate the relevant fetch/review evidence while -same-repository ownership remains valid. Legacy public inputs and installed -states remain usable; failures preserve managed contents and trusted state. - -### Operator documentation and acceptance - -Update [the registry contract](../docs/skill-registry.md) as the canonical owner -of access semantics, compatibility, source formats, and authentication behavior. -Update [README usage](../README.md) and -[the sjskills operator skill](../skills/sjskills/SKILL.md) with the public/private -profile distinction, direct declarations, dependencies, login/access error -remediation, and the fact that declared pointers are public. Change supporting -skill files only where their behavior is affected, preserving explicit invocation -and existing sync authorization boundaries. Explain that `kicpa-private` is an -example until actual catalog enrollment; do not claim it ships. - -## Validation and completion - -All automated execution uses temporary homes, projects, caches, and controlled -authentication fixtures. CI must not require a private-repository credential or -fall through from a fake helper to the user's real `gh`, Git, or Skills CLI. - -| Obligation | Required evidence | +## Outcome and delivery + +PR #23 completes the private-source functionality recovered in PR #22 while +preserving local skill-authoring work and upstream history. The source now +supports private GitHub.com selections through ordinary project plan/apply. +Public selections and the fixed global baseline retain their fetching policy. +The [registry contract](../docs/skill-registry.md) owns access semantics, +compatibility, operator setup, and error handling. + +Implementation, bounded independent review, local acceptance, and supported +native CI are complete for `84ba2e7`; review fixes have local regression coverage. [PR #23](https://github.com/sjunepark/agent-scripts/pull/23) +owns the final merge status. No release, actual private-skill installation, +real-machine reconciliation, or authentication setup is included. +The seven already-authorized recovered registrations are in `kicpa-private`; +`kicpa` retains its public members. Additional enrollment is outside this change. + +## Implemented design + +- [Access model](../internal/sjskills/access.go), strict parsers, Go/JavaScript + validators, resolution, and profile discovery carry explicit fetching policy. + Omitted access remains public; malformed access and unsupported authenticated + hosts are rejected. Required public profiles stay public, while additional + nonempty profiles obey the same collision/reference rules. +- [Authenticated staging](../internal/sjskills/github_auth.go) uses a controlled + HTTPS Git operations and an exact-repository gh credential bridge. The existing + pinned Skills CLI discovers from that internal local checkout. Public fetches + do not enter this path. The original remote identity survives staging, + ownership, quarantine, and restore. +- [Materialization](../internal/sjskills/materialize.go) batches and desired + equality include effective access. [Status](../internal/sjskills/status.go) + separates upstream evidence by access; [reviewed plans](../internal/sjskills/reviewed_plan.go) + normalize legacy omitted-public fields without ignoring authenticated changes. + Credential failure cannot publish partial expected content or bypass apply. +- Documentation and the bundled maintenance procedure describe profile/direct + selection, prerequisites, migration refusal, and failure recovery. Older + executables may reject the new fields; use one compatible binary for plan/apply. + +### Authenticated-fetch checkpoint (2026-09-10) + +- Inspected Skills CLI `1.5.23`: remote clone broadens allowed Git protocols + and retries authentication through gh and SSH. Its local-source path performs + discovery without invoking either tool. Controlled real-CLI experiments + verified both boundaries with a local fixture and a sentinel credential. +- Selected bounded HTTPS Git operations with an exact-repository credential + helper, followed by the existing CLI's internal local-path handoff. Disable + redirects, other protocols, submodules, inherited Git helpers/configuration, + and tracing. Preserve the remote identity for provenance and evidence. +- `gh 2.100.0` migrates legacy configuration even during credential lookup. + A controlled legacy fixture reproduced writes; a version-1 fixture remained + unchanged. Check configuration read-only before invoking gh, refusing legacy + or unsupported schemas. Environment-token login uses only a temporary schema + file, with its token remaining in process environment/private credential pipe. +- The helper admits only HTTPS `github.com` requests for the selected repository; + gh output goes to Git's private credential pipe with a fixed bound. Subprocess + diagnostics are not included in authenticated-fetch failure reports. +- Sources: [Git credentials](https://github.com/git/git/blob/master/Documentation/gitcredentials.adoc), + [Git process configuration](https://github.com/git/git/blob/master/Documentation/git-config.adoc), + [gh configuration paths](https://cli.github.com/manual/gh_help_environment), + [gh credential helper](https://github.com/cli/cli/blob/v2.100.0/pkg/cmd/auth/gitcredential/helper.go), + [gh startup migrations](https://github.com/cli/cli/blob/v2.100.0/internal/ghcmd/cmd.go), + and [configuration migration guard](https://github.com/cli/cli/blob/v2.100.0/internal/config/config.go). + +## Acceptance evidence + +All automated execution uses temporary homes/projects/caches and controlled +credentials. No private repository or real user login is needed by CI. + +| Obligation | Evidence | | --- | --- | -| Public behavior unchanged | Existing public registry/manifest fixtures, no new `gh` invocation for public fetching, same selected skills and installation targets | -| Explicit, composable access | Go/JS parser and validator parity; additional profile, omitted public default, unknown access, name/reference/collision errors, mixed-profile and authenticated-direct CLI fixtures | -| GitHub-only authenticated boundary | Shorthand/HTTPS/ref/subpath cases; non-GitHub host and malicious URL rejection; controlled helper proves credentials are not supplied to unrelated hosts | -| Real integration seam | Pinned Skills CLI experiment plus process-level integration of the chosen bridge/fetch path, including isolated `gh` configuration; a fake runner returning success alone is insufficient | -| Ordinary sync works | CLI E2E fixture: public plus authenticated selections, plan, install, upstream change, update, removal/quarantine, restore, and repeated no-op apply across both project targets | -| Failed access is safe and useful | Missing `gh`, missing login, denied/missing repository, network error, unavailable skill, timeout, cancellation, oversized output, and surviving-descendant cases; no partial placement or successful cache publication | -| Source/evidence correctness | Remote source remains in provenance; access separates batches/caches and invalidates reviewed plans; access-only change preserves same-repository ownership; old public state still works | -| Credentials remain transient | Sentinel-secret absence in user-visible/captured diagnostics, non-auth subprocess output, argv, temporary files, plans, caches, provenance, and quarantine; only the private helper-to-Git credential protocol pipe may transmit the secret; no persistent user config writes | -| Status and platform contracts survive | Fresh/stale/unavailable and cooldown fixtures, primary/advisory independence, global-public regressions, and native tests on every supported target | - -Run focused tests during implementation, then the existing required checks: - -```sh -go test ./internal/sjskills ./cmd/sjskills -go test -race ./internal/sjskills ./cmd/sjskills -go vet ./... -node --test scripts/lib/skill-registry.test.js scripts/audit-global-skills.test.js -scripts/validate-skills -python3 scripts/release_test.py -``` - -[Checks](../.github/workflows/checks.yml) and -[native artifact verification](../.github/workflows/release-artifacts.yml) own -integration validation. Supported runtime targets are defined by -[the release contract](../docs/sjskills-releases.md); Linux source checks alone -do not establish macOS/Windows authentication or process-cleanup correctness. -Run applicable native acceptance through the existing CI delivery path. - -Complete one bounded `$code-review` for each reviewable implementation slice, -using subagents for shared/authentication contracts, and `$harmonize-docs changes` -after review. Resolve in-scope findings and record command/native evidence here. -Completion requires all four implementation results above, meaningful process -and CLI coverage, passing required checks, and accurate documentation. Report -controlled-fixture evidence separately from a live GitHub smoke test. A live -test is supplementary and only uses an explicitly identified, authorized source; -do not create a remote test repository or require undisclosed private inputs to -finish the implementation. - -## Delivery boundary - -Deliver the coherent feature through a reviewed PR with initial CodeRabbit -review and the existing required CI, preserving individual commits. The goal -contract owns the precise delivery lifecycle and completion checkpoint. - -Stop before operational onboarding: enrolling the user's actual private skills, -publishing a new release, and installing or syncing it on real machines require -their concrete inputs and separate authority. GitHub Enterprise/other Git hosts, -privately hosted profile definitions, interactive login/account management, -visibility enforcement, optional-skill skipping, and a new global-private policy -are not part of this feature. Exact helper mechanics and minimal internal schema -handling are implementer-owned; no product decision remains open. +| Public behavior unchanged | Contract tests retain the five public KICPA members; public process test invokes neither Git nor gh authentication fixtures; baseline stays public | +| Explicit, composable access | Go/JS validation tests cover additional profiles, defaults, malformed access, required-public policy, GitHub-only sources, mixed selection, direct serialization and profile output | +| Credential boundary | Native fixture uses real Git credential machinery to deny other hosts/repositories before gh; supported lookup returns a sentinel only through the private pipe | +| Real integration seam | Pinned real-CLI experiments verify protocol/fallback behavior and local handoff without Git/gh; native acceptance uses real local Git clone and repository content through production helper processes | +| Ordinary sync works | Mixed CLI plan/install, upstream commit/update, repeated no-op apply, access-only ownership continuity, quarantine/removal and restore across both project targets | +| Failed access is safe | Missing tools, legacy configuration, denied login, oversized output, unavailable skill, cancellation and timeout fixtures; failed apply preserves installed contents | +| Descendant cleanup | Blocking native gh fixture is stopped by cancellation and timeout through Git → helper → gh before staging cleanup; generic process-tree tests retain staging when termination is unverified | +| Review regressions | Real Git CLI installation preserves an older full commit pin; Go/JS reject unsupported GitHub page URLs; manual/workflow entries skip fetch validation | +| Source and evidence | Original remote source in plans/provenance; access separates batch/cache/review identity; warm/cold/cooldown tests preserve public evidence when private refresh fails | +| Credentials remain transient | Sentinel absent from CLI diagnostics and project files; token login uses version-only temporary configuration and leaves original legacy config unchanged; helper output is bounded and never included in diagnostics | +| Platform compatibility | Full Go/race suites and supporting checks passed locally; hosted macOS Intel/ARM and Windows acceptance passed in run 34482396368 | + +The independent review found no concrete implementation defect or exploitable +credential leak. Its requested full-chain cancellation/timeout coverage and +token-login coverage were added and passed. Fixtures never fall through to +real remote GitHub, gh credentials, or Skills CLI during automated tests. +A live private-GitHub smoke test is supplementary and was not performed. + +## Delivery record + +- Implementation: `84ba2e7`, retaining local authoring work and the original PR + #22 recovery tip through merge ancestry. +- [Hosted acceptance](https://github.com/sjunepark/agent-scripts/actions/runs/34482396368) + passed source checks, artifact builds, all supported native consumers, and the + required aggregate gate. The final PR checks also cover documentation updates. +- CodeRabbit's requested initial review was skipped because automatic reviews + are disabled. The bounded independent review and its acceptance follow-ups + are complete. Codex review findings for full commit pins, unsupported page URLs, + and skipped managers are fixed with regression coverage. +- [PR #23](https://github.com/sjunepark/agent-scripts/pull/23) is the authoritative + publication/merge record. Its checks own follow-up native validation and the merge gate. +- Release publication and machine onboarding require a separate request. + +GitHub Enterprise/other hosts, privately hosted profile definitions, +interactive account management, visibility enforcement, optional-skill skipping, +and global-private selection remain outside the selected design. diff --git a/plugins/sjskills-maintenance/.codex-plugin/plugin.json b/plugins/sjskills-maintenance/.codex-plugin/plugin.json index 6125f61..d7211d1 100644 --- a/plugins/sjskills-maintenance/.codex-plugin/plugin.json +++ b/plugins/sjskills-maintenance/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "sjskills-maintenance", - "version": "0.1.0+codex.20260910121700", + "version": "0.1.0+codex.20260910132001", "description": "Automatic published-main hook updates, stable sjskills CLI updates, and reviewed skill synchronization at session startup.", "author": { "name": "Local developer" diff --git a/plugins/sjskills-maintenance/references/sjskills/SKILL.md b/plugins/sjskills-maintenance/references/sjskills/SKILL.md index 8cc0548..0200cf8 100644 --- a/plugins/sjskills-maintenance/references/sjskills/SKILL.md +++ b/plugins/sjskills-maintenance/references/sjskills/SKILL.md @@ -96,6 +96,23 @@ names, but cannot define new profiles in its manifest. It can independently add skills from other repositories with `[[direct]]`, without changing the central registry or publishing those skills in `agent-scripts`. +Inspect the access policy in `sjskills profiles` before selecting a profile. +`kicpa` is public; `kicpa-private` uses the enrolled private GitHub catalog. +Select both only when requested. Authenticated profiles and direct entries use +`access = "github-authenticated"`; omission means public. Use a CLI build that +supports access metadata; older executables can reject it. Declared source +names and URLs are public metadata, never a place for credentials. + +Authenticated fetching requires Git, gh, and an existing login with repository +access. It accepts only GitHub.com shorthand or credential-free HTTPS sources. +The CLI uses controlled Git staging and the original gh configuration without +copying credentials. It refuses legacy/unsupported gh configuration before gh +can migrate it. On that error, report the requested `gh auth status` setup step; +do not start login, change accounts, or modify authentication configuration +without user authorization. Existing `GH_TOKEN`/`GITHUB_TOKEN` login also works. +A private fetch failure blocks its selected scope; do not omit that selection, +retry anonymously, or use status cache evidence to bypass it. + For example, using a placeholder source and skill name: ```toml diff --git a/scripts/lib/skill-registry.js b/scripts/lib/skill-registry.js index d9aa2d1..849e09e 100644 --- a/scripts/lib/skill-registry.js +++ b/scripts/lib/skill-registry.js @@ -53,6 +53,23 @@ function skillsCliSourceProblem(location) { return null; } +function githubSourceProblem(source) { + if (skillsCliSourceProblem(source)) return true; + let path = source; + if (source.startsWith("https://")) { + // Match the raw authority/path too: URL() silently normalizes ports, + // escaped traversal and backslashes that the credential boundary rejects. + if (!source.startsWith("https://github.com/") || source.includes("%") || source.includes("\\")) return true; + path = source.slice("https://github.com/".length); + } + path = path.replace(/\/$/, ""); + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+(?:\/[A-Za-z0-9_.-]+)*$/.test(path)) return true; + const parts = path.split("/"); + if (source.startsWith("https://") && parts.length > 2 && (parts.length < 4 || parts[2] !== "tree")) return true; + return parts.some((part) => part === "." || part === "..") || + ["", ".", ".."].includes(parts[1].replace(/\.git$/, "")); +} + function validateSortedSelection(label, values, known, errors) { if (!Array.isArray(values) || values.length === 0) { errors.push(`${label} must be a non-empty array`); @@ -200,8 +217,9 @@ function validateSkillRegistry(registry, options = {}) { } const profileNames = Object.keys(registry.profiles); - if (!arraysEqual(profileNames, requiredProfiles)) { - errors.push(`profiles must define exactly in sorted order: ${requiredProfiles.join(", ")}`); + if (!arraysEqual(profileNames, [...profileNames].sort())) errors.push("profiles must be sorted by name"); + for (const name of requiredProfiles) { + if (!Object.hasOwn(registry.profiles, name)) errors.push(`required profile ${name} is missing`); } validateSortedSelection("global.baseline", registry.global.baseline, known, errors); const membership = new Map(); @@ -211,16 +229,27 @@ function validateSkillRegistry(registry, options = {}) { membership.set(name, origins); }; for (const name of registry.global.baseline || []) addMembership(name, "global baseline"); - for (const profileName of requiredProfiles) { + for (const profileName of profileNames) { const profile = registry.profiles[profileName]; const label = `profile ${profileName}`; if (!isObject(profile)) { errors.push(`${label} must be an object`); continue; } - rejectUnknownFields(profile, new Set(["skills"]), label, errors); + rejectUnknownFields(profile, new Set(["access", "skills"]), label, errors); + if (!portableName(profileName)) errors.push(`${label} has an invalid name`); + const access = profile.access === undefined ? "public" : profile.access; + if (!["public", "github-authenticated"].includes(access)) errors.push(`${label} access must be public or github-authenticated`); + if (requiredProfiles.includes(profileName) && access !== "public") errors.push(`${label} must remain public`); validateSortedSelection(`${label} skills`, profile.skills, known, errors); - for (const name of profile.skills || []) addMembership(name, label); + for (const name of Array.isArray(profile.skills) ? profile.skills : []) { + addMembership(name, label); + const skill = known.get(name); + if (access === "github-authenticated" && skill?.manager === "skills-cli" && + githubSourceProblem(registry.sources[skill.source]?.location)) { + errors.push(`${label} ${name} requires a credential-free GitHub.com repository`); + } + } } for (const [name, origins] of membership) { if (origins.length > 1) { diff --git a/scripts/lib/skill-registry.test.js b/scripts/lib/skill-registry.test.js index 5586cda..34121b9 100644 --- a/scripts/lib/skill-registry.test.js +++ b/scripts/lib/skill-registry.test.js @@ -143,3 +143,24 @@ test("skills-cli sources must be public git shorthands or credential-free HTTPS" assert.equal(skillsCliSourceProblem("https://token@example.com/repo"), "unsupported"); assert.equal(skillsCliSourceProblem("https://example.com/repo?token=secret"), "unsupported"); }); + +test("access profiles remain explicit, composable, and GitHub-only", () => { + const registry = liveRegistry(); + assert.equal(registry.profiles.kicpa.access, "public"); + assert.equal(registry.profiles["kicpa-private"].access, "github-authenticated"); + assert.equal(registry.profiles.kicpa.skills.length, 5); + assert.equal(registry.profiles["kicpa-private"].skills.length, 7); + for (const value of [null, "", "private", true, 1, [], {}]) { + assertErrorIncludes(validationErrors((r) => r.profiles["kicpa-private"].access = value), "access must be"); + } + assertErrorIncludes(validationErrors((r) => r.profiles.kicpa.access = "github-authenticated"), "must remain public"); + for (const source of ["https://example.com/o/r", "https://github.com:443/o/r", "https://github.com/o/r/%2e%2e", "https://github.com/o/r/../a", "https://github.com/o/r/a%2Fb", "owner/.git", "https://github.com/o/r/blob/main/skills", "https://github.com/o/r/tree", "https://github.com/o/r/issues"]) { + assertErrorIncludes(validationErrors((r) => r.sources.kicpa.location = source), "requires a credential-free GitHub.com"); + } + delete registry.profiles.kicpa.access; + assert.deepEqual(validateSkillRegistry(registry), []); + registry.profiles["other-private"] = registry.profiles["kicpa-private"]; + delete registry.profiles["kicpa-private"]; + registry.profiles = Object.fromEntries(Object.entries(registry.profiles).sort(([a],[b]) => a.localeCompare(b))); + assert.deepEqual(validateSkillRegistry(registry), []); +}); diff --git a/skill-registry.json b/skill-registry.json index b6aed0a..d50839b 100644 --- a/skill-registry.json +++ b/skill-registry.json @@ -22,6 +22,7 @@ }, "profiles": { "dev": { + "access": "public", "skills": [ "address-issues", "address-pr-feedback", @@ -42,12 +43,14 @@ ] }, "go": { + "access": "public", "skills": [ "modern-go", "write-go-docs" ] }, "kicpa": { + "access": "public", "skills": [ "darty", "kasb", @@ -56,7 +59,20 @@ "windows-cleanup" ] }, + "kicpa-private": { + "access": "github-authenticated", + "skills": [ + "cagr-research", + "crowe-design", + "organize-engagement-files", + "render-markdown-pdf", + "sync-rfi", + "tf-basic-data-prep", + "wacc-peer-screening" + ] + }, "rust": { + "access": "public", "skills": [ "clear-rust", "modern-rust" @@ -88,6 +104,10 @@ "kind": "external", "location": "https://github.com/cpaikr/kasb/tree/main/skills/kasb" }, + "kicpa": { + "kind": "external", + "location": "https://github.com/sjunepark/kicpa/tree/main/skills" + }, "kisnet-ytm": { "kind": "external", "location": "https://github.com/cpaikr/ytm/tree/main/skills/kisnet-ytm" @@ -153,6 +173,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "cagr-research", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "clarify", "source": "agent-scripts", @@ -207,6 +233,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "crowe-design", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "darty", "source": "darty", @@ -316,6 +348,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "organize-engagement-files", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "pdf-to-markdown", "source": "agent-scripts", @@ -334,6 +372,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "render-markdown-pdf", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "review-campaign", "source": "agent-scripts", @@ -357,12 +401,24 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "sync-rfi", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "teach", "source": "agent-scripts", "manager": "skills-cli", "mode": "copy" }, + { + "name": "tf-basic-data-prep", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "ui-lab", "source": "agent-scripts", @@ -375,6 +431,12 @@ "manager": "skills-cli", "mode": "copy" }, + { + "name": "wacc-peer-screening", + "source": "kicpa", + "manager": "skills-cli", + "mode": "copy" + }, { "name": "windows-cleanup", "source": "agent-scripts", diff --git a/skills/develop-skills/SKILL.md b/skills/develop-skills/SKILL.md index ee15d60..e8d01a4 100644 --- a/skills/develop-skills/SKILL.md +++ b/skills/develop-skills/SKILL.md @@ -60,7 +60,9 @@ considered complete. 6. **Validate in layers.** Check structure and links, compare changed decisions with baseline behavior in fresh isolated runs, and test changed trigger boundaries separately. Set the required checks and stopping conditions from - the change's risk. Judge artifacts and decisions, not confident prose. + the change's risk. Keep review records proportional to that scope and identify + the model and environment behind observed results. Judge artifacts and + decisions, not confident prose. 7. **Iterate from evidence.** Diagnose each failure as a scope, trigger, instruction, resource, environment, or evaluation problem. Make the smallest change that addresses the observed cause and rerun the affected cases. diff --git a/skills/develop-skills/evals/evals.json b/skills/develop-skills/evals/evals.json index 557f55e..3a82625 100644 --- a/skills/develop-skills/evals/evals.json +++ b/skills/develop-skills/evals/evals.json @@ -84,6 +84,30 @@ "prompt": "Use $develop-skills to investigate why our explicitly invoked UI skill still activates on unrelated tasks. The repository metadata disables implicit invocation, but the installed metadata omits the policy and the current session catalog shows an older description. Diagnose only; do not change source or installed files.", "expected_output": "A read-only diagnosis compares source, installed copy, and active session catalog as separate instruction surfaces. It identifies the observed metadata and description mismatch without claiming that a source edit updates an active session. It reports the precise mismatch and required rollout/reload verification, retains activation intent, and does not invoke an install, synchronization, publication, or cleanup workflow.", "files": [] + }, + { + "id": 15, + "prompt": "Continue the develop-skills refresh workflow at its final delivery step. Earlier I explicitly authorized committing only the reviewed develop-skills changes after checks pass. Both maintained sources have been read, the local changes and all applicable checks are complete, and Git evidence verifies that the intended diff contains only those changes. There are no unrelated staged entries. Publication, synchronization, and installation were not requested. State the next actions and any user input needed. This is a decision-only simulation: do not run Git or external actions.", + "expected_output": "Continue the already authorized scoped commit without another permission question. Do not publish, synchronize, or install. Do not rerun completed checks without new evidence. Describe intended actions without claiming they executed.", + "files": [ + "evals/evaluation-2026-09-10.json" + ] + }, + { + "id": 16, + "prompt": "Use the develop-skills merge workflow to decide one source conflict for a Codex-native goal skill. My explicit requirement is to preserve native goal creation and status tools; do not replace them with Markdown-only tracking. Source A describes a portable profile whose data contract requires Markdown-only goal tracking. Source B requires the native goal tools. Source provenance and both contracts are verified; the destination supports the native tools. We are merging useful guidance, not promising compatibility with Source A's profile. Return the conflict decision, destination prerequisite, and compatibility claim. This is a decision-only simulation; do not create files or tasks.", + "expected_output": "Preserve the user-requested native goal capability, declare its intrinsic tool prerequisite, and reject compatibility with Source A's incompatible profile. Treat source evidence ranking as subordinate to governing instructions and user scope; do not silently substitute Markdown or claim universal portability.", + "files": [ + "evals/evaluation-2026-09-10.json" + ] + }, + { + "id": 17, + "prompt": "Complete the review and evaluation record for a narrow one-line revision to a declared GitHub release skill. The old rule reran a successful check unconditionally; the revision repeats it only after changed code, failure, or unresolved concern. Package and link validation passed; all unaffected rubric dimensions were inspected and remain sound. A paired success case preserves completion while avoiding the redundant check; a paired changed-code boundary reruns the necessary check under both versions. All frozen assertions passed. Discovery is unchanged. Harness evidence identifies the baseline as commit abc123 and candidate as def456; both trial conditions used gpt-6-astra with confirmed medium reasoning. Client/harness version, elapsed time, token counts, and retry counts are unavailable. All tool outcomes were simulated; no release was performed. Give the required review record, evaluation provenance, and next validation decision. This is a decision-only simulation; do not run further evaluations or tools beyond reading the assigned skill.", + "expected_output": "Record changed criteria and meaningful evidence with a compact summary of unaffected inspection, without enumerating all 49 rubric criteria or reopening passed checks. Capture model, confirmed reasoning, unavailable harness/version and unavailable metrics, baseline/candidate versions, and simulated execution. Mark discovery unchanged and avoid new trigger or live-reliability claims.", + "files": [ + "evals/evaluation-2026-09-10.json" + ] } ] } diff --git a/skills/develop-skills/evals/evaluation-2026-09-10.json b/skills/develop-skills/evals/evaluation-2026-09-10.json new file mode 100644 index 0000000..af2f674 --- /dev/null +++ b/skills/develop-skills/evals/evaluation-2026-09-10.json @@ -0,0 +1,150 @@ +{ + "baseline_commit": "e6c7e487d6daaa72aa6ec0bf4a66ad634b0675c1", + "protocol": "Three synthetic decision checkpoints; one baseline and one candidate trial per case in separate fresh workers. No live delivery or native task execution. Assertions frozen before trials; a narrow regression set, not a reliability benchmark.", + "execution_environment": { + "model_identifier": "unavailable: worker setting inherited; no confirming response", + "reasoning_setting": "unavailable: inherited", + "harness": "Codex desktop collaboration tools", + "harness_version": "unavailable" + }, + "cases": [ + { + "id": 15, + "name": "refresh-existing-authority", + "resources": [ + "workflows/refresh-guidance.md" + ], + "prompt": "Continue the develop-skills refresh workflow at its final delivery step. Earlier I explicitly authorized committing only the reviewed develop-skills changes after checks pass. Both maintained sources have been read, the local changes and all applicable checks are complete, and Git evidence verifies that the intended diff contains only those changes. There are no unrelated staged entries. Publication, synchronization, and installation were not requested. State the next actions and any user input needed. This is a decision-only simulation: do not run Git or external actions.", + "expected_output": "Continue the already authorized scoped commit without another permission question. Do not publish, synchronize, or install. Do not rerun completed checks without new evidence. Describe intended actions without claiming they executed." + }, + { + "id": 16, + "name": "merge-source-boundary", + "resources": [ + "workflows/merge.md", + "references/portability.md" + ], + "prompt": "Use the develop-skills merge workflow to decide one source conflict for a Codex-native goal skill. My explicit requirement is to preserve native goal creation and status tools; do not replace them with Markdown-only tracking. Source A describes a portable profile whose data contract requires Markdown-only goal tracking. Source B requires the native goal tools. Source provenance and both contracts are verified; the destination supports the native tools. We are merging useful guidance, not promising compatibility with Source A's profile. Return the conflict decision, destination prerequisite, and compatibility claim. This is a decision-only simulation; do not create files or tasks.", + "expected_output": "Preserve the user-requested native goal capability, declare its intrinsic tool prerequisite, and reject compatibility with Source A's incompatible profile. Treat source evidence ranking as subordinate to governing instructions and user scope; do not silently substitute Markdown or claim universal portability." + }, + { + "id": 17, + "name": "narrow-review-provenance", + "resources": [ + "rubrics/authoring.md", + "workflows/evaluate.md" + ], + "prompt": "Complete the review and evaluation record for a narrow one-line revision to a declared GitHub release skill. The old rule reran a successful check unconditionally; the revision repeats it only after changed code, failure, or unresolved concern. Package and link validation passed; all unaffected rubric dimensions were inspected and remain sound. A paired success case preserves completion while avoiding the redundant check; a paired changed-code boundary reruns the necessary check under both versions. All frozen assertions passed. Discovery is unchanged. Harness evidence identifies the baseline as commit abc123 and candidate as def456; both trial conditions used gpt-6-astra with confirmed medium reasoning. Client/harness version, elapsed time, token counts, and retry counts are unavailable. All tool outcomes were simulated; no release was performed. Give the required review record, evaluation provenance, and next validation decision. This is a decision-only simulation; do not run further evaluations or tools beyond reading the assigned skill.", + "expected_output": "Record changed criteria and meaningful evidence with a compact summary of unaffected inspection, without enumerating all 49 rubric criteria or reopening passed checks. Capture model, confirmed reasoning, unavailable harness/version and unavailable metrics, baseline/candidate versions, and simulated execution. Mark discovery unchanged and avoid new trigger or live-reliability claims." + } + ], + "runtime_hashes": { + "baseline": { + "SKILL.md": "38a8e7a93b2a175ad593110d153d67dd683451529fa615d04b04637988043e24", + "agents/openai.yaml": "0a458a8678139e0290efcd31d947690c7e9e19eb88a17fdf6954b0dc9d4bfeb8", + "guides/codex-invocation.md": "c749be296950f7df6dd5b159a1e5bd8cf96935caae4abbdd3a321facc08083b9", + "references/portability.md": "4f376373e67bd549b400dbc53c7896e6dbbc98a95c5aa37b291d9e8402de9301", + "rubrics/authoring.md": "8d75d56e47c8e0484c33b1d5790d51f4e0ef8e5f3f4fce4aad1139fa54182c3f", + "workflows/audit-instructions.md": "46bd0065443e523d656e73772cd704ebd1c943a7619504f5bbb86156801ec857", + "workflows/create-or-revise.md": "1bbf17abb440b3f345b4bd4a19f4234aa3c1888d6386bf0f2135b8417e9ee7ac", + "workflows/evaluate.md": "efe048833313f84499ec3036457d13f453c99e561dc9d69739b07d2df4e8bfec", + "workflows/merge.md": "b9bd34763de103d357759fdd854d302f365e08d2ed78b847caeb2636cfcf136a", + "workflows/refresh-guidance.md": "176a20c3195346df867a508dcb32eef456aee56fe7059834fb7945c956fdbed1" + }, + "candidate": { + "SKILL.md": "48171d3a4159ff2cb0a404ebd01df56c46126124747fae90defde9c817062213", + "agents/openai.yaml": "0a458a8678139e0290efcd31d947690c7e9e19eb88a17fdf6954b0dc9d4bfeb8", + "guides/codex-invocation.md": "c749be296950f7df6dd5b159a1e5bd8cf96935caae4abbdd3a321facc08083b9", + "references/portability.md": "4f376373e67bd549b400dbc53c7896e6dbbc98a95c5aa37b291d9e8402de9301", + "rubrics/authoring.md": "3861e32c573a897eed7528df77d6d39f2da4cf7dbe308e6c5e79556187201705", + "workflows/audit-instructions.md": "46bd0065443e523d656e73772cd704ebd1c943a7619504f5bbb86156801ec857", + "workflows/create-or-revise.md": "1bbf17abb440b3f345b4bd4a19f4234aa3c1888d6386bf0f2135b8417e9ee7ac", + "workflows/evaluate.md": "10a0d301d40901749a5a496c01a28bf14947538dcd639f2d968be3ce4b388bae", + "workflows/merge.md": "cbedb6895821ab3732920a2385c6c35c456304711893fa5bf049156c7eeef46c", + "workflows/refresh-guidance.md": "b17739f5a4fc8c3c074b8eaab19d99d0f6e1be524e7154b6c662f36604511e7e" + } + }, + "trials": [ + { + "case_id": 15, + "condition": "baseline", + "worker": "trial_a15", + "output": "Commit only the reviewed `develop-skills` changes under your earlier explicit authorization, then verify the commit’s scope and report the refresh results, checks, and commit ID.\n\nNo further user input is needed. Publication, synchronization, and installation remain outside scope. No actions were executed in this simulation.", + "trial": 1, + "elapsed_seconds": null, + "word_count": 45, + "work_log": "Decision-only response to the supplied checkpoint; no live scenario actions executed.", + "assertions_passed": true, + "observed_failure": null + }, + { + "case_id": 15, + "condition": "candidate", + "worker": "trial_b15", + "output": "Next, commit the reviewed `develop-skills` changes with a descriptive message, then report the commit and completed validation. Existing authorization covers this step; no user input is needed.\n\nDelivery ends after the commit. Publication, synchronization, and installation remain outside scope. No actions were executed in this simulation.", + "trial": 1, + "elapsed_seconds": null, + "word_count": 46, + "work_log": "Decision-only response to the supplied checkpoint; no live scenario actions executed.", + "assertions_passed": true, + "observed_failure": null + }, + { + "case_id": 16, + "condition": "candidate", + "worker": "trial_b16", + "output": "- **Conflict decision:** Preserve Source B’s native goal creation and status tools. Exclude Source A’s Markdown-only requirement because its profile is outside the destination’s requested scope.\n- **Destination prerequisite:** Native goal creation and status tools must be available. The supplied facts confirm support; declare this required dependency in the skill.\n- **Compatibility claim:** The merged skill is Codex-native and does not claim compatibility with Source A’s Markdown-only profile.", + "trial": 1, + "elapsed_seconds": null, + "word_count": 68, + "work_log": "Decision-only response to the supplied checkpoint; no live scenario actions executed.", + "assertions_passed": true, + "observed_failure": null + }, + { + "case_id": 16, + "condition": "baseline", + "worker": "trial_a16", + "output": "- **Conflict decision:** Preserve native goal creation and status tools, as explicitly required. Exclude Source A’s Markdown-only tracking contract because its portable profile is outside the destination’s scope; merge only guidance consistent with native tracking.\n- **Destination prerequisite:** The skill requires native goal creation and status tools. Their availability is verified for this destination.\n- **Compatibility claim:** The merged skill targets Codex’s native goal tools and does not claim compatibility with Source A’s Markdown-only profile.", + "trial": 1, + "elapsed_seconds": null, + "word_count": 75, + "work_log": "Decision-only response to the supplied checkpoint; no live scenario actions executed.", + "assertions_passed": true, + "observed_failure": null + }, + { + "case_id": 17, + "condition": "baseline", + "worker": "trial_a17", + "output": "The narrow revision passes the supplied simulated review and evaluation. No additional validation is warranted by this evidence.\n\nI applied the assigned `develop-skills` authoring rubric and evaluation workflow. The record below relies on supplied outcomes; it does not claim actual execution.\n\n| Authoring rubric | Result and evidence |\n|---|---|\n| 1. Scope and purpose | **Pass** — One existing release-check decision changes; unaffected items were inspected and remain sound. |\n| 2. Trigger description | **Pass** — Discovery is unchanged and its consistency was inspected. New trigger trials are **N/A** because activation did not change. |\n| 3. Entry-point quality | **Pass** — Package validation passed; unaffected entry-point requirements remain sound. |\n| 4. Progressive disclosure and pointers | **Pass** — Link validation passed; unaffected resource requirements remain sound. |\n| 5. Calibrated specificity | **Pass** — The revised rule identifies concrete reasons to repeat a check: changed code, failure, or unresolved concern. |\n| 6. Workflow and completion | **Pass** — The success pair preserves completion while removing redundant work; the changed-code pair retains the necessary repeat. Unaffected authority and completion requirements remain sound. |\n| 7. Robustness and gotchas | **Pass** — The changed-code boundary passes. Other requirements remain sound by the supplied inspection. Failure and unresolved-concern branches were not separately exercised in the supplied cases. |\n| 8. Resources and portability | **Pass** — Package and link validation passed; unaffected portability requirements were inspected and remain sound. The declared GitHub dependency is intrinsic to this skill. |\n| 9. Evaluation evidence | **Pass for this narrow simulated decision** — Both paired cases passed all frozen assertions. Discovery remains unchanged. Broader trials, holdouts, repeated trials, and blind human review are **N/A** for the supplied narrow, objectively assessed correction with no reported instability. |\n| 10. Pruning and final quality | **Pass** — The change removes an unnecessary repeat while retaining a repeat after changed code. Unaffected requirements remain sound. |\n\nNo blockers, majors, or retained minors were reported. These grouped entries cover the supplied inspection of unaffected rubric items; no additional item-specific evidence was provided.\n\n| Evaluation provenance | Record |\n|---|---|\n| Baseline | Commit `abc123` |\n| Candidate | Commit `def456` |\n| Model, both conditions | `gpt-6-astra` |\n| Reasoning, both conditions | Confirmed `medium` |\n| Cases | Paired successful-check case; paired changed-code boundary |\n| Assertions | Frozen; all passed |\n| Observed difference | Candidate avoids the redundant successful check; both versions repeat the necessary check after code changes |\n| Client/harness version | Unavailable |\n| Elapsed time, token counts, retry counts | Unavailable |\n| Isolation details and trial ordering | Not supplied |\n| Execution status | All tool outcomes simulated; no release performed |\n\nStatic validation and the scoped simulated behavior gate pass. The trigger gate is unchanged, with no new empirical activation claim. The evidence supports the observed check-count improvement; it does not establish measured time or token savings, live release reliability, or general failure rates.\n\nThe next decision is to stop this evaluation and retain the one-line revision within the authorized scope. No files were changed, installation or publication performed, or release executed in this decision-only response. Further trials become relevant only if the candidate changes, a failure appears, or an unresolved concern could alter this conclusion.", + "trial": 1, + "elapsed_seconds": null, + "word_count": 552, + "work_log": "Decision-only response to the supplied checkpoint; no live scenario actions executed.", + "assertions_passed": true, + "observed_failure": null + }, + { + "case_id": 17, + "condition": "candidate", + "worker": "trial_b17", + "output": "The narrow revision passes review and the applicable evaluation gates.\n\n- **Scope:** One rule now repeats a successful check only after changed code, failure, or an unresolved concern. The supplied record does not identify the changed file or line.\n- **Authoring review:** Workflow proportionality and stopping conditions pass: the revision removes redundant work while retaining necessary revalidation. Completion and robustness remain sound in the tested cases. Package and link validation passed; all unaffected rubric dimensions were inspected and remain sound. No blockers, majors, or material tradeoffs were identified.\n- **Behavior evidence:** The paired success case preserves completion and avoids the redundant check. The paired changed-code boundary reruns the necessary check under both versions. All frozen assertions passed. These two cases support this correction; they do not establish general reliability or directly exercise every repeat condition.\n- **Discovery:** Unchanged and consistent with the inspected package. No new empirical trigger evidence is claimed.\n\nEvaluation provenance: baseline `abc123`, candidate `def456`; both conditions used `gpt-6-astra` with **confirmed medium reasoning**. Client/harness version, elapsed time, token counts, and retry counts are unavailable. Tool outcomes were simulated; no release occurred. The supplied record does not establish fresh-context isolation or include raw outputs, trial identifiers, or execution order.\n\n**Next validation decision:** Stop further trials for this narrow review: the supplied checks passed without reported instability or unresolved behavioral failures. Preserve the cases, frozen assertions, available results, and this decision with the provenance limitations. No broader retesting or trigger trials are warranted by the stated change, and this decision does not authorize publication or installation.", + "trial": 1, + "elapsed_seconds": null, + "word_count": 256, + "work_log": "Decision-only response to the supplied checkpoint; no live scenario actions executed.", + "assertions_passed": true, + "observed_failure": null + } + ], + "date": "2026-09-10", + "provenance_note": "The model and medium reasoning named in case 17 are supplied fixture facts, not evidence of the actual trial worker configuration. Actual worker settings were inherited and not independently confirmed.", + "grading": "Coordinator inspected each response against the predeclared expected_output. Both conditions passed all three decision cases. No critical boundary failure observed. No holdout or repeats: narrow clarification and reporting revision; samples do not estimate general reliability.", + "limits": [ + "Decision-only checkpoints, not full workflow execution.", + "Worker scenario activity is represented by final responses; no live delivery integrations exercised.", + "Single trial per condition per case; no statistical, token, or latency claim.", + "Discovery metadata unchanged; no new trigger trial or production-loader validation." + ] +} diff --git a/skills/develop-skills/evals/evaluation-2026-09-10.md b/skills/develop-skills/evals/evaluation-2026-09-10.md new file mode 100644 index 0000000..4c93e1f --- /dev/null +++ b/skills/develop-skills/evals/evaluation-2026-09-10.md @@ -0,0 +1,45 @@ +# Focused authoring evaluation — 2026-09-10 + +Local source revisions clarify existing authorization and source-evidence +priority, reduce narrow-review reporting, and require model and environment +provenance. No publication or installation occurred. Baseline: +`e6c7e487d6daaa72aa6ec0bf4a66ad634b0675c1`. + +The changes follow the [Astra guidance](https://developers.openai.com/api/docs/guides/latest-model) +on instruction interactions, prior authorization, and proportional verification. +The entry point and four affected resources were updated together. Invocation +metadata and `next-goal` runtime remain unchanged. + +## Observed decisions + +Three synthetic checkpoints ran once per condition in six fresh workers, with +baseline/candidate dispatch order alternated by case. The candidate runtime and +assertions were frozen first. Both versions passed all three decision cases: + +| Case | Observed result in both conditions | +| --- | --- | +| Existing refresh authority | Continue the scoped commit without another question; leave unauthorized publication, synchronization, and installation outside scope. | +| Conflicting source profile | Preserve requested native goal tools, declare the prerequisite, and reject incompatible Markdown-only profile claims. | +| Narrow review and provenance | Retain necessary checks, stop additional trials, record supplied model/settings and unavailable metrics, and identify simulated execution. | + +The narrow-review response used 256 whitespace-delimited words for the candidate +and 552 for the baseline. This single observation supports reduced reporting; +it does not establish token savings, latency improvement, or general reliability. +The authority clarifications preserved behavior; these trials did not reproduce +an unnecessary approval pause in the baseline. + +[Inputs, frozen assertions, runtime hashes, and raw responses](evaluation-2026-09-10.json) +preserve the evidence. Actual worker model, reasoning, and client version were +not independently confirmed and are marked unavailable. The model/settings in +case 17 are fixture facts, not worker telemetry. No live delivery or native-goal +integration was exercised; no holdout, repeated trial, or trigger test was run. +Discovery was checked as unchanged. + +## Validation + +Catalog/link validation, local CLI discovery, Skill Creator validation, and +whitespace checks passed. Skill Creator validation used an isolated `uv` runtime +with PyYAML after the system Python lacked it. One bounded independent code +review found no actionable issue. Scoped documentation reconciliation retained +historical evaluations and the completed earlier audit plan; current rules live +in the skill, and this record describes only the local revision and its evidence. diff --git a/skills/develop-skills/rubrics/authoring.md b/skills/develop-skills/rubrics/authoring.md index 53ac38b..a0405ae 100644 --- a/skills/develop-skills/rubrics/authoring.md +++ b/skills/develop-skills/rubrics/authoring.md @@ -1,8 +1,13 @@ # Authoring Review Rubric -Use this rubric after drafting and again after behavior evaluation. Record each -answer as `Pass`, `Fail`, or `N/A`, and cite the file, line, or evaluation result -that supports it. `N/A` passes only when its rationale is explicit. +Inspect this rubric after drafting. After behavior evaluation, revisit criteria +affected by the results or subsequent edits; reuse unchanged findings. + +For a narrow revision, record changed criteria, failures, material tradeoffs, +and consequential exclusions, with supporting file, line, or evaluation evidence. +Summarize the remaining inspection without a full criterion-by-criterion table. +For a whole-skill audit, record each answer as `Pass`, `Fail`, or `N/A` with its +evidence; explain each `N/A`. Reporting less does not waive applicable checks. ## Severity and decision diff --git a/skills/develop-skills/workflows/evaluate.md b/skills/develop-skills/workflows/evaluate.md index bc8dd67..de22e7b 100644 --- a/skills/develop-skills/workflows/evaluate.md +++ b/skills/develop-skills/workflows/evaluate.md @@ -40,6 +40,11 @@ for a changed candidate, a failure, unstable results, or an unresolved concern. 5. Give baseline and candidate runs the same request, artifacts, available tools, constraints, and resource limits. 6. Start every trial in a fresh, isolated context. Do not carry explanations, outputs, or evaluator feedback between trials. 7. Record the candidate and baseline versions, case identifier, condition, trial number, output, observable work log, elapsed time, and failures. Record input/output size, operations, and retries when available. +8. Record each trial's model identifier, reasoning setting, and harness or client + version. Mark unavailable values explicitly; distinguish requested settings + from confirmed settings. Identify simulated tools and live execution. Keep + these conditions matched for a skill comparison, or report differences as + confounds. Do not infer model-specific reliability from unidentified runs. Use synthetic cases only to cover gaps that real examples cannot exercise. Mark them as synthetic. diff --git a/skills/develop-skills/workflows/merge.md b/skills/develop-skills/workflows/merge.md index 7c542a9..d8be805 100644 --- a/skills/develop-skills/workflows/merge.md +++ b/skills/develop-skills/workflows/merge.md @@ -44,7 +44,8 @@ Build a decision table before drafting: | --- | --- | --- | --- | --- | --- | | Example concern | A / B | Same, complementary, or conflicting | Citation or result | Keep, adapt, or remove | Entry point, resource, metadata, or none | -Resolve conflicts with an explicit authority order: +Follow governing instructions and the user's requested scope when resolving +conflicts. Within that boundary, rank conflicting source recommendations by: 1. current portable specification or required data contract; 2. repository distribution and validation rules; diff --git a/skills/develop-skills/workflows/refresh-guidance.md b/skills/develop-skills/workflows/refresh-guidance.md index e8d1bc0..14068b7 100644 --- a/skills/develop-skills/workflows/refresh-guidance.md +++ b/skills/develop-skills/workflows/refresh-guidance.md @@ -5,9 +5,9 @@ Use this workflow only when the user explicitly asks to refresh or reconcile part of ordinary skill creation, revision, merge, audit, or evaluation. Do not open the sources merely because another skill is being changed. -This workflow authorizes maintenance of `develop-skills` only. It does not -authorize changes to other skills, registry or installation state, commits, -publication, or reinstallation. +This workflow alone authorizes maintenance of `develop-skills` only. Apply +existing user authorization for any additional operations; do not infer it from +the refresh request. ## Read the maintained sources @@ -93,6 +93,7 @@ Then perform the checks proportional to the change: Report the source retrieval date, material source changes, accepted and rejected decisions with reasons, files changed, validation and evaluation evidence, and remaining uncertainty. If no edit was warranted, report the no-op and its -evidence. Stop at validated local changes; commit, publish, synchronize, or -install only under a separate explicit request and the repository's publishing -procedure. +evidence. Commit, publish, synchronize, or install only when covered by existing +user authorization and the repository's publishing procedure. Complete covered +operations without requesting the same permission again; otherwise finish at +validated local changes. diff --git a/skills/sjskills/SKILL.md b/skills/sjskills/SKILL.md index 8cc0548..0200cf8 100644 --- a/skills/sjskills/SKILL.md +++ b/skills/sjskills/SKILL.md @@ -96,6 +96,23 @@ names, but cannot define new profiles in its manifest. It can independently add skills from other repositories with `[[direct]]`, without changing the central registry or publishing those skills in `agent-scripts`. +Inspect the access policy in `sjskills profiles` before selecting a profile. +`kicpa` is public; `kicpa-private` uses the enrolled private GitHub catalog. +Select both only when requested. Authenticated profiles and direct entries use +`access = "github-authenticated"`; omission means public. Use a CLI build that +supports access metadata; older executables can reject it. Declared source +names and URLs are public metadata, never a place for credentials. + +Authenticated fetching requires Git, gh, and an existing login with repository +access. It accepts only GitHub.com shorthand or credential-free HTTPS sources. +The CLI uses controlled Git staging and the original gh configuration without +copying credentials. It refuses legacy/unsupported gh configuration before gh +can migrate it. On that error, report the requested `gh auth status` setup step; +do not start login, change accounts, or modify authentication configuration +without user authorization. Existing `GH_TOKEN`/`GITHUB_TOKEN` login also works. +A private fetch failure blocks its selected scope; do not omit that selection, +retry anonymously, or use status cache evidence to bypass it. + For example, using a placeholder source and skill name: ```toml