rescan filesystem for new skills when setup (#1493)

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)
+				}
+			}
 		}
 	}
 

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 <available_skills> 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 <dataDir>/skills-store) for <slug>/<version>/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: <dir>/<slug>/<version>/).
+// 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))
+}

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"])
+	}
+}

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.