diff --git a/go-api/internal/config/config.go b/go-api/internal/config/config.go index 71f2f51..fe8a499 100644 --- a/go-api/internal/config/config.go +++ b/go-api/internal/config/config.go @@ -55,6 +55,7 @@ type Config struct { HTTP HTTPConfig DB DBConfig Seed SeedConfig + Agents AgentsConfig Model ModelConfig Knowledge KnowledgeConfig } @@ -148,6 +149,23 @@ type SeedConfig struct { FixturePath string } +// AgentsConfig locates the curated agent specs that ship with the deployment. +// +// The same directory `importagents` publishes from — Dockerfile.api copies +// `agents/` to /app/agents beside the binary, and the importer's own `-dir` +// default is the same path. Pointing both at one directory is what makes the +// protected set and the published set the same set: an agent is built-in +// because the product ships its spec, not because a column says so. +// +// A missing directory is not a boot failure. This service runs in development +// checkouts and test binaries whose working directory has no `agents/`, and +// refusing to start over a protection list would take the API down to defend +// rows that deployment never created. The consequence is stated where it is +// loaded: the protected set is empty, and that is logged. +type AgentsConfig struct { + CuratedPath string +} + type LogConfig struct { Level string } @@ -279,6 +297,9 @@ func Load() (*Config, error) { Seed: SeedConfig{ FixturePath: withDefault("SEED_FIXTURE_PATH", "./seed/fixtures/seed.json"), }, + Agents: AgentsConfig{ + CuratedPath: withDefault("CURATED_AGENTS_PATH", "./agents"), + }, Knowledge: KnowledgeConfig{ EmbedProvider: strings.ToLower(strings.TrimSpace(os.Getenv("EMBED_PROVIDER"))), EmbedAPIKey: strings.TrimSpace(os.Getenv("VOYAGE_API_KEY")), diff --git a/go-api/internal/definition/curated.go b/go-api/internal/definition/curated.go new file mode 100644 index 0000000..953c3d7 --- /dev/null +++ b/go-api/internal/definition/curated.go @@ -0,0 +1,81 @@ +package definition + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strings" +) + +// CuratedIDs reads the agent specs that ship with the deployment and returns +// the set of definition ids they declare. +// +// # WHY THIS EXISTS +// +// An agent is "built-in" when the product ships its spec. There is no column +// saying so and deliberately none is added here: `importagents` publishes these +// same files into `agent_definitions` as ordinary `organization` rows, so a +// curated agent and a tenant-authored shared agent are indistinguishable in the +// table. The distinguishing fact lives on disk, in the directory the importer +// publishes FROM — which is the same fact the frontend uses, where `isShipped` +// tests membership of the ids bundled from `src/agents/**/*.md`. +// +// Reading the directory rather than listing ids in configuration keeps the +// protected set and the published set the same set by construction. Adding a +// ninth agent protects it; removing one stops protecting it; neither needs a +// code change, and neither can drift. +// +// The ids are parsed out of the frontmatter with the same parser the importer +// uses, NOT taken from the filename. A file named `analytics-agent.md` whose +// frontmatter says `id: analytics` publishes as `analytics`, and protecting the +// filename would protect nothing. +// +// A missing directory returns an empty set and no error: development checkouts +// and test binaries run from working directories that have no `agents/`, and +// this is a protection list rather than something to serve from. A directory +// that exists but holds a spec that will not parse IS an error — that same file +// would fail the importer, and staying quiet about it would leave an agent +// unprotected for a reason nobody could see. +func CuratedIDs(dir string) (map[string]bool, error) { + entries, err := os.ReadDir(dir) + if os.IsNotExist(err) { + return map[string]bool{}, nil + } + if err != nil { + return nil, fmt.Errorf("read curated agents in %s: %w", dir, err) + } + + ids := make(map[string]bool, len(entries)) + for _, e := range entries { + name := e.Name() + // README.md is documentation, not a spec — skipped by name, the same + // way cmd/importagents skips it, so that a parse failure always means + // something is actually wrong. + if e.IsDir() || !strings.HasSuffix(name, ".md") || name == "README.md" { + continue + } + raw, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil { + return nil, fmt.Errorf("read %s: %w", name, err) + } + parsed, err := ParseAgent(string(raw), Options{}) + if err != nil { + return nil, fmt.Errorf("%s: %w", name, err) + } + if parsed.ID != "" { + ids[parsed.ID] = true + } + } + return ids, nil +} + +// SortedIDs renders a set as a stable list, for logging. +func SortedIDs(set map[string]bool) []string { + out := make([]string, 0, len(set)) + for id := range set { + out = append(out, id) + } + sort.Strings(out) + return out +} diff --git a/go-api/internal/httpserver/definitions_api_test.go b/go-api/internal/httpserver/definitions_api_test.go index 7b9674d..4acbf18 100644 --- a/go-api/internal/httpserver/definitions_api_test.go +++ b/go-api/internal/httpserver/definitions_api_test.go @@ -8,6 +8,8 @@ import ( "strings" "testing" "time" + + "github.com/krow/krow-backend/go-api/internal/httpserver" ) // Phase 4E — Backend CRUD APIs for authored Agent and Skill definitions. @@ -1327,3 +1329,194 @@ Ask myself. t.Fatal("an agent naming itself as its own subagent was published") } } + +/* ── 8. Curated (built-in) agent protection ───────────────────────────────── */ + +// agentMD builds a minimal valid agent definition for a given id. +func agentMD(id, name string) string { + return fmt.Sprintf("---\nid: %s\nname: %s\nstatus: draft\nversion: 1\npages:\n - candidates\n---\n\n## Instructions\nDo the thing.\n", id, name) +} + +// TestCuratedAgentIsNotDeletable covers the protection the Agents list implies +// but React alone cannot enforce. +// +// A curated agent is published by `importagents` as an ordinary organization +// row, so nothing in the table distinguishes it from a shared agent somebody +// authored — the distinguishing fact is that the deployment ships its spec. +// Without a check at the endpoint, any operator with a terminal could delete +// the definition the RUNTIME resolves from, leaving the agent in the list and +// every run of it answering 404. +func TestCuratedAgentIsNotDeletable(t *testing.T) { + a := newAPI(t, httpserver.WithCuratedAgents("curated-agent")) + + // What importagents publishes: the curated spec, at organization visibility. + curated := a.do("POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agentMD("curated-agent", "Curated Agent"), + "visibility": "organization", + }) + if curated.code != http.StatusCreated && curated.code != http.StatusOK { + t.Fatalf("publish curated agent: got %d", curated.code) + } + curatedID := curated.record(t)["id"].(string) + + // The admin who may delete any other organization definition is refused + // this one. + del := a.do("DELETE", "/api/v1/agent-definitions/"+curatedID, nil) + if del.code != http.StatusForbidden { + t.Errorf("delete curated agent: got %d, want 403", del.code) + } + + // And it is still there — refused, not deleted-then-reported. + after := a.do("GET", "/api/v1/agent-definitions/"+curatedID, nil) + if after.code != http.StatusOK { + t.Fatalf("curated agent after refused delete: got %d, want 200", after.code) + } + if got := after.record(t)["definition_id"]; got != "curated-agent" { + t.Errorf("curated agent definition_id = %v, want curated-agent", got) + } +} + +// TestPersonalOverrideOfCuratedAgentStaysDeletable protects the revert path. +// +// "Revert to shipped" in the Agents list deletes the account's own definition +// of a shipped id. Protecting by id alone would break it, so the guard is +// scoped to the organization tier — this is the test that says so. +func TestPersonalOverrideOfCuratedAgentStaysDeletable(t *testing.T) { + a := newAPI(t, httpserver.WithCuratedAgents("curated-agent")) + + override := a.do("POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agentMD("curated-agent", "My Version"), + "visibility": "personal", + }) + if override.code != http.StatusCreated && override.code != http.StatusOK { + t.Fatalf("create personal override: got %d", override.code) + } + overrideID := override.record(t)["id"].(string) + + del := a.do("DELETE", "/api/v1/agent-definitions/"+overrideID, nil) + if del.code != http.StatusOK { + t.Errorf("delete personal override of a curated id: got %d, want 200", del.code) + } + after := a.do("GET", "/api/v1/agent-definitions/"+overrideID, nil) + if after.code != http.StatusNotFound { + t.Errorf("override after delete: got %d, want 404", after.code) + } +} + +// TestCustomAgentDeleteIsIsolated is the isolation case: removing one custom +// agent removes that agent and nothing else. +func TestCustomAgentDeleteIsIsolated(t *testing.T) { + a := newAPI(t, httpserver.WithCuratedAgents("curated-agent")) + + // A curated agent, a second custom agent, and a skill — none of which the + // delete below is about. + curated := a.do("POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agentMD("curated-agent", "Curated Agent"), "visibility": "organization", + }).record(t)["id"].(string) + keep := a.do("POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agentMD("keep-me", "Keep Me"), "visibility": "personal", + }).record(t)["id"].(string) + skill := a.do("POST", "/api/v1/skill-definitions", map[string]any{ + "markdown": validSkillMD, "visibility": "personal", + }).record(t)["id"].(string) + + target := a.do("POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": agentMD("remove-me", "Remove Me"), "visibility": "personal", + }).record(t)["id"].(string) + + if got := a.do("DELETE", "/api/v1/agent-definitions/"+target, nil); got.code != http.StatusOK { + t.Fatalf("delete custom agent: got %d", got.code) + } + + // Gone. + if got := a.do("GET", "/api/v1/agent-definitions/"+target, nil); got.code != http.StatusNotFound { + t.Errorf("removed agent: got %d, want 404", got.code) + } + // Everything else untouched. + for name, id := range map[string]string{"curated agent": curated, "other custom agent": keep} { + if got := a.do("GET", "/api/v1/agent-definitions/"+id, nil); got.code != http.StatusOK { + t.Errorf("%s after an unrelated delete: got %d, want 200", name, got.code) + } + } + if got := a.do("GET", "/api/v1/skill-definitions/"+skill, nil); got.code != http.StatusOK { + t.Errorf("skill after an unrelated agent delete: got %d, want 200", got.code) + } +} + +// TestArchiveAndRestorePreserveTheSameAgent is the persistence half of Remove. +// +// Removing an authored agent archives it. That claim is only worth anything if +// archiving keeps the row: the same uuid, the same definition_id and the same +// Markdown, so restoring returns the agent somebody wrote rather than a new one +// wearing its name. This asserts the round trip against the real endpoints. +func TestArchiveAndRestorePreserveTheSameAgent(t *testing.T) { + a := newAPI(t, httpserver.WithCuratedAgents("curated-agent")) + + const live = `--- +id: coverage-helper +name: Coverage Helper +status: published +version: 3 +pages: + - candidates +--- + +## Instructions +Find the shifts nobody has taken. +` + created := a.do("POST", "/api/v1/agent-definitions", map[string]any{ + "markdown": live, "visibility": "personal", + }) + if created.code != http.StatusCreated && created.code != http.StatusOK { + t.Fatalf("create agent: got %d", created.code) + } + rec := created.record(t) + id := rec["id"].(string) + definitionID := rec["definition_id"] + + // Remove -> archive. Same row, same body, only the status moves. + archived := a.do("PATCH", "/api/v1/agent-definitions/"+id, map[string]any{ + "markdown": strings.Replace(live, "status: published", "status: archived", 1), + }) + if archived.code != http.StatusOK { + t.Fatalf("archive agent: got %d", archived.code) + } + arc := archived.record(t) + if arc["status"] != "archived" { + t.Errorf("status after remove = %v, want archived", arc["status"]) + } + if arc["id"] != id || arc["definition_id"] != definitionID { + t.Errorf("identity changed on archive: %v/%v, want %s/%v", + arc["id"], arc["definition_id"], id, definitionID) + } + if !strings.Contains(arc["markdown"].(string), "Find the shifts nobody has taken.") { + t.Error("instructions were lost when the agent was archived") + } + + // It is still there — removal is not deletion. + if got := a.do("GET", "/api/v1/agent-definitions/"+id, nil); got.code != http.StatusOK { + t.Fatalf("removed agent should still be readable: got %d, want 200", got.code) + } + + // Restore -> the SAME agent, as a draft. + restored := a.do("PATCH", "/api/v1/agent-definitions/"+id, map[string]any{ + "markdown": strings.Replace(live, "status: published", "status: draft", 1), + }) + if restored.code != http.StatusOK { + t.Fatalf("restore agent: got %d", restored.code) + } + res := restored.record(t) + if res["status"] != "draft" { + t.Errorf("status after restore = %v, want draft", res["status"]) + } + if res["id"] != id || res["definition_id"] != definitionID { + t.Errorf("restore created a different agent: %v/%v, want %s/%v", + res["id"], res["definition_id"], id, definitionID) + } + if !strings.Contains(res["markdown"].(string), "Find the shifts nobody has taken.") { + t.Error("instructions were lost on the round trip") + } + if got := res["version"]; got != arc["version"] { + t.Errorf("version moved on a restore: %v -> %v", arc["version"], got) + } +} diff --git a/go-api/internal/httpserver/server.go b/go-api/internal/httpserver/server.go index 5fc904f..565217f 100644 --- a/go-api/internal/httpserver/server.go +++ b/go-api/internal/httpserver/server.go @@ -35,6 +35,7 @@ import ( "github.com/krow/krow-backend/go-api/internal/auth" "github.com/krow/krow-backend/go-api/internal/config" "github.com/krow/krow-backend/go-api/internal/db" + "github.com/krow/krow-backend/go-api/internal/definition" "github.com/krow/krow-backend/go-api/internal/knowledge" "github.com/krow/krow-backend/go-api/internal/runtime" "github.com/krow/krow-backend/go-api/internal/service" @@ -109,6 +110,11 @@ type serverOptions struct { // Not configuration: it describes the artefact, not the deployment, and an // environment variable could disagree with the code it claims to describe. version string + + // curatedAgents replaces the set New would otherwise read from disk. + // nil means "read the configured directory"; an empty non-nil set means + // "protect nothing", which is a thing a test needs to be able to say. + curatedAgents map[string]bool } // WithBuildVersion records which build this is. @@ -154,6 +160,21 @@ func WithClock(now func() time.Time) Option { // It does not weaken anything: the engine still loads agents through the same // loader, still runs them under the same budgets, and still authorizes through // the same principal. Only the model behind it changes. +// WithCuratedAgents names the delete-protected agent ids directly. +// +// Production loads these from disk; this exists so a test can state its own +// protected set without a directory, exactly as WithAgentEngine lets one +// supply an engine without a model credential. +func WithCuratedAgents(ids ...string) Option { + return func(o *serverOptions) { + set := make(map[string]bool, len(ids)) + for _, id := range ids { + set[id] = true + } + o.curatedAgents = set + } +} + func WithAgentEngine(e *runtime.Engine) Option { return func(o *serverOptions) { o.agents = e } } @@ -232,6 +253,30 @@ func New(cfg *config.Config, database *db.DB, log *slog.Logger, opts ...Option) // rather than becoming an agent that silently cannot do what it claims. s.definitions = s.definitions.WithToolCheck(toolRegistry.Known) + // The agents this deployment ships specs for, so DELETE refuses them at the + // endpoint rather than only in the list that renders the button. + // + // Read from the directory `importagents` publishes from, so the protected + // set is the published set by construction. A deployment without that + // directory protects nothing and says so here, once, at boot: silence would + // leave an operator believing in a guard that is not running. + curated := o.curatedAgents + if curated == nil { + loaded, err := definition.CuratedIDs(cfg.Agents.CuratedPath) + if err != nil { + return nil, fmt.Errorf("load curated agents: %w", err) + } + curated = loaded + } + if len(curated) == 0 { + log.Warn("no curated agent specs found; built-in agents are not delete-protected", + "path", cfg.Agents.CuratedPath) + } else { + log.Info("curated agents are delete-protected", + "count", len(curated), "ids", definition.SortedIDs(curated)) + } + s.definitions = s.definitions.WithCuratedAgents(curated) + mux := http.NewServeMux() mux.HandleFunc("GET /health", s.handleHealth) s.endpoints = s.routeAuth(mux) + s.routeResources(mux) + s.routeMe(mux) + diff --git a/go-api/internal/service/definitions.go b/go-api/internal/service/definitions.go index e331707..f61d3a3 100644 --- a/go-api/internal/service/definitions.go +++ b/go-api/internal/service/definitions.go @@ -34,6 +34,10 @@ type DefinitionsService struct { // unknownTools reports which of a spec's tool names are not registered. // nil means no check — the shipped importer path, which has its own. unknownTools func([]string) []string + + // curatedAgents holds the definition ids the deployment ships specs for. + // nil or empty means nothing is protected — see WithCuratedAgents. + curatedAgents map[string]bool } // NewDefinitions builds a definitions service over a repository. @@ -58,6 +62,38 @@ func (s *DefinitionsService) WithToolCheck(unknown func([]string) []string) *Def return s } +// WithCuratedAgents teaches the service which agents ship with the product. +// +// A curated agent's ORGANIZATION row is the product's own definition of it, +// published by `importagents` from the specs in the deployment image. Deleting +// that row does not remove a shipped agent from the interface — the frontend +// still bundles it — it removes the definition the RUNTIME resolves from, so +// the agent keeps appearing in the list and every run of it answers 404. That +// is a worse outcome than a refused request, and it is reachable today by any +// operator with a terminal: the list never offers the button, but the endpoint +// has never enforced what the button implies. +// +// PERSONAL rows are deliberately NOT protected, even when they carry a curated +// id. A personal definition of a shipped id is an override somebody authored, +// and deleting it is exactly the "Revert to shipped" action the Agents list +// offers. Protecting by id alone would break that. +// +// Injected rather than read here so this package does not learn where the +// specs live, and so a test can name its own protected set — the same shape as +// WithToolCheck above. +func (s *DefinitionsService) WithCuratedAgents(ids map[string]bool) *DefinitionsService { + s.curatedAgents = ids + return s +} + +// isCurated reports whether an agent definition is one the product ships. +func (s *DefinitionsService) isCurated(definitionID string) bool { + if s.curatedAgents == nil { + return false + } + return s.curatedAgents[definitionID] +} + // rejectUnknownTools fails a definition that names a tool that does not exist. func (s *DefinitionsService) rejectUnknownTools(markdown string) error { if s.unknownTools == nil { @@ -355,6 +391,18 @@ func (s *DefinitionsService) DeleteAgent(ctx context.Context, ident authctx.Iden if !known || role == domain.RoleTalent { return nil, domain.Forbidden() } + + // A curated agent is the product's own, and is refused here rather than + // merely hidden in the list. Only the organization tier is protected: a + // personal row carrying the same id is somebody's override, and removing + // it is the supported way back to the shipped definition. + // + // Placed after the role check so the answer to a talent user is the same + // 403 for every organization row, curated or not — which agents a tenant + // runs is not something an unprivileged caller should be able to probe. + if did, _ := existing["definition_id"].(string); s.isCurated(did) { + return nil, domain.Forbidden() + } } if _, err := s.repo.DeleteAgent(ctx, ident, id); err != nil {