Skip to content

feat: extract shared provider substrate - #19

Merged
zhanghanduo merged 3 commits into
mainfrom
refactor/provider-substrate
Sep 2, 2026
Merged

zhanghanduo merged 3 commits into
mainfrom
refactor/provider-substrate

Conversation

@zhanghanduo

Copy link
Copy Markdown
Collaborator

Summary

  • extract OpenAI Chat/Responses and Anthropic/Bedrock transports
  • share fallback, prompt-cache, protocol selection, and non-blocking stream primitives
  • add injected session-affinity and task pause boundaries
  • share workflow registration and boxed-answer helper

Validation

  • uv run ruff check agent_core tests
  • uv run pyright agent_core
  • uv run pytest -q (924 passed)

Downstream PRs will remain stacked on each product refactor/agent-core branch and pin commit 15996a7.

@zhanghanduo
zhanghanduo force-pushed the refactor/provider-substrate branch 3 times, most recently from 19d2224 to d7d8c96 Compare September 2, 2026 08:40
@zhanghanduo
zhanghanduo force-pushed the refactor/provider-substrate branch from d7d8c96 to 18c3cd2 Compare September 2, 2026 08:47
zhanghanduo and others added 2 commits September 2, 2026 16:58
Collection: the four new provider test files were named provider_*.py, which
pytest's default globs skip — ~1778 lines of tests never ran. Renamed to
test_provider_*.py (+67 tests now collected).

fallback: FallbackEntry.triggers gains a None "unset" state. The field default
was already ("any_error",), so `if not entry.triggers` could only ever fire on
an explicit `()` — i.e. the normalisation existed solely to destroy the
documented hard barrier. None inherits default_triggers; () survives.

anthropic: message conversion no longer raises while replaying durable history
(a raise there kills every later turn in the session, not one request).
Malformed tool arguments degrade to {}, calls missing id/name are dropped, and
a contentless assistant turn is dropped rather than emitting a zero-length text
block the API rejects.

anthropic streaming: capture signature_delta and redacted_thinking so signed
thinking replays like the non-streaming path. Adds StreamDelta.reasoning_blocks
and has the stream assembler prefer it — the flattened text channels cannot
represent a signature. Also reports reasoning_tokens on the streaming path.

protocol_client: reconcile thinking budget with max_tokens (1024 floor and
budget < max_tokens are jointly unsatisfiable below 1025; raise max_tokens
rather than emit a pair the API 400s on).

openai_responses: replay reasoning/function_call in the provider's own order —
the API pairs a reasoning item with the item that follows it, so a multi-call
turn was being replayed under a different pairing.

Consistency: reasoning_tokens uses `is not None` in all three adapters so an
explicit 0 differs from an absent field.

pause_check: recognise mapping-shaped status rows ({"status": "suspended"}),
which never matched and left suspended tasks running silently. Narrowing to str
also removes a TypeError crash path on status-less dicts.

bedrock: fall back to AWS_BEARER_TOKEN_BEDROCK and raise when still empty — the
Bearer override replaces SigV4, so ambient AWS credentials are never consulted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zhanghanduo

Copy link
Copy Markdown
Collaborator Author

Review fixes applied in 59d9c0b

Reviewed the diff and fixed 11 findings. All four CI gates pass: ruff, pyright, pytest (1052 passed), uv build.

The validation line in the PR body needs updating

924 passed is exactly the pre-existing suite. The four new provider test files were named provider_*.py, which pytest's default test_*.py / *_test.py globs skip — there is no python_files override in pyproject.toml and no conftest.py in the repo. Verified: uv run pytest tests --collect-only -q returned 924 with none of them, while running the four paths explicitly gave 68 passed. So ~1778 lines of new provider tests never ran in CI.

Renamed to test_provider_*.py. This also picks up the two session-affinity tests added in 1cd2d76, which were landing in the same skipped file.

Breakdown of the 1052: 924 pre-existing + 67 now-collected provider tests + 61 new regression tests.

Correctness fixes

fallback.py — triggers=() barrier was unreachable. FallbackEntry.triggers already defaulted to ("any_error",), so if not entry.triggers: entry.triggers = self.default_triggers could only ever fire on an explicit (). The normalisation therefore did nothing except destroy the documented hard barrier ("Empty tuple () means 'never fall through'"). A three-tier chain declaring triggers=() mid-chain to stop failover silently fell through to tier 3. The field now has a None "unset" state: None inherits default_triggers, () survives as the barrier. with_provider_stamp only escaped this because a 1-entry chain hits the idx == len-1 re-raise.

anthropic.py — three ways history conversion could permanently wedge a session. These run while replaying durable history, so a raise is not a one-request failure: the offending message is already recorded, and every later turn re-converts it and dies identically.

  • json.loads on model-supplied tool arguments raised on malformed JSON. The OpenAI path forwards arguments as an opaque string, so a truncated call is survivable there but was permanently fatal here. Now degrades to {} (on replay the arguments are historical detail — the tool already ran; the block's identity is what the following tool_result validates against). Non-object JSON is also rejected, since Anthropic's input must be an object.
  • tc["id"] / tc["function"]["name"] were unguarded. Calls missing either are now dropped with a warning — Anthropic requires both, and emitting id: "" is a guaranteed 400.
  • The blocks or [{"type": "text", "text": ""}] fallback emitted a zero-length text block, which the API rejects. Reachable from finish_reason="length" with empty content. Whitespace-only is no safer (trailing-whitespace check as the final message), so the turn is dropped instead — faithful, since there was no assistant output to replay, and safe because the Messages API combines consecutive same-role messages rather than requiring strict alternation.

anthropic.py streaming — signed thinking was unreplayable. content_block_delta handled only text_delta / thinking_delta / input_json_delta. A thinking block's cryptographic signature arrives as its own signature_delta event after the text, and redacted_thinking carries its payload on content_block_start with no deltas at all. Neither fits in the flattened reasoning_content string, so a streamed extended-thinking turn produced reasoning that thinking_format="content_block" could not replay — while the non-streaming path replayed it correctly.

Since docs/provider-substrate-boundary.md states AgentCore owns "Anthropic Messages and Bedrock, including signed thinking replay", and the stream assembler lives in this repo too, this is fixed end-to-end rather than degraded: StreamDelta gains a reasoning_blocks channel, the client rebuilds the verbatim block list in the provider's emission order (keyed by event index), and the assembler prefers it as content when present. The lstrip / think-tag normalisation is deliberately not applied to those blocks — replay needs the bytes Anthropic signed.

Worth noting this was only latent because the host forces non-streaming for protocol: anthropic; AgentCore ships the client without that guard.

protocol_client.py — thinking budget could exceed max_tokens. max(1024, min(budget, max_tokens - 1)) is unsatisfiable below max_tokens=1025: with max_tokens: 512, min(8192, 511) = 511 then max(1024, 511) = 1024, i.e. a budget larger than the response cap, violating Anthropic's budget_tokens < max_tokens and 400ing every call. The 1024 floor is the API's own minimum and cannot be negotiated, so _enabled_thinking_budget() raises max_tokens to budget + 1 and logs the adjustment.

openai_responses.py — assistant replay reordered reasoning against calls. _to_responses_input emitted all reasoning items, then the joined text, then all function_call items. The Responses API binds a reasoning item to the output item that follows it, so a turn producing reasoning → call_a → reasoning → call_b was replayed as reasoning, reasoning, call_a, call_b — a different pairing than it was generated under. The parser now records function_call blocks in place in the verbatim list (model_profile's content_block parser ignores unknown block types and preserves the list, so this rides along without touching visible text or thinking extraction) and replay walks that order. Adjacent text blocks are still joined — a concatenation within one position, not a reorder. History predating this falls back to appending from tool_calls, and tc["id"] / name are guarded the same way as the Anthropic path.

pause_check.py — mapping-shaped status rows never paused. getattr(value, "status", value) returns the dict itself for {"status": "suspended"} (a very common shape for a DB row), which never matches _STOP_STATUSES, so a suspended or aborted task kept running indefinitely with no log line. Added a Mapping branch (first) and a log line on pause. Narrowing the result to str also removes a pre-existing crash path: a status-less dict made in frozenset raise TypeError, turning a fail-open check into a crash. TaskStatus is a StrEnum, so plain strings need no conversion.

anthropic.py Bedrock — bare Authorization: Bearer on a missing key. The _prepare_request override replaces SigV4 outright, so ambient AWS credentials are never consulted — but api_key or "" produced an empty Bearer header and a 401 that looks like a bad key rather than a bypassed auth path. Now falls back to AWS_BEARER_TOKEN_BEDROCK and raises ValueError when still empty.

Consistency

reasoning_tokens zero handling. openai_responses used if reasoning:, dropping an explicit reasoning_tokens: 0, while the sibling cached_tokens (and openai_chat._usage_dict) use is not None — the exact ambiguity openai_chat's docstring says it was fixing. All three adapters now use is not None; _anthropic_reasoning_tokens returns int | None so "not reported" and "reported zero" are distinguishable.

Streamed Anthropic usage omitted reasoning tokens. The terminal StreamDelta passed only four arguments to _anthropic_usage_dict, so reasoning_tokens was always absent on the streaming path while _to_llm_response populated it. Now read off message_delta.usage.output_tokens_details.

Notes on the two commits pulled in

The branch was force-pushed, so this work was rebased with --onto onto the new head.

Reconciled with the rewritten base. protocol_of now normalises unknown values via is_wire_protocol, which made the typo warning I had added to build_protocol_client dead code — after upstream normalisation a typo like anthropc becomes more silent, not less. Moved the warning into protocol_of, where an unusable value (wrong type, or a string naming no known protocol) is logged; an absent or empty value is the documented default and stays quiet so builds don't get warning noise.

One gap in 1cd2d76. The implementation looks right — task-derived affinity is no longer frozen into the SDK's default_query and is instead scope-checked per call, with both branches covered by tests. But configure_session_scope_resolver and SessionScopeResolver were not exported from either openai_chat.__all__ or the agent_core.providers facade, and the facade is how hosts configure these global hooks (configure_session_query_resolver is already there). The documented wiring path couldn't reach the new hook. Exported both, with a facade-reachability test.

One thing I looked at and judged fine: a scope resolver returning "" at construction freezes the affinity into SDK defaults permanently. The host controls both the resolver and the headers, and returning "" is the host asserting the affinity is static — test_static_openai_affinity_survives_scope_changes pins that contract.

@zhanghanduo
zhanghanduo merged commit 94b05b3 into main Sep 2, 2026
1 check passed
@zhanghanduo
zhanghanduo deleted the refactor/provider-substrate branch September 2, 2026 09:12
@zhanghanduo

Copy link
Copy Markdown
Collaborator Author

Note for anyone pinning this downstream: the description's "pin commit 15996a7" is a typo — that commit is not on main. The right pin is 94b05b3ee2198d2f30426522fe9bc32c4e585efe, the merge commit of this PR, which includes the review-fix commit 59d9c0b.

Worth flagging because the intermediate commits are a trap. 18c3cd2 and 1cd2d76 predate 59d9c0b, and at those revisions the four new provider test files were still named provider_*.py — outside pytest's default globs, so ~1778 lines of provider tests never ran. A downstream pinned there gets the substrate without any of the seven defects 59d9c0b fixed (the () hard barrier normalised away, Anthropic message conversion raising while replaying durable history, lost thinking signatures on the streaming path, pause_check never matching a mapping-shaped status row, …) and without the test coverage that would have caught them.

ApodexHarness#515 is now pinned at 94b05b3. Full suite there: 7152 passed, 72 skipped, 3 xfailed. The bump cost exactly one test change — test_default_triggers_apply_to_unconfigured_entries had used triggers=() to mean "unconfigured", so it was asserting the pre-59d9c0b behaviour; it is split in two now, one test per side of the None / () distinction.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant