From a44af3ca2b389688452f4c92ff7019416a375a0d Mon Sep 17 00:00:00 2001 From: BaaRai <130491490+BaaRai@users.noreply.github.com> Date: Tue, 10 Nov 2026 09:46:49 +0100 Subject: [PATCH] fix(tools): harden obj against blank required names and heal seeded list_task_credentials rows The previous commit stops list_task_credentials from building its schema with obj(map[string]any{}, ""). Two gaps remained: - obj still emitted whatever it was given, so the same typo on another tool would again produce "required": [""]. obj now drops names that are blank after strings.TrimSpace and omits the "required" key when nothing is left. - Tool schemas are cached in the tools table and SeedTool is insert-only, so the code fix only reached fresh installs. Existing databases kept the malformed row and strict providers kept rejecting every worker run. healListTaskCredentialsSchema runs at startup next to the other tool seeds and removes the "required" key from that one row, only when it is exactly [""]: UPDATE tools SET schema = schema - 'required', updated_at=now() WHERE key='list_task_credentials' AND schema->'required' = '[""]'::jsonb It is idempotent (a healed row no longer matches), needs no settings flag, and leaves the description, bindings, enabled flag and every other tool untouched. tool_schema_refresh_v7 is not bumped. Tests: TestObjDropsBlankRequired and TestListTaskCredentialsSchemaHasNoBlankRequired (agent), TestHealListTaskCredentialsSchema (server, Postgres). --- agent/toolcatalog_test.go | 41 ++++++++++++++++++ agent/tools.go | 12 +++++- server/orchestration.go | 14 ++++++ server/tools_wire_test.go | 90 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 2 deletions(-) diff --git a/agent/toolcatalog_test.go b/agent/toolcatalog_test.go index 1cfc3f7..0419ef8 100644 --- a/agent/toolcatalog_test.go +++ b/agent/toolcatalog_test.go @@ -3,6 +3,7 @@ package agent import ( "context" "encoding/json" + "strings" "testing" actool "github.com/Autumn-27/norma/tool" @@ -46,6 +47,46 @@ func TestBuiltinToolSeeds(t *testing.T) { } } +// TestObjDropsBlankRequired: obj never emits a blank required name, and omits the +// "required" key entirely when no real name is left. +func TestObjDropsBlankRequired(t *testing.T) { + if got, ok := obj(map[string]any{}, "")["required"]; ok { + t.Errorf(`obj(props, "") required = %v, want no required key`, got) + } + got, _ := obj(map[string]any{}, " ", "x")["required"].([]string) + if len(got) != 1 || got[0] != "x" { + t.Errorf(`obj(props, " ", "x") required = %v, want [x]`, got) + } +} + +// TestListTaskCredentialsSchemaHasNoBlankRequired guards the seeded schema of the +// parameterless list_task_credentials tool: "required": [""] is rejected by strict +// providers, which fails every worker run. +func TestListTaskCredentialsSchemaHasNoBlankRequired(t *testing.T) { + for _, s := range BuiltinToolSeeds() { + if s.Key != "list_task_credentials" { + continue + } + raw, err := json.Marshal(s.Schema) + if err != nil { + t.Fatal(err) + } + var schema struct { + Required []string `json:"required"` + } + if err := json.Unmarshal(raw, &schema); err != nil { + t.Fatalf("schema %s: %v", raw, err) + } + for _, r := range schema.Required { + if strings.TrimSpace(r) == "" { + t.Errorf("blank required entry in schema %s", raw) + } + } + return + } + t.Fatal("list_task_credentials not seeded") +} + // TestDecorateToolInjectsDefaults verifies a schema "default" fills a missing param // before the underlying handler runs, and an explicitly-provided value is kept. func TestDecorateToolInjectsDefaults(t *testing.T) { diff --git a/agent/tools.go b/agent/tools.go index 815e014..62c25b4 100644 --- a/agent/tools.go +++ b/agent/tools.go @@ -351,8 +351,16 @@ func pidList(raw []json.RawMessage) []int64 { func obj(props map[string]any, required ...string) map[string]any { m := map[string]any{"type": "object", "properties": props} - if len(required) > 0 { - m["required"] = required + // Blank names are dropped, and "required" is omitted when nothing is left: + // strict providers reject "required": [""] and fail every run carrying the tool. + req := make([]string, 0, len(required)) + for _, r := range required { + if strings.TrimSpace(r) != "" { + req = append(req, r) + } + } + if len(req) > 0 { + m["required"] = req } return m } diff --git a/server/orchestration.go b/server/orchestration.go index 49941bd..a43dc2d 100644 --- a/server/orchestration.go +++ b/server/orchestration.go @@ -503,6 +503,7 @@ func (s *Server) seedOrchestrationTools() { s.seedWorkerReadToolsUnbind() // list_facts/list_companies/list_worker_traces unbound from worker's defaults (one-time) s.seedWorkerReadbackRebind() // repairs an old migration's wrong removal: re-binds search_all_worker_traces/get_worker_trace/node_detail back onto worker (one-time) s.seedTaskCredentialBindings() // save/list_task_credential onto worker + pentest (one-time, #3) + s.healListTaskCredentialsSchema() s.seedAutoReportFindingBinding() s.unbindGoalMetDefault() s.reseedGoalsPrompt() // goals prompt gains the "extract operation constraints" step → old DBs append a new default version (one-time) @@ -910,6 +911,19 @@ func (s *Server) seedTaskCredentialBindings() { _ = s.m.pg.SetSetting(flag, "true") } +// healListTaskCredentialsSchema repairs the list_task_credentials row seeded by +// older builds, whose schema carried "required": [""] — strict providers reject +// it, which fails every worker run. SeedTool is first-insert-only, so the code fix +// alone never reaches an already-seeded row. Only that exact malformed value on +// that one key matches, and only the "required" key is dropped: the rest of the +// schema, the description, bindings and enabled flag are untouched. A healed row +// no longer matches, so this needs no settings flag and is a no-op afterwards. +func (s *Server) healListTaskCredentialsSchema() { + if _, err := s.m.pg.Exec(`UPDATE tools SET schema = schema - 'required', updated_at=now() WHERE key='list_task_credentials' AND schema->'required' = '[""]'::jsonb`); err != nil { + log.Printf("[tools] heal list_task_credentials schema failed: %v", err) + } +} + func (s *Server) seedWorkerReadbackRebind() { const flag = "worker_readback_rebind_v2" // v2: adds node_detail if v, _, _ := s.m.pg.GetSetting(flag); v == "true" { diff --git a/server/tools_wire_test.go b/server/tools_wire_test.go index dd4850e..3e00bfb 100644 --- a/server/tools_wire_test.go +++ b/server/tools_wire_test.go @@ -103,6 +103,96 @@ func TestWireTools(t *testing.T) { } } +// TestHealListTaskCredentialsSchema: a list_task_credentials row seeded by an older +// build carries "required": [""]. The startup heal drops that key, leaves the rest of +// the row alone, changes nothing on a second run, and touches no other tool. +func TestHealListTaskCredentialsSchema(t *testing.T) { + dsn, _, err := db.DSN() + if err != nil { + t.Skipf("no database config (%v) — skipping", err) + } + pg, err := db.Open(dsn) + if err != nil { + t.Skipf("postgres unavailable (%v) — skipping", err) + } + t.Cleanup(func() { pg.Close() }) + s := &Server{m: &Manager{pg: pg}} + + const key = "list_task_credentials" + // Other tools the heal must never touch: a normal required list, and the same + // malformed value under a different key. + const probeRequired, probeBlank = "heal_probe_required", "heal_probe_blank" + const broken = `{"type":"object","properties":{},"required":[""]}` + + // The package test DB is shared: put back whatever it held for the real key. + old, err := pg.GetTool(key) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if old == nil { + pg.Exec(`DELETE FROM tools WHERE key=$1`, key) + } else { + pg.Exec(`UPDATE tools SET schema=$2::jsonb WHERE key=$1`, key, string(old.Schema)) + } + pg.Exec(`DELETE FROM tools WHERE key IN ($1,$2)`, probeRequired, probeBlank) + }) + for k, schema := range map[string]string{ + key: broken, + probeRequired: `{"type":"object","properties":{"x":{"type":"string"}},"required":["x"]}`, + probeBlank: broken, + } { + if _, err := pg.Exec(`INSERT INTO tools(key, description, schema, agents) VALUES ($1, 'legacy row', $2::jsonb, '["worker"]') +ON CONFLICT (key) DO UPDATE SET schema=EXCLUDED.schema`, k, schema); err != nil { + t.Fatal(err) + } + } + + type row struct { + schema, description, agents, updatedAt string + enabled bool + } + read := func(k string) row { + t.Helper() + var r row + if err := pg.QueryRow(`SELECT schema::text, description, agents::text, updated_at::text, enabled FROM tools WHERE key=$1`, k). + Scan(&r.schema, &r.description, &r.agents, &r.updatedAt, &r.enabled); err != nil { + t.Fatalf("read %s: %v", k, err) + } + return r + } + before, beforeRequired, beforeBlank := read(key), read(probeRequired), read(probeBlank) + + s.healListTaskCredentialsSchema() + + healed := read(key) + var schema map[string]any + if err := json.Unmarshal([]byte(healed.schema), &schema); err != nil { + t.Fatal(err) + } + if _, ok := schema["required"]; ok { + t.Fatalf("required survived the heal: %s", healed.schema) + } + if schema["type"] != "object" || schema["properties"] == nil { + t.Errorf("heal dropped more than required: %s", healed.schema) + } + if healed.description != before.description || healed.agents != before.agents || healed.enabled != before.enabled { + t.Errorf("heal changed description/bindings/enabled: %+v -> %+v", before, healed) + } + + s.healListTaskCredentialsSchema() + + if again := read(key); again != healed { + t.Errorf("second heal changed the row: %+v -> %+v", healed, again) + } + if got := read(probeRequired); got != beforeRequired { + t.Errorf("heal touched a tool with a non-empty required: %+v -> %+v", beforeRequired, got) + } + if got := read(probeBlank); got != beforeBlank { + t.Errorf("heal touched another tool key: %+v -> %+v", beforeBlank, got) + } +} + func mustJSON(v any) json.RawMessage { b, _ := json.Marshal(v) return b