diff --git a/run/agent.go b/run/agent.go index 12c8186..7a48685 100644 --- a/run/agent.go +++ b/run/agent.go @@ -37,6 +37,15 @@ type RunnableAgent struct { MaxIterations int MaxRuntime time.Duration + // MaxToolCalls caps the TOTAL number of tool calls across the whole run, + // independent of the step ceiling (one step can carry several parallel tool + // calls). 0 (or negative) = unlimited — the backward-compatible default, so + // a run that never sets it is unaffected. Enforced at step granularity: the + // loop stops after the step in which the cumulative tool-call count reaches + // the cap, surfacing ErrMaxToolCalls. Single-loop runs only today (phased + // pipelines are a follow-up). + MaxToolCalls int + // LowLevelTools are tool-registry names the run may call directly. // SkillPalette / SubAgentPalette name saved skills / sub-agents exposed as // skill__ / agent__ delegation tools, resolved through diff --git a/run/critic.go b/run/critic.go index 49c1a05..709077b 100644 --- a/run/critic.go +++ b/run/critic.go @@ -110,17 +110,35 @@ func (b *criticBinding) recordToolStart(name, args string) { } } -// maxStepsOption returns the agent step-ceiling Option. With no critic it's a -// fixed WithMaxSteps(base); with a critic it's a DYNAMIC WithMaxStepsFunc that -// polls the handle each step (so the critic can raise a long run's budget), -// falling back to base when the handle defers (MaxSteps() <= 0). -func (b *criticBinding) maxStepsOption(base int) agent.Option { - if b == nil { - return agent.WithMaxSteps(base) - } +// stepCeilingOption returns the agent step-ceiling Option, folding the run's +// tool-call cap into the (optionally critic-driven) step ceiling. Priority: +// +// 1. tool-call cap hit (maxCalls > 0 && *toolCalls >= maxCalls): return the +// completed-step count so majordomo's `stepIdx < ceiling` check fails on the +// next iteration and the loop exits (the executor relabels the resulting +// ErrMaxSteps to ErrMaxToolCalls). Never returns <= 0 — majordomo would fall +// back to its own static ceiling — so a cap hit before any step completes +// clamps to 1. +// 2. the critic's DYNAMIC ceiling, when a critic is bound and raises a long +// run's budget mid-flight (b.h.MaxSteps() > 0). +// 3. the static base (MaxIterations). +// +// With maxCalls <= 0 and no critic this is exactly the old WithMaxSteps(base): +// an unlimited-tool-call run is a true no-op. nil-safe on the binding (b == nil +// skips the critic branch). toolCalls/steps are read live from the step +// observer's counters (same goroutine as this poll — see executor.go). +func (b *criticBinding) stepCeilingOption(base, maxCalls int, toolCalls, steps *int) agent.Option { return agent.WithMaxStepsFunc(func() int { - if n := b.h.MaxSteps(); n > 0 { - return n + if maxCalls > 0 && toolCalls != nil && *toolCalls >= maxCalls { + if steps != nil && *steps > 0 { + return *steps + } + return 1 + } + if b != nil { + if n := b.h.MaxSteps(); n > 0 { + return n + } } return base }) diff --git a/run/executor.go b/run/executor.go index 19bebe8..95330dd 100644 --- a/run/executor.go +++ b/run/executor.go @@ -41,6 +41,17 @@ type Defaults struct { // absolute max. Never shorter than the run's MaxRuntime. Non-critic runs ignore // it (they keep the literal MaxRuntime kill). CriticAbsoluteMax time.Duration + + // SalvageSingleLoopMaxSteps opts a single-loop run into partial-transcript + // salvage on budget exhaustion (ErrMaxSteps / ErrToolLoop / ErrMaxToolCalls): + // instead of returning empty Output + a hard error and discarding the + // reasoning it produced, the executor reconstructs a best-effort answer from + // the step narration (like the phased path) and downgrades the run to a + // successful partial result. Default false (off): a structured-output host + // (e.g. a bounded review swarm) keeps the clean hard error, where partial + // prose masquerading as a valid answer would be worse than a loud failure. A + // host that wants the reasoning recovered (interactive research) sets it true. + SalvageSingleLoopMaxSteps bool } func (d Defaults) withFallbacks() Defaults { @@ -159,6 +170,7 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio if maxRuntime <= 0 { maxRuntime = e.cfg.Defaults.MaxRuntime } + maxCalls := ra.MaxToolCalls // <= 0 => unlimited (no tool-call ceiling) // Budget gate (pre-run): a rejected run makes no model call. if e.cfg.Ports.Budget != nil { @@ -203,7 +215,7 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio rec = e.cfg.Ports.Audit.StartRun(ctx, info) } if rec != nil { - stateAcc = NewRunStateAccessor(rec, maxIter, 0, started) + stateAcc = NewRunStateAccessor(rec, maxIter, maxCalls, started) inv.RunState = stateAcc } @@ -372,6 +384,14 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio // calls with their executed results PAIRWISE — a result without a matching // call (or a call without a result) is skipped rather than recorded as an // empty-name "ghost" step. + // Tool-call budget accounting (single-loop enforcement, RunnableAgent. + // MaxToolCalls). The step observer runs on majordomo's Run goroutine — the + // SAME goroutine that reads the step ceiling — so plain ints suffice (no + // atomics). stepCeilingOption reads these to force the loop to exit once the + // cap is reached; the caller then relabels the resulting ErrMaxSteps to + // ErrMaxToolCalls. maxCalls <= 0 (unlimited) leaves this inert. + var toolCallsSeen, stepsSeen int + var capHit bool emitter := newStepEmitter(inv.OnStep) stepObserver := func(s agent.Step) { if stateAcc != nil { @@ -398,6 +418,21 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio rec.OnTool(call, r.Content) } } + // Tool-call budget: accumulate executed calls and note the crossing once. + // Only single-loop runs enforce the cap (phased pipelines are a follow-up), + // so gate the crossing detection on Phases to avoid a misleading audit + // event on a phased run that merely happens to carry a cap. + toolCallsSeen += n + stepsSeen = s.Index + 1 + if len(ra.Phases) == 0 && maxCalls > 0 && toolCallsSeen >= maxCalls && !capHit { + capHit = true + if rec != nil { + rec.LogEvent("max_tool_calls_reached", map[string]any{ + "max_tool_calls": maxCalls, + "tool_calls": toolCallsSeen, + }) + } + } } // Shared agent options used by BOTH the single-loop path and every phase: the @@ -480,7 +515,7 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio } opts := append([]agent.Option{ agent.WithToolbox(toolbox), - critic.maxStepsOption(maxIter), + critic.stepCeilingOption(maxIter, maxCalls, &toolCallsSeen, &stepsSeen), agent.WithStepObserver(obs), }, sharedOpts...) ag := agent.New(model, e.systemPrompt(ra), opts...) @@ -492,6 +527,15 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio runRes, runErr = runAgent(runCtx, ag, input, inv.Images, agent.WithSteer(steer)) } + // A tool-call-cap exit surfaces from majordomo as ErrMaxSteps (we drove + // the step ceiling down to force the loop to stop). Relabel it so status, + // salvage, and audit report the real cause. A capped step always carries + // tool calls (a final-answer turn has zero), so capHit + ErrMaxSteps + // pinpoints a cap-triggered exit. + if capHit && errors.Is(runErr, agent.ErrMaxSteps) { + runErr = fmt.Errorf("%w (max %d)", ErrMaxToolCalls, maxCalls) + } + // ResultSchema (optional): validate the final answer and let the model // self-repair on the same conversation before the host sees it. Runs // BEFORE FinalGuard so the delivery check inspects the answer that will @@ -570,6 +614,27 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio } } } + + // Single-loop budget-exhaustion salvage (opt-in). A loop that hit its + // step/tool-call budget without composing a final answer returns empty + // Output + a hard error, discarding the reasoning it DID produce. Mirror + // the phased path: reconstruct a best-effort answer from the step + // narration and downgrade the hard error to a successful partial result. + // This runs only after a budget error — ResultSchema + FinalGuard above + // are gated on runErr == nil, so neither fired. Gated on the host default + // so a structured-output host keeps its clean hard error. + if e.cfg.Defaults.SalvageSingleLoopMaxSteps && + isBudgetExhaustion(runErr) && runRes != nil && + strings.TrimSpace(runRes.Output) == "" { + if salvaged := salvagePhaseTranscript(runRes); salvaged != "" { + runRes.Output = salvaged + + "\n\n(Note: reached its step budget before composing a final answer; the above is its partial reasoning.)" + if rec != nil { + rec.LogEvent("single_loop_salvaged", map[string]any{"error": runErr.Error()}) + } + runErr = nil // downgrade: status becomes "ok" and Output is delivered + } + } } else { // Multi-phase pipeline: each phase runs its own prompt/tier/tools/step-cap // sequentially, threading outputs through {{.}} templates. The diff --git a/run/phases.go b/run/phases.go index 4a73d89..a4e13e3 100644 --- a/run/phases.go +++ b/run/phases.go @@ -156,7 +156,7 @@ func (e *Executor) runPhases(runCtx context.Context, ra RunnableAgent, deps phas deps.rec.LogEvent("phase_failed_optional", map[string]any{"phase": phase.Name, "error": err.Error()}) } - case isPhaseBudgetExhaustion(err) && (!isLast || trimmed != ""): + case isBudgetExhaustion(err) && (!isLast || trimmed != ""): // Soft stop: the phase ran out of its step/tool budget before // composing a final answer. Not fatal — it did real work (runOnePhase // salvaged its partial transcript into output), and aborting would @@ -248,7 +248,7 @@ func (e *Executor) runOnePhase(runCtx context.Context, ra RunnableAgent, deps ph } // Budget/guard exhaustion leaves a usable partial transcript but an empty // final answer; salvage the narrated work so the pipeline can carry it forward. - if runErr != nil && isPhaseBudgetExhaustion(runErr) { + if runErr != nil && isBudgetExhaustion(runErr) { if salvaged := salvagePhaseTranscript(res); salvaged != "" { output = salvaged } @@ -278,11 +278,15 @@ func (e *Executor) phaseModel(ctx context.Context, deps phaseDeps, ra RunnableAg return modelCtx, m } -// isPhaseBudgetExhaustion reports whether err is a soft budget/guard stop (the -// loop hit its step cap or tripped a tool-error guard) — which leaves a usable -// partial transcript — as opposed to a hard error (cancellation, model failure). -func isPhaseBudgetExhaustion(err error) bool { - return errors.Is(err, agent.ErrMaxSteps) || errors.Is(err, agent.ErrToolLoop) +// isBudgetExhaustion reports whether err is a soft budget/guard stop (the loop +// hit its step cap, its tool-call cap, or tripped a tool-error guard) — which +// leaves a usable partial transcript — as opposed to a hard error (cancellation, +// model failure). Shared by the phased pipeline and the single-loop salvage in +// executor.go, so the two paths never drift on what counts as exhaustion. +func isBudgetExhaustion(err error) bool { + return errors.Is(err, agent.ErrMaxSteps) || + errors.Is(err, agent.ErrToolLoop) || + errors.Is(err, ErrMaxToolCalls) } // maxSalvageBytes bounds a salvaged partial transcript so a long phase's narrated diff --git a/run/ports.go b/run/ports.go index e78db7c..79d90ad 100644 --- a/run/ports.go +++ b/run/ports.go @@ -16,6 +16,14 @@ import ( // kill via KillCause(); the executor wraps that reason with this sentinel. var ErrCriticKill = errors.New("run: critic killed the run") +// ErrMaxToolCalls is the cause when a run reaches its RunnableAgent.MaxToolCalls +// budget before composing a final answer. It surfaces from majordomo as +// ErrMaxSteps (the kernel forces the step ceiling down to stop the loop) and is +// relabeled to this sentinel so audit + status report the real cause. Treated +// as budget exhaustion by isBudgetExhaustion, so the single-loop salvage path +// recovers a partial answer from it when enabled. +var ErrMaxToolCalls = errors.New("run: max tool calls reached without a final answer") + // Ports are the host seams the run executor consumes. Every field is nil-safe: // a light host passes the zero Ports and gets a bounded, in-memory run with no // persistence, audit, budget, critic, delegation, or delivery — which is diff --git a/run/salvage_test.go b/run/salvage_test.go new file mode 100644 index 0000000..bc359f0 --- /dev/null +++ b/run/salvage_test.go @@ -0,0 +1,250 @@ +package run_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "strings" + "sync" + "testing" + + "gitea.stevedudenhoeffer.com/steve/majordomo/agent" + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" + "gitea.stevedudenhoeffer.com/steve/majordomo/provider/fake" + + "gitea.stevedudenhoeffer.com/steve/executus/run" + "gitea.stevedudenhoeffer.com/steve/executus/tool" +) + +// salvageNoopTool is an always-succeeds tool injected via Invocation.ExtraTools +// so a scripted tool-call loop advances (and counts a real tool call) without +// tripping the tool-error guard. +func salvageNoopTool() []llm.Tool { + return []llm.Tool{{ + Name: "noop", + Description: "no-op", + Handler: func(context.Context, json.RawMessage) (any, error) { return "ok", nil }, + }} +} + +// narratingLoop returns a fake provider whose every reply NARRATES a step and +// calls noop with DISTINCT args (so the same-call-repeat guard never trips), so +// the loop never finalizes and runs until its step / tool-call budget. Each +// step's narration is recoverable by the salvage path. +func narratingLoop() *fake.Provider { + var mu sync.Mutex + var n int + return fake.New("fake", fake.WithDefault(func(_ string, _ llm.Request) fake.Step { + mu.Lock() + n++ + step := n + mu.Unlock() + return fake.ReplyWith(llm.Response{ + Parts: []llm.Part{llm.Text(fmt.Sprintf("reasoning %d: partial finding %d", step, step))}, + ToolCalls: []llm.ToolCall{{ + ID: fmt.Sprintf("c%d", step), + Name: "noop", + Arguments: []byte(fmt.Sprintf(`{"n":%d}`, step)), + }}, + }) + })) +} + +// silentLoop is like narratingLoop but writes NO prose — so there is nothing for +// the salvage path to recover. +func silentLoop() *fake.Provider { + var mu sync.Mutex + var n int + return fake.New("fake", fake.WithDefault(func(_ string, _ llm.Request) fake.Step { + mu.Lock() + n++ + step := n + mu.Unlock() + return fake.ReplyWith(llm.Response{ + ToolCalls: []llm.ToolCall{{ + ID: fmt.Sprintf("c%d", step), + Name: "noop", + Arguments: []byte(fmt.Sprintf(`{"n":%d}`, step)), + }}, + }) + })) +} + +func modelsFor(m llm.Model) run.ModelResolver { + return func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil } +} + +// TestSalvageSingleLoop_OnRecoversPartialReasoning: with the salvage default ON, +// a single-loop run that exhausts its step budget without a final answer is +// downgraded to a successful PARTIAL result carrying its step narration, instead +// of returning empty output + a hard ErrMaxSteps. +func TestSalvageSingleLoop_OnRecoversPartialReasoning(t *testing.T) { + fp := narratingLoop() + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + Defaults: run.Defaults{SalvageSingleLoopMaxSteps: true}, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "researcher", ModelTier: "m", MaxIterations: 3}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "do research") + + if res.Err != nil { + t.Fatalf("salvage ON: budget exhaustion should downgrade to success; got err %v", res.Err) + } + if !strings.Contains(res.Output, "partial finding") { + t.Errorf("salvaged output should contain the step narration; got %q", res.Output) + } + if !strings.Contains(res.Output, "step budget") { + t.Errorf("salvaged output should carry the partial-answer note; got %q", res.Output) + } +} + +// TestSalvageSingleLoop_OffKeepsHardError: with salvage OFF (the zero-value +// default), the same run returns empty output + a hard ErrMaxSteps — the +// structured-output host's clean failure. +func TestSalvageSingleLoop_OffKeepsHardError(t *testing.T) { + fp := narratingLoop() + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + // Defaults zero => SalvageSingleLoopMaxSteps false. + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 3}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "go") + + if !errors.Is(res.Err, agent.ErrMaxSteps) { + t.Fatalf("salvage OFF: expected ErrMaxSteps, got %v", res.Err) + } + if strings.TrimSpace(res.Output) != "" { + t.Errorf("salvage OFF: output should be empty; got %q", res.Output) + } +} + +// TestSalvageSingleLoop_NoProseStillErrors: salvage recovers nothing when the +// loop wrote no prose, so a hard error stands even with salvage ON. +func TestSalvageSingleLoop_NoProseStillErrors(t *testing.T) { + fp := silentLoop() + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + Defaults: run.Defaults{SalvageSingleLoopMaxSteps: true}, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 3}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "go") + + if !errors.Is(res.Err, agent.ErrMaxSteps) { + t.Fatalf("no prose to salvage: ErrMaxSteps should stand, got %v", res.Err) + } + if strings.TrimSpace(res.Output) != "" { + t.Errorf("output should be empty; got %q", res.Output) + } +} + +// TestMaxToolCalls_ForcesExitAndSurfacesSentinel: a run capped at 2 tool calls +// stops after the 2nd (well before its high step ceiling) and surfaces the +// distinct ErrMaxToolCalls sentinel. +func TestMaxToolCalls_ForcesExitAndSurfacesSentinel(t *testing.T) { + fp := narratingLoop() + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + // salvage OFF so the raw sentinel is observable. + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 50, MaxToolCalls: 2}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "go") + + if !errors.Is(res.Err, run.ErrMaxToolCalls) { + t.Fatalf("expected ErrMaxToolCalls, got %v", res.Err) + } + // It must NOT read as a plain step-budget exhaustion. + if !strings.Contains(res.Err.Error(), "max tool calls") { + t.Errorf("error should name the tool-call cap; got %q", res.Err.Error()) + } +} + +// TestMaxToolCalls_SalvagedWhenEnabled: a tool-call-cap exit is treated as budget +// exhaustion, so with salvage ON it recovers the partial reasoning too. +func TestMaxToolCalls_SalvagedWhenEnabled(t *testing.T) { + fp := narratingLoop() + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + Defaults: run.Defaults{SalvageSingleLoopMaxSteps: true}, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 50, MaxToolCalls: 2}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "go") + + if res.Err != nil { + t.Fatalf("cap + salvage: expected downgraded success, got %v", res.Err) + } + if !strings.Contains(res.Output, "partial finding") { + t.Errorf("expected salvaged narration, got %q", res.Output) + } +} + +// TestMaxToolCalls_ZeroIsUnbounded: MaxToolCalls <= 0 imposes no ceiling — a run +// that makes several tool calls then finalizes completes normally (regression: +// unlimited must be a true no-op). +func TestMaxToolCalls_ZeroIsUnbounded(t *testing.T) { + fp := fake.New("fake") + fp.Enqueue("m", + fake.ReplyWith(llm.Response{ToolCalls: []llm.ToolCall{{ID: "c1", Name: "noop", Arguments: []byte(`{"n":1}`)}}}), + fake.ReplyWith(llm.Response{ToolCalls: []llm.ToolCall{{ID: "c2", Name: "noop", Arguments: []byte(`{"n":2}`)}}}), + fake.ReplyWith(llm.Response{ToolCalls: []llm.ToolCall{{ID: "c3", Name: "noop", Arguments: []byte(`{"n":3}`)}}}), + fake.Reply("final answer after 3 tools"), + ) + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 10, MaxToolCalls: 0}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "go") + + if res.Err != nil { + t.Fatalf("unbounded run should complete: %v", res.Err) + } + if res.Output != "final answer after 3 tools" { + t.Errorf("output = %q, want the final answer", res.Output) + } +} + +// TestMaxToolCalls_CapOverridesCriticRaise: the tool-call cap is a HARD budget — +// it stops the run even when a critic is willing to raise the step ceiling far +// past it. Also proves stepCeilingOption composes with a critic. +func TestMaxToolCalls_CapOverridesCriticRaise(t *testing.T) { + h := &fakeCriticHandle{maxSteps: 50} + fp := narratingLoop() + m, _ := fp.Model("m") + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: modelsFor(m), + Ports: run.Ports{Critic: &fakeCritic{h: h}}, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 1, MaxToolCalls: 2, Critic: run.CriticConfig{Enabled: true}}, + tool.Invocation{RunID: "r", ExtraTools: salvageNoopTool()}, + "go") + + if !errors.Is(res.Err, run.ErrMaxToolCalls) { + t.Fatalf("the tool-call cap must override the critic's raised ceiling; got %v", res.Err) + } +}