Version skills too, numbered by the server rather than by their author

repo.KindSkill existed with nothing writing it. Migration 000010 says "agents
and skills version identically", the table has always accepted kind='skill',
and no path on either side ever recorded one — so an edit to a skill left no
record of what it used to say. Agents name their skills and the runtime refuses
to load one whose skill is missing, so a skill changing under a pinned agent is
the same class of problem the last two commits fixed, one layer down.

Skills are numbered differently, and not by preference. An agent's frontmatter
carries `version:`, so its author decides when a change is a new version and can
be refused for rewriting an old one. definition.Skill has no such field, the
skill_definitions table has no such column, and the vocabulary is active |
inactive rather than draft | published. Giving skills an authored version would
mean a migration, a parser change on BOTH sides of the conformance test in
internal/definition — which replays a capture of the real frontend module graph
— and an edit to all 23 shipped skills. That is a feature, not this fix.

So the server assigns it: one after whatever was last published. This is not an
invention. repo.VersionsRepo.LatestVersion was written for exactly this and
says so — "the next published version has to follow what was actually published
rather than what somebody wrote in the frontmatter" — and had no callers
outside its own test.

Because the author never names a version, there is nothing to refuse: an edit
is always a new version. What needs care instead is the opposite — a save that
changed nothing must NOT be one, or every deploy would add a version to all 23
skills and the number would stop meaning anything. Each publish is compared
against the last recorded copy first. Inactive skills are not recorded at all;
inactive is this vocabulary's draft.

Verified against a live stack:

  - first import over 23 unversioned skills: 23 skill version(s) recorded
  - second import, files unchanged: 0 recorded, total still 23
  - one skill edited: 1 recorded, that skill at v1, v2; v1 still holds the
    original text and v2 the edit

The test fails without the change — "after create: 0 version(s), want 1" — and
covers the three behaviours that matter: an edit versions, an identical save
does not, and an inactive skill is not recorded.

Both races noted on the agent path apply here as well: two simultaneous edits
can compute the same next number, and the loser's snapshot is dropped rather
than failing the author's save.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g

go-api/cmd/importagents/main.go

@@ -164,17 +164,31 @@ func run(dir, skillDir, orgSlug string, dryRun bool, timeout time.Duration) erro
 	// briefly unloadable, and inside one transaction that is invisible — but
 	// ordering them correctly costs nothing and means a future non-transactional
 	// path is not silently broken.
-	skillsWritten := 0
+	// Versions are recorded through the same transaction, so the history and
+	// the definition it describes cannot disagree: either both land or neither
+	// does.
+	versions := repo.NewVersionsRepo(tx)
+	ident := authctx.Identity{OrgID: orgID, UserID: author}
+
+	skillsWritten, skillVersions := 0, 0
 	for _, sk := range skills {
 		if err := upsertSkill(ctx, tx, orgID, author, sk); err != nil {
 			return fmt.Errorf("%s: %w", sk.name, err)
 		}
 		skillsWritten++
+
+		// Skills are numbered by the server rather than by their author — they
+		// have no `version:` to read. See snapshotSkill in internal/service,
+		// which does the same for the authoring path.
+		recorded, err := snapshotSkill(ctx, versions, ident, sk)
+		if err != nil {
+			return fmt.Errorf("%s: record version: %w", sk.name, err)
+		}
+		if recorded {
+			skillVersions++
+		}
 	}
 
-	// Versions are recorded through the same transaction, so the history and
-	// the definition it describes cannot disagree: either both land or neither
-	// does.
 	//
 	// Snapshot is what refuses a spec that changed without raising its
 	// `version:`. Every such spec is collected rather than the first one
@@ -183,9 +197,6 @@ func run(dir, skillDir, orgSlug string, dryRun bool, timeout time.Duration) erro
 	// because that refusal comes from comparing a row this code read, not from
 	// a failed statement: the INSERT is ON CONFLICT DO NOTHING, so the
 	// transaction is still healthy and the remaining specs can be checked.
-	versions := repo.NewVersionsRepo(tx)
-	ident := authctx.Identity{OrgID: orgID, UserID: author}
-
 	inserted, updated, versioned := 0, 0, 0
 	var rewrites []string
 	for _, s := range specs {
@@ -231,9 +242,9 @@ func run(dir, skillDir, orgSlug string, dryRun bool, timeout time.Duration) erro
 		return fmt.Errorf("commit: %w", err)
 	}
 
-	fmt.Printf("\n%d agent(s) published, %d updated, %d version(s) recorded, "+
-		"%d skill(s) written, into %s\n",
-		inserted, updated, versioned, skillsWritten, orgSlug)
+	fmt.Printf("\n%d agent(s) published, %d updated, %d agent version(s) recorded, "+
+		"%d skill(s) written, %d skill version(s) recorded, into %s\n",
+		inserted, updated, versioned, skillsWritten, skillVersions, orgSlug)
 	return nil
 }
 
@@ -436,3 +447,40 @@ func upsertSkill(ctx context.Context, tx pgx.Tx, orgID, author string, sk skillS
 		sk.parsed.Status, sk.parsed.Name, sk.parsed.Description, sk.parsed.Pages)
 	return err
 }
+
+// snapshotSkill records a skill version, numbered by the server.
+//
+// Skills carry no `version:` in their frontmatter, so unlike an agent there is
+// no author-supplied number to honour or to refuse. The number is one after
+// whatever was last published, and a skill whose text has not changed since
+// then is not published again — otherwise every deploy would add a version to
+// all 23 of them.
+//
+// Reports whether it wrote one, so the run can say how many changed.
+func snapshotSkill(ctx context.Context, versions *repo.VersionsRepo,
+	ident authctx.Identity, sk skillSpec) (bool, error) {
+
+	latest, err := versions.LatestVersion(ctx, ident, repo.KindSkill, sk.parsed.ID)
+	if err != nil {
+		return false, err
+	}
+	if latest > 0 {
+		stored, err := versions.Load(ctx, ident, repo.KindSkill, sk.parsed.ID, latest)
+		if err == nil && stored != nil && stored.Markdown == sk.raw {
+			return false, nil // unchanged since the last publish
+		}
+	}
+
+	if err := versions.Snapshot(ctx, ident, repo.SnapshotInput{
+		Kind:         repo.KindSkill,
+		DefinitionID: sk.parsed.ID,
+		Version:      latest + 1,
+		Markdown:     sk.raw,
+		Name:         sk.parsed.Name,
+		Description:  sk.parsed.Description,
+		Pages:        sk.parsed.Pages,
+	}); err != nil {
+		return false, err
+	}
+	return true, nil
+}

go-api/internal/httpserver/definitions_api_test.go

@@ -1,6 +1,7 @@
 package httpserver_test
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
@@ -995,3 +996,120 @@ First draft.
 		t.Errorf("rewriting a draft at the same version: status %d, want 200 (%v)", res.code, res.body)
 	}
 }
+
+// TestSkillVersionsAreRecordedAndServerNumbered covers the skill half of §3.
+//
+// Skills carry no `version:` in their frontmatter, so unlike an agent there is
+// no author-supplied number to honour and nothing to refuse: the server takes
+// the next one after whatever was last published. Before this, skills were
+// never versioned at all — repo.KindSkill existed with nothing writing it, and
+// an edit to a skill left no record of what it used to say.
+func TestSkillVersionsAreRecordedAndServerNumbered(t *testing.T) {
+	r := newRBAC(t)
+	ctx := context.Background()
+
+	count := func(definitionID string) int {
+		t.Helper()
+		var n int
+		if err := r.h.Pool.QueryRow(ctx,
+			`SELECT count(*) FROM definition_versions
+			  WHERE org_id = $1::uuid AND kind = 'skill' AND definition_id = $2`,
+			r.orgID, definitionID).Scan(&n); err != nil {
+			t.Fatalf("count skill versions: %v", err)
+		}
+		return n
+	}
+	stored := func(definitionID string, version int) string {
+		t.Helper()
+		var md string
+		if err := r.h.Pool.QueryRow(ctx,
+			`SELECT markdown FROM definition_versions
+			  WHERE org_id = $1::uuid AND kind = 'skill'
+			    AND definition_id = $2 AND version = $3`,
+			r.orgID, definitionID, version).Scan(&md); err != nil {
+			t.Fatalf("read skill v%d: %v", version, err)
+		}
+		return md
+	}
+
+	const first = `---
+id: versioned-skill
+name: Versioned Skill
+description: a skill that should acquire a history
+status: active
+pages:
+  - candidates
+---
+
+# Versioned Skill
+The first body.
+`
+
+	res := r.as(r.admin, "POST", "/api/v1/skill-definitions", map[string]any{
+		"markdown":   first,
+		"visibility": "personal",
+	})
+	if res.code != http.StatusCreated {
+		t.Fatalf("create skill: status %d (%v)", res.code, res.body)
+	}
+	id, _ := res.record(t)["id"].(string)
+	if got := count("versioned-skill"); got != 1 {
+		t.Fatalf("after create: %d version(s), want 1", got)
+	}
+
+	// An edit is always a new version — the author names no number, so there
+	// is nothing to rewrite and nothing to refuse.
+	second := strings.Replace(first, "The first body.", "The second body.", 1)
+	res = r.as(r.admin, "PATCH", "/api/v1/skill-definitions/"+id,
+		map[string]any{"markdown": second})
+	if res.code != http.StatusOK {
+		t.Fatalf("edit skill: status %d (%v)", res.code, res.body)
+	}
+	if got := count("versioned-skill"); got != 2 {
+		t.Fatalf("after an edit: %d version(s), want 2", got)
+	}
+
+	// v1 still says what it said. This is the whole point: before, the text
+	// was simply gone.
+	if md := stored("versioned-skill", 1); !strings.Contains(md, "The first body.") {
+		t.Errorf("v1 no longer holds the original text:\n%s", md)
+	}
+	if md := stored("versioned-skill", 2); !strings.Contains(md, "The second body.") {
+		t.Errorf("v2 does not hold the new text:\n%s", md)
+	}
+
+	// Saving the same text again is not a publish. Without this every save
+	// would add a version and the number would stop meaning anything.
+	res = r.as(r.admin, "PATCH", "/api/v1/skill-definitions/"+id,
+		map[string]any{"markdown": second})
+	if res.code != http.StatusOK {
+		t.Fatalf("re-saving unchanged: status %d (%v)", res.code, res.body)
+	}
+	if got := count("versioned-skill"); got != 2 {
+		t.Errorf("re-saving unchanged text added a version: %d, want 2", got)
+	}
+
+	// An inactive skill is the skill vocabulary's draft: not in service, so
+	// not recorded.
+	const inactive = `---
+id: inactive-skill
+name: Inactive Skill
+description: not in service
+status: inactive
+pages:
+  - candidates
+---
+
+# Inactive Skill
+Nothing here is published.
+`
+	res = r.as(r.admin, "POST", "/api/v1/skill-definitions", map[string]any{
+		"markdown": inactive, "visibility": "personal",
+	})
+	if res.code != http.StatusCreated {
+		t.Fatalf("create inactive skill: status %d (%v)", res.code, res.body)
+	}
+	if got := count("inactive-skill"); got != 0 {
+		t.Errorf("an inactive skill was versioned: %d, want 0", got)
+	}
+}

go-api/internal/service/definitions.go

@@ -435,7 +435,12 @@ func (s *DefinitionsService) CreateSkill(ctx context.Context, ident authctx.Iden
 		input.OwnerUserID = &ident.UserID
 	}
 
-	return s.repo.InsertSkill(ctx, ident, input)
+	rec, err := s.repo.InsertSkill(ctx, ident, input)
+	if err != nil {
+		return nil, err
+	}
+	_ = s.snapshotSkill(ctx, ident, rec)
+	return rec, nil
 }
 
 // UpdateSkill validates and applies updates to an authored skill definition.
@@ -493,7 +498,12 @@ func (s *DefinitionsService) UpdateSkill(ctx context.Context, ident authctx.Iden
 		input.Status = &status
 	}
 
-	return s.repo.UpdateSkill(ctx, ident, id, input)
+	rec, err := s.repo.UpdateSkill(ctx, ident, id, input)
+	if err != nil {
+		return nil, err
+	}
+	_ = s.snapshotSkill(ctx, ident, rec)
+	return rec, nil
 }
 
 // DeleteSkill removes a skill definition following idempotent delete semantics.
@@ -619,16 +629,6 @@ func (s *DefinitionsService) snapshotIfPublished(ctx context.Context, ident auth
 
 	name, _ := rec["name"].(string)
 	description, _ := rec["description"].(string)
-	var pages []string
-	if raw, ok := rec["pages"].([]string); ok {
-		pages = raw
-	} else if raw, ok := rec["pages"].([]any); ok {
-		for _, p := range raw {
-			if str, ok := p.(string); ok {
-				pages = append(pages, str)
-			}
-		}
-	}
 
 	return s.versions.Snapshot(ctx, ident, repo.SnapshotInput{
 		Kind:         kind,
@@ -637,7 +637,93 @@ func (s *DefinitionsService) snapshotIfPublished(ctx context.Context, ident auth
 		Markdown:     markdown,
 		Name:         name,
 		Description:  description,
-		Pages:        pages,
+		Pages:        recordPages(rec),
+	})
+}
+
+// recordPages reads a record's pages, which arrive as []string from the
+// repository and as []any when they have been through JSON.
+func recordPages(rec domain.Record) []string {
+	if raw, ok := rec["pages"].([]string); ok {
+		return raw
+	}
+	raw, ok := rec["pages"].([]any)
+	if !ok {
+		return nil
+	}
+	pages := make([]string, 0, len(raw))
+	for _, p := range raw {
+		if str, isStr := p.(string); isStr {
+			pages = append(pages, str)
+		}
+	}
+	return pages
+}
+
+// snapshotSkill records an immutable copy of a skill, numbered by the server.
+//
+// Skills carry no version. An agent's frontmatter names one, so its author
+// decides when a change is a new version and can be refused for rewriting an
+// old one. A skill has no such field, and giving it one would mean a migration,
+// a parser change on BOTH sides of the conformance test in
+// internal/definition, and an edit to all 23 shipped skills — a feature, not
+// the fix this is.
+//
+// So the number is the server's: one after whatever was last published. That
+// is what repo.VersionsRepo.LatestVersion was written for ("the next published
+// version has to follow what was actually published rather than what somebody
+// wrote in the frontmatter") and what migration 000010 means by "agents and
+// skills version identically". It was built and never wired to anything.
+//
+// Because the author never names a version, there is nothing here to refuse:
+// an edit is always a NEW version, and a save that changed nothing is not a
+// version at all. The comparison against the last published copy is what keeps
+// the history from filling with keystrokes.
+//
+// Two simultaneous edits can both compute the same next number; one wins and
+// the other's snapshot is dropped, leaving a version unrecorded. That is the
+// same narrow race the agent path has, and the same reason it is tolerated
+// here: failing an author's save to record it is the wrong trade.
+func (s *DefinitionsService) snapshotSkill(ctx context.Context, ident authctx.Identity,
+	rec domain.Record) error {
+
+	if s.versions == nil || rec == nil {
+		return nil
+	}
+	// Only a skill that is in service. "inactive" is the skill vocabulary's
+	// equivalent of a draft — see definition.SkillStatuses.
+	if status, _ := rec["status"].(string); status != "active" {
+		return nil
+	}
+
+	markdown, _ := rec["markdown"].(string)
+	definitionID, _ := rec["definition_id"].(string)
+	if markdown == "" || definitionID == "" {
+		return nil
+	}
+
+	latest, err := s.versions.LatestVersion(ctx, ident, repo.KindSkill, definitionID)
+	if err != nil {
+		return err
+	}
+	if latest > 0 {
+		stored, err := s.versions.Load(ctx, ident, repo.KindSkill, definitionID, latest)
+		if err == nil && stored != nil && stored.Markdown == markdown {
+			return nil // unchanged since the last publish
+		}
+	}
+
+	name, _ := rec["name"].(string)
+	description, _ := rec["description"].(string)
+
+	return s.versions.Snapshot(ctx, ident, repo.SnapshotInput{
+		Kind:         repo.KindSkill,
+		DefinitionID: definitionID,
+		Version:      latest + 1,
+		Markdown:     markdown,
+		Name:         name,
+		Description:  description,
+		Pages:        recordPages(rec),
 	})
 }