feat(run): critic owns the deadline — MaxRuntime becomes the soft trigger #21

Merged
steve merged 3 commits from feat/critic-owns-deadline into main 2026-06-30 15:56:31 +00:00
2 changed files with 12 additions and 19 deletions
Showing only changes of commit 79ce833dd7 - Show all commits
+4 -7
View File
@@ -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 // is the soft wake (mort's two-tier semantics — the critic first reviews once the
Review

🟡 softTrigger=MaxRuntime is overloaded as the default battery's idle-stall window (critic.go:262), delaying hang detection from ~90s to a full MaxRuntime; comment 'reviews once the run exceeds its nominal budget' misdescribes the idle-based default behavior

correctness · flagged by 1 model

🪰 Gadfly · advisory

🟡 **softTrigger=MaxRuntime is overloaded as the default battery's idle-stall window (critic.go:262), delaying hang detection from ~90s to a full MaxRuntime; comment 'reviews once the run exceeds its nominal budget' misdescribes the idle-based default behavior** _correctness · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
// run exceeds its nominal budget, and its backstop = softTrigger × multiplier). // run exceeds its nominal budget, and its backstop = softTrigger × multiplier).
// The caller (Run) always passes the resolved MaxRuntime, which withFallbacks // The caller (Run) always passes the resolved MaxRuntime, which withFallbacks
// guarantees is > 0; the 90s floor below is purely a defensive guard for a // guarantees is > 0, so no fallback is needed here. (A non-positive soft would make
// hypothetical caller that passes a non-positive value. // 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()) { func (e *Executor) startCritic(runCtx context.Context, cancelCause context.CancelCauseFunc, ra RunnableAgent, info RunInfo, softTrigger time.Duration) (*criticBinding, func()) {
noop := func() {} noop := func() {}
if !e.criticOwnsDeadline(ra) { if !e.criticOwnsDeadline(ra) {
return nil, noop return nil, noop
} }
soft := softTrigger h := e.cfg.Ports.Critic.Monitor(runCtx, info, 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)
if h == nil { if h == nil {
Outdated
Review

Unreachable 90s defensive fallback reintroduces a bare magic literal that was previously a named default (CriticSoftTimeout)

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **Unreachable 90s defensive fallback reintroduces a bare magic literal that was previously a named default (CriticSoftTimeout)** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
return nil, noop return nil, noop
} }
+8 -12
View File
@@ -35,11 +35,10 @@ type Defaults struct {
// deadline. This ceiling exists ONLY to stop a critic that never advances its // 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 // 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 // set FAR beyond any realistic backstop (default 24h): the host clamps its own
Review

Comment references external system convar value

maintainability · flagged by 1 model

  • run/executor.go:37-39 — External system reference in comment: The CriticAbsoluteMax field comment references mort's agents.critic.absolute_max_seconds = 6h — an external convar from a different codebase. While contextually helpful, embedding external system configuration details in kernel comments creates a maintenance coupling. Suggested fix: Rephrase to describe the relationship without hardcoding external values (e.g., "the host clamps its own backstop to a smaller absolute max").

🪰 Gadfly · advisory

⚪ **Comment references external system convar value** _maintainability · flagged by 1 model_ - **`run/executor.go:37-39` — External system reference in comment**: The `CriticAbsoluteMax` field comment references `mort's agents.critic.absolute_max_seconds = 6h` — an external convar from a different codebase. While contextually helpful, embedding external system configuration details in kernel comments creates a maintenance coupling. Suggested fix: Rephrase to describe the relationship without hardcoding external values (e.g., "the host clamps its own backstop to a smaller absolute max"). <sub>🪰 Gadfly · advisory</sub>
// backstop to a much smaller absolute max (e.g. mort's agents.critic. // backstop to a much smaller absolute max (e.g. a 6h host convar), so the ceiling
// absolute_max_seconds = 6h), so the ceiling never pre-empts a healthy // never pre-empts a healthy supervised run. Keep it well above the host's
// supervised run. Keep it well above the host's absolute max. Never shorter than // absolute max. Never shorter than the run's MaxRuntime. Non-critic runs ignore
// the run's MaxRuntime. Non-critic runs ignore it (they keep the literal // it (they keep the literal MaxRuntime kill).
// MaxRuntime kill).
CriticAbsoluteMax time.Duration 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 // 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 // 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 // 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 // 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. // unsupervised and we tighten the cap back down to MaxRuntime below.
// A NESTED cause-carrying layer (cancelCause) lets a critic kill surface as a // 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 hardCap := maxRuntime
if criticOwns { if criticOwns {
// Runaway guard only — the critic's own (extendable) deadline-watch is the // Runaway guard only — the critic's own (extendable) deadline-watch is the
// normal cap. Never shorter than the nominal budget, in case an operator // normal cap. max() keeps it from being shorter than the nominal budget if an
// sets MaxRuntime above the runaway ceiling (a degenerate config). // operator sets MaxRuntime above the runaway ceiling (a degenerate config).
hardCap = e.cfg.Defaults.CriticAbsoluteMax hardCap = max(e.cfg.Defaults.CriticAbsoluteMax, maxRuntime)
if hardCap < maxRuntime {
hardCap = maxRuntime
}
} }
timeoutCtx, cancelTimeout := context.WithTimeout(context.WithoutCancel(ctx), hardCap) timeoutCtx, cancelTimeout := context.WithTimeout(context.WithoutCancel(ctx), hardCap)
defer cancelTimeout() defer cancelTimeout()
2