diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/paths.go b/apps/backend/internal/agent/runtime/lifecycle/skill/paths.go index d8e6d00183..2ba654d62e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/paths.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/paths.go @@ -2,17 +2,15 @@ package skill import ( "path/filepath" - "regexp" "strings" + + "github.com/kandev/kandev/internal/common/skillslug" ) -// validSlugRe matches slugs that are safe for use in shell commands -// and on-disk paths. Anything outside this set is dropped during +// isValidSlug reports whether the given slug is safe for use in shell +// commands and on-disk paths. Anything outside this set is dropped during // delivery to avoid path-traversal or shell-quoting hazards. -var validSlugRe = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) - -// isValidSlug reports whether the given slug is safe. -func isValidSlug(s string) bool { return s != "" && validSlugRe.MatchString(s) } +func isValidSlug(s string) bool { return skillslug.WellFormed(s) } // isValidPathComponent reports whether the given filename is a single // safe path component (no separators, no traversal). Used when writing diff --git a/apps/backend/internal/office/repository/sqlite/skills.go b/apps/backend/internal/office/repository/sqlite/skills.go index 2e9d363047..ebc22b5228 100644 --- a/apps/backend/internal/office/repository/sqlite/skills.go +++ b/apps/backend/internal/office/repository/sqlite/skills.go @@ -3,10 +3,13 @@ package sqlite import ( "context" "database/sql" + "encoding/json" "fmt" + "strings" "time" "github.com/google/uuid" + "github.com/jmoiron/sqlx" "github.com/kandev/kandev/internal/office/models" ) @@ -117,6 +120,27 @@ func (r *Repository) ListSystemSkills( return skills, nil } +// ListNonSystemSkills returns all is_system = false skills for a +// workspace, ordered by slug. Used by the system-skill sync's +// slug-migration pass: user/provider-imported rows that need a +// well-formed-but-non-canonical slug normalized to canonical, and the +// conflict check before inserting a newly-bundled canonical slug. +func (r *Repository) ListNonSystemSkills( + ctx context.Context, workspaceID string, +) ([]*models.Skill, error) { + var skills []*models.Skill + err := r.ro.SelectContext(ctx, &skills, r.ro.Rebind( + `SELECT * FROM office_skills WHERE workspace_id = ? AND is_system = 0 ORDER BY slug`), + workspaceID) + if err != nil { + return nil, err + } + if skills == nil { + return []*models.Skill{}, nil + } + return skills, nil +} + // UpdateSkill updates an existing skill. func (r *Repository) UpdateSkill(ctx context.Context, skill *models.Skill) error { skill.UpdatedAt = time.Now().UTC() @@ -141,6 +165,176 @@ func (r *Repository) UpdateSkill(ctx context.Context, skill *models.Skill) error return err } +// NormalizeSkillSlug atomically changes a non-system skill's slug and +// rewrites matching desired_skills references on all active office agents. +// The skill ID and old slug are both part of the update predicate, so a +// concurrent writer cannot cause references to be rewritten for a different +// row. It returns false when the row no longer matches that identity. +func (r *Repository) NormalizeSkillSlug( + ctx context.Context, + workspaceID, skillID, oldSlug, newSlug string, +) (bool, error) { + tx, err := r.db.BeginTxx(ctx, nil) + if err != nil { + return false, fmt.Errorf("begin skill slug normalization: %w", err) + } + defer func() { _ = tx.Rollback() }() + + result, err := tx.ExecContext(ctx, tx.Rebind(` + UPDATE office_skills + SET slug = ?, updated_at = ? + WHERE id = ? AND workspace_id = ? AND slug = ? AND is_system = 0 + `), newSlug, time.Now().UTC(), skillID, workspaceID, oldSlug) + if err != nil { + return false, err + } + updated, err := result.RowsAffected() + if err != nil { + return false, fmt.Errorf("check skill slug normalization: %w", err) + } + if updated == 0 { + return false, nil + } + + updates, err := collectAgentDesiredSkillUpdates(ctx, tx, workspaceID, oldSlug, newSlug) + if err != nil { + return false, err + } + if err := applyAgentDesiredSkillUpdates(ctx, tx, workspaceID, updates); err != nil { + return false, err + } + + if err := tx.Commit(); err != nil { + return false, fmt.Errorf("commit skill slug normalization: %w", err) + } + return true, nil +} + +type agentDesiredSkillUpdate struct { + id string + original string + desired string +} + +func collectAgentDesiredSkillUpdates( + ctx context.Context, + tx *sqlx.Tx, + workspaceID, oldSlug, newSlug string, +) ([]agentDesiredSkillUpdate, error) { + rows, err := tx.QueryxContext(ctx, tx.Rebind(` + SELECT id, COALESCE(desired_skills, '') AS desired_skills + FROM agent_profiles + WHERE workspace_id = ? AND deleted_at IS NULL + `), workspaceID) + if err != nil { + return nil, fmt.Errorf("list agents for skill slug normalization: %w", err) + } + defer func() { _ = rows.Close() }() + + updates := make([]agentDesiredSkillUpdate, 0) + for rows.Next() { + var update agentDesiredSkillUpdate + if err := rows.Scan(&update.id, &update.original); err != nil { + return nil, fmt.Errorf("scan agent for skill slug normalization: %w", err) + } + update.desired, _ = replaceSkillSlugInAgentList(update.original, oldSlug, newSlug) + if update.desired != update.original { + updates = append(updates, update) + } + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("read agents for skill slug normalization: %w", err) + } + if err := rows.Close(); err != nil { + return nil, fmt.Errorf("close agents for skill slug normalization: %w", err) + } + return updates, nil +} + +func applyAgentDesiredSkillUpdates( + ctx context.Context, + tx *sqlx.Tx, + workspaceID string, + updates []agentDesiredSkillUpdate, +) error { + now := time.Now().UTC() + for _, update := range updates { + result, err := tx.ExecContext(ctx, tx.Rebind(` + UPDATE agent_profiles + SET desired_skills = ?, updated_at = ? + WHERE id = ? AND workspace_id = ? AND deleted_at IS NULL + AND COALESCE(desired_skills, '') = ? + `), update.desired, now, update.id, workspaceID, update.original) + if err != nil { + return fmt.Errorf("update agent %s for skill slug normalization: %w", update.id, err) + } + rowsAffected, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("check agent %s for skill slug normalization: %w", update.id, err) + } + if rowsAffected != 1 { + return fmt.Errorf("agent %s changed during skill slug normalization", update.id) + } + } + return nil +} + +// replaceSkillSlugInAgentList mirrors the office desired_skills reader. It +// accepts both the canonical JSON-array format and the legacy CSV format, and +// emits a deduplicated JSON array only when a value changes. +func replaceSkillSlugInAgentList(raw, oldSlug, newSlug string) (string, bool) { + raw = strings.TrimSpace(raw) + if raw == "" || raw == "null" { + return raw, false + } + values, ok := parseAgentSkillList(raw) + if !ok { + return raw, false + } + + out := make([]string, 0, len(values)) + seen := make(map[string]bool, len(values)) + changed := false + for _, value := range values { + if value == oldSlug { + value = newSlug + changed = true + } + if value == "" || seen[value] { + if value != "" { + changed = true + } + continue + } + seen[value] = true + out = append(out, value) + } + if !changed { + return raw, false + } + encoded, err := json.Marshal(out) + if err != nil { + return raw, false + } + return string(encoded), true +} + +func parseAgentSkillList(raw string) ([]string, bool) { + if strings.HasPrefix(raw, "[") { + var values []string + if err := json.Unmarshal([]byte(raw), &values); err != nil { + return nil, false + } + return values, true + } + parts := strings.Split(raw, ",") + values := make([]string, len(parts)) + for i, part := range parts { + values[i] = strings.TrimSpace(part) + } + return values, true +} + // SkillConfigFields is the subset of office_skills columns a config import // owns (see UpdateSkillConfigFields). type SkillConfigFields struct { diff --git a/apps/backend/internal/office/repository/sqlite/skills_slug_atomic_test.go b/apps/backend/internal/office/repository/sqlite/skills_slug_atomic_test.go new file mode 100644 index 0000000000..c85c9b1046 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/skills_slug_atomic_test.go @@ -0,0 +1,123 @@ +package sqlite_test + +import ( + "context" + "encoding/json" + "testing" + + "github.com/kandev/kandev/internal/office/models" +) + +func TestNormalizeSkillSlugAtomicallyUpdatesAgentReferences(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + + skill := &models.Skill{ + ID: "skill-1", + WorkspaceID: "ws-1", + Name: "Custom skill", + Slug: "custom-skill", + SourceType: "inline", + } + if err := repo.CreateSkill(ctx, skill); err != nil { + t.Fatalf("create skill: %v", err) + } + agent := &models.AgentInstance{ + ID: "agent-1", + WorkspaceID: "ws-1", + Name: "Agent", + DesiredSkills: `["custom-skill","other-skill","custom-skill"]`, + } + if err := repo.CreateAgentInstance(ctx, agent); err != nil { + t.Fatalf("create agent: %v", err) + } + + changed, err := repo.NormalizeSkillSlug(ctx, "ws-1", skill.ID, "custom-skill", "kandev-custom-skill") + if err != nil { + t.Fatalf("normalize skill slug: %v", err) + } + if !changed { + t.Fatal("normalize skill slug reported no change") + } + + gotSkill, err := repo.GetSkillBySlug(ctx, "ws-1", "kandev-custom-skill") + if err != nil { + t.Fatalf("get normalized skill: %v", err) + } + if gotSkill.ID != skill.ID { + t.Fatalf("normalized skill ID = %q, want %q", gotSkill.ID, skill.ID) + } + if _, err := repo.GetSkillBySlug(ctx, "ws-1", "custom-skill"); err == nil { + t.Fatal("old skill slug still resolves after normalization") + } + + gotAgent, err := repo.GetAgentInstance(ctx, agent.ID) + if err != nil { + t.Fatalf("get agent: %v", err) + } + var desired []string + if err := json.Unmarshal([]byte(gotAgent.DesiredSkills), &desired); err != nil { + t.Fatalf("decode desired_skills %q: %v", gotAgent.DesiredSkills, err) + } + want := []string{"kandev-custom-skill", "other-skill"} + if len(desired) != len(want) || desired[0] != want[0] || desired[1] != want[1] { + t.Fatalf("desired_skills = %v, want %v", desired, want) + } +} + +func TestNormalizeSkillSlugRollsBackWhenCanonicalSlugIsTaken(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + + oldSkill := &models.Skill{ + ID: "skill-old", + WorkspaceID: "ws-1", + Name: "Custom skill", + Slug: "custom-skill", + SourceType: "inline", + } + takenSkill := &models.Skill{ + ID: "skill-taken", + WorkspaceID: "ws-1", + Name: "Canonical skill", + Slug: "kandev-custom-skill", + SourceType: "inline", + } + for _, skill := range []*models.Skill{oldSkill, takenSkill} { + if err := repo.CreateSkill(ctx, skill); err != nil { + t.Fatalf("create skill %s: %v", skill.ID, err) + } + } + agent := &models.AgentInstance{ + ID: "agent-1", + WorkspaceID: "ws-1", + Name: "Agent", + DesiredSkills: `["custom-skill"]`, + } + if err := repo.CreateAgentInstance(ctx, agent); err != nil { + t.Fatalf("create agent: %v", err) + } + + changed, err := repo.NormalizeSkillSlug(ctx, "ws-1", oldSkill.ID, "custom-skill", "kandev-custom-skill") + if err == nil { + t.Fatal("normalization should fail when the canonical slug is taken") + } + if changed { + t.Fatal("normalization reported a change after a rolled-back transaction") + } + + gotOld, err := repo.GetSkillBySlug(ctx, "ws-1", "custom-skill") + if err != nil { + t.Fatalf("old skill row was not preserved: %v", err) + } + if gotOld.ID != oldSkill.ID { + t.Fatalf("old skill ID = %q, want %q", gotOld.ID, oldSkill.ID) + } + gotAgent, err := repo.GetAgentInstance(ctx, agent.ID) + if err != nil { + t.Fatalf("get agent: %v", err) + } + if gotAgent.DesiredSkills != agent.DesiredSkills { + t.Fatalf("desired_skills = %q after rollback, want %q", gotAgent.DesiredSkills, agent.DesiredSkills) + } +} diff --git a/apps/backend/internal/office/skills/service.go b/apps/backend/internal/office/skills/service.go index 0a41d61043..f1ade825d0 100644 --- a/apps/backend/internal/office/skills/service.go +++ b/apps/backend/internal/office/skills/service.go @@ -8,6 +8,7 @@ import ( settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/skillslug" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/shared" @@ -25,6 +26,8 @@ type skillRepo interface { GetSkillBySlug(ctx context.Context, workspaceID, slug string) (*models.Skill, error) ListSkills(ctx context.Context, workspaceID string) ([]*models.Skill, error) ListSystemSkills(ctx context.Context, workspaceID string) ([]*models.Skill, error) + ListNonSystemSkills(ctx context.Context, workspaceID string) ([]*models.Skill, error) + NormalizeSkillSlug(ctx context.Context, workspaceID, skillID, oldSlug, newSlug string) (bool, error) UpdateSkill(ctx context.Context, skill *models.Skill) error DeleteSkill(ctx context.Context, id string) error @@ -130,13 +133,22 @@ func (s *SkillService) countSkillUsage(ctx context.Context) map[string]int { // ValidateAndPrepareSkill validates and prepares a skill for creation. // Call this before CreateSkill to auto-generate slug and validate uniqueness. +// +// Slug handling follows AC-001.11/AC-001.12: a caller-supplied slug must be +// well-formed or the request is rejected outright (no coercion); a +// well-formed slug is then normalized to its canonical (kandev-prefixed) +// form before the uniqueness check runs, so both the persisted slug and the +// duplicate check operate on the same canonical value. func (s *SkillService) ValidateAndPrepareSkill(ctx context.Context, skill *models.Skill) error { if skill.Name == "" { return fmt.Errorf("skill name is required") } if skill.Slug == "" { skill.Slug = GenerateSlug(skill.Name) + } else if !skillslug.WellFormed(skill.Slug) { + return fmt.Errorf("invalid skill slug %q: must contain only letters, digits, underscore, and hyphen", skill.Slug) } + skill.Slug = skillslug.Normalize(skill.Slug) if err := s.validateSlugUnique(ctx, skill.WorkspaceID, skill.Slug, ""); err != nil { return err } @@ -156,12 +168,19 @@ func prepareSkillPackageMetadata(skill *models.Skill) { ) } -// ValidateSkillUpdate validates a skill update for slug uniqueness. +// ValidateSkillUpdate validates a skill update for slug uniqueness. Slug +// handling mirrors ValidateAndPrepareSkill: reject a not-well-formed slug +// outright (AC-001.11), otherwise normalize to canonical before the +// uniqueness check (AC-001.12). func (s *SkillService) ValidateSkillUpdate(ctx context.Context, skill *models.Skill) error { - if skill.Slug != "" { - return s.validateSlugUnique(ctx, skill.WorkspaceID, skill.Slug, skill.ID) + if skill.Slug == "" { + return nil } - return nil + if !skillslug.WellFormed(skill.Slug) { + return fmt.Errorf("invalid skill slug %q: must contain only letters, digits, underscore, and hyphen", skill.Slug) + } + skill.Slug = skillslug.Normalize(skill.Slug) + return s.validateSlugUnique(ctx, skill.WorkspaceID, skill.Slug, skill.ID) } func (s *SkillService) validateSlugUnique(ctx context.Context, workspaceID, slug, excludeID string) error { diff --git a/apps/backend/internal/office/skills/service_test.go b/apps/backend/internal/office/skills/service_test.go index f30d99934e..6720c9c78e 100644 --- a/apps/backend/internal/office/skills/service_test.go +++ b/apps/backend/internal/office/skills/service_test.go @@ -13,6 +13,7 @@ import ( "github.com/jmoiron/sqlx" _ "github.com/mattn/go-sqlite3" + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" @@ -35,6 +36,10 @@ func newTestSkillService(t *testing.T) *skills.SkillService { } t.Cleanup(func() { _ = db.Close() }) + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("provide settings store: %v", err) + } + repo, err := sqlite.NewWithDB(db, db, nil) if err != nil { t.Fatalf("new repo: %v", err) @@ -86,8 +91,92 @@ func TestValidateAndPrepareSkill_AutoGeneratesSlug(t *testing.T) { if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil { t.Fatalf("validate: %v", err) } - if skill.Slug != "code-review" { - t.Errorf("slug = %q, want %q", skill.Slug, "code-review") + if skill.Slug != "kandev-code-review" { + t.Errorf("slug = %q, want %q", skill.Slug, "kandev-code-review") + } +} + +// TestValidateAndPrepareSkill_NormalizesExplicitSlugToCanonical covers +// AC-001.12: a well-formed but non-canonical caller-supplied slug is +// normalized to canonical before persisting. +func TestValidateAndPrepareSkill_NormalizesExplicitSlugToCanonical(t *testing.T) { + svc := newTestSkillService(t) + ctx := context.Background() + + skill := &models.Skill{ + WorkspaceID: "ws-1", + Name: "My Skill", + Slug: "my-skill", + SourceType: "inline", + } + if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil { + t.Fatalf("validate: %v", err) + } + if skill.Slug != "kandev-my-skill" { + t.Errorf("slug = %q, want %q", skill.Slug, "kandev-my-skill") + } +} + +// TestValidateAndPrepareSkill_CanonicalSlugUnchanged covers AC-001.6: a +// slug that is already canonical is left byte-identical. +func TestValidateAndPrepareSkill_CanonicalSlugUnchanged(t *testing.T) { + svc := newTestSkillService(t) + ctx := context.Background() + + skill := &models.Skill{ + WorkspaceID: "ws-1", + Name: "Already Canonical", + Slug: "kandev-already-canonical", + SourceType: "inline", + } + if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil { + t.Fatalf("validate: %v", err) + } + if skill.Slug != "kandev-already-canonical" { + t.Errorf("slug = %q, want %q", skill.Slug, "kandev-already-canonical") + } +} + +// TestValidateAndPrepareSkill_RejectsNotWellFormedSlug covers AC-001.11: a +// not-well-formed caller-supplied slug is rejected outright, never coerced. +func TestValidateAndPrepareSkill_RejectsNotWellFormedSlug(t *testing.T) { + svc := newTestSkillService(t) + ctx := context.Background() + + skill := &models.Skill{ + WorkspaceID: "ws-1", + Name: "Bad Slug", + Slug: "not a valid slug!", + SourceType: "inline", + } + err := svc.ValidateAndPrepareSkill(ctx, skill) + if err == nil { + t.Fatal("expected error for not-well-formed slug") + } + if skill.Slug != "not a valid slug!" { + t.Errorf("slug was coerced from its original input: %q", skill.Slug) + } +} + +// TestValidateAndPrepareSkill_UniquenessCheckedOnCanonicalValue covers the +// second half of AC-001.12: a request whose canonical slug already exists +// is rejected, even when the request itself supplied the non-canonical +// form. +func TestValidateAndPrepareSkill_UniquenessCheckedOnCanonicalValue(t *testing.T) { + svc := newTestSkillService(t) + ctx := context.Background() + + existing := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-my-skill", SourceType: "inline"} + if err := svc.ValidateAndPrepareSkill(ctx, existing); err != nil { + t.Fatalf("validate existing: %v", err) + } + if err := svc.CreateSkill(ctx, existing); err != nil { + t.Fatalf("create existing: %v", err) + } + + colliding := &models.Skill{WorkspaceID: "ws-1", Name: "Colliding", Slug: "my-skill", SourceType: "inline"} + if err := svc.ValidateAndPrepareSkill(ctx, colliding); err == nil { + t.Fatal("expected duplicate slug error against the canonical form") } } @@ -143,6 +232,47 @@ func TestValidateAndPrepareSkill_RejectsSameSlugInSameWorkspace(t *testing.T) { } } +// --- ValidateSkillUpdate tests --- + +func TestValidateSkillUpdate_NormalizesWellFormedSlugToCanonical(t *testing.T) { + svc := newTestSkillService(t) + ctx := context.Background() + + skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-existing", SourceType: "inline"} + if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil { + t.Fatalf("validate: %v", err) + } + if err := svc.CreateSkill(ctx, skill); err != nil { + t.Fatalf("create: %v", err) + } + + skill.Slug = "renamed" + if err := svc.ValidateSkillUpdate(ctx, skill); err != nil { + t.Fatalf("update: %v", err) + } + if skill.Slug != "kandev-renamed" { + t.Errorf("slug = %q, want %q", skill.Slug, "kandev-renamed") + } +} + +func TestValidateSkillUpdate_RejectsNotWellFormedSlug(t *testing.T) { + svc := newTestSkillService(t) + ctx := context.Background() + + skill := &models.Skill{WorkspaceID: "ws-1", Name: "Existing", Slug: "kandev-existing", SourceType: "inline"} + if err := svc.ValidateAndPrepareSkill(ctx, skill); err != nil { + t.Fatalf("validate: %v", err) + } + if err := svc.CreateSkill(ctx, skill); err != nil { + t.Fatalf("create: %v", err) + } + + skill.Slug = "not a valid slug!" + if err := svc.ValidateSkillUpdate(ctx, skill); err == nil { + t.Fatal("expected error for not-well-formed slug") + } +} + func TestValidateAndPrepareSkill_RejectsInvalidSourceType(t *testing.T) { svc := newTestSkillService(t) ctx := context.Background() @@ -167,24 +297,27 @@ func TestListSkillsWithUsage(t *testing.T) { t.Fatalf("create: %v", err) } - // The lazy system-skill sync fires on the first ListSkills call - // for a workspace, populating bundled system rows alongside the - // user skill created above. The test asserts the *user* skill - // appears with the correct usage count; the system rows are - // validated separately in system_sync_test.go. + // The lazy system-skill sync fires on the first ListSkills call for + // a workspace, populating bundled system rows alongside the user + // skill created above. It also normalizes the well-formed but + // non-canonical "my-skill" slug to "kandev-my-skill" (AC-003.8), + // so the test looks for the row under its normalized slug. The + // normalization mechanics themselves are covered in + // system_sync_migration_test.go; this test only asserts the *user* + // skill survives sync with the correct usage count. list, err := svc.ListSkillsWithUsage(ctx, "ws-1") if err != nil { t.Fatalf("list: %v", err) } var userRow *skills.SkillWithUsage for _, row := range list { - if row.Slug == "my-skill" { + if row.Slug == "kandev-my-skill" { userRow = row break } } if userRow == nil { - t.Fatalf("user skill not found in list of %d rows", len(list)) + t.Fatalf("normalized user skill not found in list of %d rows", len(list)) } if userRow.UsedByCount != 0 { t.Errorf("used_by_count = %d, want 0", userRow.UsedByCount) diff --git a/apps/backend/internal/office/skills/system_sync.go b/apps/backend/internal/office/skills/system_sync.go index 2d5b0d3b22..7ae27bb66a 100644 --- a/apps/backend/internal/office/skills/system_sync.go +++ b/apps/backend/internal/office/skills/system_sync.go @@ -5,16 +5,20 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "sort" "strings" + "sync" "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgconn" "go.uber.org/zap" "gopkg.in/yaml.v3" settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/skillslug" "github.com/kandev/kandev/internal/office/configloader" "github.com/kandev/kandev/internal/office/models" ) @@ -34,6 +38,7 @@ var retiredDefaultSkillReplacements = map[string]string{ "kandev-task-comment": "kandev-task-ops", "kandev-tasks": "kandev-task-ops", "kandev-team": "kandev-team-admin", + "memory": "kandev-memory", } // SystemSkillSpec is the parsed view of a single embedded SKILL.md @@ -61,6 +66,18 @@ type SystemSyncRepo interface { UpdateSkill(ctx context.Context, skill *models.Skill) error DeleteSkill(ctx context.Context, id string) error + // ListNonSystemSkills returns every user/provider-imported row + // (is_system = false) in the workspace. Used by the bundled-insert + // slug-conflict pre-check (AC-003.3) and the user-skill slug + // normalization pass (AC-003.8/AC-003.9). + ListNonSystemSkills(ctx context.Context, workspaceID string) ([]*models.Skill, error) + + // NormalizeSkillSlug atomically changes a non-system skill's slug and + // rewrites all matching agent desired_skills references. It returns + // false when the row no longer matches the supplied identity, which + // means another writer already handled or removed it. + NormalizeSkillSlug(ctx context.Context, workspaceID, skillID, oldSlug, newSlug string) (bool, error) + // Agent-profile access used to scrub a deleted system skill's ID // out of every agent_profiles.skill_ids JSON array in the same // workspace, so retiring a bundled skill doesn't leave dangling @@ -72,10 +89,44 @@ type SystemSyncRepo interface { // SyncReport summarises one sync pass across all workspaces. The // startup caller surfaces this as a single log line so operators can // see exactly which slugs landed where after a kandev upgrade. +// +// Every per-workspace list is sorted lexicographically by slug; the +// scoped `:` entries in the aggregate report are +// sorted by workspace ID then slug (AC-003.5). type SyncReport struct { Inserted []string Updated []string Removed []string + + // Blocked lists retired system skills whose configured replacement + // was not found in this workspace this pass (e.g. its insert was + // withheld by a slug conflict). The retired row is left in place + // rather than deleted, so no agent silently loses the capability + // (AC-003.1); a later pass retries once the conflict resolves. + Blocked []string + + // Normalized lists user-skill slugs rewritten from a well-formed + // but non-canonical form to their canonical kandev-prefixed form, + // as "->new>" entries (AC-003.8). + Normalized []string + + // Conflicted lists slugs left untouched because normalizing (or + // inserting a bundled row) would collide with another row's + // current slug (AC-003.3/AC-003.9). + Conflicted []string +} + +// workspaceSyncLocks serializes SyncSystemSkills passes per +// workspace. SyncSystemSkills has multiple concurrent call sites in +// one process (lazy per-workspace sync, startup sync, config import, +// etc.); a pass that can't acquire the lock waits rather than +// skipping, so two concurrent passes over the same workspace never +// interleave their insert/retire/normalize phases (AC-003.7). +var workspaceSyncLocks sync.Map // map[string]*sync.Mutex + +func lockForWorkspace(wsID string) *sync.Mutex { + v, _ := workspaceSyncLocks.LoadOrStore(wsID, &sync.Mutex{}) + return v.(*sync.Mutex) } // SyncSystemSkills idempotently reconciles the office_skills table @@ -112,45 +163,123 @@ func SyncSystemSkills( var report SyncReport for _, wsID := range workspaceIDs { - ins, upd, rem, err := syncWorkspace(ctx, repo, wsID, bundledBySlug) + result, err := syncWorkspace(ctx, repo, wsID, bundledBySlug, log) if err != nil { log.Error("system skill sync failed for workspace", zap.String("workspace_id", wsID), zap.Error(err)) continue } - report.Inserted = append(report.Inserted, scope(wsID, ins)...) - report.Updated = append(report.Updated, scope(wsID, upd)...) - report.Removed = append(report.Removed, scope(wsID, rem)...) + report.Inserted = append(report.Inserted, scope(wsID, result.Inserted)...) + report.Updated = append(report.Updated, scope(wsID, result.Updated)...) + report.Removed = append(report.Removed, scope(wsID, result.Removed)...) + report.Blocked = append(report.Blocked, scope(wsID, result.Blocked)...) + report.Normalized = append(report.Normalized, scope(wsID, result.Normalized)...) + report.Conflicted = append(report.Conflicted, scope(wsID, result.Conflicted)...) } + sort.Strings(report.Inserted) + sort.Strings(report.Updated) + sort.Strings(report.Removed) + sort.Strings(report.Blocked) + sort.Strings(report.Normalized) + sort.Strings(report.Conflicted) log.Info("system skills synced", zap.Int("workspaces", len(workspaceIDs)), zap.Int("bundled", len(specs)), zap.Strings("inserted", report.Inserted), zap.Strings("updated", report.Updated), zap.Strings("removed", report.Removed), + zap.Strings("blocked", report.Blocked), + zap.Strings("normalized", report.Normalized), + zap.Strings("conflicted", report.Conflicted), ) return report, nil } -// syncWorkspace handles one workspace. Returns the (insert, update, -// remove) slug lists for the report. Errors propagate; the caller -// logs and continues to the next workspace so one bad row doesn't -// gate the rest. +// workspaceSyncResult carries one workspace's sync outcome between +// syncWorkspace's phases and back to SyncSystemSkills, which scopes +// each list into the aggregate SyncReport. +type workspaceSyncResult struct { + Inserted []string + Updated []string + Removed []string + Blocked []string + Normalized []string + Conflicted []string +} + +// syncWorkspace handles one workspace: reconcile bundled system +// skills (insert/update), retire orphaned system rows, then normalize +// non-canonical user-skill slugs. Errors propagate; the caller logs +// and continues to the next workspace so one bad row doesn't gate the +// rest. Holds the per-workspace lock for the whole pass so concurrent +// SyncSystemSkills call sites never interleave these phases +// (AC-003.7). func syncWorkspace( ctx context.Context, repo SystemSyncRepo, wsID string, bundled map[string]SystemSkillSpec, -) (inserted, updated, removed []string, err error) { + log *logger.Logger, +) (workspaceSyncResult, error) { + mu := lockForWorkspace(wsID) + mu.Lock() + defer mu.Unlock() + + var result workspaceSyncResult + existing, err := repo.ListSystemSkills(ctx, wsID) if err != nil { - return nil, nil, nil, err + return result, err } existingBySlug := make(map[string]*models.Skill, len(existing)) for _, s := range existing { existingBySlug[s.Slug] = s } + nonSystem, err := repo.ListNonSystemSkills(ctx, wsID) + if err != nil { + return result, fmt.Errorf("list non-system skills: %w", err) + } + nonSystemBySlug := make(map[string]*models.Skill, len(nonSystem)) + for _, s := range nonSystem { + nonSystemBySlug[s.Slug] = s + } + + if err := reconcileBundledSkills(ctx, repo, wsID, bundled, existingBySlug, nonSystemBySlug, &result); err != nil { + return result, err + } + if err := retireOrphanedSystemSkills(ctx, repo, wsID, bundled, existingBySlug, &result); err != nil { + return result, err + } + if err := normalizeUserSkillSlugs(ctx, repo, wsID, nonSystem, existingBySlug, nonSystemBySlug, &result, log); err != nil { + return result, err + } + + sort.Strings(result.Inserted) + sort.Strings(result.Updated) + sort.Strings(result.Removed) + sort.Strings(result.Blocked) + sort.Strings(result.Normalized) + sort.Strings(result.Conflicted) + return result, nil +} + +// reconcileBundledSkills walks the bundled spec set in sorted order, +// inserting missing rows and updating drifted ones. A bundled slug +// not yet present as a system row but already held by a non-system +// row is skipped and recorded as a conflict rather than inserted +// (AC-003.3); the CreateSkill unique-constraint fallback below covers +// the same outcome for the race between this pre-check and the +// insert. +func reconcileBundledSkills( + ctx context.Context, + repo SystemSyncRepo, + wsID string, + bundled map[string]SystemSkillSpec, + existingBySlug map[string]*models.Skill, + nonSystemBySlug map[string]*models.Skill, + result *workspaceSyncResult, +) error { // Walk bundled slugs in sorted order so log output is stable. slugs := make([]string, 0, len(bundled)) for slug := range bundled { @@ -162,12 +291,20 @@ func syncWorkspace( spec := bundled[slug] cur, ok := existingBySlug[slug] if !ok { + if _, taken := nonSystemBySlug[slug]; taken { + result.Conflicted = append(result.Conflicted, slug) + continue + } row := newSystemSkillRow(wsID, spec) if err := repo.CreateSkill(ctx, row); err != nil { - return inserted, updated, removed, fmt.Errorf("insert %s: %w", slug, err) + if isSlugUniqueConstraintErr(err) { + result.Conflicted = append(result.Conflicted, slug) + continue + } + return fmt.Errorf("insert %s: %w", slug, err) } existingBySlug[slug] = row - inserted = append(inserted, slug) + result.Inserted = append(result.Inserted, slug) continue } if systemSkillUpToDate(cur, spec) { @@ -175,37 +312,162 @@ func syncWorkspace( } applySystemSkillUpdate(cur, spec) if err := repo.UpdateSkill(ctx, cur); err != nil { - return inserted, updated, removed, fmt.Errorf("update %s: %w", slug, err) + return fmt.Errorf("update %s: %w", slug, err) } - updated = append(updated, slug) + result.Updated = append(result.Updated, slug) } + return nil +} - for slug, cur := range existingBySlug { +// retireOrphanedSystemSkills deletes system rows whose slug is no +// longer in the bundle. A retirement with a configured replacement +// (retiredDefaultSkillReplacements) only proceeds once that +// replacement row actually exists in this workspace: deleting the +// retired row first would strand every agent that referenced it with +// no rewritten reference (AC-003.1). When the replacement is missing +// this pass (e.g. its insert was withheld by a slug conflict above), +// the retired row is left in place and reported as blocked so a later +// pass can retry once the conflict resolves. +// +// A retired row is removed from existingBySlug along with the DB row: +// normalizeUserSkillSlugs runs after this pass and treats existingBySlug +// as the set of slugs currently taken, so a stale entry for a slug this +// same pass just freed would report a false conflict for a user skill +// that could otherwise normalize onto it. +func retireOrphanedSystemSkills( + ctx context.Context, + repo SystemSyncRepo, + wsID string, + bundled map[string]SystemSkillSpec, + existingBySlug map[string]*models.Skill, + result *workspaceSyncResult, +) error { + // The map is a snapshot, but the order of retirement still matters when + // one retired slug names another as its replacement. Walk it in a stable + // order so the outcome does not depend on Go's map iteration seed. + slugs := make([]string, 0, len(existingBySlug)) + for slug := range existingBySlug { + slugs = append(slugs, slug) + } + sort.Strings(slugs) + for _, slug := range slugs { + cur, present := existingBySlug[slug] + if !present { + continue + } if _, kept := bundled[slug]; kept { continue } - if replacement := replacementSystemSkill(existingBySlug, slug); replacement != nil { + replacementSlug, hasReplacement := retiredDefaultSkillReplacements[slug] + if hasReplacement { + if _, bundledReplacement := bundled[replacementSlug]; !bundledReplacement { + result.Blocked = append(result.Blocked, slug) + continue + } + replacement, found := existingBySlug[replacementSlug] + if !found { + result.Blocked = append(result.Blocked, slug) + continue + } if err := replaceSkillOnAgents(ctx, repo, wsID, cur, replacement); err != nil { - return inserted, updated, removed, fmt.Errorf("replace %s: %w", slug, err) + return fmt.Errorf("replace %s: %w", slug, err) } } if err := repo.DeleteSkill(ctx, cur.ID); err != nil { - return inserted, updated, removed, fmt.Errorf("delete %s: %w", slug, err) + return fmt.Errorf("delete %s: %w", slug, err) } if err := detachSkillFromAgents(ctx, repo, wsID, cur.ID); err != nil { - return inserted, updated, removed, fmt.Errorf("detach %s: %w", slug, err) + return fmt.Errorf("detach %s: %w", slug, err) } - removed = append(removed, slug) + delete(existingBySlug, slug) + result.Removed = append(result.Removed, slug) } - return inserted, updated, removed, nil + return nil } -func replacementSystemSkill(skills map[string]*models.Skill, retiredSlug string) *models.Skill { - replacementSlug, ok := retiredDefaultSkillReplacements[retiredSlug] - if !ok { - return nil +// normalizeUserSkillSlugs rewrites a well-formed but non-canonical +// user/provider skill slug to its canonical kandev-prefixed form +// (AC-003.8). The row ID is preserved, so skill_ids needs no +// rewrite — only slug-keyed desired_skills references are updated. A +// not-well-formed slug (a legacy artifact predating write-time +// validation) is left untouched and logged, never normalized +// (AC-003.9). A normalized value already held by another row is left +// untouched on both sides and logged as a conflict. +// +// The repository performs the row rename and agent-reference rewrite in +// one transaction. This prevents a concurrent writer from claiming the +// canonical slug after the snapshot and leaving references pointed at a +// different row, while also preserving retryability after a failed write. +func normalizeUserSkillSlugs( + ctx context.Context, + repo SystemSyncRepo, + wsID string, + nonSystem []*models.Skill, + existingBySlug map[string]*models.Skill, + nonSystemBySlug map[string]*models.Skill, + result *workspaceSyncResult, + log *logger.Logger, +) error { + taken := make(map[string]bool, len(existingBySlug)+len(nonSystemBySlug)) + for slug := range existingBySlug { + taken[slug] = true } - return skills[replacementSlug] + for slug := range nonSystemBySlug { + taken[slug] = true + } + + for _, row := range nonSystem { + if skillslug.Canonical(row.Slug) { + continue + } + if !skillslug.WellFormed(row.Slug) { + log.Warn("system skill sync: leaving not-well-formed user skill slug untouched", + zap.String("workspace_id", wsID), zap.String("skill_id", row.ID), zap.String("slug", row.Slug)) + continue + } + normalized := skillslug.Normalize(row.Slug) + if taken[normalized] { + log.Warn("system skill sync: leaving conflicting user skill slug unnormalized", + zap.String("workspace_id", wsID), zap.String("skill_id", row.ID), + zap.String("slug", row.Slug), zap.String("normalized_slug", normalized)) + result.Conflicted = append(result.Conflicted, row.Slug) + continue + } + oldSlug := row.Slug + changed, err := repo.NormalizeSkillSlug(ctx, wsID, row.ID, oldSlug, normalized) + if err != nil { + if isSlugUniqueConstraintErr(err) { + result.Conflicted = append(result.Conflicted, oldSlug) + continue + } + return fmt.Errorf("normalize slug %s: %w", oldSlug, err) + } + if !changed { + continue + } + taken[normalized] = true + result.Normalized = append(result.Normalized, oldSlug+"->"+normalized) + } + return nil +} + +// isSlugUniqueConstraintErr reports whether err is a UNIQUE constraint +// violation for a skill slug. SQLite's driver requires string matching, +// while pgx exposes a typed SQLSTATE 23505 error. The PostgreSQL +// constraint-name check avoids treating an unrelated duplicate (for +// example, a primary-key collision) as a slug conflict. +func isSlugUniqueConstraintErr(err error) bool { + if err == nil { + return false + } + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) { + return pgErr.Code == "23505" && + (pgErr.ConstraintName == "" || pgErr.ConstraintName == "office_skills_workspace_id_slug_key") + } + msg := err.Error() + return strings.Contains(msg, "UNIQUE constraint failed") || + strings.Contains(msg, "constraint failed: UNIQUE") } func replaceSkillOnAgents( @@ -273,13 +535,22 @@ func systemSkillUpToDate(cur *models.Skill, spec SystemSkillSpec) bool { cur.SystemVersion == spec.Version } +// replaceJSONArrayValue rewrites every occurrence of oldValue to +// newValue in a JSON-array-encoded agent_profiles column, also +// de-duplicating and dropping empties. raw may also be a legacy +// comma-separated value (see ParseDesiredSlugs in injection.go, which +// reads both formats); a legacy value is parsed the same way and, if +// changed, re-encoded as the canonical JSON-array form — this is how +// a legacy desired_skills column migrates format the next time a +// rename or replacement touches one of its entries. An unchanged +// legacy value is left exactly as found (no unnecessary rewrite). func replaceJSONArrayValue(raw, oldValue, newValue string) (string, bool) { raw = strings.TrimSpace(raw) if raw == "" || raw == "null" { return raw, false } - var values []string - if err := json.Unmarshal([]byte(raw), &values); err != nil { + values, ok := parseJSONOrLegacyCSVArray(raw) + if !ok { return raw, false } out := make([]string, 0, len(values)) @@ -309,6 +580,28 @@ func replaceJSONArrayValue(raw, oldValue, newValue string) (string, bool) { return string(encoded), true } +// parseJSONOrLegacyCSVArray parses raw as a JSON string array, falling +// back to splitting it as a legacy comma-separated value when raw +// isn't a JSON array. Mirrors ParseDesiredSlugs's format detection +// (injection.go) so both readers of these columns agree on what +// counts as "legacy". Returns ok=false only for malformed JSON input +// (a corrupt column stays a safe no-op for the caller). +func parseJSONOrLegacyCSVArray(raw string) ([]string, bool) { + if strings.HasPrefix(raw, "[") { + var values []string + if err := json.Unmarshal([]byte(raw), &values); err != nil { + return nil, false + } + return values, true + } + parts := strings.Split(raw, ",") + out := make([]string, len(parts)) + for i, p := range parts { + out[i] = strings.TrimSpace(p) + } + return out, true +} + // removeIDFromJSONArray parses a JSON-array string, removes every // occurrence of `id`, and returns the re-encoded array along with a // flag indicating whether anything was removed. Malformed input is diff --git a/apps/backend/internal/office/skills/system_sync_migration_test.go b/apps/backend/internal/office/skills/system_sync_migration_test.go new file mode 100644 index 0000000000..c6cc8c1fc3 --- /dev/null +++ b/apps/backend/internal/office/skills/system_sync_migration_test.go @@ -0,0 +1,574 @@ +package skills_test + +import ( + "context" + "strings" + "testing" + + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/skills" +) + +// TestSyncSystemSkills_ReplacesRetiredMemorySkillReference pins the +// concrete REQ-003 migration case this feature adds: once the bundled +// `memory` skill is renamed to `kandev-memory` for naming-scheme +// consistency (pending, tracked separately), a workspace with the old +// row must retire it and end up with the new canonical slug in its +// place. This test injects a synthetic bundled spec to exercise that +// path ahead of the real rename. +func TestSyncSystemSkills_ReplacesRetiredMemorySkillReference(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + "memory": { + ID: "skill-old-memory", + WorkspaceID: "ws-1", + Slug: "memory", + Name: "Memory", + IsSystem: true, + SourceType: skills.SourceTypeSystem, + }, + } + + bundled := []skills.SystemSkillSpec{{ + Slug: "kandev-memory", + Name: "Memory", + Description: "Memory guidance", + Version: "1.0.0", + Content: "---\nname: kandev-memory\ndescription: Memory guidance\n---\n# Memory\n", + ContentHash: "hash-kandev-memory", + }} + + report, err := skills.SyncSystemSkills(context.Background(), repo, []string{"ws-1"}, bundled, log) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Removed) != 1 || !strings.HasSuffix(report.Removed[0], "memory") { + t.Fatalf("expected retirement of memory, got removed=%v", report.Removed) + } + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "memory"); err == nil { + t.Error("retired memory row still present after sync") + } + got, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-memory") + if err != nil { + t.Fatalf("replacement kandev-memory missing: %v", err) + } + if !got.IsSystem { + t.Error("kandev-memory replacement missing is_system flag") + } +} + +// TestSyncSystemSkills_RetirementBlockedWhenReplacementMissing pins the +// F19-round fix: a retired skill with a configured replacement must +// not be deleted (and its agent references detached) until the +// replacement row actually exists in the workspace. Deleting first +// would strand every agent that referenced it with no rewritten +// reference. When the replacement did not land this pass, the row +// stays in place and is reported as blocked for a later pass to +// retry. +func TestSyncSystemSkills_RetirementBlockedWhenReplacementMissing(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + const oldTasksID = "skill-old-tasks" + repo.rows["ws-1"] = map[string]*models.Skill{ + "kandev-tasks": { + ID: oldTasksID, + WorkspaceID: "ws-1", + Slug: "kandev-tasks", + Name: "Tasks", + IsSystem: true, + SourceType: skills.SourceTypeSystem, + }, + } + repo.agents["ws-1"] = map[string]*settingsmodels.AgentProfile{ + "agent-1": { + ID: "agent-1", + WorkspaceID: "ws-1", + SkillIDs: mustJSONArray(t, []string{oldTasksID}), + DesiredSkills: mustJSONArray(t, []string{"kandev-tasks"}), + }, + } + + // kandev-task-ops (the configured replacement for kandev-tasks) is + // neither an existing row nor part of this pass's bundle, so it + // cannot be created this pass either. + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Removed) != 0 { + t.Fatalf("expected no removal while replacement is missing, got %v", report.Removed) + } + if len(report.Blocked) != 1 || !strings.HasSuffix(report.Blocked[0], "kandev-tasks") { + t.Fatalf("expected kandev-tasks reported as blocked, got %v", report.Blocked) + } + + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-tasks"); err != nil { + t.Error("blocked retirement must leave the office_skills row in place") + } + agent1 := repo.agents["ws-1"]["agent-1"] + if got := decodeIDs(t, agent1.SkillIDs); len(got) != 1 || got[0] != oldTasksID { + t.Errorf("blocked retirement must leave agent skill_ids untouched, got %v", got) + } + if got := decodeIDs(t, agent1.DesiredSkills); len(got) != 1 || got[0] != "kandev-tasks" { + t.Errorf("blocked retirement must leave agent desired_skills untouched, got %v", got) + } +} + +// TestSyncSystemSkills_RetirementBlockedWhenReplacementIsNotBundled ensures +// an old system row is not retired merely because a replacement row happens +// to exist in the database. The replacement must be part of the current +// bundle; otherwise this pass can delete both rows and strand references. +func TestSyncSystemSkills_RetirementBlockedWhenReplacementIsNotBundled(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + const oldTasksID = "skill-old-tasks" + repo.rows["ws-1"] = map[string]*models.Skill{ + "kandev-tasks": { + ID: oldTasksID, + WorkspaceID: "ws-1", + Slug: "kandev-tasks", + Name: "Tasks", + IsSystem: true, + SourceType: skills.SourceTypeSystem, + }, + "kandev-task-ops": { + ID: "skill-task-ops", + WorkspaceID: "ws-1", + Slug: "kandev-task-ops", + Name: "Task operations", + IsSystem: true, + SourceType: skills.SourceTypeSystem, + }, + } + repo.agents["ws-1"] = map[string]*settingsmodels.AgentProfile{ + "agent-1": { + ID: "agent-1", + WorkspaceID: "ws-1", + SkillIDs: mustJSONArray(t, []string{oldTasksID}), + DesiredSkills: mustJSONArray(t, []string{"kandev-tasks"}), + }, + } + + // Neither existing row is present in this pass's bundle. The configured + // replacement is therefore not a valid destination for retirement. + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Blocked) != 1 || !strings.HasSuffix(report.Blocked[0], "kandev-tasks") { + t.Fatalf("expected kandev-tasks reported as blocked, got %v", report.Blocked) + } + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-tasks"); err != nil { + t.Fatalf("blocked retired row must remain: %v", err) + } + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-task-ops"); err == nil { + t.Fatal("unbundled replacement row should be retired independently") + } + agent1 := repo.agents["ws-1"]["agent-1"] + if got := decodeIDs(t, agent1.SkillIDs); len(got) != 1 || got[0] != oldTasksID { + t.Errorf("blocked retirement must leave agent skill_ids untouched, got %v", got) + } + if got := decodeIDs(t, agent1.DesiredSkills); len(got) != 1 || got[0] != "kandev-tasks" { + t.Errorf("blocked retirement must leave agent desired_skills untouched, got %v", got) + } +} + +// TestSyncSystemSkills_BundledInsertConflictsWithExistingUserSkill pins +// AC-003.3: a bundled slug not yet backed by a system row, but already +// held by a non-system (user or provider-imported) row, must not be +// inserted over that row. It is skipped and recorded as a conflict so +// the mismatch is visible without silently overwriting user data. +func TestSyncSystemSkills_BundledInsertConflictsWithExistingUserSkill(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + "kandev-custom-thing": { + ID: "user-skill-1", + WorkspaceID: "ws-1", + Slug: "kandev-custom-thing", + Name: "My Custom Thing", + IsSystem: false, + SourceType: "inline", + }, + } + + bundled := []skills.SystemSkillSpec{{ + Slug: "kandev-custom-thing", + Name: "Custom Thing", + Description: "Bundled version", + Version: "1.0.0", + Content: "---\nname: kandev-custom-thing\ndescription: Bundled version\n---\n# Custom Thing\n", + ContentHash: "hash-custom-thing", + }} + + report, err := skills.SyncSystemSkills(context.Background(), repo, []string{"ws-1"}, bundled, log) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Inserted) != 0 { + t.Fatalf("expected no insert over an existing user skill, got %v", report.Inserted) + } + if len(report.Conflicted) != 1 || !strings.HasSuffix(report.Conflicted[0], "kandev-custom-thing") { + t.Fatalf("expected kandev-custom-thing reported as conflicted, got %v", report.Conflicted) + } + got, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-custom-thing") + if err != nil { + t.Fatalf("existing user row missing after sync: %v", err) + } + if got.IsSystem { + t.Error("existing user row must not become a system row") + } + if got.ID != "user-skill-1" { + t.Errorf("existing user row ID changed: now %s", got.ID) + } +} + +// TestSyncSystemSkills_NormalizesWellFormedUserSlugToCanonical pins +// AC-003.8: a well-formed but non-canonical user/provider skill slug +// is rewritten to its canonical kandev- prefixed form in place, +// preserving the row ID, and every agent's desired_skills reference +// is rewritten to match. +func TestSyncSystemSkills_NormalizesWellFormedUserSlugToCanonical(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + "my-custom-skill": { + ID: "user-skill-1", + WorkspaceID: "ws-1", + Slug: "my-custom-skill", + Name: "My Custom Skill", + IsSystem: false, + SourceType: "inline", + }, + } + repo.agents["ws-1"] = map[string]*settingsmodels.AgentProfile{ + "agent-1": { + ID: "agent-1", + WorkspaceID: "ws-1", + DesiredSkills: mustJSONArray(t, []string{"my-custom-skill"}), + }, + } + + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Normalized) != 1 || !strings.HasSuffix(report.Normalized[0], "my-custom-skill->kandev-my-custom-skill") { + t.Fatalf("expected my-custom-skill normalized, got %v", report.Normalized) + } + + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "my-custom-skill"); err == nil { + t.Error("old slug should no longer resolve after normalization") + } + got, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-my-custom-skill") + if err != nil { + t.Fatalf("normalized row missing: %v", err) + } + if got.ID != "user-skill-1" { + t.Errorf("normalization must preserve row ID, got %s", got.ID) + } + + agent1 := repo.agents["ws-1"]["agent-1"] + if got := decodeIDs(t, agent1.DesiredSkills); len(got) != 1 || got[0] != "kandev-my-custom-skill" { + t.Errorf("agent-1.desired_skills = %v, want [kandev-my-custom-skill]", got) + } +} + +// TestSyncSystemSkills_LeavesNotWellFormedUserSlugUntouched pins +// AC-003.9: a legacy row whose slug predates write-time validation +// and is not well-formed (contains characters outside the safe path +// component set) must never be normalized — there is no safe +// canonical rewrite for it, so the sync leaves it exactly as it was +// found. +func TestSyncSystemSkills_LeavesNotWellFormedUserSlugUntouched(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + const badSlug = "legacy slug!" + repo.rows["ws-1"] = map[string]*models.Skill{ + badSlug: { + ID: "user-skill-legacy", + WorkspaceID: "ws-1", + Slug: badSlug, + Name: "Legacy", + IsSystem: false, + SourceType: "inline", + }, + } + + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Normalized) != 0 { + t.Fatalf("not-well-formed slug must never be normalized, got %v", report.Normalized) + } + if len(report.Conflicted) != 0 { + t.Fatalf("not-well-formed slug is not a conflict, got %v", report.Conflicted) + } + got, err := repo.GetSkillBySlug(context.Background(), "ws-1", badSlug) + if err != nil { + t.Fatalf("legacy row missing after sync: %v", err) + } + if got.Slug != badSlug { + t.Errorf("legacy row slug changed: now %q", got.Slug) + } +} + +// TestSyncSystemSkills_LeavesConflictingNormalizedUserSlugUntouched +// pins the collision branch of AC-003.8: when the canonical form a +// well-formed slug would normalize to is already held by another row, +// neither row is touched and the collision is reported so an operator +// can resolve it manually. +func TestSyncSystemSkills_LeavesConflictingNormalizedUserSlugUntouched(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + "my-thing": { + ID: "user-skill-a", + WorkspaceID: "ws-1", + Slug: "my-thing", + Name: "My Thing (legacy)", + IsSystem: false, + SourceType: "inline", + }, + "kandev-my-thing": { + ID: "user-skill-b", + WorkspaceID: "ws-1", + Slug: "kandev-my-thing", + Name: "My Thing (canonical)", + IsSystem: false, + SourceType: "inline", + }, + } + + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Normalized) != 0 { + t.Fatalf("colliding slug must not be normalized, got %v", report.Normalized) + } + if len(report.Conflicted) != 1 || !strings.HasSuffix(report.Conflicted[0], "my-thing") { + t.Fatalf("expected my-thing reported as conflicted, got %v", report.Conflicted) + } + + a, err := repo.GetSkillBySlug(context.Background(), "ws-1", "my-thing") + if err != nil || a.ID != "user-skill-a" { + t.Errorf("legacy row must stay at its original slug, got %+v err=%v", a, err) + } + b, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-my-thing") + if err != nil || b.ID != "user-skill-b" { + t.Errorf("canonical row must be untouched, got %+v err=%v", b, err) + } +} + +// TestSyncSystemSkills_NormalizationRetriesAfterFailedReferenceRewrite +// pins the recovery contract in +// docs/specs/agents/system-design/injected-skill-naming-migration.md +// ("## Failure and recovery"): a normalization interrupted between the +// row rename and the agent-reference rewrite must be fully retried on +// the next pass, not silently left half-done. If the row were renamed +// to its canonical slug before the agent-reference rewrite is +// confirmed, a later pass would see the row as already canonical and +// skip it forever, stranding the stale agent reference. +func TestSyncSystemSkills_NormalizationRetriesAfterFailedReferenceRewrite(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + "my-custom-skill": { + ID: "user-skill-1", + WorkspaceID: "ws-1", + Slug: "my-custom-skill", + Name: "My Custom Skill", + IsSystem: false, + SourceType: "inline", + }, + } + repo.agents["ws-1"] = map[string]*settingsmodels.AgentProfile{ + "agent-1": { + ID: "agent-1", + WorkspaceID: "ws-1", + DesiredSkills: mustJSONArray(t, []string{"my-custom-skill"}), + }, + } + repo.failUpdateAgentFor["agent-1"] = true + + // SyncSystemSkills logs and continues past a per-workspace failure + // (so one bad workspace doesn't block the rest) rather than + // returning it, so the top-level call succeeds; the failure is + // checked via the row/agent state left behind instead. + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Normalized) != 0 { + t.Fatalf("normalization must not be reported while the reference rewrite failed, got %v", report.Normalized) + } + + // The row must NOT have been committed to its canonical slug: a + // half-done normalization (row renamed, reference not rewritten) + // is exactly the state the recovery contract forbids, because the + // canonical-slug gate would then skip this row on every later pass. + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "my-custom-skill"); err != nil { + t.Fatalf("row must stay at its original slug after a failed rewrite, got err=%v", err) + } + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-my-custom-skill"); err == nil { + t.Fatal("row must not be committed to its canonical slug before the reference rewrite succeeds") + } + agent1 := repo.agents["ws-1"]["agent-1"] + if got := decodeIDs(t, agent1.DesiredSkills); len(got) != 1 || got[0] != "my-custom-skill" { + t.Errorf("agent-1.desired_skills must stay untouched after a failed rewrite, got %v", got) + } + + // A later pass, with the transient failure gone, must fully + // converge: this is the "next pass repeats the reference rewrite" + // half of the recovery contract. + repo.failUpdateAgentFor["agent-1"] = false + report, err = skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("retry sync: %v", err) + } + if len(report.Normalized) != 1 || !strings.HasSuffix(report.Normalized[0], "my-custom-skill->kandev-my-custom-skill") { + t.Fatalf("expected my-custom-skill normalized on retry, got %v", report.Normalized) + } + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-my-custom-skill"); err != nil { + t.Fatalf("row must be canonical after the retry succeeds: %v", err) + } + agent1 = repo.agents["ws-1"]["agent-1"] + if got := decodeIDs(t, agent1.DesiredSkills); len(got) != 1 || got[0] != "kandev-my-custom-skill" { + t.Errorf("agent-1.desired_skills = %v, want [kandev-my-custom-skill] after retry", got) + } +} + +// TestSyncSystemSkills_RetiredSystemSlugFreedForSamePassNormalization +// pins a sequencing edge in syncWorkspace: retireOrphanedSystemSkills +// runs before normalizeUserSkillSlugs, and a system row it retires in +// this same pass must not block a user skill from normalizing onto +// that now-freed slug. Treating the just-deleted row as still "taken" +// would report a false conflict for a slug that is actually free. +func TestSyncSystemSkills_RetiredSystemSlugFreedForSamePassNormalization(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + // A stale system row for a slug no longer in the bundle, with + // no configured replacement, so it retires outright this pass. + "kandev-legacy-widget": { + ID: "system-skill-legacy", + WorkspaceID: "ws-1", + Slug: "kandev-legacy-widget", + Name: "Legacy Widget", + IsSystem: true, + SourceType: skills.SourceTypeSystem, + }, + // A user skill whose canonical target is exactly the slug the + // system row above is retiring in this same pass. + "legacy-widget": { + ID: "user-skill-1", + WorkspaceID: "ws-1", + Slug: "legacy-widget", + Name: "My Legacy Widget", + IsSystem: false, + SourceType: "inline", + }, + } + + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Removed) != 1 || !strings.HasSuffix(report.Removed[0], "kandev-legacy-widget") { + t.Fatalf("expected kandev-legacy-widget retired, got removed=%v", report.Removed) + } + if len(report.Conflicted) != 0 { + t.Fatalf("slug freed by same-pass retirement must not be reported as conflicted, got %v", report.Conflicted) + } + if len(report.Normalized) != 1 || !strings.HasSuffix(report.Normalized[0], "legacy-widget->kandev-legacy-widget") { + t.Fatalf("expected legacy-widget normalized onto the freed slug, got %v", report.Normalized) + } + got, err := repo.GetSkillBySlug(context.Background(), "ws-1", "kandev-legacy-widget") + if err != nil { + t.Fatalf("normalized row missing: %v", err) + } + if got.ID != "user-skill-1" { + t.Errorf("normalization must preserve the user row's ID, got %s", got.ID) + } + if got.IsSystem { + t.Error("normalized user row must not become a system row") + } +} + +// TestSyncSystemSkills_RenameRewritesLegacyCommaSeparatedDesiredSkills +// pins a format gap in the agent-reference rewrite: desired_skills +// predates the JSON-array persistence format on some rows and is +// still a legacy comma-separated string (ParseDesiredSlugs reads +// both; see injection.go). A slug rename must rewrite that legacy +// value too — otherwise the agent is left holding a reference to a +// slug that no longer exists once the row is renamed to its +// canonical form. +func TestSyncSystemSkills_RenameRewritesLegacyCommaSeparatedDesiredSkills(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + repo.rows["ws-1"] = map[string]*models.Skill{ + "my-custom-skill": { + ID: "user-skill-1", + WorkspaceID: "ws-1", + Slug: "my-custom-skill", + Name: "My Custom Skill", + IsSystem: false, + SourceType: "inline", + }, + } + repo.agents["ws-1"] = map[string]*settingsmodels.AgentProfile{ + "agent-1": { + ID: "agent-1", + WorkspaceID: "ws-1", + DesiredSkills: "my-custom-skill,other-skill", + }, + } + + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Normalized) != 1 || !strings.HasSuffix(report.Normalized[0], "my-custom-skill->kandev-my-custom-skill") { + t.Fatalf("expected my-custom-skill normalized, got %v", report.Normalized) + } + + agent1 := repo.agents["ws-1"]["agent-1"] + got := decodeIDs(t, agent1.DesiredSkills) + want := []string{"kandev-my-custom-skill", "other-skill"} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { + t.Errorf("agent-1.desired_skills = %q (parsed %v), want %v", agent1.DesiredSkills, got, want) + } +} diff --git a/apps/backend/internal/office/skills/system_sync_race_test.go b/apps/backend/internal/office/skills/system_sync_race_test.go new file mode 100644 index 0000000000..484114d8c4 --- /dev/null +++ b/apps/backend/internal/office/skills/system_sync_race_test.go @@ -0,0 +1,217 @@ +package skills_test + +import ( + "context" + "encoding/json" + "errors" + "strings" + "sync" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/skills" +) + +var syncTestHooks = struct { + sync.Mutex + createSkillErr map[*stubSyncRepo]map[string]error + normalizeConflictFor map[*stubSyncRepo]map[string]bool +}{ + createSkillErr: map[*stubSyncRepo]map[string]error{}, + normalizeConflictFor: map[*stubSyncRepo]map[string]bool{}, +} + +func syncTestCreateSkillError(repo *stubSyncRepo, slug string) error { + syncTestHooks.Lock() + defer syncTestHooks.Unlock() + return syncTestHooks.createSkillErr[repo][slug] +} + +func setSyncTestCreateSkillError(repo *stubSyncRepo, slug string, err error) { + syncTestHooks.Lock() + defer syncTestHooks.Unlock() + if syncTestHooks.createSkillErr[repo] == nil { + syncTestHooks.createSkillErr[repo] = map[string]error{} + } + syncTestHooks.createSkillErr[repo][slug] = err +} + +func setSyncTestNormalizeConflict(repo *stubSyncRepo, slug string) { + syncTestHooks.Lock() + defer syncTestHooks.Unlock() + if syncTestHooks.normalizeConflictFor[repo] == nil { + syncTestHooks.normalizeConflictFor[repo] = map[string]bool{} + } + syncTestHooks.normalizeConflictFor[repo][slug] = true +} + +func syncTestNormalizeConflict(repo *stubSyncRepo, slug string) bool { + syncTestHooks.Lock() + defer syncTestHooks.Unlock() + return syncTestHooks.normalizeConflictFor[repo][slug] +} + +// NormalizeSkillSlug mirrors the repository contract: the slug row and +// every affected desired_skills profile are committed as one operation. +// The in-memory implementation applies all changes only after it has +// confirmed that every profile update can succeed. +func (s *stubSyncRepo) NormalizeSkillSlug( + _ context.Context, workspaceID, skillID, oldSlug, newSlug string, +) (bool, error) { + ws := s.rows[workspaceID] + row, ok := ws[oldSlug] + if !ok || row.ID != skillID || row.IsSystem { + return false, nil + } + if syncTestNormalizeConflict(s, oldSlug) { + ws[newSlug] = &models.Skill{ + ID: "concurrent-" + newSlug, + WorkspaceID: workspaceID, + Slug: newSlug, + IsSystem: false, + } + return false, &pgconn.PgError{ + Code: "23505", + ConstraintName: "office_skills_workspace_id_slug_key", + } + } + if _, exists := ws[newSlug]; exists { + return false, &pgconn.PgError{ + Code: "23505", + ConstraintName: "office_skills_workspace_id_slug_key", + } + } + + type agentUpdate struct { + id string + desired string + } + updates := make([]agentUpdate, 0) + for id, agent := range s.agents[workspaceID] { + desired, changed := replaceStubJSONArrayValue(agent.DesiredSkills, oldSlug, newSlug) + if !changed { + continue + } + if s.failUpdateAgentFor[id] { + return false, errors.New("injected failure: agent update unavailable") + } + updates = append(updates, agentUpdate{id: id, desired: desired}) + } + + delete(ws, oldSlug) + copyRow := *row + copyRow.Slug = newSlug + ws[newSlug] = ©Row + for _, update := range updates { + copyAgent := *s.agents[workspaceID][update.id] + copyAgent.DesiredSkills = update.desired + s.agents[workspaceID][update.id] = ©Agent + } + return true, nil +} + +func replaceStubJSONArrayValue(raw, oldValue, newValue string) (string, bool) { + values := skills.ParseDesiredSlugs(raw) + if len(values) == 0 { + return raw, false + } + out := make([]string, 0, len(values)) + seen := make(map[string]bool, len(values)) + changed := false + for _, value := range values { + if value == oldValue { + value = newValue + changed = true + } + if value == "" || seen[value] { + if value != "" { + changed = true + } + continue + } + seen[value] = true + out = append(out, value) + } + if !changed { + return raw, false + } + encoded, err := json.Marshal(out) + if err != nil { + return raw, false + } + return string(encoded), true +} + +func TestSyncSystemSkills_NormalizationConflictLeavesReferencesUntouched(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + + const oldSlug = "my-custom-skill" + repo.rows["ws-1"] = map[string]*models.Skill{ + oldSlug: { + ID: "user-skill-1", + WorkspaceID: "ws-1", + Slug: oldSlug, + Name: "My Custom Skill", + IsSystem: false, + SourceType: "inline", + }, + } + repo.agents["ws-1"] = map[string]*settingsmodels.AgentProfile{ + "agent-1": { + ID: "agent-1", + WorkspaceID: "ws-1", + DesiredSkills: mustJSONArray(t, []string{oldSlug}), + }, + } + setSyncTestNormalizeConflict(repo, oldSlug) + + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, []skills.SystemSkillSpec{}, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Normalized) != 0 { + t.Fatalf("conflicting normalization must not be reported as complete: %v", report.Normalized) + } + if len(report.Conflicted) != 1 || !strings.HasSuffix(report.Conflicted[0], oldSlug) { + t.Fatalf("expected %s reported as conflicted, got %v", oldSlug, report.Conflicted) + } + if _, err := repo.GetSkillBySlug(context.Background(), "ws-1", oldSlug); err != nil { + t.Fatalf("original user row must remain after conflict: %v", err) + } + agent1 := repo.agents["ws-1"]["agent-1"] + if got := decodeIDs(t, agent1.DesiredSkills); len(got) != 1 || got[0] != oldSlug { + t.Errorf("agent desired_skills changed despite atomic conflict: %v", got) + } +} + +func TestSyncSystemSkills_ContinuesAfterPostgresSlugConflict(t *testing.T) { + repo := newStubSyncRepo() + log := logger.Default() + setSyncTestCreateSkillError(repo, "kandev-conflicting", &pgconn.PgError{ + Code: "23505", + ConstraintName: "office_skills_workspace_id_slug_key", + }) + + bundled := []skills.SystemSkillSpec{ + {Slug: "kandev-conflicting", Name: "Conflict", ContentHash: "hash-conflict"}, + {Slug: "kandev-next", Name: "Next", ContentHash: "hash-next"}, + } + report, err := skills.SyncSystemSkills( + context.Background(), repo, []string{"ws-1"}, bundled, log, + ) + if err != nil { + t.Fatalf("sync: %v", err) + } + if len(report.Conflicted) != 1 || !strings.HasSuffix(report.Conflicted[0], "kandev-conflicting") { + t.Fatalf("expected PostgreSQL slug conflict to be reported, got %v", report.Conflicted) + } + if len(report.Inserted) != 1 || !strings.HasSuffix(report.Inserted[0], "kandev-next") { + t.Fatalf("sync should continue after PostgreSQL conflict, inserted=%v", report.Inserted) + } +} diff --git a/apps/backend/internal/office/skills/system_sync_test.go b/apps/backend/internal/office/skills/system_sync_test.go index 1656e978df..b54442452a 100644 --- a/apps/backend/internal/office/skills/system_sync_test.go +++ b/apps/backend/internal/office/skills/system_sync_test.go @@ -21,12 +21,19 @@ import ( type stubSyncRepo struct { rows map[string]map[string]*models.Skill // workspaceID → slug → row agents map[string]map[string]*settingsmodels.AgentProfile // workspaceID → agentID → profile + + // failUpdateAgentFor injects an error from UpdateAgentInstance for + // the named agent ID, so a test can simulate a mid-sequence failure + // (e.g. the agent-reference rewrite half of a slug normalization) + // without it actually persisting. + failUpdateAgentFor map[string]bool } func newStubSyncRepo() *stubSyncRepo { return &stubSyncRepo{ - rows: map[string]map[string]*models.Skill{}, - agents: map[string]map[string]*settingsmodels.AgentProfile{}, + rows: map[string]map[string]*models.Skill{}, + agents: map[string]map[string]*settingsmodels.AgentProfile{}, + failUpdateAgentFor: map[string]bool{}, } } @@ -44,6 +51,20 @@ func (s *stubSyncRepo) ListSystemSkills( return out, nil } +func (s *stubSyncRepo) ListNonSystemSkills( + _ context.Context, workspaceID string, +) ([]*models.Skill, error) { + ws := s.rows[workspaceID] + out := make([]*models.Skill, 0, len(ws)) + for _, sk := range ws { + if !sk.IsSystem { + out = append(out, sk) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].Slug < out[j].Slug }) + return out, nil +} + func (s *stubSyncRepo) GetSkillBySlug( _ context.Context, workspaceID, slug string, ) (*models.Skill, error) { @@ -56,6 +77,9 @@ func (s *stubSyncRepo) GetSkillBySlug( } func (s *stubSyncRepo) CreateSkill(_ context.Context, skill *models.Skill) error { + if err := syncTestCreateSkillError(s, skill.Slug); err != nil { + return err + } if _, ok := s.rows[skill.WorkspaceID]; !ok { s.rows[skill.WorkspaceID] = map[string]*models.Skill{} } @@ -67,9 +91,19 @@ func (s *stubSyncRepo) CreateSkill(_ context.Context, skill *models.Skill) error return nil } +// UpdateSkill mirrors the real repository's update-by-ID semantics +// (sqlite.Repository.UpdateSkill writes `WHERE id = ?`): it locates +// the row by ID, not by the caller-supplied slug, so a slug rewrite +// (e.g. normalizeUserSkillSlugs) re-keys the map instead of failing +// to find a row under its now-stale old slug. func (s *stubSyncRepo) UpdateSkill(_ context.Context, skill *models.Skill) error { - if ws, ok := s.rows[skill.WorkspaceID]; ok { - if _, ok := ws[skill.Slug]; ok { + ws, ok := s.rows[skill.WorkspaceID] + if !ok { + return errors.New("not found for update") + } + for slug, sk := range ws { + if sk.ID == skill.ID { + delete(ws, slug) copy := *skill ws[skill.Slug] = © return nil @@ -90,13 +124,18 @@ func (s *stubSyncRepo) DeleteSkill(_ context.Context, id string) error { return errors.New("not found for delete") } +// ListAgentInstances returns copies, matching the real repository's +// read-from-DB semantics: a caller mutating a returned profile (as +// renameSkillSlugOnAgents does before its UpdateAgentInstance call) +// must not silently affect stored state until that write succeeds. func (s *stubSyncRepo) ListAgentInstances( _ context.Context, workspaceID string, ) ([]*settingsmodels.AgentProfile, error) { ws := s.agents[workspaceID] out := make([]*settingsmodels.AgentProfile, 0, len(ws)) for _, a := range ws { - out = append(out, a) + copy := *a + out = append(out, ©) } sort.Slice(out, func(i, j int) bool { return out[i].ID < out[j].ID }) return out, nil @@ -105,6 +144,9 @@ func (s *stubSyncRepo) ListAgentInstances( func (s *stubSyncRepo) UpdateAgentInstance( _ context.Context, agent *settingsmodels.AgentProfile, ) error { + if s.failUpdateAgentFor[agent.ID] { + return errors.New("injected failure: agent update unavailable") + } ws, ok := s.agents[agent.WorkspaceID] if !ok { return errors.New("workspace not found") diff --git a/apps/web/e2e/tests/office/skill-injection.spec.ts b/apps/web/e2e/tests/office/skill-injection.spec.ts index 298bf88eb4..8068606021 100644 --- a/apps/web/e2e/tests/office/skill-injection.spec.ts +++ b/apps/web/e2e/tests/office/skill-injection.spec.ts @@ -30,11 +30,15 @@ test("skill injection writes assigned skill to session worktree on launch", asyn slug, content, })) as { id: string; slug: string }; - expect(skill.slug).toBe(slug); + // A caller-supplied slug is normalized to its canonical kandev-prefixed + // form on write (AC-001.12); the persisted identity downstream is this + // canonical slug, not the one the request supplied. + const canonicalSlug = `kandev-${slug}`; + expect(skill.slug).toBe(canonicalSlug); // 2. Assign the skill to the seed agent profile via the test harness. // desired_skills is read from agent_profiles at launch time. - await apiClient.setProfileDesiredSkills(seedData.agentProfileId, [slug]); + await apiClient.setProfileDesiredSkills(seedData.agentProfileId, [canonicalSlug]); try { // 3. Launch a session via createTaskWithAgent (start_agent: true). @@ -65,12 +69,14 @@ test("skill injection writes assigned skill to session worktree on launch", asyn .not.toBe(""); // 5. Skill landed at the spec-defined location with the right content. - const skillFile = path.join(worktreePath, PROJECT_SKILL_DIR, `kandev-${slug}`, "SKILL.md"); + const skillFile = path.join(worktreePath, PROJECT_SKILL_DIR, canonicalSlug, "SKILL.md"); await expect .poll(() => fs.existsSync(skillFile), { timeout: 15_000, message: skillFile }) .toBe(true); const skillFileContent = fs.readFileSync(skillFile, "utf8"); - expect(skillFileContent).toContain(`---\nname: ${slug}\ndescription: ${slug}\n---\n`); + expect(skillFileContent).toContain( + `---\nname: ${canonicalSlug}\ndescription: ${canonicalSlug}\n---\n`, + ); expect(skillFileContent).toContain(`Marker: ${slug}-content`); // 6. .git/info/exclude (resolved through .git file for linked worktrees) diff --git a/apps/web/e2e/tests/office/skills-readonly.spec.ts b/apps/web/e2e/tests/office/skills-readonly.spec.ts index f3ba666885..5cb5ee90db 100644 --- a/apps/web/e2e/tests/office/skills-readonly.spec.ts +++ b/apps/web/e2e/tests/office/skills-readonly.spec.ts @@ -49,7 +49,9 @@ test.describe("Office skills - Monaco read-only mode", () => { slug: unique, content: "# Editable\n\nWorkspace-scope skill content.\n", })) as { id?: string; slug?: string }; - expect(created.slug).toBe(unique); + // A caller-supplied slug is normalized to its canonical kandev-prefixed + // form on write (AC-001.12). + expect(created.slug).toBe(`kandev-${unique}`); await testPage.goto("/office/workspace/skills"); diff --git a/apps/web/e2e/tests/office/skills.spec.ts b/apps/web/e2e/tests/office/skills.spec.ts index 66e1629c73..8c95e2186e 100644 --- a/apps/web/e2e/tests/office/skills.spec.ts +++ b/apps/web/e2e/tests/office/skills.spec.ts @@ -10,7 +10,9 @@ test.describe("Skills", () => { slug: "test-skill", content: "# Test\n\nThis is a test skill.", }); - expect((skill as Record).slug).toBe("test-skill"); + // A caller-supplied slug is normalized to its canonical kandev-prefixed + // form on write (AC-001.12). + expect((skill as Record).slug).toBe("kandev-test-skill"); const skills = await officeApi.listSkills(officeSeed.workspaceId); const list = @@ -18,7 +20,7 @@ test.describe("Skills", () => { (skills as unknown as Record[]); expect( Array.isArray(list) - ? list.some((s) => (s as Record).slug === "test-skill") + ? list.some((s) => (s as Record).slug === "kandev-test-skill") : false, ).toBe(true); }); @@ -32,7 +34,7 @@ test.describe("Skills", () => { const id = (created as Record).id as string; const fetched = await officeApi.getSkill(id); expect((fetched as Record).id).toBe(id); - expect((fetched as Record).slug).toBe("get-skill"); + expect((fetched as Record).slug).toBe("kandev-get-skill"); }); test("delete skill removes it from list", async ({ officeApi, officeSeed }) => { @@ -84,7 +86,9 @@ test.describe("Skills", () => { const imported = await officeApi.importUserHomeSkill(officeSeed.workspaceId, provider, key); expect(imported.name).toBe(name); - expect(imported.slug).toBe(key); + // A discovered provider-skill key is a well-formed slug and gets the + // same write-time canonicalization as any other slug (AC-001.12). + expect(imported.slug).toBe(`kandev-${key}`); expect(imported.source_type).toBe("user_home"); expect(imported.source_locator).toBe(`${provider}:${key}`);