Merge pull request #130 from kiserufetch/fix/list-task-credentials-empty-required

fix(tools): list_task_credentials emits invalid "required": [""] and breaks workers on strict LLM providers

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) {

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
 }

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

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" {

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