From 3de359b85f513a9c0cff06a232757d1f724c8133 Mon Sep 17 00:00:00 2001 From: dtoro Date: Tue, 14 Jul 2026 20:40:49 +0200 Subject: [PATCH] =?UTF-8?q?feat(agent):=20close=20knowledge=20loop=20?= =?UTF-8?q?=E2=80=94=20refuse=20complete=5Ftask=20without=20writeback=20(D?= =?UTF-8?q?.1+D.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D.1 — complete_task structural gate: - hadDiscovery(ctx, session) reports whether the session ran `run` successfully against a live target (NOT get_entity/list_lxcs — those are DB lookups, not new facts). A trivial Q&A that only calls get_entity is a degenerate case and must NOT be blocked. - complete_task with outcome=success is REFUSED when hadDiscovery && ! hadEntityWriteback. The refusal fires BEFORE completeTask runs, so the session stays in 'executing' state and the agent must call update_entity_attributes/create_relationship then retry complete_task. An explicit failure/partial is allowed through (the agent is acknowledging it didn't finish — no reason to force writeback). - Replaces the prior advisory warning (5.5) which the agent consistently ignored. The agent saw the warning and ended the task anyway; this gate makes the writeback a hard prerequisite for success. D.2 — propose_plan auto-append writeback step: - When the agent proposes a plan whose steps don't mention update_entity_attributes or create_relationship, D.2 appends a final 'Write back: update_entity_attributes + create_relationship + upsert_knowledge' step before persisting. The result string tells the agent it was appended. - With the seq-order enforcement (5.6) and D.1's complete_task gate, the agent must complete the writeback step (and actually call the tools) to finish. Neither relies on the agent reading SOUL.md. - Removed the old advisory writeback nudge from propose_plan's result string — D.2 makes it structural. - Updated the propose_plan tool description to state both gates crisply. Verification: - TestHadDiscoveryAndWriteback: hadDiscovery true only after a successful `run`; false after failed run, get_entity, or no calls. hadEntityWriteback true only after update_entity_attributes/create_relationship. - e2e against the live agent (oikos-nomos-1, v0.5.1): - D.2: agent proposed 3 steps (no writeback); D.2 auto-appended step 4 'Write back: update_entity_attributes + ...'. Result string said '(appended a writeback step — your plan didn't include one; step 4)'. - D.1: agent ran `run` (uptime on lxc:gitea), called complete_task, was REFUSED ('Refused: this session ran run against live targets (discovery) but did not call update_entity_attributes...'). Agent self-corrected: called update_entity_attributes, retried complete_task, succeeded. Knowledge loop closed end-to-end. Version 0.5.0 -> 0.5.1 (patch: structural enforcement of existing intent). --- VERSION | 2 +- cmd/nomos/store.go | 25 ++++++++++++++ cmd/nomos/store_test.go | 73 +++++++++++++++++++++++++++++++++++++++++ cmd/nomos/tasks.go | 69 ++++++++++++++++++++++++++++---------- 4 files changed, 151 insertions(+), 18 deletions(-) diff --git a/VERSION b/VERSION index 8f0916f..4b9fcbe 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.5.0 +0.5.1 diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index 0c1544e..e568ad7 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -726,6 +726,31 @@ func (s *store) hadEntityWriteback(ctx context.Context, sessionID string) bool { return count > 0 } +// hadDiscovery checks whether this session ran `run` successfully against a +// real target — i.e. discovered live state (versions, package counts, host +// facts, service status) that the DB didn't have. Used by complete_task to +// refuse success when discovery happened but no writeback followed (the +// knowledge-loop drift the prior warnings failed to close — the agent +// ignored advisory text, so D.1 makes it structural). +// +// Only `run` counts as discovery here, NOT get_entity/list_lxcs/etc. — those +// are DB lookups, not new facts. A trivial Q&A ("status of lxc:dns?") that +// only calls get_entity is a degenerate case (SOUL.md: "Don't invent +// attributes that don't exist") and must NOT be blocked. Only sessions that +// actually executed against a live target get the writeback gate. +func (s *store) hadDiscovery(ctx context.Context, sessionID string) bool { + if s == nil || sessionID == "" { + return false // fail safe: don't block when we can't check + } + var count int + s.pool.QueryRow(ctx, ` + SELECT COUNT(*) FROM agent_activity + WHERE session_id = $1 + AND tool_name = 'run' + AND success = true`, sessionID).Scan(&count) + return count > 0 +} + // staleGoalSession is a goal-bearing task that's gone idle without reaching // a terminal state — the idle-sweep worker's work list (fix 2+3 of // plans/2026-07-11-task-completion-safety-net.md). diff --git a/cmd/nomos/store_test.go b/cmd/nomos/store_test.go index c18ab5c..80e58e7 100644 --- a/cmd/nomos/store_test.go +++ b/cmd/nomos/store_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/dtoro/oikos/internal/db" + "github.com/google/uuid" "github.com/jackc/pgx/v5" ) @@ -238,3 +239,75 @@ func TestProposePlan_RefuseInFlight(t *testing.T) { t.Fatalf("revised step generation = %d, want 1 (fresh-start after DELETE resets generation)", revisedSteps[0].Generation) } } + +// TestHadDiscoveryAndWriteback is the store-level proof for D.1 (refuse +// complete_task when discovery ran without writeback). hadDiscovery must +// report true only after a successful `run` call; hadEntityWriteback must +// report true only after a successful update_entity_attributes or +// create_relationship call. The D.1 gate in tasks.go combines these: refuse +// success when hadDiscovery && !hadEntityWriteback. +func TestHadDiscoveryAndWriteback(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + + sess, err := s.createSession(ctx, "discovery test") + if err != nil { + t.Fatalf("createSession: %v", err) + } + + // Before any tool calls: no discovery, no writeback. + if s.hadDiscovery(ctx, sess.ID) { + t.Fatal("hadDiscovery = true before any tool calls, want false") + } + if s.hadEntityWriteback(ctx, sess.ID) { + t.Fatal("hadEntityWriteback = true before any tool calls, want false") + } + + // A `run` call (discovery) — should set hadDiscovery, not hadEntityWriteback. + agentID := uuid.New() + s.logActivity(ctx, agentID, sess.ID, "run", nil, "", "uptime output", 100, true, "corr-1") + if !s.hadDiscovery(ctx, sess.ID) { + t.Fatal("hadDiscovery = false after a successful run call, want true") + } + if s.hadEntityWriteback(ctx, sess.ID) { + t.Fatal("hadEntityWriteback = true after only a run call, want false") + } + + // A failed run call should NOT count as discovery (no facts learned). + sess2, err := s.createSession(ctx, "failed discovery test") + if err != nil { + t.Fatalf("createSession: %v", err) + } + s.logActivity(ctx, agentID, sess2.ID, "run", nil, "", "ssh timeout", 100, false, "corr-2") + if s.hadDiscovery(ctx, sess2.ID) { + t.Fatal("hadDiscovery = true after a failed run call, want false (no facts learned)") + } + + // A get_entity call should NOT count as discovery (DB lookup, not live state). + sess3, err := s.createSession(ctx, "lookup test") + if err != nil { + t.Fatalf("createSession: %v", err) + } + s.logActivity(ctx, agentID, sess3.ID, "get_entity", nil, "", "entity row", 10, true, "corr-3") + if s.hadDiscovery(ctx, sess3.ID) { + t.Fatal("hadDiscovery = true after get_entity, want false (DB lookups are not discovery)") + } + + // update_entity_attributes sets hadEntityWriteback. + sess4, err := s.createSession(ctx, "writeback test") + if err != nil { + t.Fatalf("createSession: %v", err) + } + s.logActivity(ctx, agentID, sess4.ID, "update_entity_attributes", nil, "", "ok", 10, true, "corr-4") + if !s.hadEntityWriteback(ctx, sess4.ID) { + t.Fatal("hadEntityWriteback = false after update_entity_attributes, want true") + } + // And the discovery+writeback combination (the conv3 scenario). + s.logActivity(ctx, agentID, sess4.ID, "run", nil, "", "apt-get update output", 100, true, "corr-5") + if !s.hadDiscovery(ctx, sess4.ID) { + t.Fatal("hadDiscovery = false after run+writeback, want true") + } + if !s.hadEntityWriteback(ctx, sess4.ID) { + t.Fatal("hadEntityWriteback = false after run+writeback, want true") + } +} diff --git a/cmd/nomos/tasks.go b/cmd/nomos/tasks.go index ffce2b2..310bab3 100644 --- a/cmd/nomos/tasks.go +++ b/cmd/nomos/tasks.go @@ -37,14 +37,16 @@ func taskToolDefs() []toolDef { Name: "propose_plan", Description: "Propose the full ordered plan for this task. Call ONCE, before any " + "execution, with EVERY step end-to-end (not one step at a time). FIRST step: " + - "research (prior knowledge, relations, blast radius). LAST step: write back " + - "(update_entity_attributes + create_relationship + upsert_knowledge) BEFORE " + - "complete_task — this keeps the knowledge graph from drifting. After this " + - "call: STOP and wait for operator approval (approval vocabulary: approved, " + - "yes, go, proceed, continue, ok, go ahead). Once a step has started " + - "(running/done/...), this tool REFUSES further calls — advance with " + - "update_plan_step + run instead. Re-propose only if the operator explicitly " + - "asks you to revise the whole plan.", + "research (prior knowledge, relations, blast radius). If your plan runs `run` " + + "against any target, include a LAST step: write back " + + "(update_entity_attributes + create_relationship + upsert_knowledge) — if you " + + "omit it, one is auto-appended. After this call: STOP and wait for operator " + + "approval (approval vocabulary: approved, yes, go, proceed, continue, ok, " + + "go ahead). Once a step has started (running/done/...), this tool REFUSES " + + "further calls — advance with update_plan_step + run instead. Re-propose only " + + "if the operator explicitly asks you to revise the whole plan. complete_task " + + "with outcome=success is REFUSED if you ran `run` but didn't call " + + "update_entity_attributes/create_relationship — write back before completing.", InputSchema: map[string]any{ "type": "object", "properties": map[string]any{ @@ -200,6 +202,32 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args if len(steps) == 0 { return "error: propose_plan needs at least one step with a title", true } + // D.2: auto-append a writeback step if the agent didn't include one. + // The agent consistently writes vague last steps ("record findings") + // and then skips update_entity_attributes entirely (the #1 cause of + // knowledge-graph drift). Appending an explicit writeback step makes + // the seq-order enforcement (5.6) require it to be completed last, + // and D.1's complete_task gate enforces the actual calls. Together + // they close the loop structurally — neither relies on the agent + // reading SOUL.md. + hasWritebackStep := false + for _, st := range steps { + if strings.Contains(st.Title, "update_entity_attributes") || + strings.Contains(st.Title, "create_relationship") || + strings.Contains(st.Detail, "update_entity_attributes") || + strings.Contains(st.Detail, "create_relationship") { + hasWritebackStep = true + break + } + } + appendedNote := "" + if !hasWritebackStep { + steps = append(steps, planStepInput{ + Title: "Write back: update_entity_attributes + create_relationship + upsert_knowledge", + Detail: "Call update_entity_attributes for every entity you ran against (versions, states, counts, timestamps). Call create_relationship for any edge you discovered. Then upsert_knowledge about the affected entities (pass `about` as an array).", + }) + appendedNote = fmt.Sprintf(" (appended a writeback step — your plan didn't include one; step %d)", len(steps)) + } persisted, err := a.store.proposePlan(ctx, sessionID, steps) if err != nil { if errors.Is(err, errPlanInFlight) { @@ -213,15 +241,11 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args } return fmt.Sprintf("error proposing plan: %v", err), true } - // Nudge: if the last step doesn't mention entity writeback tools, - // the graph will keep drifting — discovered facts won't be persisted. - lastStep := steps[len(steps)-1] - hasWriteback := strings.Contains(lastStep.Title+lastStep.Detail, "update_entity_attributes") || - strings.Contains(lastStep.Title+lastStep.Detail, "create_relationship") - result := fmt.Sprintf("Plan set (%d steps). STOP. Wait for operator approval — do not call run yet. Approval vocabulary: \"approved\", \"yes\", \"go\", \"proceed\", \"continue\", \"ok\", \"go ahead\". On approval, advance with update_plan_step + run. Do not call propose_plan again.", len(persisted)) - if !hasWriteback { - result += "\n\n⚠️ The final step doesn't mention update_entity_attributes or create_relationship. Without those, any facts you discovered about entities (IPs, versions, hosts, states) will be LOST — the next session starts from scratch. Consider revising the last step to include entity writeback BEFORE completing the task." - } + // The writeback step is now always present (D.2 auto-appends it if + // the agent forgot), so the old advisory nudge is replaced by the + // structural gate: D.1 refuses complete_task without the actual + // update_entity_attributes/create_relationship calls. + result := fmt.Sprintf("Plan set (%d steps)%s. STOP. Wait for operator approval — do not call run yet. Approval vocabulary: \"approved\", \"yes\", \"go\", \"proceed\", \"continue\", \"ok\", \"go ahead\". On approval, advance with update_plan_step + run. Do not call propose_plan again.", len(persisted), appendedNote) return result, true case "update_plan_step": @@ -278,6 +302,17 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args "session", sessionID, "outcome", outcome) outcome = "partial" } + // D.1: refuse success when discovery ran but no writeback followed. + // The prior advisory warning (below) was ignorable — the agent + // saw it and ended the task anyway. This gate fires BEFORE + // completeTask runs, so the session stays in 'executing' state + // and the agent must call update_entity_attributes/create_relationship + // then retry complete_task. Only blocks `success`; an explicit + // `failure` or `partial` is allowed through (the agent is + // acknowledging it didn't finish — no reason to force writeback). + if outcome == "success" && a.store.hadDiscovery(ctx, sessionID) && !a.store.hadEntityWriteback(ctx, sessionID) { + return "Refused: this session ran `run` against live targets (discovery) but did not call update_entity_attributes or create_relationship to persist what you learned. The knowledge graph will drift if you complete without writeback. Call update_entity_attributes for each entity you ran against (versions, states, counts, timestamps), and create_relationship for any edge you discovered, then call complete_task again. Outcome is held at 'executing' until you do.", true + } if err := a.store.completeTask(ctx, sessionID, outcome, summary); err != nil { return fmt.Sprintf("error completing task: %v", err), true }