diff --git a/AGENTS.md b/AGENTS.md index 1192d2c..9d77f18 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,9 +200,20 @@ 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. 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 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 +805,34 @@ 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. +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 — 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. 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) `provider.Request.SessionKey` carries a stable, opaque session identifier on diff --git a/engine/compact.go b/engine/compact.go index eae24cd..f394361 100644 --- a/engine/compact.go +++ b/engine/compact.go @@ -324,6 +324,35 @@ 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.) + // + // 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 — 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. 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 // (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..e9d4314 100644 --- a/engine/goal.go +++ b/engine/goal.go @@ -2255,6 +2255,18 @@ 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 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 // (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