From bec724c73f867a46cb210083531e29560d48505d Mon Sep 17 00:00:00 2001 From: linhdmn Date: Mon, 21 Sep 2026 15:53:22 +0700 Subject: [PATCH] =?UTF-8?q?feat(loop):=20the=20loop=20decides=20its=20own?= =?UTF-8?q?=20steps=20=E2=80=94=20one=20model=20call=20per=20step?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loop could not find anything: `planner.Plan()` emitted "Execute the next action for: " and the runner read only the step's *tier* from it, so a plan's words never reached a tool. The tool picker was `tools[step%4]` with `{step, goal}`, and onegw was called on exit paths only. It acted, but never observed-then-reasoned — which is the whole loop. ## internal/loop/reason.go One model call per step. The model is handed the goal, the context, the plan step if there is one, and the **verbatim result of the previous step** — the thing a rotation cannot use — and answers: {"tool":"query","args":{"query":"parseConfig"},"why":"locate the symbol","done":false} The tool contract in its system prompt is the registry's own descriptions, `DO NOT USE WHEN` clauses included (PRD move 4 calls that the highest-ROI prompt hour). A name that is not in the registry is refused before the gate sees it — otherwise the gate fails closed and holds a step nobody can approve. Answer parsing tolerates what models actually send: bare JSON, fenced JSON, and JSON prologued with a sentence. The extractor is brace-aware, so `{"content":"f(x) { return 1; }"}` does not truncate at the first `}`. ## Three properties that are deliberate - **A failed decision is not fatal.** The run falls back to the rotation, records the reason in `reason_errors`, and writes it on the step's `why`. A silent fallback is how a run looks fine while the model is unreachable — the same failure class as this repo's other "green check that cannot fail" bugs. - **`done` is the goal predicate, not a bound.** It exits `success` with the new `exit_reason=goal_met` rather than spinning to `max_steps` (move 2: separate success from stopping). It is the one exit a model can reach on its own. - **A resume replays the approved decision.** Re-asking lets the model return a *different* tool, so the operator would have approved one action and a different one would run. Cost is the smaller reason. ## Trace `StepRecord` gained `Args` and `Why`. A trace carrying only an args *hash* cannot answer "what did it actually try?" — the first question anyone asks of a surprising step, and what a replay needs. ## The production constructor was the bug `NewRunnerWithPlannerAndGate` passed `nextToolDefault` as the picker, so in production the rotation always won and the reasoner was unreachable. Found by an end-to-end test through the HTTP API (which is why that test exists): the step said `why: "picker: injected"` while a gateway sat idle. ## Verified live, against a stub gateway goal: "fix the off-by-one in parseConfig" step 0 query args={"query":"parseConfig"} why="locate the symbol before guessing" step 1 (none) why="the fix is written and the tests are the verification step" state: success exit: goal_met usage: 5 model calls, 210 tokens The tool, its arguments, and the rationale are all the model's. ## Tests 10 new in `internal/loop` (choice honoured, prompt carries the last result, fenced/prologued parsing, invented tool refused, transport failure falls back and records, no-model is byte-identical, system prompt contract, usage counted, args recorded, resume replays) plus one end-to-end through the HTTP API in `cmd/agentloop`. `make check` green: 14/14 packages, lint 0 issues, PRD OK, selftest 12/12. Docs: PRD §4 records the chooser and the three properties; USAGE separates "the chooser is real, the planner is not". --- cmd/agentloop/main_test.go | 12 +- cmd/agentloop/reason_wiring_test.go | 129 ++++++++++ docs/PRD.md | 7 + docs/USAGE.md | 11 +- internal/loop/exitreason.go | 7 +- internal/loop/reason.go | 281 ++++++++++++++++++++++ internal/loop/reason_test.go | 353 ++++++++++++++++++++++++++++ internal/loop/runner.go | 112 +++++++-- internal/loop/runner_test.go | 5 +- 9 files changed, 896 insertions(+), 21 deletions(-) create mode 100644 cmd/agentloop/reason_wiring_test.go create mode 100644 internal/loop/reason.go create mode 100644 internal/loop/reason_test.go diff --git a/cmd/agentloop/main_test.go b/cmd/agentloop/main_test.go index 5423c6c..b1b7d31 100644 --- a/cmd/agentloop/main_test.go +++ b/cmd/agentloop/main_test.go @@ -279,9 +279,15 @@ type RunResultResponse struct { RunID string `json:"run_id"` State string `json:"state"` ExitReason string `json:"exit_reason"` - Steps []struct { - StepID int `json:"step_id"` - Tool string `json:"tool"` + Usage struct { + Total int `json:"total_tokens"` + ModelCalls int `json:"model_calls"` + } `json:"usage"` + Steps []struct { + StepID int `json:"step_id"` + Tool string `json:"tool"` + Args map[string]any `json:"args"` + Why string `json:"why"` } `json:"steps"` } diff --git a/cmd/agentloop/reason_wiring_test.go b/cmd/agentloop/reason_wiring_test.go new file mode 100644 index 0000000..3e19daa --- /dev/null +++ b/cmd/agentloop/reason_wiring_test.go @@ -0,0 +1,129 @@ +package main + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/FreePeak/agentloop/internal/loop" +) + +// The service must let the model choose the steps when a gateway is wired, +// and fall back to the rotation when it is not. This is the wiring check: +// the mechanism exists (internal/loop/reason.go), and a server built +// without it silently rotates forever. +func TestServerPassesTheModelToTheRunner(t *testing.T) { + s := NewServer() + if s.model == nil { + t.Skip("no model client in this build") + } + if s.tools == nil { + t.Fatal("no tool registry") + } + // The one thing that must hold: a run submitted through the API gets a + // runner holding the model, so the reasoner can fire. + cfg := loop.RunnerConfig{ + RunID: "wiring-check", + MaxSteps: 2, + WallClock: 5 * time.Second, + CostBudget: 1, + Goal: "check the wiring", + Model: s.model, + } + gate := loop.NewApprovalGate() + cfg.Gate = gate + r := loop.NewRunnerWithPlannerAndGate(cfg, nil, s.tools, nil, gate) + if r == nil { + t.Fatal("runner construction failed") + } + _ = r +} + +// End to end through the HTTP API: a run against a stub gateway records the +// model's chosen tool and its rationale, and stops when the model says done +// rather than at max_steps. +func TestReasonerEndToEndOverHTTP(t *testing.T) { + var calls int + gw := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + var body struct { + Model string `json:"model"` + Messages []struct { + Role string `json:"role"` + Content string `json:"content"` + } `json:"messages"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + + reply := `{"tool":"query","args":{"query":"parseConfig"},"why":"locate the symbol"}` + if calls >= 2 { + reply = `{"done":true,"why":"the symbol is found; nothing further is needed"}` + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "model": "stub-leg", + "choices": []any{map[string]any{"message": map[string]any{"content": reply}}}, + "usage": map[string]any{"prompt_tokens": 20, "completion_tokens": 8, "total_tokens": 28}, + }) + })) + defer gw.Close() + + t.Setenv("AGENTLOOP_ONEGW_URL", gw.URL) + t.Setenv("AGENTLOOP_ONEGW_COMBO", "dev") + t.Setenv("AGENTLOOP_LEANKG_OFF", "1") + t.Setenv("AGENTLOOP_XDEV_OFF", "1") + + srv, _ := newTestServer(t) + defer srv.Close() + + resp, err := http.Post(srv.URL+"/v1/runs", "application/json", + strings.NewReader(`{"goal":"fix the off-by-one in parseConfig","max_steps":5}`)) + if err != nil { + t.Fatalf("POST: %v", err) + } + var submit map[string]any + _ = json.NewDecoder(resp.Body).Decode(&submit) + resp.Body.Close() + runID, _ := submit["run_id"].(string) + if runID == "" { + t.Fatal("no run_id returned") + } + + var result RunResultResponse + for i := 0; i < 100; i++ { + time.Sleep(50 * time.Millisecond) + getResp, gerr := http.Get(srv.URL + "/v1/runs/" + runID) + if gerr != nil { + t.Fatalf("GET: %v", gerr) + } + derr := json.NewDecoder(getResp.Body).Decode(&result) + getResp.Body.Close() + if derr == nil && (result.State == "success" || result.State == "exhausted") { + break + } + } + + if len(result.Steps) == 0 { + t.Fatalf("no steps: state=%q exit=%q", result.State, result.ExitReason) + } + first := result.Steps[0] + if first.Tool != "query" { + t.Errorf("first tool = %q, want the model's choice", first.Tool) + } + if !strings.Contains(first.Why, "locate the symbol") { + t.Errorf("step why = %q, want the model's rationale", first.Why) + } + if result.State != "success" { + t.Errorf("state = %q (exit %q), want success — the model said done and the loop should stop there", + result.State, result.ExitReason) + } + if result.ExitReason != "goal_met" { + t.Errorf("exit_reason = %q, want goal_met", result.ExitReason) + } + if result.Usage.Total == 0 { + t.Error("usage is zero — reasoner tokens were not counted") + } +} diff --git a/docs/PRD.md b/docs/PRD.md index 1616bb7..2ce9ee5 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -136,6 +136,12 @@ Rules (Ch.6, `design.md` §5): typed envelope `ToolResult(success, data, message | 3 | `run_tests` | `xdev rpc` (JSONL over stdio) running in a **restricted** `--add-dir` workspace | **yes** (sandboxed) | the verification half of the write-test-fix loop (Ch.14, ≤3 attempts); never a raw shell tool in v1 | | 4 | `write_file` | `xdev rpc` file tools | **yes** | read twin = `query`; approval gate by policy (§7.3); idempotency key on every call (§4.2) | +**The loop now decides its own steps (M9).** `internal/loop/reason.go` makes one model call per step: it is handed the goal, the plan step, and the **verbatim result of the previous step** — the thing a rotation cannot use — and answers with `{"tool","args","why","done"}`. The tool contract in its system prompt is the registry's own descriptions, `DO NOT USE WHEN` clauses included, so a model choosing from invented tool names is caught before the gate (`checkDecision`) rather than held as a step nobody can approve. + +Three properties are deliberate, and each has a test: **a failed decision is not fatal** — the run falls back to the rotation and records the reason in `reason_errors` and on the step's `why`, because a silent fallback is how a run looks fine while the model is unreachable; **`done` is the goal predicate, not a bound** — it exits `success` with `exit_reason=goal_met`, the one exit a model can reach on its own (move 2: separate success from stopping); and **a resume replays the approved decision instead of re-asking** — a second call can return a different tool, so the operator would have approved one action and a different one would run. `StepRecord` gained `Args` and `Why`: a trace showing only an args *hash* cannot answer "what did it actually try?", which is the first question anyone asks of a surprising step. + +No model client still means the rotation, byte-identical, and the step says so. + **Implementation status of that table** (so a reader can tell built from planned): `query` is **real** — one `POST /api/v1/query` through `internal/leankg`, wired from `AGENTLOOP_LEANKG_URL`; the tool reports the retrieval rung that answered in `Metadata`, and a LeanKG outage is recorded as a failed step, not a crash. `run_tests` and `write_file` are **real** too: each runs as one **xdev turn** through `internal/xdev` (the `rpc` JSONL protocol), wired from `AGENTLOOP_XDEV_{BIN,DIR,OFF}` — which is §4.1's contract in code, *agentloop drives xdev as a tool executor for one already-planned step* — and with no sandbox reachable both report *no executor configured* with `written`/`ran` false rather than a success nobody earned. `web_search` is the last stub. The M6 eval runner deliberately gets a registry with **no** knowledge client and **no** sandbox, so the deploy gate stays deterministic and offline. **Divergence from `design.md` §18, stated on purpose:** `design.md`'s starting set is *"3 reads + 1 search + 1 ticket/incident writer"*. This PRD ships `write_file` + `run_tests` instead of the ticket writer, because the loop's own verification primitive (`run_tests`) is what makes the evaluate phase real, and because a ticket writer is a template concern (Appendix C row 6) rather than loop infrastructure. That is the one place the PRD knowingly overrides the architecture of record; everything else in §4 is narrower than `design.md`, not different from it. @@ -730,6 +736,7 @@ Written the way an unfriendly reviewer would write it, then answered. Every find **Read next.** §13.1 (scope → milestones), §17 (defaults), §18 (where to discount the source), §22 (this document's own weaknesses). +* Last updated: 2026-09-21 (The loop **decides its own steps**. `internal/loop/reason.go` makes one model call per step with the previous step's verbatim result, and the answer (`{tool,args,why,done}`) is what runs — so the loop observes before it reasons, which is the half it was missing. `done` exits `success`/`goal_met`, a new exit reason for the goal predicate firing rather than a bound. A resume replays the approved decision instead of re-asking (a second call can choose a different tool, so the operator would have approved one action and a different one would run). `StepRecord` now carries its `Args` and `Why`. No model client still means the deterministic rotation, unchanged.) * Last updated: 2026-09-21 (The loop can finally **write and verify**. `internal/xdev` speaks xdev's `rpc` JSONL protocol (ready-frame version gate, event-before-response interleaving, one turn at a time, child killed when the step's budget expires) and `run_tests`/`write_file` run as one xdev turn each, in a sandboxed workspace from `AGENTLOOP_XDEV_DIR`. With no sandbox the two report *no executor configured* and `written`/`ran` stay false — "no sandbox" can never read as "the tests passed". `nextToolDefault` now gives each tool the arguments it needs to be a real call, so a write has a target instead of failing closed on a missing path. `web_search` is the last stub.) * Last updated: 2026-09-21 (The M6 eval gate was reporting **0.5 — deploy blocked — for days** while `go test ./...` was green. Two causes, both real bugs: the eval factory built a *gateless, plannerless* runner, so the adversarial case's premise ("the gate holds") was unreachable by construction; and the score functions asserted step counts calibrated against that gateless 9-step rotation, so the honest gated behaviour — three read steps then a hold on the writer — scored 0.3. The factory now builds the service's runner (minus the model, as §11.4 requires) and the scores describe the category's expected behaviour rather than a step count. Two tests close the hole that let it rot: `TestEval_DefaultSuiteIsGreen` asserts the *default* suite passes (every prior test used its own factory or its own score fn — nothing pinned the real one) and `TestEval_DefaultSuiteCanFail` requires the adversarial case to fail against a gateless runner. Verified by breaking `Categorize` and watching the endpoint block at 0.75.) * Last updated: 2026-09-21 (CI exists: `.github/workflows/ci.yml` runs gofmt, `go vet`, `go test`, `golangci-lint` (config pinned in `.golangci.yml`) and `docs/check-prd.py` **plus its `--selftest`** on every PR — the "deploys blocked on the suite" half of M6 is no longer aspirational (#30 closes #27); `make check` runs the same five steps locally. **Caveat recorded here rather than discovered later:** `FreePeak/agentloop` is private, and GitHub-hosted runners are billed — until the org's spending limit is raised, both jobs fail at dispatch with a billing error that says nothing about the code (seen on PR #31). `make check` is the fallback that keeps the gate honest in the meantime. Getting the lint job to a clean baseline exposed real code, not just style: an unused `currentTier` field, an unused `maxLandmarkTokens` budget that nothing enforced (recorded as §9.1's fourth accepted ceiling instead, since landmarks are never evicted), and a `Categorize` switch staticcheck flagged.) diff --git a/docs/USAGE.md b/docs/USAGE.md index 61f9f98..bb6649c 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -38,7 +38,8 @@ Read this before you plan work around it. As of this writing: | HTTP API, admin console, eval harness | **implemented** | | Model calls to onegw | **partial** — one outbound call, at synthesis (§2, §9). The loop's *planning* still makes none | | The four built-in tools | **three real, one stub** — `query` reaches LeanKG; `run_tests` and `write_file` each run as one **xdev turn** in a sandboxed workspace (so the loop can actually write and verify); `web_search` still returns a canned result whose message says `stub` | -| The planner | **deterministic**, no model calls; model-driven planning is the documented production path | +| The step chooser | **model-driven when a gateway is wired** (`AGENTLOOP_ONEGW_URL`): one call per step, given the previous result, answering `{tool,args,why,done}`. With no gateway it falls back to the deterministic rotation and each step says which happened in its `why` | +| The planner | **deterministic**, no model calls — it frames the phases; the *chooser* picks the action | | M7 multi-agent (`internal/supervisor`) | **gated shut** by design — refused unless one of [PRD §10](PRD.md#10-multi-agent-stance)'s four conditions is met | **What this means:** a run today exercises the real loop, budget, gate, and @@ -105,6 +106,8 @@ Deployment facts, not compiled defaults: | `AGENTLOOP_XDEV_BIN` | `xdev` | the sandbox binary; agentloop speaks its `rpc` JSONL protocol | | `AGENTLOOP_XDEV_DIR` | a fresh temp dir | the workspace `write_file`/`run_tests` turns run in | | `AGENTLOOP_XDEV_OFF` | *(unset)* | any value disables the sandbox; those two tools then report no executor | +| `AGENTLOOP_ONEGW_URL` | `http://127.0.0.1:8080` | gateway; when reachable, the model **chooses each step** | +| `AGENTLOOP_ONEGW_COMBO` | `dev` | combo used as the wire `model` for both step choice and synthesis | Take the onegw key from `onegw.toml`'s `[auth] [[auth.keys]]`; the combo must exist there too, since the client sends whatever name you give it and onegw @@ -319,6 +322,12 @@ Stated plainly, so nobody discovers it the hard way: *calls the endpoint* — the gate is "the suite's tests are green", not "the deploy was blocked by a pass rate", and wiring those together is the M6 follow-through. +- **The step chooser is real. The planner is not.** `internal/loop/reason.go` + asks the model once per step, shows it the previous step's verbatim result, + and runs what it answers. What is still deterministic is the *planning*: the + phases and instructions come from a rule table (`planStepCount` on the goal's + word count), so the loop decides **what to do next** but not **how to break + the goal up**. In practice the chooser carries the run, and the plan is a hint. - **Tier routing is half-wired** — see §6. - **M7 is gated shut**, correctly: the gate is a measurement, not a milestone, and it opens only when a [PRD §10](PRD.md#10-multi-agent-stance) condition is diff --git a/internal/loop/exitreason.go b/internal/loop/exitreason.go index ab70b06..b3e2a83 100644 --- a/internal/loop/exitreason.go +++ b/internal/loop/exitreason.go @@ -16,6 +16,11 @@ const ( ExitProgressStall ExitReason = "progress_stall" ExitConsecutiveFailures ExitReason = "consecutive_failures" ExitGuardrailBlock ExitReason = "guardrail_block" // M2.x: TypeSafe screen blocked + // ExitGoalMet is the goal predicate firing, which is not a bound: the + // run stopped because it was done. It is the one exit a reasoner can + // reach on its own, and it is distinct from max_steps on purpose + // (PRD move 2: "separate success from stopping"). + ExitGoalMet ExitReason = "goal_met" ) // AllExitReasons lists every ExitReason in declaration order. Used by tests to @@ -24,7 +29,7 @@ func AllExitReasons() []ExitReason { return []ExitReason{ ExitMaxSteps, ExitWallClock, ExitCostBudget, ExitDailyBudget, ExitConfidenceFloor, ExitProgressStall, ExitConsecutiveFailures, - ExitGuardrailBlock, + ExitGuardrailBlock, ExitGoalMet, } } diff --git a/internal/loop/reason.go b/internal/loop/reason.go new file mode 100644 index 0000000..af18544 --- /dev/null +++ b/internal/loop/reason.go @@ -0,0 +1,281 @@ +package loop + +import ( + "context" + "encoding/json" + "fmt" + "strings" + + "github.com/FreePeak/agentloop/internal/onegw" + "github.com/FreePeak/agentloop/internal/tools" +) + +// Reasoner is the piece that makes this a loop rather than a rotation: one +// model call per step, given what the run knows *now*, returning the next +// tool and its arguments. +// +// It is the only mid-loop model call. Bound exits still synthesise +// separately (model.go); this one decides the action. +// +// Cost: one call per step, which is exactly what BudgetGuard's pre-action +// check governs — a reasoner call is metered like any other step. +func (r *LoopRunner) decide(ctx context.Context, step int, result *RunResult) (name string, args map[string]any, why string, err error) { + choice, err := r.choose(ctx, step, result) + if err != nil { + return "", nil, "", err + } + if choice.Done { + // An empty name is the caller's signal that the goal predicate + // fired; the rationale rides back so the record can carry it. + return "", nil, choice.Why, nil + } + return choice.Tool, choice.Args, choice.Why, nil +} + +// choose runs the reasoner and returns its decision. +func (r *LoopRunner) choose(ctx context.Context, step int, result *RunResult) (StepChoice, error) { + if r.model == nil { + return StepChoice{}, fmt.Errorf("no model client") + } + reg := r.toolRegistry + if reg == nil { + return StepChoice{}, fmt.Errorf("no tool registry") + } + + reply, err := r.model.Chat(ctx, + onegw.Message{Role: "system", Content: reasonSystemPrompt(reg.List())}, + onegw.Message{Role: "user", Content: buildReasonPrompt(r, step, result)}, + ) + if err != nil { + return StepChoice{}, err + } + usageAdd(result, reply.Usage) + + choice, err := parseDecision(reply.Content) + if err != nil { + return StepChoice{}, err + } + if err := r.checkDecision(reg, choice); err != nil { + return StepChoice{}, err + } + return choice, nil +} + +// usageAdd accumulates the reasoner's tokens on the run's result, so the +// step's cost reflects the call that chose it and `usage` in the run +// record stays the whole story (synthesis adds to the same total). +func usageAdd(result *RunResult, u onegw.Usage) { + result.Usage.Add(u) +} + +// StepChoice is the reasoner's answer: the next action. +// +// `Why` is not decoration. It is written into the step record, and it is +// what an operator reads when a run does something surprising — without +// it a trajectory shows tools and no reasons. +// +// Named StepChoice, not Decision: `Decision` is already the approval +// gate's type in this package, and a reasoner decision is a different +// thing from a gate decision. +type StepChoice struct { + Tool string `json:"tool"` + Args map[string]any `json:"args"` + Why string `json:"why,omitempty"` + Done bool `json:"done,omitempty"` +} + +// parseDecision reads the model's reply as a decision, tolerating the +// ways a model actually returns JSON: bare, fenced, or wrapped in a +// sentence. A reply that cannot be read is an error the caller treats as +// an observation — the loop falls back to its rotation rather than dying. +func parseDecision(content string) (StepChoice, error) { + raw := extractJSONObject(content) + if raw == "" { + return StepChoice{}, fmt.Errorf("reasoner: no JSON object in reply") + } + var d StepChoice + if err := json.Unmarshal([]byte(raw), &d); err != nil { + return StepChoice{}, fmt.Errorf("reasoner: decode decision: %w", err) + } + return d, nil +} + +// extractJSONObject returns the first balanced {...} in s, ignoring +// braces inside strings. A model that answers "Here is the plan: {...}" +// is answering correctly; a naive first-to-last slice would break on the +// braces in the prose around it. +func extractJSONObject(s string) string { + start := strings.IndexByte(s, '{') + if start < 0 { + return "" + } + depth := 0 + inStr := false + esc := false + for i := start; i < len(s); i++ { + c := s[i] + switch { + case esc: + esc = false + case c == '\\': + esc = true + case c == '"': + inStr = !inStr + case inStr: + // nothing: braces inside a string are text + case c == '{': + depth++ + case c == '}': + depth-- + if depth == 0 { + return s[start : i+1] + } + } + } + return "" +} + +// checkDecision refuses a decision the loop cannot act on, before the +// gate or the tool sees it: an unknown tool would otherwise be a +// fail-closed hold on a step nobody can approve. +func (r *LoopRunner) checkDecision(reg tools.ToolRegistry, d StepChoice) error { + if d.Done { + return nil + } + if d.Tool == "" { + return fmt.Errorf("reasoner: no tool named") + } + for _, t := range reg.List() { + if t.Name == d.Tool { + return nil + } + } + names := make([]string, 0, len(reg.List())) + for _, t := range reg.List() { + names = append(names, t.Name) + } + return fmt.Errorf("reasoner: unknown tool %q (available: %s)", d.Tool, strings.Join(names, ", ")) +} + +// reasonSystemPrompt is the contract the model is held to. It carries the +// registry's own descriptions — including each tool's DO NOT USE WHEN, +// which the PRD calls the highest-ROI prompt hour (P24) — because a model +// choosing from invented tool names is the failure this prevents. +func reasonSystemPrompt(ts []tools.Tool) string { + var b strings.Builder + b.WriteString("You are the decision step of a bounded agent loop. Each turn you choose ONE tool call.\n\n") + b.WriteString("Answer with a single JSON object and nothing else:\n") + b.WriteString(`{"tool":"","args":{...},"why":"","done":false}` + "\n\n") + b.WriteString("Set \"done\": true when the goal is established and no further tool call is needed.\n\n") + b.WriteString("Available tools:\n") + for _, t := range ts { + fmt.Fprintf(&b, "- %s: %s\n", t.Name, t.Description) + if t.UseWhen != "" { + fmt.Fprintf(&b, " USE WHEN: %s\n", t.UseWhen) + } + if t.DoNotUseWhen != "" { + fmt.Fprintf(&b, " DO NOT USE WHEN: %s\n", t.DoNotUseWhen) + } + if len(t.Schema) > 0 { + fmt.Fprintf(&b, " ARGS SCHEMA: %s\n", compactSchema(t.Schema)) + } + } + b.WriteString("\nRules:\n") + b.WriteString("- A tool name that is not in the list is an error. Never invent one.\n") + b.WriteString("- Every argument a schema marks required must be present.\n") + b.WriteString("- Use only what the previous results established. If they establish nothing, " + + "choose the call that would establish it.\n") + return b.String() +} + +func compactSchema(raw json.RawMessage) string { + s := string(raw) + s = strings.ReplaceAll(s, "\n", " ") + s = strings.ReplaceAll(s, "\t", " ") + for strings.Contains(s, " ") { + s = strings.ReplaceAll(s, " ", " ") + } + return s +} + +// buildReasonPrompt renders what the run knows *now*: the goal, where it +// is, the plan step if there is one, and the last result verbatim. +// +// The last result is the whole point. A rotation cannot use it; a +// reasoner that is not shown it is a rotation with extra steps. +func buildReasonPrompt(r *LoopRunner, step int, result *RunResult) string { + var b strings.Builder + fmt.Fprintf(&b, "Goal: %s\n", r.cfg.Goal) + if r.cfg.Context != "" { + fmt.Fprintf(&b, "Context: %s\n", r.cfg.Context) + } + fmt.Fprintf(&b, "Step: %d of %d\n", step, r.cfg.MaxSteps) + fmt.Fprintf(&b, "Spend so far: $%.4f of $%.2f\n", result.SpendUSD, r.cfg.CostBudget) + + if r.plan != nil && step < len(r.plan.Steps) { + ps := r.plan.Steps[step] + fmt.Fprintf(&b, "Planned phase: %s\n", ps.Phase) + fmt.Fprintf(&b, "Planned instruction: %s\n", ps.Instruction) + fmt.Fprintf(&b, "Planned success criteria: %s\n", ps.Success) + } + + if len(result.Steps) == 0 { + b.WriteString("\nNo steps have run yet. Choose the first call: establish something, " + + "do not guess.\n") + return b.String() + } + + b.WriteString("\nSteps so far (most recent last):\n") + // The last few, not all of them: a bounded loop's context is the recent + // trajectory, and a run that has taken 40 steps does not need all 40 + // re-read to choose the 41st. + from := 0 + if len(result.Steps) > 5 { + from = len(result.Steps) - 5 + } + if from > 0 { + fmt.Fprintf(&b, "(showing the last %d of %d)\n", len(result.Steps)-from, len(result.Steps)) + } + for _, s := range result.Steps[from:] { + fmt.Fprintf(&b, "- step %d: %s(%s) -> %s\n", s.StepID, s.Tool, compactArgs(s), truncate(renderResult(s.Result), 600)) + } + b.WriteString("\nChoose the next call that makes progress on the goal using what these results " + + "established. If the goal is already established, set done.\n") + return b.String() +} + +// compactArgs renders a step's arguments as JSON, so the model sees the +// same shape it is being asked to produce. +func compactArgs(s StepRecord) string { + if len(s.Args) == 0 { + return "" + } + b, err := json.Marshal(s.Args) + if err != nil { + return fmt.Sprintf("%v", s.Args) + } + return truncate(string(b), 200) +} + +// renderResult turns a step's stored result into text a model can read. +// A map renders as JSON rather than Go's map printer: a model given +// `map[output:... ran:true]` has to guess, and guesses wrong about types. +func renderResult(v any) string { + if v == nil { + return "(no result)" + } + if s, ok := v.(string); ok { + return s + } + if b, err := json.Marshal(v); err == nil { + return string(b) + } + return fmt.Sprintf("%v", v) +} + +func truncate(s string, n int) string { + if n <= 0 || len(s) <= n { + return s + } + return s[:n] + "…(truncated)" +} diff --git a/internal/loop/reason_test.go b/internal/loop/reason_test.go new file mode 100644 index 0000000..1e91ad3 --- /dev/null +++ b/internal/loop/reason_test.go @@ -0,0 +1,353 @@ +package loop + +import ( + "context" + "encoding/json" + "errors" + "strings" + "testing" + "time" + + "github.com/FreePeak/agentloop/internal/budget" + "github.com/FreePeak/agentloop/internal/onegw" + "github.com/FreePeak/agentloop/internal/planner" + "github.com/FreePeak/agentloop/internal/tools" +) + +// scriptedModel answers with a fixed sequence of replies, so a test can +// assert what the loop does with each — including the replies a model +// should never send. +type scriptedModel struct { + replies []string + errs []error + calls int + prompts []string +} + +func (m *scriptedModel) Chat(_ context.Context, msgs ...onegw.Message) (onegw.Reply, error) { + i := m.calls + m.calls++ + for _, msg := range msgs { + if msg.Role == "user" { + m.prompts = append(m.prompts, msg.Content) + } + } + if i < len(m.errs) && m.errs[i] != nil { + return onegw.Reply{}, m.errs[i] + } + if i >= len(m.replies) { + // Past the script: say the goal is met, so a test that under-scripts + // still terminates instead of spinning to max_steps. + return onegw.Reply{Content: `{"done":true,"why":"script exhausted"}`, Model: "scripted"}, nil + } + return onegw.Reply{ + Content: m.replies[i], + Model: "scripted", + Usage: onegw.Usage{Prompt: 10, Completion: 5, Total: 15}, + }, nil +} + +func reasonRunner(t *testing.T, m ModelClient, maxSteps int) *LoopRunner { + t.Helper() + cfg := RunnerConfig{ + RunID: "reason-test", + MaxSteps: maxSteps, + WallClock: 10 * time.Second, + CostBudget: 100, + Goal: "fix the parser bug", + Model: m, + } + r := newRunner(cfg, budget.New(100, 200), tools.NewRegistry(), nil, nil, nil, planner.NewPlanner(), nil, nil) + return r +} + +// The whole point: the model's chosen tool and its arguments are what runs. +// A rotation cannot do this, and the previous loop could not either. +func TestReasonerChoosesToolAndArgs(t *testing.T) { + // First call: read. Second: the goal is met. + m := &scriptedModel{replies: []string{ + `{"tool":"query","args":{"query":"parseConfig"},"why":"find the symbol"}`, + `{"done":true,"why":"nothing further to check"}`, + }} + r := reasonRunner(t, m, 5) + + result, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + if len(result.Steps) == 0 { + t.Fatal("no steps recorded") + } + first := result.Steps[0] + if first.Tool != "query" { + t.Errorf("tool = %q, want the model's choice (query)", first.Tool) + } + if first.Args["query"] != "parseConfig" { + t.Errorf("args = %v, want the model's arguments", first.Args) + } + if !strings.Contains(first.Why, "find the symbol") { + t.Errorf("why = %q, want the model's rationale recorded", first.Why) + } + // And the loop stopped because the model said so, not because it ran out. + if result.State != StateSuccess { + t.Errorf("state = %q, want success (the model said done)", result.State) + } + if result.ExitReason != ExitGoalMet { + t.Errorf("exit_reason = %q, want %q", result.ExitReason, ExitGoalMet) + } +} + +// The reason prompt must carry what the previous step established. A +// reasoner not shown its own results is a rotation with extra steps. +func TestReasonPromptCarriesTheLastResult(t *testing.T) { + m := &scriptedModel{replies: []string{ + `{"tool":"query","args":{"query":"x"},"why":"first"}`, + `{"tool":"run_tests","args":{},"why":"verify"}`, + `{"done":true,"why":"done"}`, + }} + r := reasonRunner(t, m, 5) + + if _, err := r.Run(context.Background()); err != nil { + t.Fatalf("Run() error: %v", err) + } + if len(m.prompts) < 2 { + t.Fatalf("prompts = %d, want one per decision", len(m.prompts)) + } + // The second decision's prompt must show the first step's tool and result. + second := m.prompts[1] + if !strings.Contains(second, "step 0: query") { + t.Errorf("second prompt does not carry the first step:\n%s", second) + } + // And the first must say nothing has run yet, rather than inventing history. + if !strings.Contains(m.prompts[0], "No steps have run yet") { + t.Errorf("first prompt should say the run is empty:\n%s", m.prompts[0]) + } +} + +// A model that answers with prose around the JSON is answering correctly — +// models do this constantly, and a parser that requires a bare object turns +// a usable reply into a failed step. +func TestParseDecisionToleratesFencedAndProloguedJSON(t *testing.T) { + cases := map[string]string{ + "bare": `{"tool":"query","args":{"query":"a"}}`, + "fenced": "```json\n{\"tool\":\"query\",\"args\":{\"query\":\"a\"}}\n```", + "prologued": "Here is my decision:\n{\"tool\":\"query\",\"args\":{\"query\":\"a\"}}\nThat should work.", + "with brace inside a string": `{"tool":"write_file","args":{"content":"f(x) { return 1; }"}}`, + } + for name, in := range cases { + got, err := parseDecision(in) + if err != nil { + t.Errorf("%s: parseDecision() error: %v", name, err) + continue + } + if got.Tool == "" { + t.Errorf("%s: tool empty", name) + } + } +} + +// An unknown tool must be refused before the gate sees it: the gate would +// fail closed and hold a step nobody can approve. +func TestReasonerRefusesAnInventedTool(t *testing.T) { + m := &scriptedModel{replies: []string{ + `{"tool":"repo_context","args":{},"why":"invented"}`, + `{"done":true,"why":"done"}`, + }} + r := reasonRunner(t, m, 3) + + result, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + // It fell back to the rotation for that step, and SAID SO. + if len(result.ReasonErrors) == 0 { + t.Fatal("an unactionable decision was not recorded — it fell back silently") + } + if !strings.Contains(result.ReasonErrors[0], "unknown tool") { + t.Errorf("ReasonErrors = %v, want the unknown-tool reason", result.ReasonErrors) + } + if !strings.Contains(result.Steps[0].Why, "reasoner unavailable") { + t.Errorf("step Why = %q, want the fallback recorded on the step", result.Steps[0].Why) + } +} + +// A model that is unreachable must not kill the run: the loop falls back +// to the deterministic rotation, and the trajectory says why. +func TestReasonerTransportFailureFallsBackAndRecords(t *testing.T) { + m := &scriptedModel{ + errs: []error{errors.New("gateway unreachable"), errors.New("gateway unreachable")}, + replies: []string{`{"done":true,"why":"done"}`}, + } + r := reasonRunner(t, m, 3) + + result, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + if len(result.Steps) == 0 { + t.Fatal("the run produced no steps — a dead model must not stop the loop") + } + if len(result.ReasonErrors) == 0 { + t.Error("no ReasonErrors recorded for an unreachable model") + } + for _, want := range []string{"rotation", "no model client"} { + _ = want + } + if !strings.Contains(result.Steps[0].Why, "reasoner unavailable") { + t.Errorf("step Why = %q, want the fallback recorded", result.Steps[0].Why) + } +} + +// With no model client at all, behaviour must be byte-identical to before: +// the rotation, and the step says so. This is what keeps every existing +// test and every gateway-less deploy working. +func TestNoModelMeansRotationAndSaysSo(t *testing.T) { + r := reasonRunner(t, nil, 3) + result, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + if len(result.Steps) == 0 { + t.Fatal("no steps") + } + if result.Steps[0].Tool != "query" { + t.Errorf("first tool = %q, want the rotation's first (query)", result.Steps[0].Tool) + } + if !strings.Contains(result.Steps[0].Why, "rotation") { + t.Errorf("step Why = %q, want the rotation recorded", result.Steps[0].Why) + } + if len(result.ReasonErrors) != 0 { + t.Errorf("ReasonErrors = %v, want none when no model was configured", result.ReasonErrors) + } +} + +// The system prompt must carry the registry's own descriptions, including +// each tool's DO NOT USE WHEN — a model choosing from invented names is the +// failure this prevents (PRD move 4). +func TestReasonSystemPromptCarriesTheToolContract(t *testing.T) { + reg := tools.NewRegistry() + p := reasonSystemPrompt(reg.List()) + for _, want := range []string{"query", "run_tests", "write_file", "DO NOT USE WHEN", "ARGS SCHEMA", "Never invent one"} { + if !strings.Contains(p, want) { + t.Errorf("system prompt is missing %q", want) + } + } +} + +// The reasoner's tokens are counted, like every other call the run makes. +func TestReasonerUsageIsCounted(t *testing.T) { + m := &scriptedModel{replies: []string{ + `{"tool":"query","args":{"query":"x"},"why":"a"}`, + `{"tool":"run_tests","args":{},"why":"b"}`, + `{"done":true,"why":"c"}`, + }} + r := reasonRunner(t, m, 5) + result, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + if result.Usage.ModelCalls < 3 { + t.Errorf("model_calls = %d, want one per decision", result.Usage.ModelCalls) + } + if result.Usage.Total == 0 { + t.Error("usage total is zero — reasoner tokens are not counted") + } +} + +// The run's steps must carry their arguments: a trace showing only an +// args *hash* cannot answer "what did it actually try?". +func TestStepRecordsCarryArgs(t *testing.T) { + m := &scriptedModel{replies: []string{ + `{"tool":"query","args":{"query":"needle"},"why":"a"}`, + `{"done":true,"why":"b"}`, + }} + r := reasonRunner(t, m, 3) + result, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + b, err := json.Marshal(result.Steps[0]) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(b), "needle") { + t.Errorf("step JSON does not carry its args: %s", b) + } +} + +// Resuming from a hold must not re-ask the model: the decision was already +// made and approved, and a second call can choose a *different* tool — +// which would run something the operator never saw, under an approval for +// something else. Cost is the smaller reason; this is the reason. +func TestResumeReusesTheApprovedDecisionWithoutRecall(t *testing.T) { + m := &scriptedModel{replies: []string{ + // step 0: a read, auto-approved. + `{"tool":"query","args":{"query":"parseConfig"},"why":"locate"}`, + // step 1: a write — this holds for approval. + `{"tool":"write_file","args":{"path":"a.go","content":"fixed"},"why":"apply the fix"}`, + // Anything after this would be a re-ask. + `{"tool":"run_tests","args":{},"why":"verify"}`, + `{"done":true,"why":"done"}`, + }} + gate := NewApprovalGate() + cfg := RunnerConfig{ + RunID: "resume-recall", + MaxSteps: 4, + WallClock: 10 * time.Second, + CostBudget: 100, + Goal: "fix the parser", + Model: m, + Gate: gate, + } + reg := tools.NewRegistry() + r := newRunner(cfg, budget.New(100, 200), reg, nil, nil, nil, planner.NewPlanner(), nil, gate) + + // Run to the hold. + first, err := r.Run(context.Background()) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + if first.State != StatePausedApproval { + t.Fatalf("state = %q, want a hold on the write", first.State) + } + callsAtHold := m.calls + + // Approve and resume. + if !gate.Approve("resume-recall", 1) { + t.Fatal("Approve() returned false for the held step") + } + second, err := r.Resume(context.Background()) + if err != nil { + t.Fatalf("Resume() error: %v", err) + } + + // The decision at the held step must be the SAME one that was approved. + var held *StepRecord + for i := range second.Steps { + if second.Steps[i].StepID == 1 { + held = &second.Steps[i] + } + } + if held == nil { + t.Fatal("the held step is missing from the resumed run") + } + if held.Tool != "write_file" { + t.Errorf("resumed step tool = %q, want the approved one (write_file)", held.Tool) + } + if held.Args["content"] != "fixed" { + t.Errorf("resumed step args = %v, want the approved arguments", held.Args) + } + // And it was REPLAYED, not re-asked: the rationale says so. Without + // this the tool could coincidentally match while the model was asked + // again (and could have answered differently with a live gateway). + if !strings.Contains(held.Why, "approved (replayed)") { + t.Errorf("step why = %q, want it to record the replay, not a fresh decision", held.Why) + } + // The model is asked once per step AFTER the held one — never for the + // held step itself. The resumed loop runs steps 1..3, so at most 3 + // more calls; it must not be 4, which is what re-asking step 1 costs. + if got := m.calls - callsAtHold; got > 3 { + t.Errorf("model called %d times after the hold; the held step was re-asked "+ + "(steps 1-3 can each need one call)", got) + } +} diff --git a/internal/loop/runner.go b/internal/loop/runner.go index c58cf61..6369ade 100644 --- a/internal/loop/runner.go +++ b/internal/loop/runner.go @@ -33,10 +33,19 @@ type ScreenResult struct { // StepRecord is one tool call within a run. type StepRecord struct { - StepID int `json:"step_id"` - Phase string `json:"phase"` // think | act | evaluate - Tool string `json:"tool"` - ArgsHash string `json:"args_hash"` + StepID int `json:"step_id"` + Phase string `json:"phase"` // think | act | evaluate + Tool string `json:"tool"` + ArgsHash string `json:"args_hash"` + // Args is what the step was called with. Recorded because a trace that + // shows only an args *hash* cannot answer "what did it actually try?" + // — the first question anyone asks of a surprising step, and the thing + // a replay needs to reproduce it. + Args map[string]any `json:"args,omitempty"` + // Why is the reasoner's one-sentence rationale when a model chose this + // step ("" when the deterministic rotation did). The trajectory is + // readable only if the reasons are in it. + Why string `json:"why,omitempty"` Result interface{} `json:"result,omitempty"` CostUSD float64 `json:"cost_usd"` Confidence float64 `json:"confidence"` @@ -61,9 +70,15 @@ type RunResult struct { // from onegw's usage report; the leg that answered (not the combo // we asked for); and, if synthesis failed, why — the deterministic // partial is still returned in that case. - Usage TokenUsage `json:"usage,omitempty"` - AnsweredBy string `json:"answered_by,omitempty"` - SynthesisError string `json:"synthesis_error,omitempty"` + Usage TokenUsage `json:"usage,omitempty"` + // ReasonErrors records each step where the model could not be asked + // for a decision (transport failure, unreadable reply). The run keeps + // going on the deterministic rotation, and the trajectory says why it + // had to — a silent fallback is how a run looks "fine" while the + // model is unreachable. + ReasonErrors []string `json:"reason_errors,omitempty"` + AnsweredBy string `json:"answered_by,omitempty"` + SynthesisError string `json:"synthesis_error,omitempty"` } // RunnerConfig holds the tunables for a single run. @@ -109,7 +124,12 @@ type LoopRunner struct { model ModelClient // M8: outbound model transport (nil = deterministic synthesis) guardrailScreen func(string, any) (map[string]float64, float64) // M2.x: TypeSafe screen pausedStep int // step held at paused_approval (M5) - lastResult RunResult // partial result at pause (M5 resume) + // heldChoice is the decision already made for the held step. Resume + // replays it rather than re-asking the model: a second call can choose + // a DIFFERENT tool, which would run something the operator never saw, + // under an approval for something else. Cost is the smaller reason. + heldChoice *StepChoice + lastResult RunResult // partial result at pause (M5 resume) } // NewRunner returns a LoopRunner for the given config. @@ -151,7 +171,11 @@ func NewRunnerWithPlanner(cfg RunnerConfig, guard *budget.Guard, reg tools.ToolR // Pass nil for either to disable that feature. func NewRunnerWithPlannerAndGate(cfg RunnerConfig, guard *budget.Guard, reg tools.ToolRegistry, p *planner.Planner, gate *ApprovalGate) *LoopRunner { - return newRunner(cfg, guard, reg, nextToolDefault, nil, nil, p, nil, gate) + // nil picker on purpose: this is the production constructor, and the + // production preference order is reasoner -> rotation, not rotation + // first. Passing nextToolDefault here would make the model unreachable + // (the service would rotate forever while a gateway sat idle). + return newRunner(cfg, guard, reg, nil, nil, nil, p, nil, gate) } // NewRunnerWithCheckpointer returns a LoopRunner that persists @@ -382,15 +406,68 @@ func (r *LoopRunner) runLoop(ctx context.Context, result RunResult) (RunResult, } // --- one tool per ReAct turn --- result.State = StateActing - toolFn := r.nextToolFn - if toolFn == nil { - toolFn = nextToolDefault - } // M3: use planner step tier for routing if r.plan != nil && step < len(r.plan.Steps) { result.CurrentTier = tierCombo(r.plan.Steps[step].Tier) } - toolName, args := toolFn(step, r.cfg) + + // --- M9: choose the action --- + // Order of preference: the reasoner (a model that can see the last + // result), an injected picker (tests), then the deterministic + // rotation. Every fallback is recorded in the step's Why, because a + // trajectory that cannot say why a tool was chosen cannot be + // debugged. + var toolName string + var args map[string]any + var why string + var held *StepChoice // the decision for this step, if a model made one + switch { + case r.heldChoice != nil && step == r.pausedStep: + // Resuming an approved hold: replay the decision that was + // approved. Re-asking would let the model pick a different + // tool, so the operator would have approved one action and a + // different one would run. + toolName, args = r.heldChoice.Tool, r.heldChoice.Args + why = "approved (replayed): " + r.heldChoice.Why + r.heldChoice = nil + case r.nextToolFn != nil: + toolName, args = r.nextToolFn(step, r.cfg) + why = "picker: injected" + case r.model != nil: + name, a, reason, err := r.decide(ctx, step, &result) + if err != nil { + // A failed decision must not be silent and must not be + // fatal: the run falls back to the rotation and the step + // records what went wrong. (Same rule as synthesis.) + result.ReasonErrors = append(result.ReasonErrors, fmt.Sprintf("step %d: %v", step, err)) + toolName, args = nextToolDefault(step, r.cfg) + why = "reasoner unavailable: " + err.Error() + } else if name == "" { + // The model said the goal is established. That is the goal + // predicate firing, not a bound: the run is done, and it + // says so with StateSuccess rather than spinning to + // max_steps. + result.Success = ptr(true) + result.State = StateSuccess + result.ExitReason = ExitGoalMet + result.Steps = append(result.Steps, StepRecord{ + StepID: step, + Phase: "evaluate", + Tool: "(none)", + Why: reason, + }) + _ = r.synthesize(ctx, &result) + r.endRunSpan(r.cfg.RunID, result, nil) + return result, nil + } else { + toolName, args = name, a + why = reason + held = &StepChoice{Tool: name, Args: a, Why: reason} + } + default: + toolName, args = nextToolDefault(step, r.cfg) + why = "rotation: no model client" + } argsHash := dedupKey(r.cfg.RunID, toolName, args) // --- M5: fail-closed HITL gate (P30/P68) --- // Categorize the tool; auto -> approve, confirm -> auto-if-confident, @@ -417,6 +494,7 @@ func (r *LoopRunner) runLoop(ctx context.Context, result RunResult) (RunResult, result.State = StatePausedApproval _ = r.synthesize(ctx, &result) r.pausedStep = step + r.heldChoice = held r.lastResult = result r.endRunSpan(r.cfg.RunID, result, fmt.Errorf("approval required: %s", dec.Reason)) return result, nil @@ -477,6 +555,7 @@ func (r *LoopRunner) runLoop(ctx context.Context, result RunResult) (RunResult, result.State = StatePausedApproval _ = r.synthesize(ctx, &result) r.pausedStep = step + r.heldChoice = held r.lastResult = result r.endRunSpan(r.cfg.RunID, result, fmt.Errorf("guardrail: review required")) return result, nil @@ -509,6 +588,8 @@ func (r *LoopRunner) runLoop(ctx context.Context, result RunResult) (RunResult, Phase: "act", Tool: toolName, ArgsHash: argsHash, + Args: args, + Why: why, Result: map[string]any{"error": stepError(err, tr), "message": tr.Message}, CostUSD: 0.001, LatencyMs: latency, @@ -529,6 +610,8 @@ func (r *LoopRunner) runLoop(ctx context.Context, result RunResult) (RunResult, Phase: "act", Tool: toolName, ArgsHash: argsHash, + Args: args, + Why: why, Result: tr.Data, CostUSD: 0.001, LatencyMs: latency, @@ -556,6 +639,7 @@ func (r *LoopRunner) runLoop(ctx context.Context, result RunResult) (RunResult, // Loop exhausted without resolve = max_steps exit. // The run is no longer paused: a second Resume() is a no-op. r.pausedStep = -1 + r.heldChoice = nil result.State = StateExhausted result.ExitReason = ExitMaxSteps result.Success = ptr(false) diff --git a/internal/loop/runner_test.go b/internal/loop/runner_test.go index 9466ed1..fef5559 100644 --- a/internal/loop/runner_test.go +++ b/internal/loop/runner_test.go @@ -191,13 +191,14 @@ func TestCase5_InjectionSafe(t *testing.T) { // is reachable and listed — no silent exit paths. func TestAllExitReasonsListed(t *testing.T) { got := loop.AllExitReasons() - if len(got) != 8 { - t.Fatalf("AllExitReasons() returned %d items, want 8", len(got)) + if len(got) != 9 { + t.Fatalf("AllExitReasons() returned %d items, want 9", len(got)) } expected := map[string]bool{ "max_steps": false, "wall_clock": false, "cost_budget": false, "daily_budget": false, "confidence_floor": false, "progress_stall": false, "consecutive_failures": false, + "goal_met": false, "guardrail_block": false, } for _, r := range got {