From 76f76308cc59e937ef6e78d7fe477a37cbc57446 Mon Sep 17 00:00:00 2001 From: dtoro Date: Sat, 11 Jul 2026 20:13:48 +0200 Subject: [PATCH] =?UTF-8?q?fix(agent):=20D1-D3=20cleanups=20=E2=80=94=20de?= =?UTF-8?q?ad=20code,=20N+1=20query,=20unvalidated=20outcome=20enum?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes D1-D3 of plans/2026-07-11-nomos-agent-code-review.md: - D1: deleted isTaskTool — defined, never called (dispatch already checks handleTaskTool's own `handled` return value). - D2: recordTouched issued one SELECT per entity slug found in a tool call's args; batched into one `WHERE slug = ANY($1)` query. Verified live: a turn naming three separate entities recorded involves edges for all three via the single batched lookup. - D3: complete_task's outcome had a declared enum (success|failure|partial) in its tool schema but nothing validated it — an out-of-enum value (model typo or a weaker model not respecting the schema) silently persisted as-is, with only "failure" special-cased (anything else became status='done' regardless of what the value actually said). Now validated in handleTaskTool: empty defaults to "success" (unchanged), a recognized value passes through, anything else defaults to "partial" (safer than silently treating an unrecognized value as success) with a warning logged. Verified live: instructed the agent to call complete_task with outcome="unclear" — persisted as outcome='partial', not the literal invalid string. Co-Authored-By: Claude Opus 4.8 --- cmd/nomos/store.go | 42 +++++++++++++++++++++++++++++++++--------- cmd/nomos/tasks.go | 29 ++++++++++++++++++----------- 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index 691a38f..e719618 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -716,15 +716,39 @@ func (s *store) recordTouched(ctx context.Context, sessionID, toolName string, a `SELECT entity_id FROM agent_sessions WHERE id = $1`, sessionID).Scan(&taskEntityID); err != nil || taskEntityID == uuid.Nil { return // no task entity to anchor edges on } - q := sqlcgen.New(s.pool) + + // One batched lookup instead of a SELECT per slug — a tool call naming + // several entities (e.g. a multi-target comparison) used to issue N + // round-trips here for N slugs found in its args. + slugList := make([]string, 0, len(slugs)) for slug := range slugs { - var entID uuid.UUID - var etype string - if err := s.pool.QueryRow(ctx, - `SELECT id, type FROM entities WHERE slug = $1`, slug).Scan(&entID, &etype); err != nil { - continue // unknown slug — skip + slugList = append(slugList, slug) + } + rows, err := s.pool.Query(ctx, + `SELECT id, type, slug FROM entities WHERE slug = ANY($1)`, slugList) + if err != nil { + return + } + type found struct { + id uuid.UUID + etype string + } + matched := make(map[string]found, len(slugList)) + for rows.Next() { + var f found + var slug string + if rows.Scan(&f.id, &f.etype, &slug) == nil { + matched[slug] = f } - if touchExcludedTypes[etype] || entID == taskEntityID { + } + rows.Close() + if err := rows.Err(); err != nil { + return + } + + q := sqlcgen.New(s.pool) + for slug, f := range matched { + if touchExcludedTypes[f.etype] || f.id == taskEntityID { continue } // Idempotent involves edge (task → entity), same guard as upsert_knowledge. @@ -734,10 +758,10 @@ func (s *store) recordTouched(ctx context.Context, sessionID, toolName string, a WHERE NOT EXISTS ( SELECT 1 FROM relationships WHERE source_id = $1 AND target_id = $2 AND type = 'involves' AND valid_to IS NULL)`, - taskEntityID, entID) + taskEntityID, f.id) // Live pulse for the panel. correlation_id = sessionID lets the frontend // filter to the active task. - _ = observability.Event(ctx, q, "entity.touched", &entID, "info", "nomos", sessionID, + _ = observability.Event(ctx, q, "entity.touched", &f.id, "info", "nomos", sessionID, map[string]any{"slug": slug, "tool": toolName}) } } diff --git a/cmd/nomos/tasks.go b/cmd/nomos/tasks.go index b5d0fe4..ce4cb00 100644 --- a/cmd/nomos/tasks.go +++ b/cmd/nomos/tasks.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "log/slog" "strings" ) @@ -136,15 +137,6 @@ func taskToolDefs() []toolDef { } } -func isTaskTool(name string) bool { - switch name { - case "set_goal", "propose_plan", "update_plan_step", "ask_operator", "complete_task": - return true - default: - return false - } -} - // toInt coerces a JSON tool-arg number (float64 after unmarshal) to int. func toInt(v any) int { switch n := v.(type) { @@ -248,8 +240,23 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args case "complete_task": outcome, _ := args["outcome"].(string) summary, _ := args["summary"].(string) - if outcome == "" { - outcome = "success" + switch outcome { + case "": + outcome = "success" // no outcome given at all — assume success, the common case + case "success", "failure", "partial": + // valid, use as-is + default: + // The tool schema declares an enum, but a weaker model (or a + // typo) can still send anything — an unrecognized value used to + // persist as-is, silently, with only "failure" special-cased + // (store.completeTask derives status='failed' from it; anything + // else became status='done' regardless of what the value + // actually said). Default to "partial" rather than silently + // treating an unrecognized value as "success" — safer to + // under-claim than over-claim a task's outcome. + slog.Warn("nomos: complete_task got an unrecognized outcome, defaulting to partial", + "session", sessionID, "outcome", outcome) + outcome = "partial" } if err := a.store.completeTask(ctx, sessionID, outcome, summary); err != nil { return fmt.Sprintf("error completing task: %v", err), true