fix(agent): D1-D3 cleanups — dead code, N+1 query, unvalidated outcome enum
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user