From ebc4a22fd2a9ec5909b4793e72c809571bd2c0e8 Mon Sep 17 00:00:00 2001 From: Luan Vu Date: Sat, 21 Mar 2026 08:00:39 +0700 Subject: [PATCH] fix(skills): use DB file_path for directory resolution + list archived skills (#322) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skills): use DB file_path for skill directory resolution + list archived skills Two fixes: 1. Skills store methods (ListSkills, ListAllSkills, GetSkill, GetSkillByID, LoadSkill) constructed BaseDir from s.baseDir + slug + version, ignoring the file_path column stored in the database. When GOCLAW_DATA_DIR differs from the directory used during initial skill seeding, this caused dependency rescans to look in the wrong directory, find no scripts, and silently skip — leaving missing_deps stale in the database even after successful pip/npm installs. Fix: buildSkillInfo now accepts an optional filePath parameter from the DB and uses it as the authoritative BaseDir when set. 2. ListSkills only returned skills with status='active', hiding archived skills from the admin UI. Skills with missing dependencies are archived on seed, making them invisible — admins couldn't see them to install deps and reactivate. Now includes archived skills in the listing. * fix(skills): use 'deleted' status for soft-delete, distinct from 'archived' DeleteSkill previously set status='archived', which is also used for skills with missing dependencies. Now that ListSkills includes archived skills (so admins can see and fix missing deps), soft-deleted skills were reappearing after page refresh. Use status='deleted' for user-deleted skills to keep them hidden while allowing archived (missing deps) skills to remain visible in the admin UI. * fix(skills): complete file_path resolution + exclude deleted from rescan - ListAllSkills: add `status != 'deleted'` to prevent rescan from resurrecting user-deleted skills - ListAccessible: add file_path to SELECT so runtime skill resolution uses DB path instead of stale baseDir - SearchByEmbedding: use DB file_path when available for path resolution - CreateSkillManaged: ON CONFLICT visibility reset now matches both 'archived' and 'deleted' statuses - Update delete result message and status comment --------- Co-authored-by: Luvu182 <208665161+Luvu182@users.noreply.github.com> Co-authored-by: viettranx --- internal/store/pg/skills.go | 17 +++++++----- internal/store/pg/skills_content.go | 40 +++++++++++++++++---------- internal/store/pg/skills_crud.go | 10 +++---- internal/store/pg/skills_embedding.go | 12 ++++++-- internal/store/pg/skills_grants.go | 7 +++-- internal/tools/skill_manage.go | 2 +- 6 files changed, 54 insertions(+), 34 deletions(-) diff --git a/internal/store/pg/skills.go b/internal/store/pg/skills.go index 87cce0be..4b860069 100644 --- a/internal/store/pg/skills.go +++ b/internal/store/pg/skills.go @@ -62,9 +62,10 @@ func (s *PGSkillStore) ListSkills() []store.SkillInfo { s.mu.RUnlock() // Cache miss or TTL expired → query DB - // Returns active + system skills (and disabled ones — admin UI needs to see them to toggle back). + // Returns active + archived + system skills. Archived skills are shown dimmed in the UI + // so admins can see missing deps and re-activate after installing them. rows, err := s.db.Query( - `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, frontmatter FROM skills WHERE status = 'active' OR is_system = true ORDER BY name`) + `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, frontmatter, file_path FROM skills WHERE status IN ('active', 'archived') OR is_system = true ORDER BY name`) if err != nil { return nil } @@ -79,10 +80,11 @@ func (s *PGSkillStore) ListSkills() []store.SkillInfo { var version int var isSystem, enabled bool var depsRaw, fmRaw []byte - if err := rows.Scan(&id, &name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw, &fmRaw); err != nil { + var filePath *string + if err := rows.Scan(&id, &name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw, &fmRaw, &filePath); err != nil { continue } - info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir) + info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility info.Tags = tags info.IsSystem = isSystem @@ -110,7 +112,7 @@ func (s *PGSkillStore) ListSkills() []store.SkillInfo { // Disabled skills are excluded — no point scanning or updating them. func (s *PGSkillStore) ListAllSkills() []store.SkillInfo { rows, err := s.db.Query( - `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps FROM skills WHERE enabled = true ORDER BY name`) + `SELECT id, name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path FROM skills WHERE enabled = true AND status != 'deleted' ORDER BY name`) if err != nil { return nil } @@ -125,10 +127,11 @@ func (s *PGSkillStore) ListAllSkills() []store.SkillInfo { var version int var isSystem, enabled bool var depsRaw []byte - if err := rows.Scan(&id, &name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw); err != nil { + var filePath *string + if err := rows.Scan(&id, &name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw, &filePath); err != nil { continue } - info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir) + info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility info.Tags = tags info.IsSystem = isSystem diff --git a/internal/store/pg/skills_content.go b/internal/store/pg/skills_content.go index 5a612a24..e041cdc5 100644 --- a/internal/store/pg/skills_content.go +++ b/internal/store/pg/skills_content.go @@ -18,13 +18,15 @@ var skillFrontmatterRe = regexp.MustCompile(`(?s)^---\n(.*?)\n---\n?`) func (s *PGSkillStore) LoadSkill(name string) (string, bool) { var slug string var version int + var filePath *string err := s.db.QueryRow( - "SELECT slug, version FROM skills WHERE slug = $1 AND status = 'active'", name, - ).Scan(&slug, &version) + "SELECT slug, version, file_path FROM skills WHERE slug = $1 AND status = 'active'", name, + ).Scan(&slug, &version, &filePath) if err != nil { return "", false } - content, err := readSkillContent(s.baseDir, slug, version) + info := buildSkillInfo("", "", slug, nil, version, s.baseDir, filePath) + content, err := readSkillFile(info.Path) if err != nil { return "", false } @@ -83,13 +85,14 @@ func (s *PGSkillStore) GetSkill(name string) (*store.SkillInfo, bool) { var tags []string var version int var isSystem bool + var filePath *string err := s.db.QueryRow( - "SELECT id, name, slug, description, visibility, tags, version, is_system FROM skills WHERE slug = $1 AND status = 'active'", name, - ).Scan(&id, &skillName, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem) + "SELECT id, name, slug, description, visibility, tags, version, is_system, file_path FROM skills WHERE slug = $1 AND status = 'active'", name, + ).Scan(&id, &skillName, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &filePath) if err != nil { return nil, false } - info := buildSkillInfo(id.String(), skillName, slug, desc, version, s.baseDir) + info := buildSkillInfo(id.String(), skillName, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility info.Tags = tags info.IsSystem = isSystem @@ -132,15 +135,16 @@ func (s *PGSkillStore) GetSkillByID(id uuid.UUID) (store.SkillInfo, bool) { var version int var isSystem, enabled bool var depsRaw []byte + var filePath *string err := s.db.QueryRow( - `SELECT name, slug, description, visibility, tags, version, is_system, status, enabled, deps + `SELECT name, slug, description, visibility, tags, version, is_system, status, enabled, deps, file_path FROM skills WHERE id = $1`, id, - ).Scan(&name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw) + ).Scan(&name, &slug, &desc, &visibility, pq.Array(&tags), &version, &isSystem, &status, &enabled, &depsRaw, &filePath) if err != nil { return store.SkillInfo{}, false } - info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir) + info := buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir, filePath) info.Visibility = visibility info.Tags = tags info.IsSystem = isSystem @@ -172,30 +176,36 @@ func (s *PGSkillStore) GetSkillOwnerIDBySlug(slug string) (string, bool) { return ownerID, true } -func buildSkillInfo(id, name, slug string, desc *string, version int, baseDir string) store.SkillInfo { +// buildSkillInfo constructs a SkillInfo. If filePath (from DB file_path column) is set, +// it is used as BaseDir; otherwise BaseDir is constructed from baseDir/slug/version. +// This ensures skills seeded under a previous data directory are still resolved correctly. +func buildSkillInfo(id, name, slug string, desc *string, version int, baseDir string, filePath *string) store.SkillInfo { d := "" if desc != nil { d = *desc } + skillDir := fmt.Sprintf("%s/%s/%d", baseDir, slug, version) + if filePath != nil && *filePath != "" { + skillDir = *filePath + } return store.SkillInfo{ ID: id, Name: name, Slug: slug, - Path: fmt.Sprintf("%s/%s/%d/SKILL.md", baseDir, slug, version), - BaseDir: fmt.Sprintf("%s/%s/%d", baseDir, slug, version), + Path: skillDir + "/SKILL.md", + BaseDir: skillDir, Source: "managed", Description: d, Version: version, } } -func readSkillContent(baseDir, slug string, version int) (string, error) { - path := fmt.Sprintf("%s/%s/%d/SKILL.md", baseDir, slug, version) +// readSkillFile reads a SKILL.md file, normalizes line endings, and strips frontmatter. +func readSkillFile(path string) (string, error) { data, err := os.ReadFile(path) if err != nil { return "", err } - // Normalize line endings (Windows CRLF → LF) and strip frontmatter content := strings.ReplaceAll(string(data), "\r\n", "\n") content = strings.ReplaceAll(content, "\r", "\n") content = skillFrontmatterRe.ReplaceAllString(content, "") diff --git a/internal/store/pg/skills_crud.go b/internal/store/pg/skills_crud.go index 5397a525..b6cd7dea 100644 --- a/internal/store/pg/skills_crud.go +++ b/internal/store/pg/skills_crud.go @@ -18,7 +18,7 @@ type SkillCreateParams struct { Description *string OwnerID string Visibility string - Status string // "active" or "archived" (defaults to "active" if empty) + Status string // "active", "archived" (missing deps), or "deleted" (user-deleted) Version int FilePath string FileSize int64 @@ -73,9 +73,9 @@ func (s *PGSkillStore) DeleteSkill(id uuid.UUID) error { return fmt.Errorf("delete skill user grants: %w", err) } - // Soft-delete the skill itself - if _, err := tx.Exec("UPDATE skills SET status = 'archived' WHERE id = $1", id); err != nil { - return fmt.Errorf("archive skill: %w", err) + // Soft-delete the skill (use 'deleted' status, distinct from 'archived' which means missing deps) + if _, err := tx.Exec("UPDATE skills SET status = 'deleted' WHERE id = $1", id); err != nil { + return fmt.Errorf("delete skill: %w", err) } if err := tx.Commit(); err != nil { @@ -142,7 +142,7 @@ func (s *PGSkillStore) CreateSkillManaged(ctx context.Context, p SkillCreatePara version = EXCLUDED.version, frontmatter = EXCLUDED.frontmatter, file_path = EXCLUDED.file_path, file_size = EXCLUDED.file_size, file_hash = EXCLUDED.file_hash, - visibility = CASE WHEN skills.status = 'archived' THEN 'private' ELSE skills.visibility END, + visibility = CASE WHEN skills.status IN ('archived', 'deleted') THEN 'private' ELSE skills.visibility END, status = 'active', updated_at = NOW() RETURNING id`, id, p.Name, p.Slug, p.Description, p.OwnerID, p.Visibility, version, diff --git a/internal/store/pg/skills_embedding.go b/internal/store/pg/skills_embedding.go index a136b100..41af1fc0 100644 --- a/internal/store/pg/skills_embedding.go +++ b/internal/store/pg/skills_embedding.go @@ -23,7 +23,7 @@ func (s *PGSkillStore) SearchByEmbedding(ctx context.Context, embedding []float3 vecStr := vectorToString(embedding) rows, err := s.db.QueryContext(ctx, - `SELECT name, slug, COALESCE(description, ''), version, + `SELECT name, slug, COALESCE(description, ''), version, file_path, 1 - (embedding <=> $1::vector) AS score FROM skills WHERE status = 'active' AND enabled = true AND embedding IS NOT NULL @@ -41,10 +41,16 @@ func (s *PGSkillStore) SearchByEmbedding(ctx context.Context, embedding []float3 for rows.Next() { var r store.SkillSearchResult var version int - if err := rows.Scan(&r.Name, &r.Slug, &r.Description, &version, &r.Score); err != nil { + var filePath *string + if err := rows.Scan(&r.Name, &r.Slug, &r.Description, &version, &filePath, &r.Score); err != nil { continue } - r.Path = fmt.Sprintf("%s/%s/%d/SKILL.md", s.baseDir, r.Slug, version) + // Use DB file_path when available; fall back to baseDir construction. + if filePath != nil && *filePath != "" { + r.Path = *filePath + "/SKILL.md" + } else { + r.Path = fmt.Sprintf("%s/%s/%d/SKILL.md", s.baseDir, r.Slug, version) + } results = append(results, r) } return results, nil diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index 4110d565..ed0a3c7d 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -114,7 +114,7 @@ func (s *PGSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID, us // Access logic: public → all, private → owner only, internal → check grants. func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.SkillInfo, error) { rows, err := s.db.QueryContext(ctx, - `SELECT DISTINCT s.name, s.slug, s.description, s.version FROM skills s + `SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1 LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND sug.user_id = $2 WHERE s.status = 'active' AND ( @@ -134,11 +134,12 @@ func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, us var name, slug string var desc *string var version int - if err := rows.Scan(&name, &slug, &desc, &version); err != nil { + var filePath *string + if err := rows.Scan(&name, &slug, &desc, &version, &filePath); err != nil { slog.Warn("skill_grants: scan error in ListAccessible", "error", err) continue } - result = append(result, buildSkillInfo("", name, slug, desc, version, s.baseDir)) + result = append(result, buildSkillInfo("", name, slug, desc, version, s.baseDir, filePath)) } return result, rows.Err() } diff --git a/internal/tools/skill_manage.go b/internal/tools/skill_manage.go index 040c29ab..1d088c17 100644 --- a/internal/tools/skill_manage.go +++ b/internal/tools/skill_manage.go @@ -350,7 +350,7 @@ func (t *SkillManageTool) executeDelete(ctx context.Context, args map[string]any t.loader.BumpVersion() } - return NewResult(fmt.Sprintf("Skill %q archived and removed from search.", slug)) + return NewResult(fmt.Sprintf("Skill %q deleted and removed from search.", slug)) } // maxCopySize limits total companion file copy to 20MB (matching publish_skill).