From 5b5ee4148e9d5b48afd4c9c62e775da89bedf574 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 30 Jun 2026 11:03:40 -0400 Subject: [PATCH 1/3] =?UTF-8?q?feat(run):=20critic=20owns=20the=20deadline?= =?UTF-8?q?=20=E2=80=94=20MaxRuntime=20becomes=20the=20soft=20trigger?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a run enables the critic (Ports.Critic set + RunnableAgent.Critic.Enabled), the kernel no longer hard-caps it at MaxRuntime. MaxRuntime becomes the SOFT trigger (passed to startCritic, used by the host critic as its wake + the base for its extendable backstop); the critic's deadline-watch is the real hard cancel. This restores mort's old agentexec two-tier timeout semantics — a slow-but-progressing run (e.g. a parent agent blocked on a 30-min animate render) is given room up to the critic's backstop instead of being killed at the nominal MaxRuntime. Specifics: - run/executor.go: the WithTimeout(MaxRuntime) is now conditional. Non-critic runs keep the literal MaxRuntime kill (→ "timeout"). Critic-owned runs get a GENEROUS WithTimeout at the new Defaults.CriticAbsoluteMax (default 6h) as a failsafe ceiling only — it never fires before the critic's backstop, and it guarantees a broken/nil host handle can't run unbounded. - run/critic.go: startCritic takes the resolved MaxRuntime as the soft trigger (falling back to Defaults.CriticSoftTimeout, then 90s), instead of always using the global CriticSoftTimeout. - Defaults.CriticAbsoluteMax added (withFallbacks default 6h). - Tests: non-critic dies at MaxRuntime; critic-owned survives past it; soft trigger == MaxRuntime. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Jo75sqmeVPgFUWZQBn179X --- run/critic.go | 12 +++- run/critic_deadline_test.go | 131 ++++++++++++++++++++++++++++++++++++ run/executor.go | 60 ++++++++++++----- 3 files changed, 186 insertions(+), 17 deletions(-) create mode 100644 run/critic_deadline_test.go diff --git a/run/critic.go b/run/critic.go index cbba6d6..83fb984 100644 --- a/run/critic.go +++ b/run/critic.go @@ -31,12 +31,20 @@ type criticBinding struct { // "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. -func (e *Executor) startCritic(runCtx context.Context, cancelCause context.CancelCauseFunc, ra RunnableAgent, info RunInfo) (*criticBinding, func()) { +// +// 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). +// It falls back to the configured CriticSoftTimeout when the run set no MaxRuntime. +func (e *Executor) startCritic(runCtx context.Context, cancelCause context.CancelCauseFunc, ra RunnableAgent, info RunInfo, softTrigger time.Duration) (*criticBinding, func()) { noop := func() {} if e.cfg.Ports.Critic == nil || !ra.Critic.Enabled { return nil, noop } - soft := e.cfg.Defaults.CriticSoftTimeout + soft := softTrigger + if soft <= 0 { + soft = e.cfg.Defaults.CriticSoftTimeout + } if soft <= 0 { soft = 90 * time.Second // defensive: withFallbacks normally guarantees >0 } diff --git a/run/critic_deadline_test.go b/run/critic_deadline_test.go new file mode 100644 index 0000000..30ddc16 --- /dev/null +++ b/run/critic_deadline_test.go @@ -0,0 +1,131 @@ +package run_test + +import ( + "context" + "sync" + "testing" + "time" + + "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" +) + +// slowToolInvocation builds an Invocation whose session factory adds a "slow" +// tool that sleeps for d (respecting ctx). The model script calls it once, then +// answers — so the run's wall-clock is dominated by d, letting a test set a tiny +// MaxRuntime and observe whether MaxRuntime hard-cancels the run. +func slowToolInvocation(runID string, d time.Duration) tool.Invocation { + slow := llm.DefineTool("slow", "sleeps for a while", + func(ctx context.Context, _ struct{}) (any, error) { + select { + case <-time.After(d): + return "ok", nil + case <-ctx.Done(): + return nil, ctx.Err() + } + }) + return tool.Invocation{ + RunID: runID, + SessionToolFactory: func(_ tool.AgentSession) tool.SessionTools { + return tool.SessionTools{Tools: []llm.Tool{slow}} + }, + } +} + +func slowModel() llm.Model { + fp := fake.New("fake") + fp.Enqueue("m", + fake.ReplyWith(llm.Response{ToolCalls: []llm.ToolCall{{ID: "c1", Name: "slow", Arguments: []byte(`{}`)}}}), + fake.Reply("done"), + ) + m, _ := fp.Model("m") + return m +} + +// TestNoCritic_MaxRuntimeIsHardCap: the legacy contract is preserved — without a +// critic, MaxRuntime is a literal WithTimeout that kills a run whose work outlasts +// it. The slow tool (200ms) outlasts MaxRuntime (20ms), so runCtx cancels mid-tool +// and the run ends in error (timeout). +func TestNoCritic_MaxRuntimeIsHardCap(t *testing.T) { + m := slowModel() + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 5, MaxRuntime: 20 * time.Millisecond}, + slowToolInvocation("r", 200*time.Millisecond), "go") + if res.Err == nil { + t.Fatalf("non-critic run should hard-timeout at MaxRuntime; got output=%q err=nil", res.Output) + } +} + +// TestCriticOwnsDeadline_SurvivesPastMaxRuntime: the fix — when the critic owns the +// deadline (Ports.Critic set + Critic.Enabled), MaxRuntime becomes the SOFT trigger +// and is NOT a hard cap. The fake critic exposes no hard deadline (Deadline()==zero, +// no kill), so the only hard ceiling is CriticAbsoluteMax (10s here). The slow tool +// (200ms) outlasts the tiny MaxRuntime (20ms) but the run completes — proving the +// old agentexec two-tier semantics are restored. +func TestCriticOwnsDeadline_SurvivesPastMaxRuntime(t *testing.T) { + m := slowModel() + h := &fakeCriticHandle{} // Deadline()==zero → no hard deadline, no kill + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, + Ports: run.Ports{Critic: &fakeCritic{h: h}}, + Defaults: run.Defaults{CriticAbsoluteMax: 10 * time.Second}, + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "watched", ModelTier: "m", MaxIterations: 5, MaxRuntime: 20 * time.Millisecond, + Critic: run.CriticConfig{Enabled: true}}, + slowToolInvocation("r", 200*time.Millisecond), "go") + if res.Err != nil { + t.Fatalf("critic-owned run must survive past MaxRuntime (soft trigger); got err=%v", res.Err) + } + if res.Output != "done" { + t.Errorf("output = %q, want %q", res.Output, "done") + } +} + +// capturingCritic records the soft trigger the executor passes to Monitor. +type capturingCritic struct { + mu sync.Mutex + soft time.Duration + h run.CriticHandle +} + +func (c *capturingCritic) Monitor(_ context.Context, _ run.RunInfo, soft time.Duration) run.CriticHandle { + c.mu.Lock() + c.soft = soft + c.mu.Unlock() + return c.h +} + +// TestCriticSoftTriggerIsMaxRuntime: the soft trigger handed to the host critic is +// the run's resolved MaxRuntime (mort's two-tier model — the critic first wakes once +// the run exceeds its nominal budget), NOT the global Defaults.CriticSoftTimeout. +func TestCriticSoftTriggerIsMaxRuntime(t *testing.T) { + fp := fake.New("fake") + fp.Enqueue("m", fake.Reply("done")) + m, _ := fp.Model("m") + cc := &capturingCritic{h: &fakeCriticHandle{}} + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, + Ports: run.Ports{Critic: cc}, + Defaults: run.Defaults{CriticSoftTimeout: 90 * time.Second}, // distinct from MaxRuntime below + }) + const wantSoft = 7 * time.Minute + ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxRuntime: wantSoft, Critic: run.CriticConfig{Enabled: true}}, + tool.Invocation{RunID: "r"}, "go") + cc.mu.Lock() + got := cc.soft + cc.mu.Unlock() + if got != wantSoft { + t.Errorf("soft trigger = %v, want the agent's MaxRuntime %v (not Defaults.CriticSoftTimeout)", got, wantSoft) + } +} diff --git a/run/executor.go b/run/executor.go index c950f98..0458798 100644 --- a/run/executor.go +++ b/run/executor.go @@ -30,6 +30,13 @@ type Defaults struct { MaxSameToolCallRepeats int // retry-storm guard; default 3 CompactionThresholdRatio float64 // fraction of model context to compact at; default 0.7 CriticSoftTimeout time.Duration // idle window before the critic wakes; default 90s + // CriticAbsoluteMax is the failsafe wall-clock ceiling for a critic-OWNED run + // (Ports.Critic set AND the agent enables it). For such a run MaxRuntime is the + // SOFT trigger, not a hard cap, and the critic's extendable backstop is the + // normal deadline — so this ceiling only fires if the critic never acts (a + // broken/nil host handle). Default 6h; never shorter than the run's MaxRuntime. + // Non-critic runs ignore it (they keep the literal MaxRuntime kill). + CriticAbsoluteMax time.Duration } func (d Defaults) withFallbacks() Defaults { @@ -54,6 +61,9 @@ func (d Defaults) withFallbacks() Defaults { if d.CriticSoftTimeout <= 0 { d.CriticSoftTimeout = 90 * time.Second } + if d.CriticAbsoluteMax <= 0 { + d.CriticAbsoluteMax = 6 * time.Hour + } return d } @@ -265,18 +275,36 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio postRun = st.PostRun } - // Run context: bound by MaxRuntime, detached from the caller's deadline so a - // lane/queue wait doesn't eat the run budget (mort's V10 lesson). Caller - // cancellation still propagates via MergeCancellation. Created BEFORE the - // step observer so the observer forwards the merged run context (not a - // possibly-cancelled caller ctx) to OnStep consumers. - // MaxRuntime stays a WithTimeout so its DeadlineExceeded propagates through the - // child chain (→ "timeout"), preserving the run's-own-timeout vs caller-cancel - // distinction. A NESTED cause-carrying layer lets a critic kill surface as a - // distinct "killed" without disturbing that: only an ErrCriticKill cause is - // consulted in statusFor; a generic run error or a caller cancel is classified - // by the run error itself. - timeoutCtx, cancelTimeout := context.WithTimeout(context.WithoutCancel(ctx), maxRuntime) + // Run context: detached from the caller's deadline so a lane/queue wait doesn't + // eat the run budget (mort's V10 lesson). Caller cancellation still propagates + // via MergeCancellation. Created BEFORE the step observer so the observer + // forwards the merged run context (not a possibly-cancelled caller ctx) to + // OnStep consumers. + // + // Two-tier timeout: who owns the hard deadline depends on the critic. + // - NO critic (the default): MaxRuntime is a literal WithTimeout. Its + // DeadlineExceeded propagates through the child chain (→ "timeout"), + // preserving the run's-own-timeout vs caller-cancel distinction. + // - critic OWNS the deadline (Ports.Critic set + ra.Critic.Enabled): + // MaxRuntime becomes the SOFT trigger (passed to startCritic), and the + // critic's extendable backstop — watched in startCritic, which cancels via + // cancelCause — is the real deadline. A slow-but-progressing run is given + // room up to the backstop; only a stalled one is killed. We still wrap a + // GENEROUS WithTimeout at CriticAbsoluteMax so a broken/nil critic handle + // can't run unbounded; that ceiling never fires before the critic's backstop. + // A NESTED cause-carrying layer (cancelCause) lets a critic kill surface as a + // distinct "killed": only an ErrCriticKill cause is consulted in statusFor; a + // generic run error, a backstop expiry, or a caller cancel is classified by the + // run error itself. + criticOwnsDeadline := e.cfg.Ports.Critic != nil && ra.Critic.Enabled + hardCap := maxRuntime + if criticOwnsDeadline { + hardCap = e.cfg.Defaults.CriticAbsoluteMax + if hardCap < maxRuntime { + hardCap = maxRuntime // the failsafe ceiling is never shorter than the nominal budget + } + } + timeoutCtx, cancelTimeout := context.WithTimeout(context.WithoutCancel(ctx), hardCap) defer cancelTimeout() runCtx, cancelCause := context.WithCancelCause(timeoutCtx) defer cancelCause(nil) @@ -287,9 +315,11 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio checkpointCause = func() error { return context.Cause(runCtx) } // Critic (optional): monitors the run for a stall, can nudge/extend/kill via - // its host Escalator. Its hard deadline is bound to runCtx (cancel on pass). - // nil-safe: no-op when no critic is configured or the agent doesn't enable it. - critic, stopCritic := e.startCritic(runCtx, cancelCause, ra, info) + // its host Escalator. When it owns the deadline, MaxRuntime is its soft trigger + // (so a slow-but-progressing run survives past it); its extendable backstop is + // bound to runCtx (cancel on pass). nil-safe: no-op when no critic is configured + // or the agent doesn't enable it. + critic, stopCritic := e.startCritic(runCtx, cancelCause, ra, info, maxRuntime) defer stopCritic() // Step instrumentation: accumulate Result.Steps + fire inv.OnStep, feed the From cb4c612461c394d327927d2b874372e39e0f4b46 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 30 Jun 2026 11:32:46 -0400 Subject: [PATCH 2/3] fix(run): address gadfly review of the critic-deadline PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 11 findings were real (3 clusters): - Failsafe ceiling could pre-empt the critic's backstop (e9c9483f, 9109317b, d5a9bf0d, 76ad171e): CriticAbsoluteMax was 6h, but the host's backstop (MaxRuntime × multiplier, or its own absolute max) can reach 6h+, so the ceiling fired first and reintroduced a premature hard cap. Now CriticAbsoluteMax is a 24h RUNAWAY guard set far beyond any realistic backstop (the host clamps its own backstop to a much smaller absolute max, e.g. mort's 6h convar), so it never pre-empts a healthy supervised run. Comments corrected. - nil Monitor handle lost the MaxRuntime cap (df016a6f, 9dd42827): a critic-enabled run whose host Monitor returned no handle had no deadline-watch and was bounded only by the generous ceiling. Added an unsupervised-run failsafe that re-wraps runCtx to the nominal MaxRuntime when the critic is enabled but didn't arm. New test TestCriticOwnsDeadline_NilHandleFallsBackToMaxRuntime. - CriticSoftTimeout vestigial / dead fallback (f7764919, 9805bebe, 6864086f, b2b11721): the soft trigger is now always the resolved MaxRuntime (> 0), so the CriticSoftTimeout field + its startCritic fallback were unreachable. Removed the field entirely; the remaining 90s floor is documented as defensive-only. - DRY (f30ce827): extracted e.criticOwnsDeadline(ra), now the single predicate used by both Run and startCritic so they can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Jo75sqmeVPgFUWZQBn179X --- run/critic.go | 19 ++++++++---- run/critic_deadline_test.go | 28 +++++++++++++++-- run/critic_test.go | 4 +-- run/executor.go | 61 +++++++++++++++++++++++++------------ 4 files changed, 81 insertions(+), 31 deletions(-) diff --git a/run/critic.go b/run/critic.go index 83fb984..d5eafdd 100644 --- a/run/critic.go +++ b/run/critic.go @@ -22,6 +22,14 @@ 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 @@ -35,18 +43,17 @@ type criticBinding struct { // 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). -// It falls back to the configured CriticSoftTimeout when the run set no MaxRuntime. +// The caller (Run) always passes the resolved MaxRuntime, which withFallbacks +// guarantees is > 0; the 90s floor below is purely a defensive guard for a +// hypothetical caller that passes a non-positive value. func (e *Executor) startCritic(runCtx context.Context, cancelCause context.CancelCauseFunc, ra RunnableAgent, info RunInfo, softTrigger time.Duration) (*criticBinding, func()) { noop := func() {} - if e.cfg.Ports.Critic == nil || !ra.Critic.Enabled { + if !e.criticOwnsDeadline(ra) { return nil, noop } soft := softTrigger if soft <= 0 { - soft = e.cfg.Defaults.CriticSoftTimeout - } - if soft <= 0 { - soft = 90 * time.Second // defensive: withFallbacks normally guarantees >0 + soft = 90 * time.Second // defensive only; the sole caller passes MaxRuntime (>0) } h := e.cfg.Ports.Critic.Monitor(runCtx, info, soft) if h == nil { diff --git a/run/critic_deadline_test.go b/run/critic_deadline_test.go index 30ddc16..8d78c8b 100644 --- a/run/critic_deadline_test.go +++ b/run/critic_deadline_test.go @@ -106,7 +106,7 @@ func (c *capturingCritic) Monitor(_ context.Context, _ run.RunInfo, soft time.Du // TestCriticSoftTriggerIsMaxRuntime: the soft trigger handed to the host critic is // the run's resolved MaxRuntime (mort's two-tier model — the critic first wakes once -// the run exceeds its nominal budget), NOT the global Defaults.CriticSoftTimeout. +// the run exceeds its nominal budget), not some global/default value. func TestCriticSoftTriggerIsMaxRuntime(t *testing.T) { fp := fake.New("fake") fp.Enqueue("m", fake.Reply("done")) @@ -116,7 +116,6 @@ func TestCriticSoftTriggerIsMaxRuntime(t *testing.T) { Registry: tool.NewRegistry(), Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, Ports: run.Ports{Critic: cc}, - Defaults: run.Defaults{CriticSoftTimeout: 90 * time.Second}, // distinct from MaxRuntime below }) const wantSoft = 7 * time.Minute ex.Run(context.Background(), @@ -126,6 +125,29 @@ func TestCriticSoftTriggerIsMaxRuntime(t *testing.T) { got := cc.soft cc.mu.Unlock() if got != wantSoft { - t.Errorf("soft trigger = %v, want the agent's MaxRuntime %v (not Defaults.CriticSoftTimeout)", got, wantSoft) + t.Errorf("soft trigger = %v, want the agent's MaxRuntime %v", got, wantSoft) + } +} + +// TestCriticOwnsDeadline_NilHandleFallsBackToMaxRuntime: the agent enables the +// critic but the host Monitor returns NO handle (nil) — there is no deadline-watch, +// so the run is unsupervised. It must fall back to the nominal MaxRuntime hard cap +// (the slow 200ms tool outlasts the 20ms MaxRuntime → the run errors), NOT run free +// up to the generous CriticAbsoluteMax runaway ceiling. +func TestCriticOwnsDeadline_NilHandleFallsBackToMaxRuntime(t *testing.T) { + m := slowModel() + cc := &capturingCritic{} // h is the nil interface → Monitor returns a nil handle + ex := run.New(run.Config{ + Registry: tool.NewRegistry(), + Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, + Ports: run.Ports{Critic: cc}, + Defaults: run.Defaults{CriticAbsoluteMax: time.Hour}, // generous ceiling; must NOT be what bounds the run + }) + res := ex.Run(context.Background(), + run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 5, MaxRuntime: 20 * time.Millisecond, + Critic: run.CriticConfig{Enabled: true}}, + slowToolInvocation("r", 200*time.Millisecond), "go") + if res.Err == nil { + t.Fatalf("critic-enabled run with a nil Monitor handle must fall back to the MaxRuntime hard cap; got output=%q err=nil", res.Output) } } diff --git a/run/critic_test.go b/run/critic_test.go index e617568..8047dee 100644 --- a/run/critic_test.go +++ b/run/critic_test.go @@ -61,8 +61,8 @@ func TestCriticRaisesStepCeiling(t *testing.T) { Registry: tool.NewRegistry(), Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil }, Ports: run.Ports{Critic: &fakeCritic{h: h}}, - // large soft timeout so the deadline-watch never interferes in the test - Defaults: run.Defaults{CriticSoftTimeout: time.Hour}, + // The fake handle's Deadline() is zero (no hard deadline), so the + // deadline-watch never interferes regardless of the soft trigger. }) res := ex.Run(context.Background(), run.RunnableAgent{Name: "x", ModelTier: "m", MaxIterations: 1, Critic: run.CriticConfig{Enabled: true}}, diff --git a/run/executor.go b/run/executor.go index 0458798..4b75010 100644 --- a/run/executor.go +++ b/run/executor.go @@ -29,13 +29,17 @@ type Defaults struct { MaxConsecutiveToolErrors int // loop guard; default 3 MaxSameToolCallRepeats int // retry-storm guard; default 3 CompactionThresholdRatio float64 // fraction of model context to compact at; default 0.7 - CriticSoftTimeout time.Duration // idle window before the critic wakes; default 90s - // CriticAbsoluteMax is the failsafe wall-clock ceiling for a critic-OWNED run - // (Ports.Critic set AND the agent enables it). For such a run MaxRuntime is the - // SOFT trigger, not a hard cap, and the critic's extendable backstop is the - // normal deadline — so this ceiling only fires if the critic never acts (a - // broken/nil host handle). Default 6h; never shorter than the run's MaxRuntime. - // Non-critic runs ignore it (they keep the literal MaxRuntime kill). + // CriticAbsoluteMax is the RUNAWAY ceiling for a critic-OWNED run (Ports.Critic + // set AND the agent enables it). For such a run MaxRuntime is the SOFT trigger, + // not a hard cap, and the critic's own extendable backstop is the normal + // deadline. This ceiling exists ONLY to stop a critic that never advances its + // deadline (a broken host handle) from running forever, so it is deliberately + // set FAR beyond any realistic backstop (default 24h): the host clamps its own + // backstop to a much smaller absolute max (e.g. mort's agents.critic. + // absolute_max_seconds = 6h), so the ceiling never pre-empts a healthy + // supervised run. Keep it well above the host's absolute max. Never shorter than + // the run's MaxRuntime. Non-critic runs ignore it (they keep the literal + // MaxRuntime kill). CriticAbsoluteMax time.Duration } @@ -58,11 +62,8 @@ func (d Defaults) withFallbacks() Defaults { if d.CompactionThresholdRatio <= 0 { d.CompactionThresholdRatio = 0.7 } - if d.CriticSoftTimeout <= 0 { - d.CriticSoftTimeout = 90 * time.Second - } if d.CriticAbsoluteMax <= 0 { - d.CriticAbsoluteMax = 6 * time.Hour + d.CriticAbsoluteMax = 24 * time.Hour } return d } @@ -289,19 +290,26 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio // MaxRuntime becomes the SOFT trigger (passed to startCritic), and the // critic's extendable backstop — watched in startCritic, which cancels via // cancelCause — is the real deadline. A slow-but-progressing run is given - // room up to the backstop; only a stalled one is killed. We still wrap a - // GENEROUS WithTimeout at CriticAbsoluteMax so a broken/nil critic handle - // can't run unbounded; that ceiling never fires before the critic's backstop. + // room up to that backstop; only a stalled one is killed. The base context + // gets a WithTimeout at CriticAbsoluteMax (default 24h) purely as a RUNAWAY + // guard for a critic that never advances its deadline: it is set FAR beyond + // any realistic backstop (the host clamps its own backstop to a much smaller + // absolute max, e.g. mort's 6h convar), so it does NOT pre-empt a healthy + // supervised run. If the host critic fails to ARM (nil handle), the run is + // unsupervised and we tighten the cap back down to MaxRuntime below. // A NESTED cause-carrying layer (cancelCause) lets a critic kill surface as a // distinct "killed": only an ErrCriticKill cause is consulted in statusFor; a // generic run error, a backstop expiry, or a caller cancel is classified by the // run error itself. - criticOwnsDeadline := e.cfg.Ports.Critic != nil && ra.Critic.Enabled + criticOwns := e.criticOwnsDeadline(ra) hardCap := maxRuntime - if criticOwnsDeadline { + if criticOwns { + // Runaway guard only — the critic's own (extendable) deadline-watch is the + // normal cap. Never shorter than the nominal budget, in case an operator + // sets MaxRuntime above the runaway ceiling (a degenerate config). hardCap = e.cfg.Defaults.CriticAbsoluteMax if hardCap < maxRuntime { - hardCap = maxRuntime // the failsafe ceiling is never shorter than the nominal budget + hardCap = maxRuntime } } timeoutCtx, cancelTimeout := context.WithTimeout(context.WithoutCancel(ctx), hardCap) @@ -310,9 +318,6 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio defer cancelCause(nil) runCtx, mergeCancel := MergeCancellation(runCtx, ctx) defer mergeCancel() - // The finalize defer (top of Run) now has a run context to read the - // cancellation cause from (shutdown vs critic-kill vs deadline vs cancel). - checkpointCause = func() error { return context.Cause(runCtx) } // Critic (optional): monitors the run for a stall, can nudge/extend/kill via // its host Escalator. When it owns the deadline, MaxRuntime is its soft trigger @@ -322,6 +327,22 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio critic, stopCritic := e.startCritic(runCtx, cancelCause, ra, info, maxRuntime) defer stopCritic() + // Unsupervised-run failsafe: the agent enabled the critic (so the base context + // got the generous runaway ceiling instead of MaxRuntime), but the host Monitor + // returned no handle — there is no deadline-watch. Without this the run would be + // bounded only by the 24h ceiling. Tighten it back to the nominal MaxRuntime so + // an unsupervised run can't hold its slot far past budget. mort's adapter always + // arms when the flag is set, so this is pure defence in depth. + if criticOwns && critic == nil { + var cancelUnsupervised context.CancelFunc + runCtx, cancelUnsupervised = context.WithTimeout(runCtx, maxRuntime) + defer cancelUnsupervised() + } + // The finalize defer (top of Run) now has a run context to read the + // cancellation cause from (shutdown vs critic-kill vs deadline vs cancel). Set + // AFTER the unsupervised-failsafe re-wrap so it reads the context the loop runs on. + checkpointCause = func() error { return context.Cause(runCtx) } + // Step instrumentation: accumulate Result.Steps + fire inv.OnStep, feed the // audit recorder, and keep the live iteration counter fresh. majordomo's // step observer hands us each completed iteration; we zip the model's tool From 79ce833dd7e5a6cf575243dfa6e7a31d96644ce0 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 30 Jun 2026 11:54:38 -0400 Subject: [PATCH 3/3] fix(run): address round-2 gadfly nits (max(), drop dead soft fallback, decouple doc) All low-severity follow-ups on the critic-deadline change: - hardCap uses max(CriticAbsoluteMax, maxRuntime) instead of a nested if (723193a7). - Drop the now-dead 90s soft-trigger fallback + its bare literal: the sole caller passes the resolved MaxRuntime (>0), and Run's unsupervised-run failsafe bounds even an impossible 0 (8d377051, 2f86bf58). - Decouple the kernel doc from a named downstream convar ("a 6h host convar") (730c67fc). Graded false-positive: agent.go BackstopMultiplier validation (handled in the host; not in this diff), the 24h default "magic number" (matches every withFallbacks default), and the defer-in-conditional pattern (idiomatic). Kept: the thorough two-tier comment (this logic regressed once) and the rare-path nested timer. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Jo75sqmeVPgFUWZQBn179X --- run/critic.go | 11 ++++------- run/executor.go | 20 ++++++++------------ 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/run/critic.go b/run/critic.go index d5eafdd..49c1a05 100644 --- a/run/critic.go +++ b/run/critic.go @@ -44,18 +44,15 @@ func (e *Executor) criticOwnsDeadline(ra RunnableAgent) bool { // 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; the 90s floor below is purely a defensive guard for a -// hypothetical caller that passes a non-positive value. +// 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 } - soft := softTrigger - if soft <= 0 { - soft = 90 * time.Second // defensive only; the sole caller passes MaxRuntime (>0) - } - h := e.cfg.Ports.Critic.Monitor(runCtx, info, soft) + h := e.cfg.Ports.Critic.Monitor(runCtx, info, softTrigger) if h == nil { return nil, noop } diff --git a/run/executor.go b/run/executor.go index 4b75010..7a3ffb3 100644 --- a/run/executor.go +++ b/run/executor.go @@ -35,11 +35,10 @@ type Defaults struct { // deadline. This ceiling exists ONLY to stop a critic that never advances its // deadline (a broken host handle) from running forever, so it is deliberately // set FAR beyond any realistic backstop (default 24h): the host clamps its own - // backstop to a much smaller absolute max (e.g. mort's agents.critic. - // absolute_max_seconds = 6h), so the ceiling never pre-empts a healthy - // supervised run. Keep it well above the host's absolute max. Never shorter than - // the run's MaxRuntime. Non-critic runs ignore it (they keep the literal - // MaxRuntime kill). + // backstop to a much smaller absolute max (e.g. a 6h host convar), so the ceiling + // never pre-empts a healthy supervised run. Keep it well above the host's + // absolute max. Never shorter than the run's MaxRuntime. Non-critic runs ignore + // it (they keep the literal MaxRuntime kill). CriticAbsoluteMax time.Duration } @@ -294,7 +293,7 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio // gets a WithTimeout at CriticAbsoluteMax (default 24h) purely as a RUNAWAY // guard for a critic that never advances its deadline: it is set FAR beyond // any realistic backstop (the host clamps its own backstop to a much smaller - // absolute max, e.g. mort's 6h convar), so it does NOT pre-empt a healthy + // absolute max, e.g. a 6h host convar), so it does NOT pre-empt a healthy // supervised run. If the host critic fails to ARM (nil handle), the run is // unsupervised and we tighten the cap back down to MaxRuntime below. // A NESTED cause-carrying layer (cancelCause) lets a critic kill surface as a @@ -305,12 +304,9 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio hardCap := maxRuntime if criticOwns { // Runaway guard only — the critic's own (extendable) deadline-watch is the - // normal cap. Never shorter than the nominal budget, in case an operator - // sets MaxRuntime above the runaway ceiling (a degenerate config). - hardCap = e.cfg.Defaults.CriticAbsoluteMax - if hardCap < maxRuntime { - hardCap = maxRuntime - } + // normal cap. max() keeps it from being shorter than the nominal budget if an + // operator sets MaxRuntime above the runaway ceiling (a degenerate config). + hardCap = max(e.cfg.Defaults.CriticAbsoluteMax, maxRuntime) } timeoutCtx, cancelTimeout := context.WithTimeout(context.WithoutCancel(ctx), hardCap) defer cancelTimeout()