From 02e7329b5dc2f122cca2341284e561a7880983c9 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Wed, 12 Aug 2026 11:48:15 -0400 Subject: [PATCH 1/3] fix(engine): pin evaluator EffortOff, inherit summarizer session effort engine/goal.go's evaluator request and engine/compact.go's compaction summary request never set provider.Request.Effort, so both ran at EffortUnset. On openaicompat, unset omits reasoning_effort, and some gateway models reason by default, burning the evaluator's 256-token budget on reasoning before it emits a verdict. The evaluator is a classifier: pin message.EffortOff, never inherit the session's level. The summarizer is a writing task: inherit the session's current effort via Session.Effort(). Fixes #124 --- AGENTS.md | 27 +++++++++++++++++++++++--- engine/compact.go | 7 +++++++ engine/compact_test.go | 33 +++++++++++++++++++++++++++++++ engine/goal.go | 8 ++++++++ engine/goal_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1192d2c..fb06f7b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,9 +200,16 @@ loop toward a natural-language completion condition. Turn 1 prompts the raw condition; after **every** turn an independent, TOOL-LESS evaluator model (`GoalOptions.Evaluator`, resolved through the same `Config.Providers` registry, `MaxTokens` 256) is asked to answer `MET: ` / `NOT MET: ` -(parsed leniently). A NOT MET verdict re-prompts with a fixed-template guidance -message carrying the reason; MET returns `Achieved`. `MaxTurns` (0 = unlimited) -bounds it. Evaluation is advisory: a retryable-class provider error from the +(parsed leniently). The evaluator request always pins `message.EffortOff` +(`runEvaluator`, `engine/goal.go`) — it is a classifier, not a reasoning task, +and it never inherits the session's own effort level. `EffortOff` suppresses +reasoning on every adapter (an explicit `"off"` on openaicompat, no thinking +block on anthropic, a strip on openai Responses), so a gateway model that +reasons by default cannot burn the evaluator's 256-token budget on reasoning +before it ever emits a verdict. (Issue #124.) A NOT MET verdict re-prompts +with a fixed-template guidance message carrying the reason; MET returns +`Achieved`. `MaxTurns` (0 = unlimited) bounds it. Evaluation is advisory: a +retryable-class provider error from the evaluator call rides the matching in-boundary backoff before the boundary counts as failed — the long weather-tier schedule (`goalRetryableMaxAttempts`, ~30min) for `overloaded`/`rate_limited`/ @@ -794,6 +801,20 @@ an empty string as "clear to provider default", and rejects an unknown session current level is read back on `GET /session/{id}` (`effort`), the same way the current model is. +**Effort at the three request-build sites is NOT uniform, by design.** The +main turn (`streamTurn`, `engine/engine.go`) sends `s.Effort()` — the +session's current level, read fresh every request. The two internal +tool-less calls diverge from that and from each other (issue #124): the +goal-loop evaluator (`runEvaluator`, `engine/goal.go`) always pins +`EffortOff` — see "Goal loop" above — because it is a classifier the model +must answer in one line, and reasoning-by-default gateway models can burn +its 256-token budget before ever emitting a verdict. The compaction +summarizer (`runCompactionSummary`, `engine/compact.go`) instead inherits +`s.Effort()`, the same as the main turn, because summarization is a real +writing task that benefits from the session's own quality setting; +`EffortUnset` stays `EffortUnset` there. Do not fold these two internal +sites onto one shared rule — one is a classifier, the other is prose. + ### Session affinity (prompt-cache routing hint) `provider.Request.SessionKey` carries a stable, opaque session identifier on diff --git a/engine/compact.go b/engine/compact.go index eae24cd..16d7f86 100644 --- a/engine/compact.go +++ b/engine/compact.go @@ -324,6 +324,13 @@ func (s *Session) runCompactionSummary(ctx context.Context, model message.ModelR // SessionKey names this session for an adapter that forwards it as // a routing/cache-affinity hint (see provider.Request.SessionKey). SessionKey: s.ID, + // Unlike the goal evaluator (a classifier that always pins + // EffortOff — see runEvaluator), the summarizer benefits from the + // session's own quality setting: read it fresh from live session + // state, not the value the session started with. EffortUnset stays + // EffortUnset here — this only forwards, never overrides. (Issue + // #124.) + Effort: s.Effort(), } // The summarizer's stream gets the same idle watchdog worker turns get // (see armIdleWatchdog): maybeAutoCompact runs at the top of every diff --git a/engine/compact_test.go b/engine/compact_test.go index f694cbb..1f25210 100644 --- a/engine/compact_test.go +++ b/engine/compact_test.go @@ -110,6 +110,39 @@ func TestCompactFoldsOldestPrefixKeepsRecentTurns(t *testing.T) { } } +// TestCompactSummaryRequestInheritsSessionEffort is the red-first test for +// issue #124: unlike the goal evaluator (a classifier that pins off), the +// compaction summarizer benefits from the session's own quality setting, so +// its request must carry the session's CURRENT effort level (set via +// SetEffort, read fresh, not the zero value the session started with). +// Drives the production entry point (Session.Compact -> runCompactionSummary). +func TestCompactSummaryRequestInheritsSessionEffort(t *testing.T) { + prov := &scriptedProvider{name: "test", turns: [][]provider.Event{ + compactTurn("one", provider.Usage{InputTokens: 10}), + compactTurn("two", provider.Usage{InputTokens: 10}), + compactTurn("three", provider.Usage{InputTokens: 10}), + compactSummaryTurn("SUMMARY", provider.Usage{InputTokens: 10}), + }} + s := NewSession(Config{ + Providers: provider.Registry{"test": prov}, + Model: message.ModelRef{Provider: "test", Model: "m1"}, + }) + s.SetEffort(message.EffortMedium) + runTurns(t, s, 3) + + if _, err := s.Compact(context.Background(), CompactOptions{KeepTurns: 1}); err != nil { + t.Fatal(err) + } + + if n := len(prov.requests); n == 0 { + t.Fatal("no requests captured") + } + summaryReq := prov.requests[len(prov.requests)-1] + if summaryReq.Effort != message.EffortMedium { + t.Errorf("summarizer request Effort = %q, want %q", summaryReq.Effort, message.EffortMedium) + } +} + // TestCompactSummaryBannerMarksSyntheticOrigin asserts the summary text // carries the visible synthesized-and-marked banner, mirroring // message.SyntheticOrphanResultText's spirit — a transcript reader can never diff --git a/engine/goal.go b/engine/goal.go index 5d17dfc..782a5c7 100644 --- a/engine/goal.go +++ b/engine/goal.go @@ -2255,6 +2255,14 @@ func (s *Session) runEvaluator(ctx context.Context, condition string, evaluator // SessionKey names this session for an adapter that forwards it as // a routing/cache-affinity hint (see provider.Request.SessionKey). SessionKey: s.ID, + // The evaluator is a classifier, not a reasoning task: it always + // pins EffortOff, never the session's own level (see AGENTS.md's + // "Goal loop" section). Since a7c5cce, EffortOff suppresses + // reasoning on every adapter (an explicit "off" on openaicompat, no + // thinking block on anthropic, a strip on openai Responses) — so + // this keeps the evaluator's MaxTokens 256 budget from being spent + // on reasoning before it ever emits MET/NOT MET. (Issue #124.) + Effort: message.EffortOff, } // The evaluator's stream gets the same idle watchdog worker turns get // (see armIdleWatchdog): it runs at EVERY goal turn boundary, so a diff --git a/engine/goal_test.go b/engine/goal_test.go index cff2733..ded7195 100644 --- a/engine/goal_test.go +++ b/engine/goal_test.go @@ -323,6 +323,50 @@ func TestPursueGoalAchievedSecondTurn(t *testing.T) { } } +// TestPursueGoalEvaluatorRequestPinsEffortOff is the red-first test for +// issue #124: the evaluator is a classifier, not a reasoning task, so its +// request must pin message.EffortOff regardless of the session's own +// effort level — never inherit it, and never leave it at EffortUnset. This +// drives the production entry point (PursueGoal -> runEvaluator) rather +// than calling runEvaluator directly, so it proves the wiring a real +// evaluator call goes through. +func TestPursueGoalEvaluatorRequestPinsEffortOff(t *testing.T) { + prov := &goalProvider{ + worker: [][]provider.Event{ + asstTurn(provider.StopEndTurn, &message.Text{Text: "all done"}), + }, + eval: [][]provider.Event{ + evalTurn("MET: done"), + }, + } + s := goalSession(t, prov, t.TempDir()) + // Give the session a non-off effort level so a bug that inherits it + // (instead of pinning off) would be observable. + s.SetEffort(message.EffortHigh) + + res, err := s.PursueGoal(context.Background(), "finish it", GoalOptions{Evaluator: evalModel}) + if err != nil { + t.Fatal(err) + } + if !res.Achieved { + t.Fatalf("result = %+v, want achieved", res) + } + + var evalReqs int + for _, rq := range prov.requests { + if len(rq.Tools) != 0 { + continue // worker request + } + evalReqs++ + if rq.Effort != message.EffortOff { + t.Errorf("evaluator request Effort = %q, want %q", rq.Effort, message.EffortOff) + } + } + if evalReqs != 1 { + t.Fatalf("evaluator requests = %d, want 1", evalReqs) + } +} + // TestPursueGoalWorkerReasoningEmptyProviderData is the round-2 forensic // regression guard reconstructed at the goal-loop level: the actual shape // the incident logs show (two complete goal-supervised turns, then death From 8d1b7c839b74a76889b688ed8e7e74f31690f751 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Wed, 12 Aug 2026 11:58:42 -0400 Subject: [PATCH 2/3] docs(engine): correct EffortOff claim, document compaction residuals Round-1 review of PR #125 found the evaluator's new comment and AGENTS.md overclaimed EffortOff's effect on openai Responses: reasoningEffort omits the reasoning object for EffortOff exactly as it does for EffortUnset, so a gpt-5-class model still reasons by default there. Correct the claim and name the residual explicitly. Also document two residuals the summarizer's effort inheritance surfaces (a non-off level can bypass compactionMaxTokens; folded history can newly hit the anthropic enable-direction thinking hazard) and file them as follow-ups (#126, #127) rather than expand this PR's scope. --- AGENTS.md | 20 +++++++++++++++----- engine/compact.go | 9 +++++++++ engine/goal.go | 14 +++++++++----- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fb06f7b..5b19626 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -202,11 +202,15 @@ condition; after **every** turn an independent, TOOL-LESS evaluator model `MaxTokens` 256) is asked to answer `MET: ` / `NOT MET: ` (parsed leniently). The evaluator request always pins `message.EffortOff` (`runEvaluator`, `engine/goal.go`) — it is a classifier, not a reasoning task, -and it never inherits the session's own effort level. `EffortOff` suppresses -reasoning on every adapter (an explicit `"off"` on openaicompat, no thinking -block on anthropic, a strip on openai Responses), so a gateway model that -reasons by default cannot burn the evaluator's 256-token budget on reasoning -before it ever emits a verdict. (Issue #124.) A NOT MET verdict re-prompts +and it never inherits the session's own effort level. On openaicompat, +`EffortOff` sends the literal `"off"`; on anthropic, it emits no thinking +block — both routes now spend none of the evaluator's 256-token budget on +reasoning. (Issue #124.) The openai Responses route is a known residual: +`reasoningEffort` (`provider/openai/transcode.go`) omits the `reasoning` +object for `EffortOff` exactly as it does for `EffortUnset`, and a +gpt-5-class model reasons by default with no adapter-level way to disable +it — so an evaluator on that route can still spend its budget on reasoning. +A NOT MET verdict re-prompts with a fixed-template guidance message carrying the reason; MET returns `Achieved`. `MaxTurns` (0 = unlimited) bounds it. Evaluation is advisory: a retryable-class provider error from the @@ -814,6 +818,12 @@ summarizer (`runCompactionSummary`, `engine/compact.go`) instead inherits writing task that benefits from the session's own quality setting; `EffortUnset` stays `EffortUnset` there. Do not fold these two internal sites onto one shared rule — one is a classifier, the other is prose. +Known residual (not addressed by issue #124, filed as a follow-up): a +non-off session effort can raise the summarizer's effective output cap +above `compactionMaxTokens` (the anthropic and openai adapters both bump +the cap for reasoning), and openaicompat applies no cap floor at all, so a +reasoning-heavy summary can truncate silently — `runCompactionSummary` has +no `StopReason` guard to catch it. ### Session affinity (prompt-cache routing hint) diff --git a/engine/compact.go b/engine/compact.go index 16d7f86..df16476 100644 --- a/engine/compact.go +++ b/engine/compact.go @@ -330,6 +330,15 @@ func (s *Session) runCompactionSummary(ctx context.Context, model message.ModelR // state, not the value the session started with. EffortUnset stays // EffortUnset here — this only forwards, never overrides. (Issue // #124.) + // + // Known residual, deliberately not addressed here (see AGENTS.md's + // scope-discipline rule): a non-off level lets the anthropic and + // openai adapters raise this request's effective output cap above + // compactionMaxTokens (anthropic's thinking-budget bump, openai's + // reasoningOutputFloor), and openaicompat sends reasoning_effort + // with no such floor at all, so a reasoning-heavy summary can be + // truncated at compactionMaxTokens with no StopReason guard here to + // catch it. Filed as a follow-up rather than expanded in this PR. Effort: s.Effort(), } // The summarizer's stream gets the same idle watchdog worker turns get diff --git a/engine/goal.go b/engine/goal.go index 782a5c7..e9d4314 100644 --- a/engine/goal.go +++ b/engine/goal.go @@ -2257,11 +2257,15 @@ func (s *Session) runEvaluator(ctx context.Context, condition string, evaluator SessionKey: s.ID, // The evaluator is a classifier, not a reasoning task: it always // pins EffortOff, never the session's own level (see AGENTS.md's - // "Goal loop" section). Since a7c5cce, EffortOff suppresses - // reasoning on every adapter (an explicit "off" on openaicompat, no - // thinking block on anthropic, a strip on openai Responses) — so - // this keeps the evaluator's MaxTokens 256 budget from being spent - // on reasoning before it ever emits MET/NOT MET. (Issue #124.) + // "Goal loop" section). Since a7c5cce, EffortOff sends the literal + // "off" on openaicompat and no thinking block on anthropic — both + // routes now spend none of the evaluator's MaxTokens 256 budget on + // reasoning. openai Responses is a known residual: reasoningEffort + // omits the reasoning object for EffortOff exactly as it does for + // EffortUnset (provider/openai/transcode.go), and a gpt-5-class + // model reasons by default with no adapter-level way to disable it + // — so an evaluator on that route can still spend its budget on + // reasoning. (Issue #124.) Effort: message.EffortOff, } // The evaluator's stream gets the same idle watchdog worker turns get From 4e6044ab24a3936653fffc250dc261baf3203560 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Wed, 12 Aug 2026 12:04:21 -0400 Subject: [PATCH 3/3] docs(engine): cite follow-up issues and state consequence in residual notes Round-2 review of PR #125 found the residual notes for issues #126 and #127 named neither issue number and did not state the output-cap consequence in concrete terms. Add both citations and name the consequence: a cap raised from 1024 to ~20480 tokens at EffortHigh delivers far less context reduction, at the layer whose own failure runs to a hard overflow that clears an active goal. --- AGENTS.md | 14 +++++++++++--- engine/compact.go | 17 +++++++++++++++-- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5b19626..9d77f18 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -818,12 +818,20 @@ summarizer (`runCompactionSummary`, `engine/compact.go`) instead inherits writing task that benefits from the session's own quality setting; `EffortUnset` stays `EffortUnset` there. Do not fold these two internal sites onto one shared rule — one is a classifier, the other is prose. -Known residual (not addressed by issue #124, filed as a follow-up): a +Known residual (not addressed by issue #124, filed as issue #126): a non-off session effort can raise the summarizer's effective output cap above `compactionMaxTokens` (the anthropic and openai adapters both bump -the cap for reasoning), and openaicompat applies no cap floor at all, so a +the cap for reasoning — up to ~20480 tokens at `EffortHigh`, versus the +documented 1024 cap), and openaicompat applies no cap floor at all, so a reasoning-heavy summary can truncate silently — `runCompactionSummary` has -no `StopReason` guard to catch it. +no `StopReason` guard to catch it. A raised cap also delivers less context +reduction from this call, at the layer whose own failure runs to a hard +overflow that clears an active goal. A second, related residual (issue +#127): the summarizer sends folded history containing `ToolCall` parts +from turns that ran with no thinking block, and a non-off level here +enables thinking over that same history — the documented ENABLE-direction +"thinking blocks expected before tool_use" reject case, just reached from +compaction instead of a live turn. ### Session affinity (prompt-cache routing hint) diff --git a/engine/compact.go b/engine/compact.go index df16476..f394361 100644 --- a/engine/compact.go +++ b/engine/compact.go @@ -335,10 +335,23 @@ func (s *Session) runCompactionSummary(ctx context.Context, model message.ModelR // scope-discipline rule): a non-off level lets the anthropic and // openai adapters raise this request's effective output cap above // compactionMaxTokens (anthropic's thinking-budget bump, openai's - // reasoningOutputFloor), and openaicompat sends reasoning_effort + // reasoningOutputFloor — up to ~20480 tokens at EffortHigh, versus + // the documented 1024 cap), and openaicompat sends reasoning_effort // with no such floor at all, so a reasoning-heavy summary can be // truncated at compactionMaxTokens with no StopReason guard here to - // catch it. Filed as a follow-up rather than expanded in this PR. + // catch it. A raised cap also delivers less context reduction from + // this call, at the layer whose own failure runs to a hard overflow + // that clears an active goal. Filed as issue #126 rather than + // expanded in this PR. + // + // Second known residual (issue #127): this call sends the folded + // range's real history, which can include assistant messages + // carrying ToolCall parts from turns that ran with no thinking + // block. A non-off level here enables thinking over that same + // history — the documented ENABLE-direction "thinking blocks + // expected before tool_use" reject case (see + // provider/anthropic/transcode.go), just reached from compaction + // instead of a live turn. Also filed rather than expanded here. Effort: s.Effort(), } // The summarizer's stream gets the same idle watchdog worker turns get