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/agent/tools_creds.go b/agent/tools_creds.go index b8f4cd5..5b07df1 100644 --- a/agent/tools_creds.go +++ b/agent/tools_creds.go @@ -64,7 +64,7 @@ func (t *ToolSet) listTaskCredentials() actool.CoreTool { return writeTool( "list_task_credentials", "List this task's test accounts: user-declared accounts (e.g. program-provided aliases; a user-declared entry with an empty password means: register an account on that domain with a strong password, then record it via save_task_credential) plus accounts recorded by earlier runs. Check this BEFORE registering a new account, and use matching entries for authenticated testing.", - obj(map[string]any{}, ""), + obj(map[string]any{}), func(_ context.Context, _ json.RawMessage) (actool.Result, error) { if t.ts == nil { return actool.Errorf("list_task_credentials is not enabled: task store is not initialized"), nil diff --git a/server/orchestration.go b/server/orchestration.go index 842d0ce..835b2f1 100644 --- a/server/orchestration.go +++ b/server/orchestration.go @@ -536,6 +536,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) @@ -944,6 +945,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