diff --git a/go-api/internal/httpserver/definitions_api_test.go b/go-api/internal/httpserver/definitions_api_test.go index 0f93de1..45ab184 100644 --- a/go-api/internal/httpserver/definitions_api_test.go +++ b/go-api/internal/httpserver/definitions_api_test.go @@ -883,3 +883,115 @@ func TestAgentCreateRejectsAnUnknownToolName(t *testing.T) { t.Fatalf("a real tool was refused: status %d (%v)", ok.code, ok.body) } } + +// TestPublishedVersionCannotBeRewritten covers §3: a published version is +// immutable, and editing publishes a NEW one. +// +// The failure this guards against was silent rather than loud. Editing a +// published agent without raising the frontmatter version used to answer 200: +// the live row took the new text, the append-only history kept the old, and +// two different definitions were both called v1. runtime.LoadAgentVersion +// resolves a pin by returning the CURRENT definition whenever the pinned +// number equals the current one, so a conversation "pinned to v1" then ran the +// rewritten instructions while the audit trail showed the originals. +func TestPublishedVersionCannotBeRewritten(t *testing.T) { + r := newRBAC(t) + + const published = `--- +id: pinned-agent +name: Pinned Agent +description: published, and therefore immutable at this version +status: published +version: 1 +pages: + - candidates +--- + +## Instructions +The original instructions. +` + + res := r.as(r.admin, "POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": published, + "visibility": "personal", + }) + if res.code != http.StatusCreated { + t.Fatalf("create published agent: status %d (%v)", res.code, res.body) + } + id, _ := res.record(t)["id"].(string) + if id == "" { + t.Fatal("created agent has no id") + } + + // Same version number, different body: refused. + rewritten := strings.Replace(published, + "The original instructions.", "Rewritten instructions.", 1) + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+id, + map[string]any{"markdown": rewritten}) + if res.code != http.StatusConflict { + t.Fatalf("rewriting published v1: status %d, want 409 (%v)", res.code, res.body) + } + + // And the refusal actually protected something — the live definition is + // unchanged, not merely reported as unchanged. + res = r.as(r.admin, "GET", "/api/v1/agent-definitions/"+id, nil) + if res.code != http.StatusOK { + t.Fatalf("re-read agent: status %d (%v)", res.code, res.body) + } + md, _ := res.record(t)["markdown"].(string) + if !strings.Contains(md, "The original instructions.") { + t.Errorf("the refused edit still changed the stored definition:\n%s", md) + } + if strings.Contains(md, "Rewritten instructions.") { + t.Errorf("the refused edit was applied anyway:\n%s", md) + } + + // Republishing the SAME version with the SAME content stays a no-op, so a + // save that changes nothing is not turned into an error. + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+id, + map[string]any{"markdown": published}) + if res.code != http.StatusOK { + t.Errorf("republishing v1 unchanged: status %d, want 200 (%v)", res.code, res.body) + } + + // Raising the version is the supported way to publish a change. + bumped := strings.Replace(rewritten, "version: 1", "version: 2", 1) + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+id, + map[string]any{"markdown": bumped}) + if res.code != http.StatusOK { + t.Fatalf("publishing v2: status %d, want 200 (%v)", res.code, res.body) + } + res = r.as(r.admin, "GET", "/api/v1/agent-definitions/"+id, nil) + md, _ = res.record(t)["markdown"].(string) + if !strings.Contains(md, "Rewritten instructions.") { + t.Errorf("v2 did not take the new text:\n%s", md) + } + + // A draft carries no such promise: it is not published, so it may be + // rewritten in place as often as its author likes. + const draft = `--- +id: draft-agent +name: Draft Agent +description: still a draft +status: draft +version: 1 +pages: + - candidates +--- + +## Instructions +First draft. +` + res = r.as(r.admin, "POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": draft, "visibility": "personal", + }) + if res.code != http.StatusCreated { + t.Fatalf("create draft: status %d (%v)", res.code, res.body) + } + draftID, _ := res.record(t)["id"].(string) + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+draftID, + map[string]any{"markdown": strings.Replace(draft, "First draft.", "Second draft.", 1)}) + if res.code != http.StatusOK { + t.Errorf("rewriting a draft at the same version: status %d, want 200 (%v)", res.code, res.body) + } +} diff --git a/go-api/internal/service/definitions.go b/go-api/internal/service/definitions.go index 3eea942..b335d3a 100644 --- a/go-api/internal/service/definitions.go +++ b/go-api/internal/service/definitions.go @@ -234,6 +234,11 @@ func (s *DefinitionsService) CreateAgent(ctx context.Context, ident authctx.Iden input.OwnerUserID = &ident.UserID } + if err := s.refusePublishedRewrite(ctx, ident, repo.KindAgent, + agent.ID, markdown, agent.Status, agent.Version); err != nil { + return nil, err + } + rec, err := s.repo.InsertAgent(ctx, ident, input) if err != nil { return nil, err @@ -296,6 +301,11 @@ func (s *DefinitionsService) UpdateAgent(ctx context.Context, ident authctx.Iden input.Status = &agent.Status input.Version = &agent.Version input.Pages = agent.Pages + + if err := s.refusePublishedRewrite(ctx, ident, repo.KindAgent, + agent.ID, markdown, agent.Status, agent.Version); err != nil { + return nil, err + } } else if statusRaw, ok := patch["status"]; ok && statusRaw != nil { status, isStr := statusRaw.(string) if !isStr || (status != "draft" && status != "published" && status != "archived") { @@ -515,6 +525,52 @@ func (s *DefinitionsService) DeleteSkill(ctx context.Context, ident authctx.Iden /* ── Publishing ─────────────────────────────────────────────────────────── */ +// refusePublishedRewrite fails a publish that would change a version already +// published, BEFORE anything is written. +// +// snapshotIfPublished below deliberately never fails a save: the author's work +// is already stored and losing it to protect a record of it is the wrong trade. +// That is right for a recording failure — the disk, the pool, the network — and +// wrong for exactly one case. When repo.VersionsRepo.Snapshot refuses because +// the version already says something different, that is not the history failing +// to record; it is §3 firing. Swallowing it leaves two different definitions +// both called v2: the live row the runtime serves, and the snapshot the history +// shows. runtime.LoadAgentVersion resolves a pin by returning the CURRENT +// definition whenever the pinned number equals the current one, so the run gets +// the changed text while the audit trail says otherwise. +// +// So the conflict is detected here instead, before the write, where refusing +// costs the author nothing but a version bump. The post-write snapshot keeps +// its original contract for every other kind of failure. +// +// A concurrent publish of the same number with different content can still slip +// past this check and be caught by the unique index afterwards, where it is +// swallowed as before. That leaves the live row ahead of its snapshot, which is +// the pre-existing behaviour and not something this guard makes worse. +func (s *DefinitionsService) refusePublishedRewrite(ctx context.Context, ident authctx.Identity, + kind repo.VersionKind, definitionID, markdown, status string, version int) error { + + if s.versions == nil || status != "published" || definitionID == "" || version < 1 { + return nil + } + + stored, err := s.versions.Load(ctx, ident, kind, definitionID, version) + if err != nil || stored == nil { + // Absent (the ordinary case for a new version) or unreadable. Either + // way there is no published text to contradict, so this is not the + // place to fail the save. + return nil + } + if stored.Markdown == markdown { + return nil // republishing the same version unchanged is a no-op + } + + return domain.Conflict(fmt.Sprintf( + "version %d of %q is already published and says something different; "+ + "raise the version in the frontmatter to publish a change", + version, definitionID)) +} + // snapshotIfPublished records an immutable copy when a definition is published. // // §3: editing publishes a NEW version, and a published version never changes.