Files
executus/run/critic.go
T
steveandClaude Opus 4.8 13be3022fd
executus CI / test (pull_request) Successful in 2m2s
Gadfly review (reusable) / review (pull_request) Successful in 9m5s
Adversarial Review (Gadfly) / review (pull_request) Successful in 9m6s
feat(run): single-loop max-steps salvage + enforce MaxToolCalls
Two gaps surfaced by a live fan-out smoke in a downstream host (mort):

1) A single-loop run that exhausts its step budget (ErrMaxSteps / ErrToolLoop)
   returned empty Output + a hard error, discarding all the reasoning it had
   produced — unlike the phased path, which already salvages a partial
   transcript and continues. Add opt-in single-loop salvage
   (Defaults.SalvageSingleLoopMaxSteps, default off): on budget exhaustion with
   empty Output, reconstruct a best-effort answer from the step narration
   (reusing salvagePhaseTranscript) and downgrade the run to a successful
   partial result. Off by default so a structured-output host keeps its clean
   hard error; an interactive host opts in.

2) RunnableAgent carried no tool-call ceiling — MaxIterations bounds STEPS, but
   one step can dispatch several tool calls, so a host's "max tool calls" cap
   had nowhere to land. Add RunnableAgent.MaxToolCalls (<=0 = unlimited,
   backward-compatible): the step observer counts executed calls and, once the
   cap is reached, stepCeilingOption forces the loop to exit; the resulting
   ErrMaxSteps is relabeled to the new ErrMaxToolCalls sentinel. Enforced
   single-loop only for now (phased is a follow-up). A cap hit is budget
   exhaustion, so salvage recovers its partial reasoning too.

isPhaseBudgetExhaustion becomes the shared isBudgetExhaustion (now also matching
ErrMaxToolCalls) so the phased and single-loop paths never drift, and the
RunStateAccessor now reports the real tool-call cap (was hardcoded 0).

Additive + backward-compatible: new optional DTO field, new opt-in Defaults
flag, new exported sentinel; unlimited/off reproduce prior behavior exactly. No
new dependencies (go mod tidy clean). Tests cover salvage on/off/no-prose, cap
enforcement + sentinel, cap+salvage, unlimited no-op, and cap-overrides-critic.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Q1eJh5NVR11z6RzyxLANMK
2026-07-17 15:35:50 -04:00

155 lines
6.1 KiB
Go
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
package run
import (
"context"
"fmt"
"time"
"gitea.stevedudenhoeffer.com/steve/majordomo/agent"
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
)
// criticDeadlineCheck is how often the deadline-watch goroutine polls the
// critic's hard deadline. Small relative to any realistic soft timeout.
const criticDeadlineCheck = time.Second
// criticBinding wires a CriticHandle into a run: the executor forwards activity
// (steps + tool starts) to it, binds the run's hard cancellation to the critic's
// extendable deadline, and exposes the critic's Steer messages as an agent
// RunOption. All methods are nil-safe so the executor can call them
// unconditionally when no critic is configured.
type criticBinding struct {
h CriticHandle
}
// criticOwnsDeadline reports whether a critic is configured AND this run enables
// it — the single predicate that decides the two-tier-timeout path. Used by BOTH
// Run (to choose the generous runaway ceiling over the literal MaxRuntime cap) and
// startCritic (the arm/no-op gate), so the two can never drift.
func (e *Executor) criticOwnsDeadline(ra RunnableAgent) bool {
return e.cfg.Ports.Critic != nil && ra.Critic.Enabled
}
// startCritic begins critic monitoring for this run when one is configured and
// the agent enables it. It launches a goroutine that cancels runCtx (via
// cancelCause) the moment the critic's hard deadline passes — the critic may
// extend that deadline, so a healthy-but-slow run is given room while a hung one
// is killed. When the deadline passes because the critic KILLED the run
// (KillCause() != nil), the cancellation cause is ErrCriticKill (→ status
// "killed"); when the backstop simply expired, it is context.DeadlineExceeded (→
// "timeout"). Returns (nil, no-op stop) when there is no critic. The caller MUST
// defer the returned stop.
//
// softTrigger is the run's resolved MaxRuntime: for a critic-owned run MaxRuntime
// is the soft wake (mort's two-tier semantics — the critic first reviews once the
// run exceeds its nominal budget, and its backstop = softTrigger × multiplier).
// The caller (Run) always passes the resolved MaxRuntime, which withFallbacks
// guarantees is > 0, so no fallback is needed here. (A non-positive soft would make
// the host Monitor return no handle, and Run's unsupervised-run failsafe then bounds
// the run at MaxRuntime — so even that impossible case stays bounded.)
func (e *Executor) startCritic(runCtx context.Context, cancelCause context.CancelCauseFunc, ra RunnableAgent, info RunInfo, softTrigger time.Duration) (*criticBinding, func()) {
noop := func() {}
if !e.criticOwnsDeadline(ra) {
return nil, noop
}
h := e.cfg.Ports.Critic.Monitor(runCtx, info, softTrigger)
if h == nil {
return nil, noop
}
done := make(chan struct{})
go func() {
// A host CriticHandle.Deadline() that panics must not crash the process
// (this runs on its own goroutine, so the executor's top-level recover
// can't catch it). Log-free best-effort: just stop watching.
defer func() { _ = recover() }()
t := time.NewTicker(criticDeadlineCheck)
defer t.Stop()
for {
select {
case <-done:
return
case <-runCtx.Done():
return
case <-t.C:
// A zero deadline = no hard cap (not yet set); otherwise cancel
// once we're at or past it, distinguishing an explicit kill from a
// natural backstop expiry so the run gets the right status.
if d := h.Deadline(); !d.IsZero() && !time.Now().Before(d) {
if cause := h.KillCause(); cause != nil {
cancelCause(fmt.Errorf("%w: %s", ErrCriticKill, cause.Error()))
} else {
cancelCause(context.DeadlineExceeded)
}
return
}
}
}
}()
return &criticBinding{h: h}, func() {
close(done)
h.Stop()
}
}
func (b *criticBinding) recordStep(iter int, resp *llm.Response) {
if b != nil {
b.h.RecordStep(iter, resp)
}
}
// recordToolStart forwards a tool call to the critic. NOTE: majordomo's step
// observer only fires AFTER an iteration completes, so this currently lands
// post-tool, not at dispatch — the activity clock is refreshed once per
// iteration, not mid-tool. A single very long tool call (e.g. a 30-min render)
// therefore won't refresh the clock until it returns; a host that runs such
// tools should feed interim progress to its Critic (mort's InstallProgressBridge
// pattern). A true pre-dispatch refresh needs a majordomo hook (follow-up).
func (b *criticBinding) recordToolStart(name, args string) {
if b != nil {
b.h.RecordToolStart(name, args)
}
}
// 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 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
})
}
// drainSteer returns the critic's queued steer messages (nil-safe), so the
// executor can merge them with the session steer mailbox into one WithSteer.
func (b *criticBinding) drainSteer() []llm.Message {
if b == nil {
return nil
}
return b.h.Steer()
}