Skip to content

feat(llm): request-side thinking control, effective-policy records, dropped-block diagnostic (#625) - #660

Merged
gadievron merged 3 commits into
masterfrom
feat/625-thinking-control
Sep 21, 2026
Merged

gadievron merged 3 commits into
masterfrom
feat/625-thinking-control

Conversation

@gadievron

@gadievron gadievron commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

What

Exposes request-side thinking control for the Anthropic-format providers and makes the effective policy visible to checkpoint decisions and step reports — plus a dropped-block diagnostic for content-vs-usage reconciliation. Closes #625.

The instrument rule (default unchanged)

A provider entry may now set thinking:

"llm_providers": {
  "ant": {"type": "anthropic", "thinking": {"type": "enabled", "budget_tokens": 2048}}
}

The value passes verbatim as the request's thinking parameter (the SDK validates the shape — newer SDKs accept adaptive; older enabled/disabled with a budget). Absent (the default) sends no thinking key at all — the request is byte-identical to before, so verdict behavior is unchanged. Threading is capability-conditional exactly like #604's request_timeout, with the same one-time-per-type warning when a provider type doesn't consume the knob.

Where the effective policy is recorded

  • Checkpoint identity: fingerprint_for_binding folds a non-None policy into the KEY — a resumed run under a different thinking policy will not adopt stale checkpoints (it's a different instrument). A None policy folds nothing, so the digest — and every existing default-config checkpoint — is unchanged by this upgrade.
  • Step reports: each LLM phase's {step}.report.json inputs now carry {"provider", "model", "thinking"} (analyze, enhance, verify, report, dynamic-test, app-context, llm-reachability, generate-context), via a single binding_policy_summary helper read off the live adapter — the record states what was actually in effect.

The dropped-block diagnostic

The response translation drops block kinds it can't deliver (thinking, redacted_thinking, refusal) while forwarding their usage. It now counts them per kind on CompletionResult.dropped_block_kinds — a count, never a token split, since usage cannot split thinking. This gives the reconciliation the issue asked for: delivered content vs N dropped blocks alongside the forwarded usage. The existing one-time-per-kind stderr warning stays.

Testing

New regression test (test_issue625_thinking_control.py, 13 cases): config parse (verbatim + absent-is-None), registry threading + the unconsumed-knob warning, request shape for both adapters (configured key present; default key absent), the dropped-block counts, the fingerprint fold (configured changes the digest; default digest unchanged), and the policy summary. Full adapter/config/identity suites green: 185 passed. The full test directory runs 1974 passed, 148 skipped — one pre-existing failure (test_issue520_blackout_silence.py::test_fold_advisory_reaches_the_envelope_channel) fails identically on pristine master (verified on the clean worktree) and is unrelated to this change.

Deferred (on record)

  • Carrying prior reasoning into multi-turn history (the issue's third observation): replaying thinking blocks would change the instrument's conversation shape — separate evaluation, not mixed into a usage-accounting change.
  • Dropped-block counting for the OpenAI/Google adapters: their response shapes have no per-block drop loop today; the CompletionResult field is generic and they adopt counting when they carry droppable kinds.

Coordination

…ropped-block diagnostic (#625)

Expose the SDK's thinking parameter without changing the default
instrument: a provider entry may set "thinking" (verbatim dict —
the SDK validates the shape; newer SDKs accept adaptive, older
enabled/disabled with a budget), threaded capability-conditionally
through build_adapter exactly like #604's request_timeout, with the
same one-time-per-type warning when a provider type does not consume
it. anthropic + bedrock adapters declare the kwarg; absent (the
default) sends NO thinking key — the request stays byte-identical to
the pre-change shape, so verdict behavior is unchanged (#242's
instrument rule).

The effective policy is now visible where decisions read it:
- the checkpoint KEY folds a non-None thinking policy into
  fingerprint_for_binding (a resumed run under a different policy must
  not adopt stale checkpoints); None folds nothing, so the digest —
  and existing default-config checkpoints — survive unchanged;
- every LLM phase's step report carries {provider, model, thinking}
  via binding_policy_summary (scanner's seven LLM steps + the CLI's
  generate-context).

The dropped-block diagnostic: the block translation counted, per kind,
the response content blocks it drops (thinking, redacted_thinking,
refusal...) while usage forwards — CompletionResult.dropped_block_kinds
(a COUNT, never a token split; usage cannot split thinking), so
content-vs-usage reconciliation has a number to reconcile against.
The one-time-per-kind stderr warning stays.
…drops thinking blocks (#625 T1 retro)

The retro bug-hunt round found a reachable bug in the thinking control:
a thinking-enabled request WITH tools requires the thinking blocks
preserved on the echoed assistant turn (Anthropic's documented contract),
and this adapter's multi-turn loop echo filters to text/tool-use — so
iteration 2 would 400 after iteration 1 is billed. The per-provider knob
means one enabled entry hits every tool-using phase (enhance, verify).

Refuse the combination loudly before the paid call (both adapters);
disabled thinking with tools stays allowed; thinking without tools stays
allowed. Three regression tests pin the guard.
@gadievron

Copy link
Copy Markdown
Collaborator Author

Collision found and a bug fixed — the retro process review (2026-09-21) surfaced both:

1. This PR collides with #658 (opened 2026-09-20, branch fix/625-thinking-policy, same issue #625, six shared files). My collision check at authoring time was file-overlap-shaped and missed it — the anchor-keyed gh pr list --state all --search "625" finds it in one query. The honest comparison, re-derived from both diffs:

#658 this PR
Policy granularity per-phase per-provider (one knob hits every phase)
Tool-loop guard build-time + adapter backstop was absent (see below — now added)
Dropped-block diagnostic per-kind on result + step reports per-kind on result only
Checkpoint fold per-phase extra_key in scanner fingerprint_for_binding (one site, all phases)
Go wizard preserves hand-authored keys untouched

Recommendation: #658 survives; this PR is superseded. The two pieces worth absorbing into #658 from here: the fingerprint_for_binding-level fold (structurally better — one site, can't miss a phase) and the binding_policy_summary wiring into the standalone generate-context command. I'm not closing this PR (that's the maintainer's call) — saying it here so the decision is on the record.

2. The retro bug-hunt found a reachable bug in this branch — fixed in the latest push. A thinking-enabled request WITH tools requires thinking blocks preserved on the echoed assistant turn; the multi-turn loop echo filters to text/tool-use, so iteration 2 would 400 after iteration 1 is billed. The per-provider knob meant one enabled entry hit every tool-using phase. The latest commit refuses the combination loudly at build time (both adapters), with three regression tests. If #658 survives instead, its existing guard covers this — but the finding stands for any future reimplementation.

…ters (#625 review)

The committed guard coverage exercised the Anthropic adapter only; the
review verified the guard fires in both by manual probe but the suite
should pin it — now parameterized over anthropic + bedrock.
@gadievron

Copy link
Copy Markdown
Collaborator Author

Correction to an earlier claim in this body: the "one pre-existing failure (test_issue520_blackout_silence…) fails identically on pristine master" statement was wrong as a behavior claim — the test reads source files by cwd-relative path and passes 13/13 from the CI working-directory libs/openant-core; the historical run was invoked from the repo root. Not a master failure; the earlier wording held the tree constant but not the invocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant