From 73ea2253c2c56b39eba75ea1c1f54466d0b8a0e5 Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Thu, 2 Jul 2026 22:49:40 +0900 Subject: [PATCH] Allow lock update to remove stale skill pins --- README.md | 5 ++- internal/cli/root_test.go | 22 ++++++++++ internal/core/source_lock.go | 43 +++++++++++++++---- internal/core/source_lock_test.go | 68 +++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 0fac6e7..37967ea 100644 --- a/README.md +++ b/README.md @@ -184,8 +184,9 @@ coordinates still match, so it will not silently advance a branch. Use `kanon lock check` in CI to fail when a git skill provider is missing from the lockfile, its configured coordinates changed, its declared ref now resolves somewhere else, or the cached content does not match the locked hash. Use -`kanon lock update ` or `kanon lock update --all` to intentionally -refresh lock entries. +`kanon lock update ` to intentionally refresh a provider lock +entry or remove a stale entry for a removed provider. Use +`kanon lock update --all` to refresh every enabled git skill provider. Co-owned config files that the agent also writes are merged instead of replaced. For Codex `config.toml` and Claude `.claude.json` / project `.mcp.json`, Kanon diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 9a7b55a..46bba3c 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -319,6 +319,28 @@ func TestLockUpdateAllRefreshesExistingPinAfterBranchMoves(t *testing.T) { } } +func TestLockUpdateNamedRemovedSkillDropsStaleEntry(t *testing.T) { + repo, ref := newRemoteSkillRepo(t, "version one\n") + home := t.TempDir() + writeRemoteSkillConfig(t, home, repo, ref) + runKanon(t, home, "lock") + + writeFile(t, filepath.Join(home, "kanon.yaml"), "version: 1\n") + runKanon(t, home, "lock", "update", "remote-review") + + lock, _, err := core.LoadSourceLock(home) + if err != nil { + t.Fatal(err) + } + if len(lock.Sources) != 0 { + t.Fatalf("lock update kept removed skill entry: %#v", lock.Sources) + } + checkOut := runKanon(t, home, "lock", "check") + if !strings.Contains(checkOut, "kanon.lock is valid.") { + t.Fatalf("unexpected check output: %s", checkOut) + } +} + func TestRenderUsesLockedRemoteSkillAfterBranchMoves(t *testing.T) { repo, ref := newRemoteSkillRepo(t, "version one\n") home := t.TempDir() diff --git a/internal/core/source_lock.go b/internal/core/source_lock.go index 7377eb6..4b4ca19 100644 --- a/internal/core/source_lock.go +++ b/internal/core/source_lock.go @@ -126,18 +126,14 @@ func UpdateRemoteSkillSources(cfg *Config, home string, names []string, all bool } items := enabledRemoteSkillSources(cfg) currentByName := map[string]remoteSkillSource{} + currentOwners := map[string]bool{} updateNames := map[string]bool{} for _, item := range items { if _, ok := currentByName[item.Name]; ok { return nil, fmt.Errorf("git skill provider name %q is ambiguous", item.Name) } currentByName[item.Name] = item - } - for _, name := range names { - if _, ok := currentByName[name]; !ok { - return nil, fmt.Errorf("enabled git skill provider %q not found", name) - } - updateNames[name] = true + currentOwners[item.Owner] = true } lock, _, err := LoadSourceLock(home) @@ -145,6 +141,21 @@ func UpdateRemoteSkillSources(cfg *Config, home string, names []string, all bool return nil, err } existing := sourceLockEntriesByOwner(lock) + staleByName := map[string]bool{} + for _, entry := range existing { + if !currentOwners[entry.Owner] { + for _, name := range sourceLockEntryProviderNames(entry.Owner) { + staleByName[name] = true + } + } + } + for _, name := range names { + if _, ok := currentByName[name]; !ok && !staleByName[name] { + return nil, fmt.Errorf("git skill provider %q not found in enabled config or kanon.lock", name) + } + updateNames[name] = true + } + entries := map[string]SourceLockEntry{} for _, item := range items { if updateNames[item.Name] { @@ -166,7 +177,10 @@ func UpdateRemoteSkillSources(cfg *Config, home string, names []string, all bool entries[item.Owner] = entry } for _, name := range names { - item := currentByName[name] + item, ok := currentByName[name] + if !ok { + continue + } entry, err := resolveSourceLockEntry(home, item) if err != nil { return nil, err @@ -283,6 +297,21 @@ func sourceLockEntriesByOwner(lock *SourceLock) map[string]SourceLockEntry { return entries } +func sourceLockEntryProviderNames(owner string) []string { + var names []string + seen := map[string]bool{} + for _, prefix := range []string{"skill.git.", "skill_source.", "skill."} { + if strings.HasPrefix(owner, prefix) { + name := strings.TrimPrefix(owner, prefix) + if name != "" && !seen[name] { + names = append(names, name) + seen[name] = true + } + } + } + return names +} + func entryMatchesRemoteSource(entry SourceLockEntry, source RemoteSource) error { normalized, err := normalizeRemoteSource(source) if err != nil { diff --git a/internal/core/source_lock_test.go b/internal/core/source_lock_test.go index 0ff00dd..7463ee9 100644 --- a/internal/core/source_lock_test.go +++ b/internal/core/source_lock_test.go @@ -33,6 +33,74 @@ func TestUpdateRemoteSkillSourcesCompletesMissingEntries(t *testing.T) { } } +func TestUpdateRemoteSkillSourcesAcceptsRemovedSkillName(t *testing.T) { + home := t.TempDir() + repoOne := remoteSkillRepo(t, "one") + repoTwo := remoteSkillRepo(t, "two") + original := &Config{ + Version: 1, + Skills: []Skill{ + {Name: "one", Git: &GitSkill{URL: repoOne.path, Ref: repoOne.ref}}, + {Name: "two", Git: &GitSkill{URL: repoTwo.path, Ref: repoTwo.ref}}, + }, + } + initial, err := LockRemoteSkillSources(original, home) + if err != nil { + t.Fatal(err) + } + if _, err := WriteSourceLock(home, initial); err != nil { + t.Fatal(err) + } + + updated, err := UpdateRemoteSkillSources(&Config{ + Version: 1, + Skills: []Skill{ + {Name: "two", Git: &GitSkill{URL: repoTwo.path, Ref: repoTwo.ref}}, + }, + }, home, []string{"one"}, false) + if err != nil { + t.Fatal(err) + } + if _, ok := sourceLockEntry(updated, "skill.git.one"); ok { + t.Fatalf("lock kept removed skill entry: %#v", updated.Sources) + } + if _, ok := sourceLockEntry(updated, "skill.git.two"); !ok { + t.Fatalf("lock missing remaining skill entry: %#v", updated.Sources) + } +} + +func TestUpdateRemoteSkillSourcesAcceptsAmbiguousLegacyRemovedSkillName(t *testing.T) { + home := t.TempDir() + if _, err := WriteSourceLock(home, &SourceLock{Sources: []SourceLockEntry{{ + Owner: "skill.git.foo", + Type: "git", + URL: "https://example.invalid/repo.git", + Ref: "main", + ResolvedRef: "abc123", + ContentSHA256: "sha256:abc123", + }}}); err != nil { + t.Fatal(err) + } + + updated, err := UpdateRemoteSkillSources(&Config{Version: 1}, home, []string{"git.foo"}, false) + if err != nil { + t.Fatal(err) + } + if len(updated.Sources) != 0 { + t.Fatalf("lock kept removed legacy skill entry: %#v", updated.Sources) + } +} + +func TestUpdateRemoteSkillSourcesRejectsUnknownName(t *testing.T) { + _, err := UpdateRemoteSkillSources(&Config{Version: 1}, t.TempDir(), []string{"missing"}, false) + if err == nil { + t.Fatal("expected unknown provider error") + } + if !strings.Contains(err.Error(), `git skill provider "missing" not found in enabled config or kanon.lock`) { + t.Fatalf("unexpected error: %v", err) + } +} + func TestLockRemoteSkillSourcesIncludesSkillDirectories(t *testing.T) { home := t.TempDir() repo := remoteSkillDirectoryRepo(t)