mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
c906776efd253c3aa1326d7dd4458a5c007cbbab
14 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
c906776efd |
fix(832): the serving lane's capabilities reach the wire fold
The per-attempt prepare_wire closure folded mid-conversation system turns with the PRIMARY binding's capabilities on every lane, so a fallback whose chat template rejects non-leading system roles failed on the self-inflicted wire shape and burned its own health record — the wrong-dialect class the walk's binding snapshot guards against elsewhere. model_turn now passes the serving lane to prepare_wire, and the session's closure folds with that lane's capabilities; callers without a lane in hand (the token-table re-fold) keep the primary default. Pre-fold prepared once with primary caps for every lane, so this is a named improvement, not a parity break. The arm-duties hook rode the same unguarded two-statement supersession window the _CancelRef docstring accepts only for the stream register: a force-cancel claiming a new generation between the superseded read and the hook let an orphan's late registration null the successor's usage slots and record spurious creation health. on_stream_armed now generation-gates itself, shrinking the accepted window's harm back to the register-only class. Test hygiene: the two overflow-compact tests are one parametrized body; arm_session mints a fresh ArmedHandle per create (provider.handles, _armed_handle = latest) matching the one-handle-per-create rule of real adapters. The duplicate sanitize pass stands as designed (accepted for wire parity); its perf note rides #979. All three product fixes are mutation-probed. |
||
|
Patrick Buckley
|
90e55f92ca |
docs(832): shorten the branch's comments to their constraints
Comment-only sweep over the diff's prose: origin archaeology, next-line narration, and review-thread talk go; each surviving comment states the constraint the code cannot show, re-wrapped to the file's width. The ruled-behavior restatements in the parity transforms and the contract docstrings (eager append, cancel-predicate pairing, carry ownership, the plant call's carve-outs) keep every named invariant. |
||
|
Patrick Buckley
|
06ec1a8629 |
fix(832): the boundary carry belongs to the run owner
The mandated cross-lane interleave angle found the two residual holes in the reasoning-boundary close: the close was gated on not-in_think, so an open inline think block at the boundary never closed and the later state flip relabeled held chain-of-thought as displayed ANSWER text; and the carry parked in the splitter's own pending was re-read under whatever state later flushes hit, relabeling a content-state tail as reasoning. close_run() now closes unconditionally (as the drain does) and RETURNS the partial-tag tail; the consumer owns the carry in a state-immune slot mirroring the drain's separate variable — re-fed when content resumes so a split tag still reassembles, flushed as content at tool, finish, and cancel boundaries, and included in the partial-content rule. The trailing citations footer is now HELD and folded once at stream end over the full answer — structurally the drain's post-loop fold — instead of folding at arrival, which diverged from the commit whenever a lax gateway emitted content after finish. Two non-mirror fixes: the fallback-failure UI line carries the exception class only (its text can embed a credential-bearing base_url; detail goes to the server log, same rule as the re-issue log arm), and a never-armed Stop (creation window, no prior death, zero tokens) writes NO assistant row again — restoring pre-fold semantics; a marker-only row would replay to the model as context on every later turn. Armed zero-token Stops still record their marker. Hygiene riding along: the parity runner zeroes the ladder backoff (the exhaust scenario was sleeping 3.2s of real backoff per suite run, with the retry-notice transform strings updated in step); test_session's porting docstring points at the helper's real module; test_cancel and test_session wrap the shared session factory instead of re-implementing its defaults; arm_session's armed handle is an ArmedHandle with real closed state instead of a MagicMock that satisfies any assertion; and send() derives the tool-call list once for both the persisted mirror and the executed set. All fixes are mutation-probed: re-gating the close, discarding the carry, dropping the promote gate, unredacting the fallback line, and restoring the arrival-time fold each fail their pins. |
||
|
Patrick Buckley
|
aa4371ea99 |
fix(832): retire the dead attempt's armed state in the re-create window
Between a mid-stream death and the next begin_attempt there is no live attempt, but the consumer kept the dead attempt's armed _CancelRef: a Stop in that window re-emitted the discarded splitter carry as fresh content behind a duplicate stream_end, and a walk-preamble failure was classified as another armed death, replacing the operator-actionable stream-death error. end_attempt() now pronounces the attempt dead at partial-capture; the consumer gains a single per-attempt initializer (_reset_attempt), a lane-free constructor (one resolve_lane walk per turn), and a saw-chunk classifier fallback so a never-arming adapter's mid-stream death still classifies mid-stream instead of silently double-rendering the same lane. Wire-preparation failures are typed at the seam: model_turn wraps prepare_wire raises in WirePreparationError, both walk arms forward it verbatim (no health record, no fallback walk — a session-data fault would otherwise paint every backend degraded), the fatal formatter gets a dedicated branch, and the re-issue ladder's last-death mask exempts it alongside BackendAuthUnavailableError so an auth outage mid-turn is not misdiagnosed as a network flap. Riding fixes: the tag-scan gate gets its single spelling (lane_scans_inline_reasoning) shared by drain and display; the citations fold's separator+gate become a shared pair in _protocol; _build_main_lane stops passing config_store (dead derivation — the session's own knobs replace both values it feeds); the debug wire dump is ruled per-invocation (the overflow-recovery re-print is the dump that diagnoses the recovery) and pinned; dead delegates _ensure_tool_call_ids and _finalize_provider_blocks deleted; the parity runner adapts to the pre-fold seam signature by inspection and refuses to record a harness-shape TypeError as a baseline; the streaming provider fakes move to tests/_session_helpers (their tree-wide home) and test_cancel's duplicate helper is deleted; committed parity pins restate their rulings in full; architecture.md's circuit-breaker section is replaced by the real passive health-tracker story and the send-flow diagram stops attributing tool-call assembly to the display consumer; stale pre-fold names and ragged comment paragraphs cleaned. New pins are mutation-probed: disabling end_attempt, the saw-chunk fallback, the auth exemption, or the WirePreparationError arm each fails its pin. |
||
|
Patrick Buckley
|
bc3fa60011 |
fix(providers): segregate inline reasoning at the drain seam
Passthrough servers (parserless vLLM/llama.cpp, LM Studio, bare gateways) emit reasoning as literal <think>/<reasoning> blocks inside content, and only three of nine drained lanes stripped them: web_fetch tool results persisted raw think blocks into every following turn (#940), judge verdicts parsed through tag noise, and a draft verdict inside a think block could shadow the real one at the output guard. One rule at the seam now. drain_stream accumulates content in RUNS bounded by interleaving signals (provider-parsed reasoning deltas, tool-call deltas) with the interactive consumer's within-chunk ordering — reasoning, then content, then the tool-call close — and splits each run through split_inline_reasoning, the one-shot form of the interactive lane's ThinkTagSplitter: a pure raw split, exactly equivalent to the streaming form on every catalog case. One trim policy exists and the drain owns it: blank edge lines are trimmed once over the joined runs when a tag was consumed, so tag residue dies at the edges while genuine inter-run paragraph separators survive. Extracted text is appended to result.reasoning after any server-parsed reasoning with a blank-line boundary and rides the native lane as the reasoning_text synth block. Orphan CLOSE tags deliberately pass through byte-identical: a close whose open never arrived is indistinguishable from prose QUOTING the tag, and drained lanes routinely quote third-party text — reclassifying would let a malicious page containing the literal tag destroy the extraction that cites it. The title lane keeps a local rfind peel as display-string formatting. The citations footer folds only onto non-blank content — sourcing for an answer that does not exist is dropped rather than handed to emptiness checks as a footer-only "answer". Every private strip is deleted: the title lane's strip, the summarizer strip, _strip_reasoning itself, and the optimizer's five regexes (_strip_markdown_fence is now the one fence rule, applied to normalized model output only, never to or-fallback values). Think-only and whitespace-only responses drain to blank content, and every lane's no-answer fallback gates on blankness: web_fetch returns an honest extraction-error card, the intent judge takes the empty-retry ladder, the task-agent synthesis reports "(no output)", and the optimizer keeps the current observer system and prompt verbatim on no-answer passes. Final-say reads (optimizer analyst, eval final_content, the notify hook) use trajectory.final_assistant_text — the last assistant turn only, never an earlier narration presented as the conclusion — while last_assistant_text is the salvage walk (task_agent partial-work recovery), skipping tool-call-only, all-reasoning, and whitespace-only turns. Perception memoizes every completed description immediately, including an empty one — one perceive per key, ever — under a commit-lock guard so an empty result never overwrites a concurrently memoized real description; an all-reasoning perception model pins the placeholder until restart, and the remediation is server-side (a reasoning parser or the template thinking toggle on the perception alias). A true double-reasoning shape (inline-extracted text alongside a native reasoning block) logs chars-only at the drain, where it is distinguishable from the routine reasoning_delta mirror. The dialect's semantics are pinned as one table (tests/_reasoning_dialect.py) driven through shared fixtures (think_tag_stream, seam_provider): one-shot conformance, the exact one-shot/streaming equivalence property, the drain seam rules including quoted-tag safety, run-boundary and separator-preservation pins, per-lane pins for all nine lanes, and the empty-content assistant wire shape. Closes #965. Closes #940. |
||
|
Patrick Buckley
|
7776cc0c2f |
fix(streaming): probe on_stream_discarded for pre-existing UIs and format the hoisted fake
PR feedback round: - on_stream_discarded now follows on_compaction's compat pattern for a hook added after UIs exist in the wild: the protocol member carries a REAL no-op default (an explicit subclass inherits a correct implementation — a UI without server-side turn buffers has nothing to truncate), and both call sites route through a getattr probe, so a duck-typed UI predating the hook degrades to no-truncate instead of raising an AttributeError from the very arm that is handling a stream death — which would replace the wire failure with the attribute error in the retry gate. Pinned with a hook-less-UI retry test. - tests/_session_helpers.py gains the formatting pass the RecordingUI hoist bypassed (the CI lint failure). |
||
|
Patrick Buckley
|
1f9f462b66 |
fix(streaming): gate the dead-segment discard on the backoff surviving the Stop window
Fifth review round — four small correctness edges, none in the retry semantics: - The server-buffer discard now runs only AFTER the backoff survives a Stop: a cancel during the window persists the promoted partial to history, and the idle-state payload (drained from the turn buffer) must carry the same text — discarding first rendered the cancelled turn empty on the dashboard while the transcript had it. Pinned with a real-buffer test; the spinner and fresh segment watermark follow the truncate so a later discard cannot resurrect the dead segment. - stream.retry's dead_content_chars reports THIS death's flushed text only — the Stop-preservation carry retains the previous attempt's partial by design, and logging its length re-attributed the same discarded spend to consecutive retry lines. - The changelog entry for the post-finish-blip rename no longer claims the usage_captured field was dropped; it is emitted and pinned. - The retry suite's module docstring states the shipped finalize contract (stream_end + backoff-gated stream_discarded, never turn_committed) instead of the superseded pair. - RecordingUI is hoisted into tests/_session_helpers next to NullUI — this branch already paid the per-file-fake tax once when a protocol method grew — and a stale deferral sentence is dropped from the fatal-formatter comment. |
||
|
Patrick Buckley
|
7d4d76e097 |
fix(providers): PR review — orphan deltas arm the finish shim, test style
- Orphan argument deltas count as delivered output for the finish_reason_optional shim, exactly as they count as a streamed signal for the terminal harvest: a lax Responses server that never announces items AND never sends a terminal event still delivered its tool call — with the tolerance declared that is a completion, not an IncompleteStreamError. (Review caught the shim/harvest inconsistency the round-9 fix introduced.) - Test style: single import style for the model_turn module, assert on a local instead of a call expression, drop a pass-through lambda. |
||
|
Patrick Buckley
|
8fa0e7a29e |
fix(providers): review round 7 — id-disciplined slots, all-lane finish tolerance, retry backoff
Correctness:
- ToolCallSlotter: a slot whose id is KNOWN never splits on an id-less
delta — on an id-disciplined server new calls arrive with ids, so an
id-less fragment (the call's FIRST name announcement included) is
always a continuation. Round-6 regression: {id} → {name} → {args}
emission split into an unnamed id-bearing call plus a nameless twin.
Also: a name arriving for a slot with no name yet never splits
(args-first emission), and a bare same-name delta after complete
arguments merges as a redundant footer instead of minting a phantom
zero-argument call that would re-run a side-effecting tool.
- finish_reason_optional is honored on every drained lane, not just
Chat Completions: Anthropic shims a missing message_delta
stop_reason + message_stop pair, Responses a missing terminal event
(both with collected blocks riding the shimmed finish) — the
documented capabilities-JSON remediation now works on the
anthropic-compatible/responses-compat gateways it was written for,
matching the retired non-streaming paths' tolerance.
- Responses: an in-band error/response.failed frame arriving AFTER the
terminal event is teardown noise — log and end the stream instead of
raising away a generation already in hand (the in-band twin of
drain_stream's post-finish transport-blip tolerance).
- model_turn drain retries pace like the SDK request retry they
replace: 0.5s base, doubling, ±50% jitter — instant re-issues
re-hit the still-active rate limit/overload and synchronize into
fleet-scale retry bursts.
- Responses slot bookkeeping survives lax servers: slots minted by a
counter (len(dict) collided calls after a duplicate/empty item-id
overwrite), orphan argument deltas route to the most recently
announced call instead of hardwired slot 0.
Cleanup:
- on_tool_call_delta now receives the normalized ToolCallDelta plus the
raw SDK delta — Google's capture accumulates the exact bytes the
mirror sees (the byte-identical extraction no longer exists twice).
- _ArgsScanner feeds only fully id-less slots (its verdict is never
consulted for id'd slots — dominant-case hot path).
- Anthropic retryable set hoisted to a class constant (per-access
frozenset allocation, same pattern already fixed on Responses).
- GoogleProvider class docstring names the hook-based capture instead
of the deleted _extract_tool_calls override.
|
||
|
Patrick Buckley
|
56b7674dfa |
fix(providers): review round 3 — in-band error events, terminal-marker tolerance, adapter-owned de-fusion
Correctness: - Responses _iter_stream handles the SDK's in-band `error` SSE event (ResponseErrorEvent is YIELDED, not raised, and no response.failed need follow): the real API code/message now surfaces — code-gated for retryability like response.failed — instead of the stream exhausting finish-less and hiding the cause behind a retried IncompleteStreamError. - Anthropic message_stop supplies a missing stop_reason: it is a genuine terminal marker, so a compat /v1/messages shim whose message_delta omits stop_reason completes (blocks intact) rather than failing a generation that arrived — tolerance the retired non-streaming default provided, restored without weakening the died-mid-response gate. - A Responses terminal event without its response payload still emits the finish reason its type implies (lax compat servers), losing only usage/blocks rather than the whole result. Dispositions held (documented, not re-coded): the complete-or-error gate stays for finish-less Chat Completions streams — indistinguishable in-band from a died generation, and silent partial-storage is the worse failure; CHANGELOG now names the shape and each provider's accepted terminal markers. supports_streaming deletion and the stream_options wire delta were ruled earlier and keep their release-note remediations. Cleanup: index-degenerate de-fusion MOVED from drain_stream into the chat adapter's iterator (mirroring the Anthropic iterator's index assignment) so the interactive loop is fixed too and the drain returns to a plain mirror of the main-loop accumulator; a parametrized test locks "IncompleteStreamError is retryable" across all six provider lanes instead of trusting per-adapter memory; scripted_anthropic_client joins scripted_chat_client (shared _ScriptedClient class, no function attrs) and the two remaining hand-rolled anthropic closures convert. |
||
|
Patrick Buckley
|
3ffa8b9057 |
fix(providers): review round 2 — complete-or-error drain, code-gated retries, truncation-safe blocks
Correctness (3 confirmed + 2 plausible, all fixed): - drain_stream now raises typed, retryable IncompleteStreamError when a stream exhausts without any finish reason — every adapter emits one on a healthy stream, so its absence means the generation died mid-response behind a cleanly-closing proxy. This restores the retired transport's complete-or-error contract (a half-generated compaction summary was previously returned as finish=stop and stored, silently replacing real history) and DELETES round 1's suffix-info fold: with no finish-less success path there is nothing to classify, so a trailing status ping can never be stored as content either. - Index-degenerate parallel tool calls get distinct slots: a delta whose id differs from its slot's opens a new call (id-less fragments still follow their index's current call), so historical compat servers that emit every parallel call at index 0 no longer fuse distinct calls into concatenated garbage arguments. Result order stays index-sorted (stable) like the retired array parse. - response.failed retryability is code-gated: only transient codes (server_error, rate_limit_exceeded) raise the retryable typed error; deterministic rejections (invalid prompt, image fetch, policy) raise plain RuntimeError and stop retry loops on attempt zero instead of running the full backoff ladder against a doomed request. - Terminal Responses events rebuild provider_blocks from response.output when present: the item being generated at max_output_tokens truncation never receives output_item.done, and storing a reasoning item without its required following item made the next turn's replay a 400. - merge_usage's base case uses dataclasses.replace so a future UsageInfo field can't be silently zeroed on drained lanes. Cleanup: run_abortable_with_deadline bundles the three-point abort wiring (ref + cancel_ref + on_abandon) so it cannot be half-wired — both judges converted; scripted_chat_client hoists the 14 chat-lane fake_create closures (call scripts + .calls recording replace per-test counter cells); fake_chat_stream gains reasoning=, collapsing the reasoning-capture suite's hand-rolled chunk shape; FakeAnthropicBlock hoists the duplicated _Block test class; the class and judge PlantUML diagrams drop the retired create_completion flow. Also converts test_model_registry's agent-model fakes, which returned legacy response objects that iterated as EMPTY streams — they only passed through the old drain's silent finish=stop default, exactly the hazard the new gate exists to catch. |
||
|
Patrick Buckley
|
1e7ad7bcb6 |
feat(providers): one transport — drain create_streaming, retire create_completion (#831)
Every single-shot lane (model_turn: judges, titles, compaction, web-fetch extraction, perception, eval, optimizer) now samples through the provider's streaming entry and accumulates via a shared drain_stream(), deleting create_completion from the Protocol and all three adapters (xai/google inherit). Request shaping can no longer drift between the two consumption styles, and callers keep the exact CompletionResult contract. The drain mirrors the main loop's proven chunk semantics: per-field max-merge for usage (Anthropic splits prompt/completion across message_start/message_delta), tool-call assembly by delta index, provider_blocks from the terminal emission, trailing citation info folded back into content (byte-matching the old format_citations append), mid-stream status pings dropped. Also in this change: - model_turn grows cancel_ref; both judges wire their run_with_deadline abandon paths to a new StreamAbortRef (deadline.py) that closes the SDK stream — a timed-out judge call now aborts its HTTP read instead of pinning a daemon thread until the next upstream chunk. The append hook covers the arrival race, mirroring ChatSession._CancelRef. - Responses streaming gains the response.incomplete terminal handler (truncated runs were mislabeled finish=stop and lost final usage AND collected provider_blocks) and a refusal handler ([Refused: …] content, matching the retired non-streaming rendering). Both also fix the main chat loop, which shared the gaps. - supports_streaming capability flag deleted (zero readers) along with its admin capability tile; o1-era models that reject streaming need a model alias pointing at a current model (release-noted). - Helpers that existed only for the deleted transport go with it: Responses._parse_response, chat/google._extract_tool_calls. Known behavioral deltas (release-noted): OpenAI-compatible servers that ignore stream_options.include_usage stop producing usage rows on these lanes; multiple Anthropic text blocks concatenate without the old "\n" joint (matching the main loop); model_turn lanes no longer risk client read-timeouts on long generations — the reason the Anthropic adapter already drained a stream internally. Tests: new test_drain_stream.py pins the accumulator rules; shared fakes (as_stream, fake_chat_stream, fake_anthropic_stream) migrate 11 suites to the streaming transport, with the task-agent and adapter suites now exercising the real _iter_stream + drain path end to end. |
||
|
Patrick Buckley
|
b0937683ae |
fix(model-turn): apply the #827 phase-1 review round
Behavior fixes, per review + house rules: - Judges no longer pin temperature=0.0 — the lane inherits the model's configured temperature (ModelConfig.temperature via resolve_lane), and model_turn omits the kwarg entirely when nothing resolves. House rule: code never pins a temperature; modern models often misbehave below 1.0, so the model's configuration is the source of truth. This also dissolves the extra_body-overrides-judge-pins collision: operator pins reaching the judge lane is the doctrine working. - Session-fallback judges inherit the session's registry alias (session_model_alias threaded from ChatSession), so the registry- resolved extra_params / replay flag / vLLM attach apply on the default judge.model-unset configuration instead of only on explicit aliases. - Blank-id native lanes are repaired, not dropped: model_turn backfills the manufactured mirror ids into blank-id native client tool blocks pairwise (the #825 1:1 ordering invariant), so thought_signature survives Google's blank-id compat responses and thinking blocks keep their continuity on blank-id locals. Only blank ids are ever written — a provider-assigned id (possibly signature-covered) is never touched — and any pairing mismatch falls back to the #825-converged total drop. - model_turn(mint=...) without wire_id_map now raises: minted ids are unrestorable without the recovery map, and the two parameters were independently optional by accident. - Lane resolution reads ONE defensively-fetched ModelConfig (_get_config_or_none): a registry hot-reload mid-resolution can't mix config generations, and an alias that raced away degrades each facet to its miss behavior instead of aborting a judge constructor into the silent session-model downgrade. Extraction hygiene, per review: - Dead session wrappers deleted (_resolve_server_type, _maybe_synth_reasoning_block, _get_server_compat) and their tests re-pointed at the module functions; the stranded reasoning-types comment and two stale doc pointers cleaned up. - Speculative extra_headers / resolve_attachments pass-throughs dropped from model_turn until a caller lands (phase 2/4 reintroduces them with their lane). - _server_type_of(cfg) is the one reader of server_compat.server_type; the vLLM-attach gate and resolve_server_type both use it, retiring the change-both-readers discipline comment. - dataclasses import hoisted; module docstring restated as the durable contract (grep callers for coverage) instead of a rotting snapshot. - mock_completion_result shared in tests/_session_helpers.py — one definition of "every field the re-ingest reads". |
||
|
Patrick Buckley
|
33865ca9d2 |
fix(reasoning): apply full-stack review findings
Multi-stage /review on the full Phase 1+2+3+4 stack surfaced 9 findings (0 critical, 3 major, 5 minor, 1 nit, 1 uncertain). All applied. Major * perf-1 (session_routes.py:2402): make_history_handler ran sync storage.load_workstream_config inside async def history on the cold- workstream path, blocking the event loop on every dashboard /history request for non-resident workstreams. Every other storage call in the same handler correctly used asyncio.to_thread. Wrap the sync call in asyncio.to_thread (preserving the existing try/except so a DB failure still degrades to the conservative-default branch instead of bubbling out). * q-2 (test_reasoning_audit_log_discipline.py): the security-sensitive test (reasoning text never lands at INFO+ severity) only covered the 4 Phase 1 surfaces. Phase 2 added the strip predicate in AnthropicProvider._convert_messages and Phase 3 added 3 more code paths that touch reasoning text — none guarded. Added 4 parallel tests using the existing capture-and-walk infrastructure: OpenAIResponsesProvider.extract_reasoning_text, OpenAIChatCompletionsProvider.extract_reasoning_text, ChatSession._stream_response (drives the synth-block stamp via a fake reasoning-emitting stream), AnthropicProvider._convert_messages with replay_reasoning_to_model=False (drives the Phase 2 strip predicate). * q-1 (model_registry.py:42): the persist_reasoning flag name implied storage-control but actually gates UI rehydration only — operators flipping it could reasonably expect "stop persisting reasoning" but storage of reasoning bytes happens in provider_data regardless. Renamed everywhere to surface_persisted_reasoning: ModelConfig field, migration 052 column (renaming in-place since 052 is not yet on main), schema, MODEL_DEFINITION_MUTABLE allowlist, _postgresql.py + _sqlite.py CRUD impls, _protocol.py create_model_definition signature, 3 console_schemas Pydantic models, console/server.py admin POST + PUT, model_registry row mapper, history_decoration.py helper parameter, server.py _build_history local var, session_routes.py make_history_handler local var, sdk/events.py HistoryEvent docstring, admin.js form id + override pill label, index.html form input id + UI label + tooltip, coordinator.js (none needed), and every test that referenced the old field name. The admin tooltip now reads "Storage of reasoning bytes is unaffected by this flag — they ride in provider_data regardless" so the decoupling stays explicit at the operator surface. Minor * bug-1 (history_decoration.py:336): dispatcher discriminated on provider_content[0]["type"] only. Anthropic's redacted_thinking blocks (sealed by the safety system) can appear before, after, or interleaved with regular thinking blocks per the API docs. When a redacted block lands first, the dispatcher returned "" and the UI silently lost the surrounding thinking text. Registered "redacted_thinking" as a second key in _BLOCK_TYPE_PROVIDER_FACTORY pointing at the same AnthropicProvider factory — the existing extractor's type=="thinking" filter already correctly skips redacted blocks while walking the full list. Regression test added. * q-3 (_protocol.py:155): replay_reasoning_to_model defaults split across 9 sites — operator-side defaults to False (matches DB server_default), provider-API defaults to True (back-compat with direct callers). Original "pick False everywhere" fix would have silently flipped behaviour for any direct provider caller. Instead documented the intentional bifurcation in the Protocol's create_streaming docstring. * q-4+q-5 (_protocol.py:107 + 3 providers): MAX_REASONING_DISPLAY_BYTES was enforced via Python str slicing which counts code points, not UTF-8 bytes — 4-byte CJK/emoji glyphs would blow past the byte ceiling. Renamed to MAX_REASONING_DISPLAY_CHARS to match actual behaviour. Hoisted the 4-line truncation pattern into a shared _join_reasoning_with_cap helper in _protocol.py; each provider's extractor becomes a single line at the tail. * q-6 (tests/_session_helpers.py): _NullUI + _make_session were duplicated verbatim between test_session_replay_reasoning.py and test_session_synth_reasoning_block.py. Hoisted to a shared tests/_session_helpers.py module (importable, leading underscore so pytest doesn't try to collect it). test_model_registry.py's _make_session has a different signature (registry/model_alias args + _FakeUI) and is not a candidate for sharing. Nit * q-7 (history_decoration.py:286): _make_provider_factory used a dict-as-cell workaround for closure read-only scope. Replaced with the more idiomatic nonlocal pattern. Lint + test gate * ruff check + ruff format -- clean. * mypy -- no issues across all 191 source files. * pytest -m 'not live' -- 6115 passed (3 deselected). Net +5 tests (4 audit-log discipline + 1 redacted_thinking dispatcher). Refinements vs the dedupe output (caught during sanity rendering the report) * perf-1 fix preserved the try/except wrapper. The original "wrap in to_thread" one-liner would have let an OperationalError bubble out instead of degrading to the fallback branch. * q-3 fix explicitly documented the bifurcation rather than collapsing both sides to False. "Pick False everywhere" would silently flip back-compat behaviour for direct provider callers. * q-1 fix included the admin.js:5292 fallback site (m.persist_reasoning !== false) that the original threaded-change list missed. * q-6 fix verified the third _make_session in test_model_registry.py is structurally different (different signature + different UI helper) and intentionally NOT a dedupe target. |