From 377948708be2bb6cb1ac915d14f6221a5dbfeea3 Mon Sep 17 00:00:00 2001 From: Suriyakumarvijayanayagam Date: Sat, 29 Aug 2026 14:43:49 +0530 Subject: [PATCH] =?UTF-8?q?Enforce=20=C2=A73's=20monotonic=20version=20and?= =?UTF-8?q?=20its=20DAG=20requirement=20at=20publish?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rules §3 states and nothing checked. MONOTONIC. The rewrite guard added earlier compares content at ONE version number, so republishing an OLDER number with the text originally published under it looked like a no-op: nothing conflicted, nothing was refused, and the live row silently reverted. The agent then reads v1 in the UI while the newest thing anybody approved was v2. The test publishes v1, publishes v2, republishes v1 byte-for-byte, and asserts both the refusal and that the live row is still v2. Without the guard it answers 200 and the row goes back to version 1. DAG. §3 says cycle detection runs at publish; only the runtime depth cap existed. That cap means a cycle was never a safety problem — it was a budget one. Every run entering the loop spends its whole allowance delegating in a circle before terminating, and the author learns about it from a bill rather than from the publish that created it. definition.FindSubagentCycle is a pure function over id -> subagent ids, so it is tested directly: chains, diamonds, self-reference, loops not involving the first agent walked, and a 5000-long chain that would matter if this were written to recurse carelessly. It REPORTS the cycle ("a -> b -> c -> a") rather than merely detecting one, because an operator otherwise has to find it by hand across a set of specs. The report is deterministic — a test runs it fifty times over a graph with two cycles and requires the same answer, since Go randomises map iteration and an error message that changes between identical runs is one nobody trusts. Wired into both publish paths. importagents has every spec in hand, which is the only place that is cheaply true. The API builds the graph from organization-visible agents plus the incoming definition standing in for its stored self — otherwise an edit that CREATES a cycle is checked against the version that did not have one and passes. Personal agents are excluded: they are invisible to everyone else so cannot complete anyone else's loop, and reading them would mean reading other people's drafts to validate your own. An edge to an agent that is not in the set is ignored rather than reported. That is a different failure with a different message (runtime.unknown_subagent), and conflating them prints "cycle detected" for what is actually a typo. The cycle test bumps the version on the loop-closing edit. Without that the rewrite guard refuses it for changing published text, the test passes for the wrong reason, and it would keep passing with cycle detection deleted — which is how it was first written, and what running it without the guard showed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PJvibeSc1JYXjatankqM1g --- go-api/cmd/importagents/main.go | 27 ++++ go-api/internal/definition/graph.go | 103 ++++++++++++ go-api/internal/definition/graph_test.go | 101 ++++++++++++ .../httpserver/definitions_api_test.go | 152 ++++++++++++++++++ go-api/internal/service/definitions.go | 88 ++++++++++ 5 files changed, 471 insertions(+) create mode 100644 go-api/internal/definition/graph.go create mode 100644 go-api/internal/definition/graph_test.go diff --git a/go-api/cmd/importagents/main.go b/go-api/cmd/importagents/main.go index 3124576..0df24f7 100644 --- a/go-api/cmd/importagents/main.go +++ b/go-api/cmd/importagents/main.go @@ -116,6 +116,20 @@ func run(dir, skillDir, orgSlug string, dryRun bool, timeout time.Duration) erro // written. The runtime refuses to load an agent with a missing dependency, // so importing one without its skills produces an agent that exists and // cannot run — a failure that surfaces per request instead of here. + // §3: subagents must form a DAG, checked at PUBLISH. Every spec is in hand + // here, which is the only place that is cheaply true — so this is where the + // check belongs. The runtime depth cap still bounds a cycle that reaches + // run time by another route. + graph := make(map[string][]string, len(specs)) + for _, s := range specs { + graph[s.parsed.ID] = s.parsed.Subagents + } + if cycle := definition.FindSubagentCycle(graph); cycle != "" { + return fmt.Errorf("the subagent graph has a cycle: %s\n\n"+ + "Nothing was written. Delegation follows these edges, so a loop is a "+ + "run that delegates until it runs out of budget.", cycle) + } + if missing := missingSkills(specs, skills); len(missing) > 0 { return fmt.Errorf("%d skill(s) named by an agent are not in %s: %s", len(missing), skillDir, strings.Join(missing, ", ")) @@ -494,6 +508,19 @@ func snapshotSkill(ctx context.Context, versions *repo.VersionsRepo, func snapshotAgent(ctx context.Context, versions *repo.VersionsRepo, ident authctx.Identity, s spec) (recorded bool, conflict string, err error) { + // §3 calls the version monotonic and nothing enforced it. A spec edited + // from an older copy republishes an older number whose content still + // matches what was published under it — no conflict, no complaint, and the + // deployed agent quietly goes backwards. + latest, err := versions.LatestVersion(ctx, ident, repo.KindAgent, s.parsed.ID) + if err != nil { + return false, "", err + } + if latest > 0 && s.parsed.Version < latest { + return false, definition.ErrVersionWentBackwards( + s.parsed.ID, latest, s.parsed.Version).Error(), nil + } + stored, err := versions.Load(ctx, ident, repo.KindAgent, s.parsed.ID, s.parsed.Version) if err != nil { var apiErr *domain.Error diff --git a/go-api/internal/definition/graph.go b/go-api/internal/definition/graph.go new file mode 100644 index 0000000..78af68e --- /dev/null +++ b/go-api/internal/definition/graph.go @@ -0,0 +1,103 @@ +package definition + +import ( + "fmt" + "sort" + "strings" +) + +// FindSubagentCycle reports the first delegation cycle in a set of agents, or +// "" if the graph is acyclic. +// +// §3: "subagents must form a DAG. Cycle detection runs at publish." This is the +// publish-time half. The runtime half is runtime.MaxDelegationDepth, which +// bounds a cycle that reaches run time anyway — because a graph can only be +// checked against the agents the checker was GIVEN, and an agent published +// while another is being edited can complete a loop neither publish saw. +// +// The returned string names the cycle in the order it was walked, so an +// operator can see which edge to cut: +// +// a -> b -> c -> a +// +// Edges pointing at agents not in the set are ignored rather than treated as +// missing. Resolving those is a different check with a different message +// (runtime.unknown_subagent), and conflating the two produces "cycle detected" +// for what is actually a typo. +func FindSubagentCycle(subagents map[string][]string) string { + // Depth-first search tracking the path, so the cycle can be REPORTED + // rather than merely detected — "there is a cycle" leaves an operator to + // find it by hand across a set of specs. + // + // Recursive, and deliberately: a goroutine stack grows on demand, so depth + // here costs memory rather than a crash, and a 5000-long chain is covered + // by a test. An explicit stack would buy nothing and lose the path + // bookkeeping that makes the message useful. + const ( + unvisited = 0 + onPath = 1 + done = 2 + ) + state := make(map[string]int, len(subagents)) + + // Sorted, so the same set of agents always reports the same cycle. An + // error message that changes between runs on identical input is one + // nobody trusts. + roots := make([]string, 0, len(subagents)) + for id := range subagents { + roots = append(roots, id) + } + sort.Strings(roots) + + var path []string + var walk func(id string) string + walk = func(id string) string { + switch state[id] { + case done: + return "" + case onPath: + // Found it. Report from the first occurrence of this id, so the + // message is the cycle itself and not the walk that reached it. + for i, seen := range path { + if seen == id { + return strings.Join(append(append([]string{}, path[i:]...), id), " -> ") + } + } + return id + " -> " + id + } + + state[id] = onPath + path = append(path, id) + for _, next := range subagents[id] { + if _, known := subagents[next]; !known { + continue // not ours to judge; see the doc comment + } + if cycle := walk(next); cycle != "" { + return cycle + } + } + path = path[:len(path)-1] + state[id] = done + return "" + } + + for _, id := range roots { + if cycle := walk(id); cycle != "" { + return cycle + } + } + return "" +} + +// ErrVersionWentBackwards describes a publish that lowers a version. +// +// §3 calls the version monotonic. Nothing enforced it: the upsert wrote +// whatever the frontmatter said, so a spec edited from an older copy silently +// rolled a deployed agent backwards — no conflict, because the older version's +// content still matched what was published under that number. +func ErrVersionWentBackwards(id string, from, to int) error { + return fmt.Errorf( + "%q is published at version %d and this publishes version %d; "+ + "a version is monotonic, so raise it above %d rather than lowering it", + id, from, to, from) +} diff --git a/go-api/internal/definition/graph_test.go b/go-api/internal/definition/graph_test.go new file mode 100644 index 0000000..f28e61a --- /dev/null +++ b/go-api/internal/definition/graph_test.go @@ -0,0 +1,101 @@ +package definition_test + +import ( + "strings" + "testing" + + "github.com/krow/krow-backend/go-api/internal/definition" +) + +func TestFindSubagentCycle(t *testing.T) { + for _, tc := range []struct { + name string + graph map[string][]string + want string // "" means acyclic; otherwise a substring the report must contain + }{ + {"empty", map[string][]string{}, ""}, + {"no edges", map[string][]string{"a": nil, "b": nil}, ""}, + {"a chain is not a cycle", map[string][]string{ + "a": {"b"}, "b": {"c"}, "c": nil, + }, ""}, + {"a diamond is not a cycle", map[string][]string{ + "a": {"b", "c"}, "b": {"d"}, "c": {"d"}, "d": nil, + }, ""}, + {"self reference", map[string][]string{"a": {"a"}}, "a -> a"}, + {"two-agent loop", map[string][]string{ + "a": {"b"}, "b": {"a"}, + }, "a -> b -> a"}, + {"longer loop", map[string][]string{ + "a": {"b"}, "b": {"c"}, "c": {"a"}, + }, "a -> b -> c -> a"}, + {"cycle not involving the first agent walked", map[string][]string{ + "a": {"b"}, "b": {"c"}, "c": {"b"}, + }, "b -> c -> b"}, + {"an edge to an unknown agent is not a cycle", map[string][]string{ + "a": {"nowhere"}, + }, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + got := definition.FindSubagentCycle(tc.graph) + switch { + case tc.want == "" && got != "": + t.Errorf("reported a cycle %q in an acyclic graph", got) + case tc.want != "" && got == "": + t.Errorf("missed the cycle; want something containing %q", tc.want) + case tc.want != "" && !strings.Contains(got, tc.want): + t.Errorf("cycle = %q, want it to contain %q", got, tc.want) + } + }) + } +} + +// The report must be stable: the same graph reported differently on different +// runs is an error message nobody trusts, and map iteration order in Go is +// deliberately random. +func TestFindSubagentCycleIsDeterministic(t *testing.T) { + graph := map[string][]string{ + "e": {"f"}, "f": {"e"}, + "a": {"b"}, "b": {"c"}, "c": {"a"}, + "z": nil, "y": {"z"}, + } + first := definition.FindSubagentCycle(graph) + if first == "" { + t.Fatal("no cycle found in a graph with two") + } + for i := 0; i < 50; i++ { + if got := definition.FindSubagentCycle(graph); got != first { + t.Fatalf("run %d reported %q, first run reported %q — the report "+ + "depends on map iteration order", i, got, first) + } + } +} + +// A deep chain must not overflow the stack. An author supplies this graph. +func TestFindSubagentCycleHandlesADeepChain(t *testing.T) { + graph := map[string][]string{} + const n = 5000 + for i := 0; i < n; i++ { + graph[itoa(i)] = []string{itoa(i + 1)} + } + graph[itoa(n)] = nil + if got := definition.FindSubagentCycle(graph); got != "" { + t.Errorf("reported a cycle %q in a %d-long chain", got, n) + } + // And the same chain closed into a loop is found. + graph[itoa(n)] = []string{itoa(0)} + if definition.FindSubagentCycle(graph) == "" { + t.Error("missed a cycle closing a long chain") + } +} + +func itoa(i int) string { + if i == 0 { + return "0" + } + var b []byte + for i > 0 { + b = append([]byte{byte('0' + i%10)}, b...) + i /= 10 + } + return string(b) +} diff --git a/go-api/internal/httpserver/definitions_api_test.go b/go-api/internal/httpserver/definitions_api_test.go index cfb395b..7b9674d 100644 --- a/go-api/internal/httpserver/definitions_api_test.go +++ b/go-api/internal/httpserver/definitions_api_test.go @@ -1175,3 +1175,155 @@ The instructions, unchanged throughout. res.code, res.body) } } + +// TestPublishedVersionCannotGoBackwards covers §3's "monotonic". +// +// The rewrite guard only compares content at ONE version number, so an older +// number republished with the text that was originally published under it +// looked like a no-op: no conflict, nothing to refuse, and the live row +// silently reverted. The agent in the UI then reads v1 while the newest thing +// anybody approved was v2. +func TestPublishedVersionCannotGoBackwards(t *testing.T) { + r := newRBAC(t) + + const v1 = `--- +id: monotonic-agent +name: Monotonic Agent +description: published twice, then rolled back +status: published +version: 1 +pages: + - candidates +--- + +## Instructions +The first version. +` + res := r.as(r.admin, "POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": v1, "visibility": "personal", + }) + if res.code != http.StatusCreated { + t.Fatalf("create v1: status %d (%v)", res.code, res.body) + } + id, _ := res.record(t)["id"].(string) + + v2 := strings.Replace(strings.Replace(v1, "version: 1", "version: 2", 1), + "The first version.", "The second version.", 1) + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+id, map[string]any{"markdown": v2}) + if res.code != http.StatusOK { + t.Fatalf("publish v2: status %d (%v)", res.code, res.body) + } + + // Back to v1, byte-for-byte what v1 said. Nothing here conflicts — which + // is exactly why it used to succeed. + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+id, map[string]any{"markdown": v1}) + if res.code != http.StatusConflict { + t.Fatalf("republishing v1 after v2: status %d, want 409 (%v)", res.code, res.body) + } + + // And the live definition is still v2, not silently reverted. + res = r.as(r.admin, "GET", "/api/v1/agent-definitions/"+id, nil) + if got := fmt.Sprint(res.record(t)["version"]); got != "2" { + t.Errorf("live version = %s, want 2 — the refused publish rolled it back anyway", got) + } +} + +// TestSubagentCycleIsRefusedAtPublish covers §3's DAG requirement. +// +// runtime.MaxDelegationDepth bounds a cycle that reaches run time, so this is +// not a safety hole — it is a budget one. Every run that entered the loop would +// spend its whole allowance delegating in a circle before terminating, and the +// person who wrote the loop would learn about it from a bill rather than from +// the publish that created it. +func TestSubagentCycleIsRefusedAtPublish(t *testing.T) { + r := newRBAC(t) + + // version is a parameter so the loop-closing edit can BUMP it. Otherwise + // the rewrite guard refuses that edit for changing published text, the + // test passes for the wrong reason, and it would keep passing with cycle + // detection removed entirely. + agent := func(id, name string, version int, subagents ...string) string { + var sub string + if len(subagents) > 0 { + sub = "subagents:\n" + for _, s := range subagents { + sub += " - " + s + "\n" + } + } + return fmt.Sprintf(`--- +id: %s +name: %s +description: part of a delegation graph +status: published +version: %d +pages: + - candidates +%s--- + +## Instructions +Delegate. +`, id, name, version, sub) + } + + // A, with no subagents yet. + res := r.as(r.admin, "POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agent("cycle-a", "Cycle A", 1), "visibility": "organization", + }) + if res.code != http.StatusCreated { + t.Fatalf("create A: status %d (%v)", res.code, res.body) + } + idA, _ := res.record(t)["id"].(string) + + // B delegates to A. Still a DAG. + res = r.as(r.admin, "POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agent("cycle-b", "Cycle B", 1, "cycle-a"), "visibility": "organization", + }) + if res.code != http.StatusCreated { + t.Fatalf("create B pointing at A: status %d, want 201 — a chain is not a cycle (%v)", + res.code, res.body) + } + + // Now close the loop: A delegates to B. + res = r.as(r.admin, "PATCH", "/api/v1/agent-definitions/"+idA, map[string]any{ + "markdown": agent("cycle-a", "Cycle A", 2, "cycle-b"), + }) + if res.code != http.StatusUnprocessableEntity && res.code != http.StatusBadRequest { + t.Fatalf("closing the loop: status %d, want a validation failure (%v)", res.code, res.body) + } + if body := fmt.Sprint(res.body); !strings.Contains(body, "cycle") { + t.Errorf("the refusal did not mention a cycle: %v", res.body) + } + + // A must be unchanged — refused, not half-applied. + res = r.as(r.admin, "GET", "/api/v1/agent-definitions/"+idA, nil) + if md, _ := res.record(t)["markdown"].(string); strings.Contains(md, "cycle-b") { + t.Error("the refused edit was applied anyway") + } +} + +// A self-reference is the shortest cycle and the easiest to write by accident. +func TestSelfReferencingSubagentIsRefused(t *testing.T) { + r := newRBAC(t) + + const md = `--- +id: narcissus-agent +name: Narcissus Agent +description: names itself +status: published +version: 1 +pages: + - candidates +subagents: + - narcissus-agent +--- + +## Instructions +Ask myself. +` + res := r.as(r.admin, "POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": md, "visibility": "organization", + }) + if res.code == http.StatusCreated { + t.Fatal("an agent naming itself as its own subagent was published") + } +} diff --git a/go-api/internal/service/definitions.go b/go-api/internal/service/definitions.go index 861b570..e331707 100644 --- a/go-api/internal/service/definitions.go +++ b/go-api/internal/service/definitions.go @@ -234,6 +234,13 @@ func (s *DefinitionsService) CreateAgent(ctx context.Context, ident authctx.Iden input.OwnerUserID = &ident.UserID } + if err := s.refuseVersionGoingBackwards(ctx, ident, repo.KindAgent, + agent.ID, agent.Status, agent.Version); err != nil { + return nil, err + } + if err := s.refuseSubagentCycle(ctx, ident, agent.ID, agent.Subagents); err != nil { + return nil, err + } if err := s.refusePublishedRewrite(ctx, ident, repo.KindAgent, agent.ID, markdown, agent.Status, agent.Version); err != nil { return nil, err @@ -302,6 +309,13 @@ func (s *DefinitionsService) UpdateAgent(ctx context.Context, ident authctx.Iden input.Version = &agent.Version input.Pages = agent.Pages + if err := s.refuseVersionGoingBackwards(ctx, ident, repo.KindAgent, + agent.ID, agent.Status, agent.Version); err != nil { + return nil, err + } + if err := s.refuseSubagentCycle(ctx, ident, agent.ID, agent.Subagents); err != nil { + return nil, err + } if err := s.refusePublishedRewrite(ctx, ident, repo.KindAgent, agent.ID, markdown, agent.Status, agent.Version); err != nil { return nil, err @@ -535,6 +549,80 @@ func (s *DefinitionsService) DeleteSkill(ctx context.Context, ident authctx.Iden /* ── Publishing ─────────────────────────────────────────────────────────── */ +// refuseVersionGoingBackwards enforces §3's "monotonic". +// +// Nothing enforced it before. A spec edited from an older copy republishes an +// older number whose content still matches what was published under it, so the +// rewrite guard sees no conflict and the deployed agent quietly goes +// backwards — the version in the UI reads 2 while the newest thing anybody +// approved was 3. +func (s *DefinitionsService) refuseVersionGoingBackwards(ctx context.Context, + ident authctx.Identity, kind repo.VersionKind, definitionID, status string, version int) error { + + if s.versions == nil || status != "published" || definitionID == "" || version < 1 { + return nil + } + latest, err := s.versions.LatestVersion(ctx, ident, kind, definitionID) + if err != nil || latest == 0 { + // Unreadable, or nothing published yet. Neither is grounds to refuse a + // save: the rewrite guard is what protects published text, and this + // only orders the numbers. + return nil + } + if version < latest { + return domain.Conflict(definition.ErrVersionWentBackwards(definitionID, latest, version).Error()) + } + return nil +} + +// refuseSubagentCycle enforces §3's DAG at publish. +// +// The graph is every organization-visible agent plus the one being published, +// with the incoming definition standing in for its stored self — otherwise an +// edit that CREATES a cycle is checked against the version that did not have +// one, and passes. +// +// Personal agents are not included. They are invisible to everyone else, so +// they cannot complete a loop for anybody else, and loading them would mean +// reading other people's drafts to validate your own. +func (s *DefinitionsService) refuseSubagentCycle(ctx context.Context, + ident authctx.Identity, definitionID string, subagents []string) error { + + if len(subagents) == 0 { + return nil + } + rows, _, err := s.repo.ListAgents(ctx, ident, repo.DefinitionListParams{ + Visibility: "organization", Limit: 500, + }) + if err != nil { + // A graph we could not read is not a graph we can call cyclic. The + // runtime depth cap is what holds when this cannot run. + return nil + } + + graph := make(map[string][]string, len(rows)+1) + for _, rec := range rows { + id, _ := rec["definition_id"].(string) + markdown, _ := rec["markdown"].(string) + if id == "" || markdown == "" || id == definitionID { + continue // the incoming definition replaces its stored self, below + } + if parsed, err := definition.ParseAgent(markdown, definition.Options{}); err == nil && parsed != nil { + graph[id] = parsed.Subagents + } + } + graph[definitionID] = subagents + + if cycle := definition.FindSubagentCycle(graph); cycle != "" { + return domain.Validation( + "that would create a delegation cycle: "+cycle+ + ". Delegation follows these edges, so a loop is a run that delegates "+ + "until it runs out of budget.", + map[string]string{"subagents": "cycle"}) + } + return nil +} + // refusePublishedRewrite fails a publish that would change a version already // published, BEFORE anything is written. //