From 6a316fc23d97060baa7174bbfa9aa62f033e65e6 Mon Sep 17 00:00:00 2001 From: bilogic <946010+bilogic@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:30:08 +0800 Subject: [PATCH] rescan filesystem for new skills when setup (#1493) --- cmd/gateway_setup.go | 18 +++ internal/skills/reconciler.go | 172 +++++++++++++++++++++ internal/skills/reconciler_test.go | 238 +++++++++++++++++++++++++++++ internal/store/pg/skills_crud.go | 14 ++ 4 files changed, 442 insertions(+) create mode 100644 internal/skills/reconciler.go create mode 100644 internal/skills/reconciler_test.go diff --git a/cmd/gateway_setup.go b/cmd/gateway_setup.go index 94fb461d..f168fd0a 100644 --- a/cmd/gateway_setup.go +++ b/cmd/gateway_setup.go @@ -621,6 +621,24 @@ func setupSkillsSystem( } } } + + // Register on-disk managed skills (skills-store) that are missing from + // the database. A skill placed directly into the tenant's skills-store + // without a skills row is invisible to agents (skill visibility is + // DB-driven), which manifests as goclaw not detecting a skill the user + // typed triggers for. Reconcile closes that gap idempotently. + if reconcileStore, ok := pgStores.Skills.(skills.ManagedSkillStore); ok { + reconciler := skills.NewReconciler(reconcileStore) + if n, err := reconciler.Reconcile( + context.Background(), + store.MasterTenantID, + storeDirs[0], + ); err != nil { + slog.Warn("skills-store reconcile failed", "error", err) + } else if n > 0 { + slog.Info("skills-store reconcile complete", "registered", n) + } + } } } diff --git a/internal/skills/reconciler.go b/internal/skills/reconciler.go new file mode 100644 index 00000000..437de843 --- /dev/null +++ b/internal/skills/reconciler.go @@ -0,0 +1,172 @@ +package skills + +import ( + "context" + "crypto/sha256" + "fmt" + "log/slog" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// ManagedSkillStore is the minimal interface needed by the reconciler. +// PGSkillStore satisfies it; other backends may omit it and the reconcile step +// is skipped at setup. +type ManagedSkillStore interface { + // SkillExists reports whether a skills row already exists for the slug in + // the current tenant (any status, including deleted). + SkillExists(ctx context.Context, slug string) (bool, error) + // CreateSkillManaged registers a skill row (tenant-scoped via ctx). + CreateSkillManaged(ctx context.Context, p store.SkillCreateParams) (uuid.UUID, error) + // BumpVersion invalidates cached skill listings after a change. + BumpVersion() +} + +// Reconciler registers on-disk managed skills (skills-store) that are missing +// from the database so they become visible to agents. Skills are the +// DB-registered, agent-filtered list; a skill placed directly into a tenant's +// skills-store directory without a skills row would otherwise be silently +// invisible in and skill_search. +type Reconciler struct { + store ManagedSkillStore +} + +// NewReconciler creates a reconciler backed by the given skill store. +func NewReconciler(s ManagedSkillStore) *Reconciler { + return &Reconciler{store: s} +} + +// Reconcile scans managedDir (a single tenant's skills-store, e.g. the master +// tenant's /skills-store) for //SKILL.md directories +// and registers any slug that has no skills row. Registration is idempotent: +// existing rows (active, archived, or deleted) are left untouched and their +// versions are never bumped. Returns the number of skills registered. +func (r *Reconciler) Reconcile(ctx context.Context, tenantID uuid.UUID, managedDir string) (int, error) { + if managedDir == "" { + return 0, nil + } + ctx = store.WithTenantID(ctx, tenantID) + + entries, err := os.ReadDir(managedDir) + if err != nil { + return 0, nil // directory may not exist yet (fresh install) + } + + registered := 0 + for _, e := range entries { + if !e.IsDir() || strings.HasPrefix(e.Name(), "_") { + continue // _shared/ is a helper dir, not a skill + } + slug := e.Name() + + version, versionDir := latestManagedVersion(managedDir, slug) + if version < 0 { + continue + } + skillFile := filepath.Join(versionDir, "SKILL.md") + data, err := os.ReadFile(skillFile) + if err != nil { + slog.Debug("skills reconcile: skip dir without SKILL.md", "slug", slug) + continue + } + + exists, err := r.store.SkillExists(ctx, slug) + if err != nil { + slog.Warn("skills reconcile: existence check failed", "slug", slug, "error", err) + continue + } + if exists { + continue + } + + // Register the on-disk skill with metadata parsed from its frontmatter. + name := slug + description := "" + if meta := parseMetadata(skillFile); meta != nil { + if meta.Name != "" { + name = meta.Name + } + description = meta.Description + } + fmMap := map[string]string{} + if fm := extractFrontmatter(string(data)); fm != "" { + fmMap = parseSimpleYAML(fm) + } + + // Only an explicit public visibility can be honored: private requires an + // owner match and internal requires agent grants, neither of which the + // reconciler can establish. Public keeps the skill usable by all agents + // in the tenant (still tenant-scoped — never cross-tenant). + visibility := VisibilityPublic + if v := strings.ToLower(strings.TrimSpace(fmMap["visibility"])); v != "" && v == VisibilityPublic { + visibility = VisibilityPublic + } else if v != "" { + slog.Info("skills reconcile: registering as public because reconciler cannot grant "+v, + "slug", slug, "frontmatter_visibility", v) + } + + hash := fmt.Sprintf("%x", sha256.Sum256(data)) + desc := description + _, err = r.store.CreateSkillManaged(ctx, store.SkillCreateParams{ + Name: name, + Slug: slug, + Description: &desc, + OwnerID: "system", + Visibility: visibility, + Status: "active", + Version: version, + FilePath: versionDir, + FileSize: int64(len(data)), + FileHash: &hash, + Frontmatter: fmMap, + }) + if err != nil { + slog.Warn("skills reconcile: registration failed", "slug", slug, "error", err) + continue + } + registered++ + slog.Info("skills reconcile: registered on-disk skill", "slug", slug, "version", version) + } + + if registered > 0 { + r.store.BumpVersion() + } + return registered, nil +} + +// latestManagedVersion returns the highest-numbered version subdirectory for a +// skill slug within a managed skills directory (structure: ///). +// Mirrors the Loader's version-resolution so the reconciler doesn't need a Loader +// instance. Returns (version, path) or (-1, "") if no valid version exists. +func latestManagedVersion(managedDir, slug string) (int, string) { + slugDir := filepath.Join(managedDir, slug) + entries, err := os.ReadDir(slugDir) + if err != nil { + return -1, "" + } + + var versions []int + for _, e := range entries { + if !e.IsDir() { + continue + } + v, err := strconv.Atoi(e.Name()) + if err != nil || v < 1 { + continue + } + versions = append(versions, v) + } + if len(versions) == 0 { + return -1, "" + } + + sort.Sort(sort.Reverse(sort.IntSlice(versions))) + latestVer := versions[0] + return latestVer, filepath.Join(slugDir, strconv.Itoa(latestVer)) +} diff --git a/internal/skills/reconciler_test.go b/internal/skills/reconciler_test.go new file mode 100644 index 00000000..4ea91be6 --- /dev/null +++ b/internal/skills/reconciler_test.go @@ -0,0 +1,238 @@ +package skills + +import ( + "context" + "crypto/sha256" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type reconcilerStore struct { + registered []store.SkillCreateParams + exists map[string]bool + bumped int +} + +func (s *reconcilerStore) SkillExists(_ context.Context, slug string) (bool, error) { + return s.exists[slug], nil +} + +func (s *reconcilerStore) CreateSkillManaged(_ context.Context, p store.SkillCreateParams) (uuid.UUID, error) { + s.registered = append(s.registered, p) + s.exists[p.Slug] = true + return uuid.New(), nil +} + +func (s *reconcilerStore) BumpVersion() { s.bumped++ } + +func writeSkillFile(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + t.Fatalf("create skill dir: %v", err) + } + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatalf("write skill: %v", err) + } +} + +func TestReconciler_RegistersMissingOnDiskSkill(t *testing.T) { + managedDir := filepath.Join(t.TempDir(), "skills-store") + content := "---\nname: connect-google-account\ndescription: Connect a user's Google account\n---\n\n# Google\n" + writeSkillFile(t, filepath.Join(managedDir, "connect-google-account", "1", "SKILL.md"), content) + + storeStub := &reconcilerStore{exists: map[string]bool{}} + r := NewReconciler(storeStub) + + n, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir) + if err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if n != 1 { + t.Fatalf("registered = %d, want 1", n) + } + if storeStub.bumped != 1 { + t.Fatalf("bump count = %d, want 1", storeStub.bumped) + } + if len(storeStub.registered) != 1 { + t.Fatalf("registered rows = %d, want 1", len(storeStub.registered)) + } + p := storeStub.registered[0] + if p.Slug != "connect-google-account" { + t.Fatalf("slug = %q, want connect-google-account", p.Slug) + } + if p.Name != "connect-google-account" { + t.Fatalf("name = %q, want connect-google-account", p.Name) + } + if p.Visibility != VisibilityPublic { + t.Fatalf("visibility = %q, want public (reconciler cannot grant private/internal)", p.Visibility) + } + if p.Status != "active" { + t.Fatalf("status = %q, want active", p.Status) + } + if p.Version != 1 { + t.Fatalf("version = %d, want on-disk version 1", p.Version) + } + if p.FilePath != filepath.Join(managedDir, "connect-google-account", "1") { + t.Fatalf("file path = %q, want version dir", p.FilePath) + } + wantHash := fmt.Sprintf("%x", sha256.Sum256([]byte(content))) + if p.FileHash == nil || *p.FileHash != wantHash { + t.Fatalf("file hash = %v, want %s", p.FileHash, wantHash) + } + if p.Description == nil || *p.Description != "Connect a user's Google account" { + t.Fatalf("description = %v, want frontmatter description", p.Description) + } +} + +func TestReconciler_PicksLatestVersion(t *testing.T) { + managedDir := filepath.Join(t.TempDir(), "skills-store") + writeSkillFile(t, filepath.Join(managedDir, "doc-helper", "1", "SKILL.md"), "---\nname: doc-helper\n---\n# v1\n") + writeSkillFile(t, filepath.Join(managedDir, "doc-helper", "2", "SKILL.md"), "---\nname: doc-helper\ndescription: v2\n---\n# v2\n") + + storeStub := &reconcilerStore{exists: map[string]bool{}} + r := NewReconciler(storeStub) + + if _, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir); err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if len(storeStub.registered) != 1 { + t.Fatalf("registered rows = %d, want 1", len(storeStub.registered)) + } + p := storeStub.registered[0] + if p.Version != 2 { + t.Fatalf("version = %d, want latest disk version 2", p.Version) + } + if p.Description == nil || *p.Description != "v2" { + t.Fatalf("description = %v, want latest version metadata", p.Description) + } +} + +func TestReconciler_SkipsExistingSkills(t *testing.T) { + managedDir := filepath.Join(t.TempDir(), "skills-store") + writeSkillFile(t, filepath.Join(managedDir, "already-registered", "1", "SKILL.md"), "---\nname: already-registered\n---\n") + + storeStub := &reconcilerStore{exists: map[string]bool{"already-registered": true}} + r := NewReconciler(storeStub) + + n, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir) + if err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if n != 0 { + t.Fatalf("registered = %d, want 0", n) + } + if len(storeStub.registered) != 0 { + t.Fatalf("created rows = %d, want 0 (idempotent)", len(storeStub.registered)) + } + if storeStub.bumped != 0 { + t.Fatalf("bump count = %d, want 0", storeStub.bumped) + } +} + +func TestReconciler_DoesNotResurrectDeletedSkill(t *testing.T) { + // A deleted skill keeps its directory (DeleteSkill is a soft delete); the + // reconciler must not re-register it. + managedDir := filepath.Join(t.TempDir(), "skills-store") + writeSkillFile(t, filepath.Join(managedDir, "removed", "1", "SKILL.md"), "---\nname: removed\n---\n") + + storeStub := &reconcilerStore{exists: map[string]bool{"removed": true}} + r := NewReconciler(storeStub) + + if _, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir); err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if len(storeStub.registered) != 0 { + t.Fatalf("re-registered deleted skill = %d rows, want 0", len(storeStub.registered)) + } +} + +func TestReconciler_SkipsSharedAndVersionlessDirs(t *testing.T) { + managedDir := filepath.Join(t.TempDir(), "skills-store") + writeSkillFile(t, filepath.Join(managedDir, "_shared", "SKILL.md"), "shared helper, not a skill") + writeSkillFile(t, filepath.Join(managedDir, "no-version", "SKILL.md"), "not versioned") + + storeStub := &reconcilerStore{exists: map[string]bool{}} + r := NewReconciler(storeStub) + + n, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir) + if err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if n != 0 { + t.Fatalf("registered = %d, want 0", n) + } + if len(storeStub.registered) != 0 { + t.Fatalf("registered rows = %d, want 0", len(storeStub.registered)) + } +} + +func TestReconciler_RegistersAgainWhenRowDeletedByOtherWriter(t *testing.T) { + // Sanity check for the idempotency guarantee after a successful run: + // a second run over the same dir is a no-op. + managedDir := filepath.Join(t.TempDir(), "skills-store") + writeSkillFile(t, filepath.Join(managedDir, "foo", "1", "SKILL.md"), "---\nname: foo\n---\n") + + storeStub := &reconcilerStore{exists: map[string]bool{}} + r := NewReconciler(storeStub) + + first, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir) + if err != nil { + t.Fatalf("first Reconcile error: %v", err) + } + second, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir) + if err != nil { + t.Fatalf("second Reconcile error: %v", err) + } + if first != 1 || second != 0 { + t.Fatalf("runs = (%d, %d), want (1, 0)", first, second) + } + if len(storeStub.registered) != 1 { + t.Fatalf("registered rows = %d, want 1", len(storeStub.registered)) + } +} + +func TestReconciler_MissingManagedDirIsNoop(t *testing.T) { + storeStub := &reconcilerStore{exists: map[string]bool{}} + r := NewReconciler(storeStub) + + n, err := r.Reconcile(context.Background(), store.MasterTenantID, filepath.Join(t.TempDir(), "does-not-exist")) + if err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if n != 0 || len(storeStub.registered) != 0 { + t.Fatalf("registered = %d rows=%d, want no-op", n, len(storeStub.registered)) + } +} + +func TestReconciler_FrontmatterVisibilityHonoredOnlyWhenPublic(t *testing.T) { + managedDir := filepath.Join(t.TempDir(), "skills-store") + writeSkillFile(t, filepath.Join(managedDir, "pub", "1", "SKILL.md"), + "---\nname: pub\nvisibility: public\n---\n") + writeSkillFile(t, filepath.Join(managedDir, "priv", "1", "SKILL.md"), + "---\nname: priv\nvisibility: private\n---\n") + + storeStub := &reconcilerStore{exists: map[string]bool{}} + r := NewReconciler(storeStub) + + if _, err := r.Reconcile(context.Background(), store.MasterTenantID, managedDir); err != nil { + t.Fatalf("Reconcile error: %v", err) + } + if len(storeStub.registered) != 2 { + t.Fatalf("registered rows = %d, want 2", len(storeStub.registered)) + } + vis := map[string]string{} + for _, p := range storeStub.registered { + vis[p.Slug] = p.Visibility + } + if vis["pub"] != VisibilityPublic { + t.Fatalf("pub visibility = %q, want public", vis["pub"]) + } + if vis["priv"] != VisibilityPublic { + t.Fatalf("priv visibility = %q, want public (reconciler cannot grant private)", vis["priv"]) + } +} diff --git a/internal/store/pg/skills_crud.go b/internal/store/pg/skills_crud.go index 449876d9..96fa70ef 100644 --- a/internal/store/pg/skills_crud.go +++ b/internal/store/pg/skills_crud.go @@ -222,6 +222,20 @@ func (s *PGSkillStore) GetNextVersion(ctx context.Context, slug string) int { return maxVersion + 1 } +// SkillExists reports whether a skills row exists for the slug in the current +// tenant, regardless of status (active/archived/deleted) or enabled flag. +// Used to reconcile on-disk managed skills without resurrecting deleted skills +// or racing the upsert's version bump. +func (s *PGSkillStore) SkillExists(ctx context.Context, slug string) (bool, error) { + tid := tenantIDForInsert(ctx) + var exists bool + err := s.db.QueryRowContext(ctx, + "SELECT EXISTS(SELECT 1 FROM skills WHERE slug = $1 AND tenant_id = $2)", + slug, tid, + ).Scan(&exists) + return exists, err +} + // GetSkillHashBySlug returns the file_hash and version of the latest non-deleted skill // version for the given slug, scoped to the current tenant. // Returns ok=false when no matching row exists.