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. //