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).
This commit is contained in:
BaaRai
2026-11-10 09:46:49 +01:00
parent a66c716c0a
commit a44af3ca2b
4 changed files with 155 additions and 2 deletions
+41
View File
@@ -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) {
+10 -2
View File
@@ -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
}
+14
View File
@@ -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" {
+90
View File
@@ -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