Skip to content

feat(llm): the per-phase request-side thinking policy — exposed, gated, recorded (#625) - #658

Closed
gadievron wants to merge 6 commits into
masterfrom
fix/625-thinking-policy
Closed

gadievron wants to merge 6 commits into
masterfrom
fix/625-thinking-policy

Conversation

@gadievron

@gadievron gadievron commented Sep 20, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #625. Follow-ups named below (the echo-preservation design; thinking_tokens capture; the Bedrock knob) — recorded, not filed here.

What was wrong

Two gaps, one theme — the request-side instrument:

  • No request-side thinking control while the SDK's messages.create accepts one — and the response's output_tokens includes thinking tokens, so the spend rode either way with no way to steer or explain it.
  • The response translation dropped unsupported block kinds (thinking, refusal) with a once-per-kind stderr warning while forwarding their usage — the delivered content and the billed content could differ by an invisible amount, and a dropped refusal beside a benign stop_reason could read as an empty success (the file's own comment).

This is an enhancement with the hard constraint the issue itself names: the default stays byte-identical (the #242 instrument lesson — verdict behavior is evaluated separately, never mixed with a usage-accounting change).

The fix

  • thinking on the config's phases (the SDK's own dict shapes — adaptive / enabled+budget / disabled; absent = no key sent, an explicit disabled is a distinct request that survives the round-trip). Parse-time validation rejects malformed shapes naming the config and phase; the enabled budget is bounded below the pipeline's smallest sub-call cap (4096 — the JSON-correction and stage-1-consistency requests ride the same binding at that cap, a larger budget would fail them mid-scan, and the consistency pass's exception handling would swallow it silently).
  • Tool-calling phases are gated loudly (the platform requires thinking blocks echoed back with tool results; this pipeline's loops carry text and tool-use only): enhance/verify reject a policy at registry build before any adapter is built, and an adapter-level backstop covers every other tools= call site — including the threat-model repository exploration, which the build-time list cannot name (it drives both a single-turn path and a tool loop). One adapter per (provider, thinking) tuple.
  • The policy is visible and identity-bound: recorded present-only in the step inputs and results.json; folded into the analyze/verify/llm-reachability checkpoint identities only-when-set — a resumed scan never adopts verdicts produced under a different policy (the fix(analyzer): recover reasoning-only empty-completion analyze responses #242 gen-params exclusion was conditional; a thinking policy changes ordinary successes, so the fold is the honesty mechanism). Default users' digests are unchanged — zero re-pay. The Go wizard now preserves hand-authored phase keys on re-runs.
  • The dropped blocks are counted, per kind, and persisted: dropped_blocks on every result (per-kind by design — a routine thinking drop can never bury a dropped refusal) plus a run-cumulative per-kind counter that reaches the step reports' token_usage.dropped_blocks in both the success and the accounting-error branches. Present-only — clean runs' artifacts are byte-identical.

Evidence

The de-balancing check (the intent trace)

serves: I-ctx (the attacker-model instrument knob — visible and identity-bound), I-out (the delivered-vs-billed reconciliation persisted). de-balances-checked: the default request byte-identical (full-dict equality pinned); the artifacts present-only on the default path; validate() carries no policy (the 1-token probe cannot satisfy the budget constraint — documented at the site); the #211 usage_details channel untouched (dropped_blocks is a separate field); add_prior_usage untouched; the fingerprint folds only-when-set (zero re-pay); the Go merge preserves only hand-authored keys.

Notes for the reviewer

  • Known residuals, scoped: the startup probe carries no thinking (a shape the server rejects for the model surfaces at the first real call, typed — the parse-time validation catches the client-side shapes first); the step-report dropped_blocks is run-cumulative (the Silent accounting drops: the report-phase tracker hand-off and the step-report zero-snapshot swallow failures into complete-looking zero cost #605 accepted trade — a reader summing across steps overcounts); thinking_tokens (the SDK ≥ 1.5.0 field that does split thinking from output) is deliberately a separate change; Bedrock does not consume the knob (warns once; its shared response translation does gain the counts).
  • The echo-preservation design (an opaque provider-block round-trip through the loops) is the named follow-up that unlocks thinking on the tool phases.

CI anomaly (recorded, not laundered): the tests and gitleaks workflows did not dispatch for this PR — a repo-wide pull_request dispatch outage, not a failure: two pushes (incl. a retrigger) and a fresh PR number produced only the CodeQL workflows (gh api .../workflows/247980277/runs shows the last pull_request-triggered tests run was for another branch at 09:12Z; the workflow_dispatch API is unavailable for these workflows). The CodeQL matrix IS green (Python/Go/JS-TS/actions analysis + Mergeable — 16 checks, all SUCCESS). The full test suite + Go tests are green locally against the exact commit (the receipts above — python3 -m pytest tests/ -q: 4314 passed / 34 skipped / 2 pre-existing SDK-pin failures; go test ./internal/config/: ok). The merge should wait for the tests workflow to dispatch (or the human verifies via the local receipts).

…d, recorded; the dropped blocks counted (#625)

The adapter had no request-side thinking control while the SDK accepts one;
the response translation dropped unsupported block kinds (thinking, refusal)
with a once-per-kind stderr warning while forwarding their usage — the
delivered and billed content could differ invisibly. An ENHANCEMENT with the
DEFAULT byte-identical (the #242 instrument lesson).

The change: PhaseRef.thinking (the SDK's dict shapes; parse-time validation
naming the config/phase; the enabled budget bounded below the smallest
sub-call cap — the JSON-correction/consistency requests ride the same
binding at 4096, and the consistency pass swallows exceptions silently);
the build-time tool-phase gate (enhance/verify) BEFORE any adapter build +
the adapter-level backstop covering every other tools= call site (the
threat-model repo exploration — the phase list cannot name app_context's
duality); one adapter per (provider, thinking) tuple; the policy folded
into the analyze/verify/llr checkpoint identities only-when-set (zero
re-pay for defaults); recorded present-only in the step inputs and
results.json; the Go wizard preserves hand-authored phase keys; the
per-kind dropped-block counts on every result + the run-cumulative
counter through get_totals to the step reports (both branches).

Receipts (all run at commit time, on this commit):
- RED on base 6dde272 (final bytes, copies byte-identical):
  python3 -m pytest tests/test_issue625_thinking_policy.py \
    tests/test_issue625b_exhaustion_rows.py -q
  -> 26 failed (value/signature-RED), 1 passed (the reset-lifecycle guard,
  green at base by design) in 0.99s (per-test rows: red-receipt-625.log)
- GREEN at head: the two files 27/27; the battery (anthropic-adapter,
  provider-warnings, #604, config-schema, backend-identity, #287, #211,
  #605) all green
- Full suite, matched conditions: head 2 failed, 4314 passed, 34 skipped
  in 319.53s; base 28 failed, 4288 passed, 34 skipped in 237.73s
  (the 2 failures are the pre-existing SDK-pin pair; skipped 34=34; the
  passed delta is exactly the 26 RED rows flipping)
  command: python3 -m pytest tests/ -q
- ruff check . -> All checks passed!
- go build/vet/test ./internal/config/ -> ok (incl. the new phase-merge row)
- Serves: I-ctx (the attacker-model instrument knob, visible + identity-
  bound), I-out (the dropped-vs-billed reconciliation persisted). De-
  balancing checked: the default request byte-identical (full-dict
  equality pinned); validate() carries no policy (the 1-token probe
  cannot satisfy the budget constraint — documented at the site); the
  #211 usage_details channel untouched (dropped_blocks is a separate
  field); add_prior_usage untouched; the Go merge preserves only
  hand-authored keys.
…6cb7b7d (the CodeQL workflows fired; the pull_request synchronize event reached only them)
Comment thread libs/openant-core/tests/test_issue625b_exhaustion_rows.py Fixed
Comment thread libs/openant-core/tests/test_issue625b_exhaustion_rows.py Fixed
Comment thread libs/openant-core/tests/test_issue625_thinking_policy.py Fixed
@gadievron

Copy link
Copy Markdown
Collaborator Author

Conflict with master resolved on 5b920d0 — CHANGELOG-only, both release-note sections kept (2026-09-20 above 2026-09-19), nothing else changed in the resolution. The branch's tests + gitleaks workflows are dispatching on this head for the first time now that it is conflict-free.

@sounil — a code-owner review would be appreciated once the checks land: this change touches the LLM pipeline (phase config, registry, adapters, llm_client, step-report usage) and the llm-reachability checkpoint identities, so a second pair of eyes on the gating + accounting would be valuable.

…policy reaches every LLM step's inputs (#625)

A non-consuming adapter (its class does not declare the thinking kwarg —
bedrock/openai/google today) never sends the policy, so the phase binding —
the fingerprint-extra and step-input source of truth — must not report one;
the one-time build warning stays the only surface for the unconsumed knob.
Default users are unaffected (absent stays absent).

The effective policy now also reaches the step reports of the remaining
LLM phases (app-context, enhance, verify, dynamic-test, report) present-only,
completing the comparison surface the analyze/llm-reachability twins started.

Tests: the discriminating non-consumer/consumer/default battery
(test_issue625c_effective_policy.py) plus the import-style cleanups in the
two existing test files.
Comment thread libs/openant-core/tests/test_issue625_thinking_policy.py Fixed
Four changes the review arc's challenge wave caught:

- A 400 naming thinking on a policy-configured adapter is now FATAL
  (ThinkingPolicyRejectedError, not a per-unit ERROR row): every unit
  would fail identically, and the fan-out painted an all-error scan
  green (exit 0, analyze in steps_completed, verify skipped
  no_candidates — the replayed shape). The error escapes the per-unit
  catch-all so the phase handler marks the step errored.

- The step reports' dropped_blocks are now PER-STEP DELTAS (the
  process-global counter snapshots at step entry; only this step's
  increases are written), and scan.report.json aggregates those deltas
  by sum — the run-cumulative snapshots re-reported every earlier
  step's drops (enhance reporting analyze's counts), and summing them
  overcounted by the prefix. The anthropic.py comment now describes
  the real path and the invocation-scoped (not resume-persistent)
  semantics.

- The declared SDK floor rises to anthropic[bedrock]>=0.47.0 — the
  thinking kwarg does not exist before it (verified across
  0.40/0.46/0.47), and every call would TypeError per-unit under the
  old floor. A capability test pins the installed SDK.

- The verifier and llm-reachability checkpoint folds gain discriminating
  tests: deleting either previously survived the whole 625 battery while
  enabling stale cross-policy checkpoint adoption (executed: SDK calls
  2->0 under the mutant).

Tests: test_issue625d_review_arc.py (the fatal-400 battery, the delta
semantics, the SDK floor, the fold pins) + the 625b/428 tests updated to
the delta semantics.
Comment thread libs/openant-core/tests/test_issue625d_review_arc.py Fixed
Comment thread libs/openant-core/tests/test_issue625d_review_arc.py Fixed
Comment thread libs/openant-core/tests/test_issue625d_review_arc.py Fixed
…se too (#625)

The review arc's reachability round caught the LLR per-batch catch-all
swallowing ThinkingPolicyRejectedError: a provider-rejected llm_reach
policy failed every batch identically, promoted nothing, and the scan
continued under-seeded while the step read 'partial' and the envelope
'success' — the phase that decides what gets analyzed must fail closed
exactly like analyze. _attempt re-raises it before the generic handler,
and the scanner's LLR guard lets it escape catch-and-continue.
@gadievron

Copy link
Copy Markdown
Collaborator Author

Disposition ruling needed — #625 is closed by #660; this PR needs a decision, not a conflict resolution.

#660 (46df6fd, merged 2026-09-21 17:24Z) closed #625. This branch (b58c2146) is now conflicting with master on 5 files (scanner.py, adapter.py, providers/anthropic.py, registry.py, llm_client.py — 15 hunks), and the two designs also collide semantically in the LLM config (a provider-entry thinking key vs this branch's phase-entry key). This comment lays out what each side has, verified at both trees, so the maintainer can pick a direction. Three options:

  1. Close as superseded — the follow-ups below get filed as issues and re-implemented on the landed design.
  2. Re-scope as a delta on the landed design — cherry-pick this branch's hardening; needs a granularity ruling first (phase-entry vs provider-entry thinking — provider aliases can express per-phase policies today, at config cost).
  3. Revert feat(llm): request-side thinking control, effective-policy records, dropped-block diagnostic (#625) #660 — a maintainer action; I won't create or merge a revert PR.

Gaps on landed master today (independent of this branch's fate — each verified at abd1dcf):

  • A rejected thinking policy fails per-unit, green. The guard lives inside the request path (providers/anthropic.py:206-225 — its comment says "build time" but it is per-request), and a thinking-named 400 reaches the per-unit catch-alls (core/analyzer.py:226, core/llm_reachability.py:744) → every unit errors while the CLI envelope reads success (exit 0 when no vulnerable/bypassable findings). This branch's ThinkingPolicyRejectedError makes the identical class fail closed (exit 2, zero downstream spend) — executed on both trees.
  • dropped_block_kinds is computed but never persisted — zero production readers (utilities/llm/adapter.py:226, providers/anthropic.py:378-474 produce it; nothing consumes it). This branch persists per-kind counts as per-step deltas + a scan-level aggregate.
  • anthropic[bedrock]>=0.40.0 predates the thinking parameter (it exists from 0.47.0) — the declared floor admits installs where every thinking call would TypeError. This branch floors at 0.47.0 with a capability test. (The requirements pin 1.7.0 masks this today.)
  • No budget preflight — an enabled budget_tokens ≥ the call's max_tokens reaches the SDK (executed: budget 8000 @ cap 4096 forwarded on master; this branch refuses it pre-send with zero SDK calls).

What the landed design has that this branch lacks (keep in any successor): Bedrock thinking consumption; the central fingerprint_for_binding policy fold (core/backend_identity.py:197-229); the consistent per-step policy summaries; the standalone generate-context policy record.

Design-coupled deltas on this branch (valuable only if per-phase lands): phase-entry thinking with parse-time validation; the registry-build tool-phase gate; the Go wizard's phase-entry sibling-key preservation (provider entries are already preserved on master — internal/config/config.go:379-399 vs :408-415 — so this is not a live master bug); the distributed verify/llm-reach fingerprint folds with tests.

Debts either way: the behavioral regression test for the fatal-policy guard; the pre-existing auth-error green-scan class (401 on analyze → exit 0); the standalone-CLI policy-recording scope. The branch and all its receipts stay as-is pending the ruling.

Agent: PR-MERGE ses_f3e3e9b11ffetbZil76EvwfDum

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