From 0cda877cd6ab906981890136c9d25cf2551ac00d Mon Sep 17 00:00:00 2001 From: Suriyakumarvijayanayagam Date: Fri, 28 Aug 2026 19:34:06 +0530 Subject: [PATCH] Version skills too, numbered by the server rather than by their author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g --- go-api/cmd/importagents/main.go | 68 ++++++++-- .../httpserver/definitions_api_test.go | 118 ++++++++++++++++++ go-api/internal/service/definitions.go | 112 +++++++++++++++-- 3 files changed, 275 insertions(+), 23 deletions(-) diff --git a/go-api/cmd/importagents/main.go b/go-api/cmd/importagents/main.go index f1e308b..b4a7f5e 100644 --- a/go-api/cmd/importagents/main.go +++ b/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 +} diff --git a/go-api/internal/httpserver/definitions_api_test.go b/go-api/internal/httpserver/definitions_api_test.go index 45ab184..42a2367 100644 --- a/go-api/internal/httpserver/definitions_api_test.go +++ b/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) + } +} diff --git a/go-api/internal/service/definitions.go b/go-api/internal/service/definitions.go index b335d3a..cdcd892 100644 --- a/go-api/internal/service/definitions.go +++ b/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), }) }