Refuse an edit that would rewrite an already-published version

§3 says a published version is immutable and editing publishes a new one.
The machinery for that was all present — an append-only definition_versions
table, a trigger, and repo.VersionsRepo.Snapshot, which already refuses to
store a version number whose content differs from what is stored.

Nothing acted on that refusal. snapshotIfPublished's error was discarded at
both call sites (`_ = s.snapshotIfPublished(...)`), and deliberately so: the
comment there explains that losing an author's work to protect a record of it
is the wrong trade. That is right for a recording failure and wrong for
exactly one case. A conflict is not the history failing to record; it is the
invariant firing.

The effect was silent. Editing a published agent without raising the
frontmatter version answered 200: the live row took the new text, the history
kept the old, and two different definitions were both called v1. Because
runtime.LoadAgentVersion resolves a pin by returning the CURRENT definition
whenever the pinned number equals the current one, a conversation pinned to v1
then ran the rewritten instructions while the audit trail showed the
originals. Verified against a live stack before the fix: PATCH answered 200,
agent_definitions held "SILENTLY CHANGED" and definition_versions still held
the published text, both labelled v2.

So the conflict is now detected before anything is written, where refusing
costs the author nothing but a version bump. The post-write snapshot keeps its
original contract for every other kind of failure, and republishing a version
unchanged stays the no-op it was. Drafts are untouched: they carry no promise,
and are still rewritten in place.

Not addressed here, and each its own change:

  - cmd/importagents never creates versions at all (documented at main.go:10),
    so the nine file-published organization agents are outside this entirely
    and every deploy still mutates v1 in place.
  - skill definitions never snapshot, so KindSkill exists with nothing writing
    it. Fixing that changes skill authoring behaviour and wants its own pass.
  - a concurrent publish of one version number with differing content can still
    pass this check and be caught by the unique index afterwards, where it is
    swallowed as before. That is the pre-existing behaviour, narrowed rather
    than removed.

Tests: the new case fails without the fix — the live row takes the rewritten
text at version 1 — and passes with it. Full suite green against PostgreSQL,
with only TestLive* skipped, which is what CI allows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g
This commit is contained in:
Suriyakumarvijayanayagam
2026-08-28 18:25:30 +05:30
parent f48b5606df
commit 80ba57ace3
2 changed files with 168 additions and 0 deletions

View File

@@ -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)
}
}