Experimental Native Protocol Rewrite

[Mirrowel]

Experimental Native Protocol Roadmap

This branch is for a long-running experimental rewrite that makes native protocol support the first-class extension point of rotator_library, while preserving the existing credential rotation, quota, fair-cycle, session tracking, and provider plugin strengths.

Operating Rules

• Work only on the experimental branch.
• Keep all repository work inside C:\Projects\test\LLM-API-Key-Proxy and child paths.
• Treat commits as checkpoints. A phase may contain many commits.
• Commit messages must include a body describing what changed, why, tests run, and follow-up considerations.
• Do not commit phase reports written for the user unless explicitly requested. Planning docs under docs/experimental/ are committed.
• Before each phase implementation, first produce a fresh exhaustive phase plan in conversation text, based on the current code state. Only after that plan is settled should it be written to docs/experimental/phase-N-*.md.
• After each phase implementation, call both explore and explore-heavy agents to review the work against the phase plan, external reference areas, and current proxy behavior. Fix findings and re-review as needed.
• Keep LiteLLM as a fallback path for protocols/providers that are not natively covered yet. Native protocol support should be preferred when available.

Strategic Goal

The target architecture is:

client API request
  -> protocol parse into unified representation
  -> field-cache injection
  -> adapter chain
  -> provider override hooks
  -> provider-native request build
  -> provider execution and credential rotation
  -> provider-native response/stream parse
  -> field-cache extraction
  -> adapter chain
  -> protocol formatting for the client
  -> transaction logging for every transform state

Providers should be able to declare an existing protocol and only override the parts that are genuinely provider-specific. A custom provider should usually be configurable through protocol choice, adapters, field-cache rules, auth strategy, and model options rather than requiring a large bespoke provider implementation.

Priority Order

1. Native protocol foundations, unified types, transformers, adapters, and field-cache rules.
2. OpenAI Responses API support, including future WebSocket extension points.
3. Provider work following the protocol layer: Claude Code, Codex, Copilot, Antigravity, and Gemini CLI parity review.
4. Routing and fallback groups, with optional target-group selectors later.
5. Retry, provider/model cooldown, and failover cleanup.
6. Protocol-aware quota, usage, and cost normalization.
7. Streaming library hardening: SSE now, WebSocket-ready later.
8. Config polish using .env and optional JSON. No SQLite dependency for now.
9. Extensive staged tests and review-agent verification.

Non-Goals For This Branch

• Do not make the proxy a full multi-user admin product yet.
• Do not require SQLite or Postgres for the main feature set.
• Do not remove LiteLLM before native coverage exists.
• Do not replace the existing UsageManager, fair-cycle, custom caps, or evidence-based SessionTracker.
• Do not port frontend/UI work from the external reference gateway.

Current Strengths To Preserve

• Credential-level rotation and priority-aware selection.
• Fair cycle and custom caps.
• Windowed quota tracking and quota groups.
• Evidence-based session tracking with compaction handling.
• Provider plugin discovery.
• Gemini CLI provider behavior unless a reviewed change is clearly better.
• Resilient file/JSON state writing.
• Dynamic OpenAI-compatible provider discovery.

Reference Gateway Ideas To Import Carefully

• Unified protocol/transformer style.
• Adapter registry and configurable provider/model adapters.
• Target groups and direct routing syntax, adapted into fallback-first routing.
• Responses API transformer and storage concepts.
• Stream TTFB/stall detection concepts, implemented with Python-native async primitives.
• Provider/model cooldown and retry-history concepts.
• Usage/cost normalization and provider-reported cost extraction.
• Broader provider support patterns for Claude Code, Codex, Copilot, and Antigravity.

Phase Index

1. Protocol Core.
2. Transform Pass Logging.
3. Adapter and Field Cache System.
4. Responses API and WebSocket-Ready Transport Shape.
5. Provider Protocol Overhaul.
6. Routing and Fallback Groups.
7. Retry/Cooldown/Failover Cleanup.
8. Streaming Library Upgrade.
9. Usage, Quota, and Cost Accuracy.
10. Config Polish.

Each phase may be subdivided if implementation scope becomes too large.

Completeness Matrix

This matrix exists so the branch does not lose any requested scope while phases evolve. The phase plans are still refreshed before implementation, but every item below must remain accounted for.

Requested area	Planned coverage
Protocols are priority #1	Phases 1 and 4 create native protocol foundations and Responses support before provider work.
Protocols are bases, not gospel	Phase 1 requires override-friendly protocol methods, subclassing, copy/mutate registration, and provider-specific overrides.
Move away from LiteLLM	Phase 1 adds a litellm_fallback protocol path; later providers should prefer native protocols and use LiteLLM only for unsupported coverage.
Add protocols automatically like providers	Phase 1 adds protocol auto-discovery and registry behavior modeled after provider discovery.
Cover current providers and reference providers	Phase 1 protocols must cover shapes used by current providers; Phase 5 covers Claude Code, Codex, Copilot, Antigravity, and Gemini CLI parity.
Responses API is very needed	Phase 4 is dedicated to Responses, previous_response_id, storage, SSE, and WebSocket-ready transport shape.
WebSocket support later	Phases 1, 4, and 8 require transport separation so WebSocket can be added without rewriting protocol logic.
Adapters/transformers tied to protocols	Phases 1, 2, and 3 define protocol parse/build plus transform tracing, adapter registry, and field-cache rules.
Cache and return provider fields	Phase 3 implements configurable extraction/injection rules for request, response, and stream fields with scope and mode controls.
Reasoning content and similar fields	Phase 3 explicitly covers reasoning content, thinking signatures, prompt cache keys, response IDs, and provider session IDs.
Return all possible or last user/assistant use	Phase 3 modes include last, all, last_user_turn, last_assistant_turn, and per_tool_call.
Per-model custom provider behavior	Phases 3, 5, and 10 cover provider/model field cache rules, adapters, model options, and optional JSON config.
Transaction logging after every transform	Phase 2 adds ordered request, response, and stream transform trace passes and integrates them with transaction logging.
Comments, docstrings, and key decisions	All implementation phases require docstrings for public abstractions and comments for non-obvious transform, protocol, and future-extension decisions.
Providers are priority #2	Phase 5 follows protocol foundations with Claude Code, Codex, Copilot, Antigravity, and Gemini CLI parity review.
Antigravity comparison	Phase 5 explicitly compares the reference Antigravity behavior against src/rotator_library/providers/_retired/.
Routing is interesting	Phase 6 implements fallback chains first, with target-group selectors later if useful.
Fallback groups preferred over target groups	Phase 6 starts with ordered fallback groups and only adds target-group-style selectors after that base works.
Retry/cooldown/failover cleanup	Phase 7 makes provider/model cooldown real, adds retry history, backoff, retry-after precedence, and success reset.
Quota/usage/cost improvements	Phase 9 adds protocol-aware normalizers, provider-reported cost extraction, structured cost fields, and checker abstractions while keeping existing usage engines.
Streaming as library capability	Phase 8 hardens streaming below the proxy route layer with TTFB, TTFT, stall detection, cancellation, and transport-aware stream events.
Config via env/json, no SQLite	Phase 10 adds optional JSON config with env overrides and validation. SQLite remains out of scope.
Multi-user proxy later	The branch keeps multi-user/admin features as a future expansion and only preserves extension points where natural.
Exhaustive tests in stages	Every phase requires tests alongside implementation and phase-end review by both explore and explore-heavy.
Reports are for the user, not git	06-phase-workflow.md says planning docs are committed, but phase reports are not committed by default.

Code Quality Expectations

• Public protocol, adapter, transport, field-cache, and provider-extension classes must have docstrings that explain intent, override points, and future expansion hooks.
• Non-obvious transformations must have comments explaining why data is changed, preserved, reordered, or intentionally dropped.
• Lossy protocol conversions must be documented at the conversion site.
• Future WebSocket, target-group, and multi-user extension seams should be noted in comments where they affect today's design.
• Tests should prefer golden fixtures for protocol shapes and focused unit tests for transform edge cases.

[mirrobot-agent]

Starting my review of the third-pass audit remediation changes — covering protocol output correctness, native operation guardrails, streaming hardening, Responses runtime improvements, cost accounting, routing parity, and much more across ~100 files. I'll analyze each subsystem and report back with a bundled review.

[mirrobot-agent]

Overall Assessment

This is a massive third-pass audit remediation that addresses findings from the explore and explore-heavy reviewers across all 10 phases of the experimental rewrite. The changeset spans ~9,000 lines across 100 files and is impressively well-scoped — each change maps back to a specific finding in third-pass-audit-findings.md.

The code quality is high. The streaming hardening (TTFB/stall timeouts, heartbeats, upstream close safety, CancelledError handling) is thorough and consistent across both the chat-completions StreamingHandler and the Responses stream_events path. The protocol output correctness fixes (OpenAI Chat/Responses usage formatting, legacy function_call handling, Ollama format_response, native operation guardrails) are all well-tested. The cost accounting improvements (provider-reported cost preservation, cache-write double-count fix, SSE cost comment parsing, structured cost breakdowns) are comprehensive and correctly prioritized.

Status of Previous Feedback

All 12 issues from the initial review and 5 suggestions from the follow-up have been addressed in this changeset:

• service.py CancelledError: Now caught by the Responses finally block and the Anthropic wrapper's finally block.
• Streaming adapter chain gap: The NativeProviderExecutor.stream() now runs stream-event adapter chains and extracts usage/cost.
• Bare except Exception: pass: Replaced with structured error handling and _can_retry_stream_after_error with explicit emitted_output tracking.
• Multi-turn tool use: _parent_output_to_messages now handles function_call and custom_tool_call items, and _message_to_chat handles tool role messages.
• Auth header mismatch: ClaudeCodeProvider now supports auto/x-api-key/bearer modes via CLAUDE_CODE_AUTH_HEADER.
• Hard litellm dependency: Not directly addressed in this diff, but the cost system now has provider-reported cost that bypasses litellm pricing — worth verifying this is tracked.

Key Suggestions

• Model-scoped cooldown bypasses failure-history threshold — contradicts Phase 7c plan (see inline)
• finally block recovery can raise — acquire_task.result() can replace the original exception (see inline)
• Polling logic duplication — next_upstream_chunk and acquire_upstream_stream share ~90% of their structure (see inline)
• Disconnect task leak — if is_disconnected() raises, the task is never cleaned up (see inline)
• Gemini cache-write double-count inconsistency — OpenAI path fixed but Gemini path not (see inline)
• Private _save_to_disk coupling — fragile dependency on provider cache internals (see inline)

Architectural Notes

The new streaming runtime settings system (get_stream_runtime_settings()) is well-designed — lazy-loaded via _retry_settings() to avoid import cycles, with env overriding JSON. The routing attempt history (_append_routing_attempt_history) is a solid observability addition that correctly sanitizes error types and tracks timing. The native provider cross-protocol formatting (client_protocol_name on NativeProviderContext) is a clean solution to the "responses formatted in provider protocol" blocker.

One architectural observation: the Responses streaming path now has its own independent polling/timeout/heartbeat implementation rather than reusing the StreamingHandler infrastructure. This is understandable given the different event model (Responses events vs SSE chunks), but it means streaming policy changes must be applied in two places.

This review was generated by an AI assistant.

[mirrobot-agent]

This hard early return for scope == "model" bypasses the failure-history threshold check entirely:

if scope == "model":
    return ProviderCooldownDecision(True, duration=duration, reason="model_capacity_cooldown", scope=scope, model=model, backoff_level=backoff_level)

This means a single server_error without retry_after will always start a model-scoped cooldown immediately, even if it's the first transient failure. The Phase 7c plan states: "No-retry_after server_error and api_connection should start cooldown only after FailureHistory.backoff_for() crosses the configured threshold." The provider-scoped path (below) correctly checks the threshold, but the model-scoped path does not.

If this is intentional (model-capacity errors are more actionable than generic transients), consider adding a comment explaining the policy difference. Otherwise, the model-scoped path should also check backoff_level > 0 before returning True.

Suggested change

	if error_type in {"server_error", "api_connection"} and default_duration >= provider_cooldown_min_seconds:
	backoff_level = 0
	duration = int(default_duration)
	if scope == "model":
	return ProviderCooldownDecision(True, duration=duration, reason="model_capacity_cooldown", scope=scope, model=model, backoff_level=backoff_level)
	if failure_history is None:
	return ProviderCooldownDecision(False, reason="missing_failure_history", scope=scope, model=model if scope == "model" else None)
	backoff = failure_history.backoff_for(provider=provider, error_type=error_type, scope=scope, model=model if scope == "model" else None, default_duration=duration)
	if scope == "model":
	if backoff_level <= 0 and failure_history is not None:
	return ProviderCooldownDecision(False, reason="transient_backoff_threshold_not_met", scope=scope, model=model)
	return ProviderCooldownDecision(True, duration=duration, reason="model_capacity_cooldown", scope=scope, model=model, backoff_level=backoff_level)

[mirrobot-agent]

This recovery path in the finally block can itself raise an exception, replacing the original one:

if chat_stream is None and acquire_task is not None and acquire_task.done() and not acquire_task.cancelled():
    chat_stream = acquire_task.result()  # Can raise!

If acquire_task completed with a ResponsesServiceError (e.g., from a TTFB timeout), calling .result() will re-raise that error inside the finally block, masking whatever exception triggered the finally in the first place.

Suggested change

	yield ResponsesStreamEvent("response.failed", failed)
	self._trace(transaction_logger, "stream_done_event", {"raw": "done"}, direction="stream", stage="final", metadata={"transport": transport, "failed": True})
	yield ResponsesStreamEvent("done", {}, terminal=True)
	finally:
	if chat_stream is None and acquire_task is not None and acquire_task.done() and not acquire_task.cancelled():
	try:
	chat_stream = acquire_task.result()
	stream_iterator = chat_stream.__aiter__()
	except Exception:
	chat_stream = None
	await cancel_task(pending_next_task)
	if chat_stream is None and acquire_task is not None and acquire_task.done() and not acquire_task.cancelled():
	try:
	chat_stream = acquire_task.result()
	stream_iterator = chat_stream.__aiter__()
	except Exception:
	chat_stream = None

[mirrobot-agent]

next_upstream_chunk (~90 lines) and acquire_upstream_stream (~55 lines) share nearly identical polling/disconnect/timeout/heartbeat logic. The structural pattern is the same: create task -> loop with asyncio.wait -> check disconnect -> check timeout -> emit heartbeat -> get result.

Any future change to the polling pattern (e.g., adding a new policy check) must be applied in both places. Consider extracting a shared _poll_upstream_async(task_factory, *, first: bool) helper that both call sites use, with the task factory being the only difference (stream_iterator.__anext__ vs client.acompletion).

[mirrobot-agent]

If request.is_disconnected() raises an unexpected exception, the disconnect_task created via asyncio.create_task(request.is_disconnected()) will never be awaited or cancelled. Python 3.9+ warns about unretrieved task exceptions, and Python 3.12+ logs Task was destroyed but it is pending! warnings.

Consider wrapping the disconnect task creation and cleanup in a try/finally pattern within the wait loop, or cancelling it explicitly in all exit paths (including the except Exception re-raise below).

Suggested change

	done, _ = await asyncio.wait(wait_tasks, timeout=wait_seconds)
	if disconnect_task is not None:
	if disconnect_task in done and disconnect_task.result():
	stream_cancelled = True
	next_task.cancel()
	with contextlib.suppress(asyncio.CancelledError, StopAsyncIteration):
	await next_task
	await close_upstream("client_disconnect")
	return
	if not disconnect_task.done():
	disconnect_task.cancel()
	with contextlib.suppress(asyncio.CancelledError):
	await disconnect_task
	if next_task in done:
	chunk = next_task.result()
	break
	timeout_error = _stream_timeout_error(monitor, stream_settings)
	if timeout_error:
	next_task.cancel()
	with contextlib.suppress(asyncio.CancelledError, StopAsyncIteration):
	await next_task
	await close_upstream(timeout_error[0], force=True)
	disconnect_task = None
	try:
	if request is not None:
	disconnect_task = asyncio.create_task(request.is_disconnected())
	wait_tasks.add(disconnect_task)
	done, _ = await asyncio.wait(wait_tasks, timeout=wait_seconds)
	if disconnect_task is not None:
	if disconnect_task in done and disconnect_task.result():
	stream_cancelled = True
	next_task.cancel()
	with contextlib.suppress(asyncio.CancelledError, StopAsyncIteration):
	await next_task
	await close_upstream("client_disconnect")
	return
	if not disconnect_task.done():
	disconnect_task.cancel()
	with contextlib.suppress(asyncio.CancelledError):
	await disconnect_task
	finally:
	if disconnect_task is not None and not disconnect_task.done():
	disconnect_task.cancel()
	with contextlib.suppress(asyncio.CancelledError):
	await disconnect_task

[mirrobot-agent]

The OpenAI-like path now correctly subtracts both cache_read and cache_write from prompt_tokens to avoid double-counting:

input_tokens = max(0, prompt_tokens - cache_read - cache_write)

However, _from_gemini_usage still only subtracts cache_read:

input_tokens=max(0, prompt_tokens - cache_read)

If a Gemini provider starts reporting cache_write_tokens (or cachedWriteContentTokens), those tokens would be double-counted in the normalized total. The Gemini protocol already parses cache_write_tokens from the response, so this is a latent consistency gap rather than a hypothetical.

[mirrobot-agent]

This directly accesses a private method on the provider cache:

flush = getattr(self._cache, "_save_to_disk", None)
if callable(flush):
    await flush()

If the provider cache implementation refactors _save_to_disk to a different name (e.g., _flush_to_disk, _persist), saves will silently stop flushing to disk, and the Responses store will appear to lose data between restarts.

Consider either: (a) adding a public flush() or sync() method to the cache interface that this code calls, or (b) adding a comment documenting the coupling so future maintainers know to update this call site.

[greptile-apps]

Too many files changed for review. (231 files found, 100 file limit)

Bypass the limit by tagging @greptile-apps to review.

[mirrobot-agent]

Follow-up Review: Phase 10c Config Completion

This incremental changeset delivers Phase 10c — startup secret masking, provider runtime config wiring (protocol/adapter/field-cache/quota-groups from JSON), pricing tolerance for invalid env values, routing target validation, cost accounting double-count fix, and the bot-setup workflow rework. Well-scoped with good test coverage (new tests for provider runtime config, startup masking, pricing fallback, routing validation, and cost accounting).

Assessment of New Changes:

• Startup secret masking (startup_display.py, main.py): Clean implementation. The mask_secret_for_display helper correctly redacts short values fully and exposes only edge fragments for longer ones — a solid improvement over printing the raw PROXY_API_KEY.
• Cost accounting fix (accounting.py): The _sum_cost_breakdown change correctly prevents double-counting when both upstream_inference_cost (total) and its split fields are present. Good test coverage with test_upstream_inference_total_wins_over_split_fields.
• Provider runtime config wiring (provider_interface.py, config/experimental.py): The JSON override layer is well-designed — provider declarations remain the base, JSON config overrides, and env vars take final precedence for quota groups. The subclass streaming override pattern (check JSON → delegate to base or return False) is a reasonable safety gate. Test coverage is thorough.
• Pricing tolerance (config/experimental.py): The try/except ExperimentalConfigError: continue in _pricing_from_env correctly makes invalid values non-fatal while preserving valid components.
• Routing validation (routing/config.py): Direct route targets are now validated via parse_route_target at config load — good early failure.
• Bot-setup rework (.github/actions/bot-setup/action.yml): Moving model inputs to env: context (avoiding ${{ }} interpolation in run script) and adding chmod 600 on the generated config are both security improvements. The jq -c . validation on the base config is a good guardrail.

Performance Concern (see inline comments):

The new _get_runtime_config() on ProviderInterface loads the full JSON config from disk on every call, with no caching. Since it's invoked from 6+ provider methods that are each called multiple times per request, a single request path can trigger 5+ file reads + JSON parses + validations. Two additional redundancy points compound this (double validation, throwaway config round-trip for field cache parsing).

Status of Previous Feedback:

The 6 suggestions from the third-pass review remain unaddressed in this incremental diff (none of the affected files — retry_policy.py, responses/service.py, client/streaming.py, responses/store.py, and the Gemini _from_gemini_usage path in accounting.py — were modified here). The most impactful still-open items are the model-scoped cooldown threshold bypass, the finally block recovery that can mask exceptions, and the disconnect task leak in streaming.

This review was generated by an AI assistant.

[mirrobot-agent]

Performance concern: _get_runtime_config() triggers load_experimental_config() on every invocation — a full file read + JSON parse + validation cycle. This method is called from get_protocol_name, get_adapter_names, get_adapter_config, get_field_cache_rules, supports_native_streaming, and _get_effective_quota_groups. A single call to should_use_native_protocol() alone triggers 3+ disk reads (it chains supports_native_streaming → _get_runtime_config, then get_protocol_name → _get_runtime_config, then supports_native_operation → get_protocol_name → _get_runtime_config).

Since the JSON config file rarely changes at runtime, consider memoizing the config load per process (or with a short TTL/mtime check).

Suggested change

	def _get_runtime_config(self, model: str = "") -> Any:
	"""Return optional JSON runtime config for this provider.
	The helper keeps config loading lazy so provider imports do not depend on
	the experimental config layer during startup discovery.
	"""
	from ..config.experimental import get_provider_runtime_config
	return get_provider_runtime_config(self._provider_config_key(), model)
	_runtime_config_cache: Any = None
	def _get_runtime_config(self, model: str = "") -> Any:
	"""Return optional JSON runtime config for this provider.
	The helper keeps config loading lazy so provider imports do not depend on
	the experimental config layer during startup discovery.
	"""
	if type(self)._runtime_config_cache is not None:
	return type(self)._runtime_config_cache
	from ..config.experimental import get_provider_runtime_config
	type(self)._runtime_config_cache = get_provider_runtime_config(self._provider_config_key(), model)
	return type(self)._runtime_config_cache

[mirrobot-agent]

This _validate_provider_sections({provider: raw}) call is redundant: load_experimental_config (line 127) already calls _validate_provider_sections on ALL providers, including this one. Since get_provider_runtime_config is called on every provider method invocation (see the caching concern on ProviderInterface._get_runtime_config), this re-runs protocol/adapter registry lookups unnecessarily. Consider removing this line since the config was already validated at load time.

[mirrobot-agent]

_configured_provider_field_cache creates a throwaway ExperimentalConfig via load_config_from_mapping just to call parse_field_cache_rules. This triggers another _reject_secret_keys scan and _validate_provider_sections pass on data that was already validated when get_provider_runtime_config loaded the full config. Consider refactoring parse_field_cache_rules to accept raw rule data directly, or extracting the rule-parsing logic into a helper that doesn't require a full config round-trip.

[mirrobot-agent]

Follow-up Review: Phase 11 Session-Tracking Hardening

This increment is large and mostly lands Phase 11 — a substantial rewrite of session inference (structural compaction detection, replay/context bindings, global logical session domains, schema-3 persistence), plus the Responses-API unforgeable access-capability model, retiring the Gemini CLI OAuth provider, DiffusionGemma reasoning handling, the no-reset quota exhaustion policy, streaming response-evidence hardening, and auth-header redaction in the raw/transaction loggers.

Assessment of New Changes:

• Session-tracker rewrite is well-reasoned and conservatively scoped. Compaction now requires structural replacement of more than half the parent high-water history (_retained_history_ratio), probes are restricted to early user/system/developer roles, unmarked summaries need ≥2 distinct response events plus request-side evidence, and authoritative identity overrides replay/context bindings. The eviction ranking (_anchor_eviction_key) keeping compaction_context/compaction_replay above ordinary evidence is a nice touch. Test coverage is exceptional (119+ tests including restart, concurrency, and stateful multi-provider simulations).
• Responses capability tokens are a real security improvement. Composite (domain, response_id) storage, per-response random nonces with only the hash persisted, hmac.compare_digest verification, recursive credential redaction from stored requests and traces, and legacy-entry migration/rejection all close the forgeable-deterministic-header gap. The streaming in-progress state correctly carries scope_access_hash so it is immediately capability-accessible.
• Auth-header redaction (_redact_headers, _make_json_safe) and the move to datetime.now(timezone.utc) are solid, low-risk hardening.
• No-reset quota policy is a clean, opt-in fallback with good env validation and per-provider defaults.

Key Suggestions (see inline):

• Ad-hoc bundle: domains now each mint a dedicated, never-evicted UsageManager + usage file — a latent unbounded-growth path under high-cardinality private traffic.
• The no-reset policy guard relies on implicit and/or precedence; one explicit pair of parens would make it regression-proof.
• The DiffusionGemma model list is duplicated between the streaming handler and nvidia_provider.py.

Nitpick: The disable_scope policy name implies whole-scope disabling, but _handle_no_reset_quota_exhaustion just applies a (very long) model/group cooldown identical to the cooldown path — the only difference is the 1-year default duration. If true scope-level disabling isn't planned, a clearer name (e.g. cooldown_long) would reduce confusion.

Status of Previous Feedback:
The third-pass items remain unaddressed in this diff (none of retry_policy.py, the responses/service.py streaming poll/finally block, or usage/accounting.py's Gemini cache-write path were touched here, and responses/store.py:119 still reaches into the private _save_to_disk). Still-open: model-scoped cooldown bypassing the failure-history threshold, acquire_task.result() able to mask the original exception in the finally block, the duplicated next_upstream_chunk/acquire_upstream_stream polling logic, the disconnect-task leak in client/streaming.py, the Gemini cache-write double-count, and the private _save_to_disk coupling. None are blockers for this experimental branch, but the auth/streaming ones are worth circling back to.

Overall Status: Strong, well-tested increment. No blocking issues; the inline items are non-blocking improvements.

This review was generated by an AI assistant.

[mirrobot-agent]

Isolating each ad-hoc bundle: domain into its own usage manager (and its own classifiers/bundle_<hash>/usage_<provider>.json file) is the right call for correctness, but UsageManagerRegistry.managers has no eviction. Under high-cardinality ad-hoc traffic (callers passing unique api_keys + private=true per request), every distinct bundle mints a long-lived UsageManager plus a new on-disk usage file, so memory and file counts grow without bound for the life of the process.

Named classifiers are bounded by the operator, but ad-hoc bundles are not. Consider an LRU/TTL cap (or a dedicated registry for ad-hoc bundles) so stale private-bundle managers and their files are reaped after idle.

[mirrobot-agent]

This guard relies on Python's and binding tighter than or:

policy == "warn_only" or configured_duration <= 0 and policy == "cooldown"

parses as warn_only or (duration <= 0 and cooldown). That happens to be intended (a cooldown policy with zero duration falls back to warn-only, while disable_scope with zero duration still reaches the 1-year default below), but it is easy to misread and brittle if the terms are ever reordered. Making the grouping explicit prevents a future regression.

Suggested change

	if policy == "warn_only" or configured_duration <= 0 and policy == "cooldown":
	if policy == "warn_only" or (configured_duration <= 0 and policy == "cooldown"):

[mirrobot-agent]

_MIXED_REASONING_CONTENT_MODELS duplicates the DiffusionGemma identity already declared as DIFFUSION_GEMMA_MODEL_EXACT in nvidia_provider.py. The two lists must stay in sync manually, and model.lower() in _MIXED_REASONING_CONTENT_MODELS only matches two hard-coded prefixed forms, so a differently-prefixed alias silently skips the reasoning/content split.

Consider exposing the canonical model set from the provider (e.g. a shared constant or a provider hook like needs_mixed_reasoning_split(model)) so there is a single source of truth.

[mirrobot-agent]

Starting my review of the Protocol Interoperability correction — the new canonical streaming converter, cross-protocol request/response formatting, Gemini client routes, structured error propagation, and dynamic provider config. This is a large architectural increment; I'll analyze each subsystem and report back with a bundled review.

[mirrobot-agent]

Overall Assessment

This increment delivers the Protocol Interoperability correction — making client, provider, and output protocols independently selectable across all four generative APIs (OpenAI Chat, Responses, Anthropic Messages, Gemini). This is the most architecturally significant change since the initial protocol layer was introduced.

Strengths:

• The new canonical.py module is excellent — it centralizes stop-reason mapping, tool-choice normalization, structured-output conversion, tool-result resolution, and message coalescing into one well-documented location that all four protocols share. The de-duplication logic in message_tool_calls / message_reasoning correctly handles the dual representation (message-level + content-block-level).
• The validation.py module enforces meaning-changing field preservation before transport: it rejects cross-protocol conversions that would silently drop tool calls, media without identity, unsupported modalities, or provider-bound Responses controls. This is the right place to fail early.
• StructuredAPIResponseError with to_protocol_payload() is a clean solution for formatting terminal errors in the selected client protocol without leaking internal state.
• The field-cache store improvements (bounded appends with max_values/max_bytes, LRU eviction on InMemoryFieldCacheStore, atomic update_async on ProviderCacheFieldStore) directly address the previously flagged unbounded-growth concerns.
• New Gemini client routes (generateContent, streamGenerateContent, countTokens) with proper auth handling (x-goog-api-key, query key, and Bearer fallback) close the "no public Gemini client route" gap.
• The config validation for custom providers (_configured_api_base, _configured_endpoint_paths, _configured_auth_mode) is thorough — rejecting credential-bearing URLs, fragments, and cross-origin endpoints.

Architectural Notes

The streaming architecture now has a multi-stage conversion pipeline:

native provider SSE → native parse_stream_event → canonical event
  → Chat format_stream_event → Chat SSE string (operational format)
  → Chat parse_stream_event → canonical event
  → output format_stream_event → client SSE string

This round-trips through Chat SSE as the operational lingua franca, which is architecturally clean but means canonical events are serialized and re-parsed twice for cross-protocol output. The _stream_native_response double-parse finding below is a specific instance of this.

Key Suggestions

• Gemini stream usage drops cache_write_tokens from promptTokenCount — see inline.
• Double stream frame parsing in Responses native streaming — see inline.
• Three-way merge in _canonical_request_for_native can silently strip source-native fields on fallback — see inline.
• _in_band_error_payload may false-positive on non-fatal provider warning frames — see inline.
• Gemini ProtocolError propagation in streaming should emit a graceful terminal frame — see inline.

Nitpicks

• _tool_result_text in openai_chat.py duplicates tool_result_text from canonical.py — consider reusing the canonical helper.
• _responses_item_delta / _responses_item_done use list(state.item_ids).index(key) (O(n)) for output index lookup — negligible for typical streams but could use a parallel index map.
• get_models on DynamicOpenAICompatibleProvider changed from sync to async — verify all callers are updated.

Status of Previous Feedback

The following previously-open items remain unaddressed in this diff (none of the affected files were modified here):

• retry_policy.py — model-scoped cooldown bypasses failure-history threshold
• responses/service.py — acquire_task.result() can mask original exception in finally block; duplicated polling logic between next_upstream_chunk / acquire_upstream_stream
• client/streaming.py — disconnect-task leak if is_disconnected() raises
• usage/accounting.py — Gemini _from_gemini_usage cache-write double-count (the streaming converter has the same class of issue — see inline)
• responses/store.py — private _save_to_disk coupling

Overall Status: Strong, well-structured increment. No blocking issues; the inline items are non-blocking improvements for correctness and robustness.

This review was generated by an AI assistant.

[mirrobot-agent]

_gemini_usage computes promptTokenCount as input_tokens + cache_read_tokens but omits cache_write_tokens. The OpenAI (_openai_usage) and Responses (_responses_usage) formatters in this same file both add cache_read_tokens + cache_write_tokens. If a source provider reported cache-write tokens, the Gemini-formatted prompt count will be too low, and totalTokenCount will no longer equal promptTokenCount + candidatesTokenCount.

Suggested change

	return None
	return {
	"promptTokenCount": usage.input_tokens + usage.cache_read_tokens,
	"candidatesTokenCount": usage.output_tokens,
	"promptTokenCount": usage.input_tokens + usage.cache_read_tokens + usage.cache_write_tokens,

[mirrobot-agent]

Each raw_frame is parsed twice: once at line ~476 via self.protocol.parse_stream_event(raw_frame, response_context) for session/usage tracking, and again inside converter.convert(raw_frame) via source_protocol.parse_stream_event. For high-throughput streams this doubles JSON decode + parse work per frame. Consider exposing a method on ProtocolStreamConverter that accepts the already-parsed UnifiedStreamEvent (e.g., converter.format_event(event)) so the tracking pass and the formatting pass share one parse.

[mirrobot-agent]

_canonical_request_for_native performs a three-way merge (original canonical × Chat baseline × Chat attempted) to overlay transform/callback mutations onto the source-native request. The merge relies on positional zip alignment and identity-key matching, which can silently misapply edits if transforms reorder messages, insert/delete turns, or change block types. When an item can't be matched, _without_source_artifacts strips raw and extra — losing source-native fields the design intends to preserve. Consider emitting a trace warning when the fallback path is taken so silent data loss is diagnosable.

[mirrobot-agent]

classify_error now calls get_retry_after(e) for explicit error types. Previously, an explicit error_type short-circuited without checking retry-after. This changes retry behavior: a StructuredAPIResponseError with error_type="rate_limit" will now pick up retry-after from its embedded response/headers dicts (added to get_retry_after at lines 700–716). This is likely intentional, but verify it doesn't cause unexpected same-key retries for structured errors that previously triggered immediate credential rotation.

[mirrobot-agent]

_in_band_error_payload is called at the top of _process_chunk and raises StreamedAPIError for any chunk containing an "error" key or error event type. Some providers emit non-fatal warning/deprecation frames with an error-like shape alongside valid data. This would terminate the stream on the first such frame. Consider gating on explicit terminal semantics (e.g., HTTP 4xx/5xx status codes, response.failed events, or a configurable allow-list) rather than any "error" key presence.

[mirrobot-agent]

_format_gemini raises ProtocolError when terminal tool-call arguments aren't valid JSON. This is architecturally correct (Gemini requires complete function-call objects), but the exception propagates through convert_protocol_stream unhandled — the executor's generic except Exception will emit a generic "Streaming request failed" error rather than a protocol-specific terminal frame. Consider catching this inside the converter and emitting a terminal error event in the output protocol so the client receives a meaningful error.