feat(advisor): review-gate advisor backend and advisor route type - #359
feat(advisor): review-gate advisor backend and advisor route type#359eric-liu-nvidia wants to merge 5 commits into
Conversation
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
Signed-off-by: zengyuanl <zengyuanl@nvidia.com>
WalkthroughThe PR adds advisor-gated executor routes with configurable reviews, retries, streaming, statistics, logging, and presets. It also preserves native Anthropic request bodies, normalizes reasoning effort, supports Docker network modes, and adds documentation and tests. ChangesAdvisor routing
Native Anthropic passthrough
Docker network mode configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (11)
tests/test_advisor_config.py (1)
41-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the two uncovered gate validators.
advisor_config.pyadds_pattern_compiles(Lines 167-175) and_pattern_trigger_requires_pattern(Lines 177-183). No test in this file exercises either path. Both are new validation rules that reject invalid operator configuration.As per coding guidelines: "Write unit tests for new roles and bug fixes."
🧪 Proposed tests
def test_accepts_mixed_wire_tiers(self) -> None: mixed = _config(advisor={"model": "deepseek/deepseek-r2", "base_url": "http://adv.test", "api_key": "k", "format": "openai"}) assert mixed.advisor.format == BackendFormat.OPENAI assert mixed.executor.format == BackendFormat.ANTHROPIC + + def test_rejects_invalid_gate_trigger_pattern(self) -> None: + with pytest.raises(pydantic.ValidationError, match="not a valid regex"): + _config(gate_trigger_pattern="task_complete[") + + def test_pattern_trigger_requires_pattern(self) -> None: + with pytest.raises(pydantic.ValidationError, match="non-empty gate_trigger_pattern"): + _config(gate_trigger="pattern") + + def test_pattern_trigger_accepts_pattern(self) -> None: + cfg = _config(gate_trigger="pattern", gate_trigger_pattern=r'task_complete["\s>:]*true') + assert cfg.gate_trigger == "pattern"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_advisor_config.py` around lines 41 - 55, Add unit tests in test_advisor_config.py covering both validator paths: verify an invalid pattern is rejected by _pattern_compiles, and verify a trigger configuration requiring a pattern is rejected by _pattern_trigger_requires_pattern. Assert pydantic.ValidationError and match the relevant validation message, using the existing _config helper and test style.Source: Coding guidelines
switchyard/lib/backends/advisor_config.py (2)
167-175: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the compiled pattern.
_pattern_compilescompiles the regex and discards it. The advisor loop must recompile the pattern on every gate check unless it caches it separately.model_configalready setsarbitrary_types_allowed=True, so the model can hold a compiled pattern.The ast-grep ReDoS hint on Line 171 is a false positive here:
gate_trigger_patternis operator configuration, not caller-supplied request data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/backends/advisor_config.py` around lines 167 - 175, Update the advisor configuration model around _pattern_compiles to retain the compiled regex instead of discarding it, using a model field or private attribute compatible with the existing arbitrary_types_allowed setting. Ensure the advisor loop reuses this cached pattern for gate checks while preserving validation errors for invalid gate_trigger_pattern values.Source: Linters/SAST tools
150-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_target_model_non_emptymay be unreachable.The test at
tests/test_advisor_config.pylines 35-39 states thatcoerce_llm_targetrejects empty model ids during coercion, before this validator runs. If that holds, this validator never raises and the error message it defines is never observable. Either remove it or keep it as a documented defense with a comment that explains why it stays.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/backends/advisor_config.py` around lines 150 - 155, Resolve the redundant _target_model_non_empty validator by removing it if coerce_llm_target already rejects empty model identifiers; otherwise retain it only with a concise comment documenting its defense-in-depth purpose and why it remains necessary.switchyard/lib/processors/routing_log_response_processor.py (1)
193-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not call the private
_appendfrom outside the class.
emit_auxiliary_recordis a module-level function. It reaches intosink._append, a private method. Add a public method onRoutingLogResponseProcessorand call that instead. The private name then stays free to change.♻️ Proposed public method
def _write_record(self, ctx: ProxyContext, served_model: str, response: ChatResponse) -> None: ... - self._append(record) + self.append_record(record) - def _append(self, record: dict[str, Any]) -> None: + def append_record(self, record: dict[str, Any]) -> None: """Append one record as a JSON line; write failures never propagate."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/processors/routing_log_response_processor.py` at line 193, Add a public record-emission method to RoutingLogResponseProcessor that delegates to its internal append behavior, then update the module-level emit_auxiliary_record function to call that public method instead of sink._append. Keep _append private and preserve the existing record contents and emission behavior.switchyard/lib/backends/advisor_loop_backend.py (4)
610-615: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant local imports.
BackendFormatis already imported at Line 89 andresolve_llm_targetat Line 92. The function-local re-imports at Lines 612-613 add no cycle protection.♻️ Proposed cleanup
def _build_advisor_caller(config: AdvisorConfig) -> AdvisorCaller: """Build the advisor caller for ``config.advisor``, dispatched on its format.""" - from switchyard.lib.backends.llm_target import BackendFormat - from switchyard.lib.backends.multi_llm_backend import resolve_llm_target - target = resolve_llm_target(config.advisor)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/backends/advisor_loop_backend.py` around lines 610 - 615, Remove the redundant local imports of BackendFormat and resolve_llm_target from _build_advisor_caller, reusing the existing module-level imports instead; leave the target resolution logic unchanged.
1042-1051: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the audit records through the logger.
_audit_seedand_audit_reviewwrite directly tosys.stderrand flush on every call. This bypasses log level control, structured log routing, and log capture in tests.reply_headalso puts model reply text on stderr with no way to disable it.Use
log.info(...)with the same JSON payload so operators can control the output.Also applies to: 1228-1252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/backends/advisor_loop_backend.py` around lines 1042 - 1051, Update _audit_seed and _audit_review to emit the existing JSON payload through log.info(...) instead of writing to or flushing sys.stderr, preserving the advisor_seed/advisor_review record contents. Also replace reply_head’s direct stderr output with the logger so model reply text follows the same controllable logging path.
718-718: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap these two lines to stay within 100 characters.
Line 718 is about 104 characters and line 803 is about 102 characters.
As per coding guidelines: "Keep lines within 100 characters."
♻️ Proposed wrapping
- content = getattr(getattr(choices[0], "message", None), "content", None) if choices else None + message = getattr(choices[0], "message", None) if choices else None + content = getattr(message, "content", None)- has_tool_use = bool(message.get("tool_calls")) or choice.get("finish_reason") == "tool_calls" + has_tool_use = ( + bool(message.get("tool_calls")) or choice.get("finish_reason") == "tool_calls" + )Also applies to: 803-803
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/backends/advisor_loop_backend.py` at line 718, Wrap the long `content = getattr(...)` expression in the relevant backend code so each line stays within 100 characters, preserving its existing behavior and applying the same formatting to the corresponding line near the second occurrence.Source: Coding guidelines
174-192: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPer-instance session state grows without bound.
_stall_fired,_seed_advice,_sessions_seen,_reviews_by_scope,_failed_consults_by_scope, and_budget_logged_scopesonly ever gain entries. The module docstring states the design target is a long-lived gateway shared by many tasks, so a benchmark campaign adds one or more entries per session and never releases them._seed_adviceholds full advice text, so its growth is the largest.Bound these maps, for example with a fixed-capacity FIFO/LRU eviction keyed by insertion order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@switchyard/lib/backends/advisor_loop_backend.py` around lines 174 - 192, Bound all per-instance session state collections—_stall_fired, _seed_advice, _sessions_seen, _reviews_by_scope, _failed_consults_by_scope, and _budget_logged_scopes—with a fixed-capacity FIFO or LRU eviction policy keyed by insertion order. Ensure new session keys evict the oldest entries once capacity is reached, while preserving existing lookups, budget behavior, and seed advice semantics.tests/test_advisor_loop_backend.py (2)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
pytest-mockfor the non-HTTP fakes.The fakes use
unittest.mock.MagicMockandAsyncMock. The HTTP paths correctly userespx.As per path instructions for
tests/**/*.py: "Userespxfor HTTP mocking andpytest-mockfor general mocking."Also applies to: 96-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_advisor_loop_backend.py` at line 16, Replace the unittest.mock MagicMock and AsyncMock usage in the advisor loop backend tests with pytest-mock’s mocker fixture, while preserving the existing fake behavior and respx-based HTTP mocking. Update the affected setup and test cases around the imported mocks and the referenced lines to use mocker-created synchronous and asynchronous mocks.Source: Path instructions
80-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the over-length assertion and fixture lines.
Lines 81, 217, 218, 500, 502, and 503 each exceed 100 characters. Lines 516, 523, and 527 are also at or over the limit.
As per coding guidelines: "Keep lines within 100 characters."
♻️ Example wrapping for Lines 217-218
- assert any(m.get("role") == "assistant" and m.get("content") == "I think I'm done" for m in redo_msgs) - assert any(m.get("role") == "user" and "empty-input case" in (m.get("content") or "") for m in redo_msgs) + assert any( + m.get("role") == "assistant" and m.get("content") == "I think I'm done" + for m in redo_msgs + ) + assert any( + m.get("role") == "user" and "empty-input case" in (m.get("content") or "") + for m in redo_msgs + )Also applies to: 217-218, 500-503
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_advisor_loop_backend.py` around lines 80 - 81, Wrap the over-100-character assertion and fixture lines in the test module, including the event dictionary around the visible content_block_start fixture and the referenced lines near 217–218, 500–503, 516, 523, and 527. Preserve the existing test data and behavior while formatting each statement to stay within 100 characters.Source: Coding guidelines
tests/test_route_bundle.py (1)
294-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLine 296 exceeds 100 characters.
Split the assignment from the
type: ignorecomment or shorten the subscript chain.As per coding guidelines: "Keep lines within 100 characters."
♻️ Proposed wrapping
def test_responses_format_rejected(self) -> None: bundle = _advisor_bundle() - bundle["routes"]["myrouter/advisor"]["executor"]["format"] = "responses" # type: ignore[index] + route = bundle["routes"]["myrouter/advisor"] # type: ignore[index] + route["executor"]["format"] = "responses" # type: ignore[index] with pytest.raises(RouteBundleConfigError, match="responses"): build_route_bundle_table(bundle)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_route_bundle.py` around lines 294 - 298, Shorten the assignment in test_responses_format_rejected so every line remains within 100 characters, while preserving the type: ignore[index] suppression and the existing bundle mutation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 262-263: Correct the description above the switchyard serve
command to state that examples/route.yaml serves only the passthrough and noop
routes, leaving the commented-out advisor-gate configuration unchanged.
- Around line 199-200: Add separate project-tree entries for advisor_prompts.py
and advisor_presets.py alongside advisor_config.py in the documented advisor
file map, and remove their parenthetical mention from the advisor_config.py
description.
In `@benchmark/run-baseline.sh`:
- Around line 885-897: Replace the binary if/else around
SWITCHYARD_DOCKER_NETWORK_MODE with an explicit host|bridge case and reject
unsupported values before constructing Docker arguments. Document
SWITCHYARD_DOCKER_NETWORK_MODE and its accepted host and bridge values in
usage(), and add dry-run coverage for both valid modes plus invalid input.
- Around line 885-890: Update the host-network branch in run-baseline.sh to
require an explicitly supplied --harbor-server-url, or derive and validate a
host-reachable URL before proceeding; otherwise fail fast with a clear error.
Preserve the existing --network host behavior, ensure bridge-only network
arguments are omitted, and add dry-run coverage for host mode and the
missing-URL failure using the contract in prepare_harbor_dataset.py and usage().
In `@switchyard/cli/launchers/launcher_runtime.py`:
- Around line 135-145: Annotate the empty tiers dictionary in the advisor branch
of the route-summary logic with its string-to-optional-string type so strict
mypy accepts it. Also preserve the route’s default_model fallback when executor
or advisor lacks a model, rather than returning a summary containing None;
update the tier resolution in this branch while retaining the existing
executor/advisor summary format.
In `@switchyard/cli/switchyard_cli.py`:
- Around line 36-46: Update RoutingLogResponseProcessor.emit_auxiliary_record so
its _append file write and lock wait run off the event loop, using
asyncio.to_thread or the existing async writer mechanism; preserve _lock
serialization and ensure callers correctly await or schedule the asynchronous
auxiliary write.
In `@switchyard/lib/backends/advisor_loop_backend.py`:
- Around line 759-772: Update the executor usage accounting in _completion_usage
and _consume_anthropic_stream to reuse the cache-inclusive Anthropic token
helper used by _advisor_usage, so cache_creation_input_tokens is included in
prompt/input token totals for both streamed and non-streamed paths. Preserve
existing OpenAI handling and cached-token reporting.
In `@switchyard/lib/backends/advisor_presets.py`:
- Line 53: Update the base_url parameter docstring in the relevant preset
definition to describe the Anthropic Messages endpoint rather than an
OpenAI-compatible gateway, matching the BackendFormat.ANTHROPIC configuration
and module documentation.
In `@switchyard/lib/processors/reasoning_effort_normalizer.py`:
- Around line 29-36: Update _VALID_REASONING_EFFORT in
switchyard/lib/processors/reasoning_effort_normalizer.py:29-36 to include none
and minimal so both values pass through unchanged. Extend the parametrized
pass-through test in tests/test_reasoning_effort_normalizer.py:40-43 with
minimal and none, while retaining super-mega as the unknown-value case.
In `@switchyard/lib/processors/routing_log_response_processor.py`:
- Around line 173-206: Make emit_auxiliary_record asynchronous and offload
sink._append to a worker thread with asyncio.to_thread, matching the existing
process implementation. Update every call in advisor_loop_backend.py, including
async call(...), to await emit_auxiliary_record(...); preserve the current
record contents and no-sink behavior.
In `@tests/test_advisor_config.py`:
- Around line 18-26: Annotate the _config function’s overrides parameter with an
appropriate mapping type compatible with AdvisorConfig keyword arguments, while
preserving the existing return annotation and behavior so strict mypy no longer
treats the function as partially annotated.
---
Nitpick comments:
In `@switchyard/lib/backends/advisor_config.py`:
- Around line 167-175: Update the advisor configuration model around
_pattern_compiles to retain the compiled regex instead of discarding it, using a
model field or private attribute compatible with the existing
arbitrary_types_allowed setting. Ensure the advisor loop reuses this cached
pattern for gate checks while preserving validation errors for invalid
gate_trigger_pattern values.
- Around line 150-155: Resolve the redundant _target_model_non_empty validator
by removing it if coerce_llm_target already rejects empty model identifiers;
otherwise retain it only with a concise comment documenting its defense-in-depth
purpose and why it remains necessary.
In `@switchyard/lib/backends/advisor_loop_backend.py`:
- Around line 610-615: Remove the redundant local imports of BackendFormat and
resolve_llm_target from _build_advisor_caller, reusing the existing module-level
imports instead; leave the target resolution logic unchanged.
- Around line 1042-1051: Update _audit_seed and _audit_review to emit the
existing JSON payload through log.info(...) instead of writing to or flushing
sys.stderr, preserving the advisor_seed/advisor_review record contents. Also
replace reply_head’s direct stderr output with the logger so model reply text
follows the same controllable logging path.
- Line 718: Wrap the long `content = getattr(...)` expression in the relevant
backend code so each line stays within 100 characters, preserving its existing
behavior and applying the same formatting to the corresponding line near the
second occurrence.
- Around line 174-192: Bound all per-instance session state
collections—_stall_fired, _seed_advice, _sessions_seen, _reviews_by_scope,
_failed_consults_by_scope, and _budget_logged_scopes—with a fixed-capacity FIFO
or LRU eviction policy keyed by insertion order. Ensure new session keys evict
the oldest entries once capacity is reached, while preserving existing lookups,
budget behavior, and seed advice semantics.
In `@switchyard/lib/processors/routing_log_response_processor.py`:
- Line 193: Add a public record-emission method to RoutingLogResponseProcessor
that delegates to its internal append behavior, then update the module-level
emit_auxiliary_record function to call that public method instead of
sink._append. Keep _append private and preserve the existing record contents and
emission behavior.
In `@tests/test_advisor_config.py`:
- Around line 41-55: Add unit tests in test_advisor_config.py covering both
validator paths: verify an invalid pattern is rejected by _pattern_compiles, and
verify a trigger configuration requiring a pattern is rejected by
_pattern_trigger_requires_pattern. Assert pydantic.ValidationError and match the
relevant validation message, using the existing _config helper and test style.
In `@tests/test_advisor_loop_backend.py`:
- Line 16: Replace the unittest.mock MagicMock and AsyncMock usage in the
advisor loop backend tests with pytest-mock’s mocker fixture, while preserving
the existing fake behavior and respx-based HTTP mocking. Update the affected
setup and test cases around the imported mocks and the referenced lines to use
mocker-created synchronous and asynchronous mocks.
- Around line 80-81: Wrap the over-100-character assertion and fixture lines in
the test module, including the event dictionary around the visible
content_block_start fixture and the referenced lines near 217–218, 500–503, 516,
523, and 527. Preserve the existing test data and behavior while formatting each
statement to stay within 100 characters.
In `@tests/test_route_bundle.py`:
- Around line 294-298: Shorten the assignment in test_responses_format_rejected
so every line remains within 100 characters, while preserving the type:
ignore[index] suppression and the existing bundle mutation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2161eae2-b736-4173-968d-93313d637cdb
📒 Files selected for processing (20)
AGENTS.mdbenchmark/run-baseline.shcrates/switchyard-components/src/backends/anthropic.rscrates/switchyard-components/tests/adversarial_native_backends.rsexamples/route.yamlswitchyard/__init__.pyswitchyard/cli/launchers/launcher_runtime.pyswitchyard/cli/route_bundle.pyswitchyard/cli/switchyard_cli.pyswitchyard/lib/backends/__init__.pyswitchyard/lib/backends/advisor_config.pyswitchyard/lib/backends/advisor_loop_backend.pyswitchyard/lib/backends/advisor_presets.pyswitchyard/lib/backends/advisor_prompts.pyswitchyard/lib/processors/reasoning_effort_normalizer.pyswitchyard/lib/processors/routing_log_response_processor.pytests/test_advisor_config.pytests/test_advisor_loop_backend.pytests/test_reasoning_effort_normalizer.pytests/test_route_bundle.py
| │ │ ├── advisor_loop_backend.py # AdvisorLoopBackend (advisor review gate) | ||
| │ │ ├── advisor_config.py # AdvisorConfig (+ advisor_prompts, advisor_presets) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
List the rehomed advisor files explicitly.
Add separate tree entries for advisor_prompts.py and advisor_presets.py. The project structure is a file map, but the current entry lists only advisor_config.py and hides two modules in a parenthetical.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 199 - 200, Add separate project-tree entries for
advisor_prompts.py and advisor_presets.py alongside advisor_config.py in the
documented advisor file map, and remove their parenthetical mention from the
advisor_config.py description.
| # Serve the minimal Python YAML bundle (noop, passthrough, and advisor routes). | ||
| switchyard serve --routes examples/route.yaml --port 4000 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the route-bundle description.
examples/route.yaml keeps advisor-gate commented out at Lines 17-22. This command therefore serves only the passthrough and noop routes, not an advisor route. Change the description or enable the advisor route.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 262 - 263, Correct the description above the
switchyard serve command to state that examples/route.yaml serves only the
passthrough and noop routes, leaving the commented-out advisor-gate
configuration unchanged.
| if [[ "\${SWITCHYARD_DOCKER_NETWORK_MODE:-bridge}" == "host" ]]; then | ||
| # Host networking: for upstreams only routable from the host (VPN / | ||
| # corp-internal gateways that Docker bridge networks cannot reach). | ||
| # Pair with --harbor-server-url http://<host-ip>:<port> so task | ||
| # containers reach the server at the host address. | ||
| DOCKER_RUN_ARGS+=(--network host) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Require a reachable Harbor URL for host networking.
When SWITCHYARD_DOCKER_NETWORK_MODE=host, the server does not join ${SWITCHYARD_DOCKER_NETWORK} and does not publish ${SWITCHYARD_DOCKER_SERVICE_NAME} on the task network. The documented default for --harbor-server-url is the Dockerized Switchyard service. A host-mode run without that option can therefore leave Harbor unable to reach the server.
Fail fast unless --harbor-server-url is supplied, or derive a verified host-reachable URL. Add dry-run coverage for host mode, including the absence of bridge-only arguments.
The network contract is defined by benchmark/prepare_harbor_dataset.py and the usage() text in benchmark/run-baseline.sh.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/run-baseline.sh` around lines 885 - 890, Update the host-network
branch in run-baseline.sh to require an explicitly supplied --harbor-server-url,
or derive and validate a host-reachable URL before proceeding; otherwise fail
fast with a clear error. Preserve the existing --network host behavior, ensure
bridge-only network arguments are omitted, and add dry-run coverage for host
mode and the missing-URL failure using the contract in prepare_harbor_dataset.py
and usage().
| if [[ "\${SWITCHYARD_DOCKER_NETWORK_MODE:-bridge}" == "host" ]]; then | ||
| # Host networking: for upstreams only routable from the host (VPN / | ||
| # corp-internal gateways that Docker bridge networks cannot reach). | ||
| # Pair with --harbor-server-url http://<host-ip>:<port> so task | ||
| # containers reach the server at the host address. | ||
| DOCKER_RUN_ARGS+=(--network host) | ||
| else | ||
| DOCKER_RUN_ARGS+=( | ||
| --network "\${SWITCHYARD_DOCKER_NETWORK}" | ||
| --network-alias "\${SWITCHYARD_DOCKER_SERVICE_NAME}" | ||
| -p "127.0.0.1:$(q "${PORT}"):$(q "${PORT}")" | ||
| ) | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate and document SWITCHYARD_DOCKER_NETWORK_MODE.
The else branch treats every value except exact host as bridge. A typo or unsupported value can silently make a host-only upstream unreachable while the run continues with the wrong topology.
Use an explicit host|bridge case and reject other values. Document the variable and its accepted values in usage(). Add dry-run tests for both modes and invalid input.
The accepted-value contract should be visible in the user-facing usage() block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/run-baseline.sh` around lines 885 - 897, Replace the binary if/else
around SWITCHYARD_DOCKER_NETWORK_MODE with an explicit host|bridge case and
reject unsupported values before constructing Docker arguments. Document
SWITCHYARD_DOCKER_NETWORK_MODE and its accepted host and bridge values in
usage(), and add dry-run coverage for both valid modes plus invalid input.
| if route_type == "advisor": | ||
| tiers = {} | ||
| for field in ("executor", "advisor"): | ||
| tier = route.get(field) | ||
| tiers[field] = ( | ||
| tier.get("model") if isinstance(tier, _Mapping) else tier | ||
| ) | ||
| return ( | ||
| f"advisor: executor={tiers['executor']}, " | ||
| f"advisor={tiers['advisor']}" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Annotate tiers for strict mypy.
tiers = {} at Line 136 has no annotation and no initial values, so strict mypy reports var-annotated.
Also note the rendered summary shows executor=None, advisor=None when a route omits the tier models, because the branch returns before the route: {default_model} fallback.
As per coding guidelines: "Use type hints throughout; code must satisfy strict mypy checking."
🐛 Proposed fix
if route_type == "advisor":
- tiers = {}
+ tiers: dict[str, object] = {}
for field in ("executor", "advisor"):
tier = route.get(field)
tiers[field] = (
tier.get("model") if isinstance(tier, _Mapping) else tier
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@switchyard/cli/launchers/launcher_runtime.py` around lines 135 - 145,
Annotate the empty tiers dictionary in the advisor branch of the route-summary
logic with its string-to-optional-string type so strict mypy accepts it. Also
preserve the route’s default_model fallback when executor or advisor lacks a
model, rather than returning a summary containing None; update the tier
resolution in this branch while retaining the existing executor/advisor summary
format.
Source: Coding guidelines
| def _completion_usage(body: Any, *, is_openai: bool) -> dict[str, int]: | ||
| """Read ``_ExecTurn`` token counts from a non-streamed completion body.""" | ||
| usage = body.get("usage") if isinstance(body, dict) else None | ||
| prompt_tokens, completion_tokens = _usage_tokens(usage) | ||
| details = usage if isinstance(usage, dict) else {} | ||
| if is_openai: | ||
| cached = (details.get("prompt_tokens_details") or {}).get("cached_tokens") or 0 | ||
| else: | ||
| cached = details.get("cache_read_input_tokens") or 0 | ||
| return { | ||
| "input_tokens": prompt_tokens or 0, | ||
| "output_tokens": completion_tokens or 0, | ||
| "cached_tokens": int(cached), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Executor token accounting drops Anthropic cache-creation tokens.
_completion_usage reads only input_tokens and cache_read_input_tokens for the Anthropic wire. _consume_anthropic_stream does the same at Line 740. _advisor_usage folds cache_creation_input_tokens into prompt_tokens and documents that this matches the routing-log processor's accounting.
Both records land in the same routing log through _emit_routing_usage. On gateways that auto-cache server-side (the case _advisor_usage documents at Line 1191), the review_gate_discarded record under-reports prompt tokens while the advisor_review record does not. Per-model cost attribution is then inconsistent between the two tiers.
Reuse the cache-inclusive helper for the executor path.
🐛 Proposed fix
def _completion_usage(body: Any, *, is_openai: bool) -> dict[str, int]:
"""Read ``_ExecTurn`` token counts from a non-streamed completion body."""
usage = body.get("usage") if isinstance(body, dict) else None
- prompt_tokens, completion_tokens = _usage_tokens(usage)
- details = usage if isinstance(usage, dict) else {}
- if is_openai:
- cached = (details.get("prompt_tokens_details") or {}).get("cached_tokens") or 0
- else:
- cached = details.get("cache_read_input_tokens") or 0
+ tokens = _advisor_usage(usage)
return {
- "input_tokens": prompt_tokens or 0,
- "output_tokens": completion_tokens or 0,
- "cached_tokens": int(cached),
+ "input_tokens": tokens["prompt_tokens"],
+ "output_tokens": tokens["completion_tokens"],
+ "cached_tokens": tokens["cached_tokens"],
}Apply the same inclusion in _consume_anthropic_stream so the streamed path reports cache_creation_input_tokens too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _completion_usage(body: Any, *, is_openai: bool) -> dict[str, int]: | |
| """Read ``_ExecTurn`` token counts from a non-streamed completion body.""" | |
| usage = body.get("usage") if isinstance(body, dict) else None | |
| prompt_tokens, completion_tokens = _usage_tokens(usage) | |
| details = usage if isinstance(usage, dict) else {} | |
| if is_openai: | |
| cached = (details.get("prompt_tokens_details") or {}).get("cached_tokens") or 0 | |
| else: | |
| cached = details.get("cache_read_input_tokens") or 0 | |
| return { | |
| "input_tokens": prompt_tokens or 0, | |
| "output_tokens": completion_tokens or 0, | |
| "cached_tokens": int(cached), | |
| } | |
| def _completion_usage(body: Any, *, is_openai: bool) -> dict[str, int]: | |
| """Read ``_ExecTurn`` token counts from a non-streamed completion body.""" | |
| usage = body.get("usage") if isinstance(body, dict) else None | |
| tokens = _advisor_usage(usage) | |
| return { | |
| "input_tokens": tokens["prompt_tokens"], | |
| "output_tokens": tokens["completion_tokens"], | |
| "cached_tokens": tokens["cached_tokens"], | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@switchyard/lib/backends/advisor_loop_backend.py` around lines 759 - 772,
Update the executor usage accounting in _completion_usage and
_consume_anthropic_stream to reuse the cache-inclusive Anthropic token helper
used by _advisor_usage, so cache_creation_input_tokens is included in
prompt/input token totals for both streamed and non-streamed paths. Preserve
existing OpenAI handling and cached-token reporting.
|
|
||
| Args: | ||
| api_key: Inference Hub API key, used for both tiers (one tenancy). | ||
| base_url: OpenAI-compatible gateway base URL. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the base_url docstring.
The docstring calls base_url an "OpenAI-compatible gateway base URL". Both tiers in this preset use BackendFormat.ANTHROPIC and the module comment describes the Anthropic Messages endpoint. Align the wording.
📝 Proposed docstring fix
- base_url: OpenAI-compatible gateway base URL.
+ base_url: Gateway base URL for the Anthropic Messages endpoint.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| base_url: OpenAI-compatible gateway base URL. | |
| base_url: Gateway base URL for the Anthropic Messages endpoint. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@switchyard/lib/backends/advisor_presets.py` at line 53, Update the base_url
parameter docstring in the relevant preset definition to describe the Anthropic
Messages endpoint rather than an OpenAI-compatible gateway, matching the
BackendFormat.ANTHROPIC configuration and module documentation.
| #: OpenAI-compatible values the upstream accepts. ``"max"`` is non-standard | ||
| #: but supported by NVIDIA Hub's LiteLLM for reasoning-budget overrides. | ||
| _VALID_REASONING_EFFORT = frozenset({"low", "medium", "high", "max"}) | ||
|
|
||
| #: Aliases the upstream rejects → the nearest valid value. | ||
| _REASONING_EFFORT_ALIASES = { | ||
| "xhigh": "high", | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The reasoning-effort allow-list omits the low-effort values. _VALID_REASONING_EFFORT lists only low, medium, high, and max, so minimal and none fall to the "high" default and the caller gets more reasoning than requested. The test suite locks in that behavior because it exercises only an obviously invalid value.
switchyard/lib/processors/reasoning_effort_normalizer.py#L29-L36: addnoneandminimalto_VALID_REASONING_EFFORTso they pass through unchanged.tests/test_reasoning_effort_normalizer.py#L40-L43: extend the parametrized pass-through test withminimalandnone, and keepsuper-megaas the unknown-value case.
📍 Affects 2 files
switchyard/lib/processors/reasoning_effort_normalizer.py#L29-L36(this comment)tests/test_reasoning_effort_normalizer.py#L40-L43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@switchyard/lib/processors/reasoning_effort_normalizer.py` around lines 29 -
36, Update _VALID_REASONING_EFFORT in
switchyard/lib/processors/reasoning_effort_normalizer.py:29-36 to include none
and minimal so both values pass through unchanged. Extend the parametrized
pass-through test in tests/test_reasoning_effort_normalizer.py:40-43 with
minimal and none, while retaining super-mega as the unknown-value case.
| def emit_auxiliary_record( | ||
| *, | ||
| session_id: str | None, | ||
| task: str | None, | ||
| model: str, | ||
| tier: str, | ||
| prompt_tokens: int = 0, | ||
| cached_tokens: int = 0, | ||
| cache_creation_tokens: int = 0, | ||
| completion_tokens: int = 0, | ||
| ) -> None: | ||
| """Append a proxy-internal usage record to the registered routing log. | ||
|
|
||
| The record shape matches ``_write_record`` exactly, so | ||
| ``snapshot_session`` (and with it ``/v1/routing/session-stats``) | ||
| aggregates chain-terminal and proxy-internal usage alike. | ||
| """ | ||
| sink = _AUX_SINK | ||
| if sink is None: | ||
| return | ||
| sink._append({ | ||
| "ts": datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3] + "Z", | ||
| "task": task, | ||
| "trial_id": None, | ||
| "session_id": session_id, | ||
| "model": model, | ||
| "tier": tier, | ||
| "prompt_tokens": prompt_tokens, | ||
| "cached_tokens": cached_tokens, | ||
| "cache_creation_tokens": cache_creation_tokens, | ||
| "completion_tokens": completion_tokens, | ||
| "reasoning_tokens": 0, | ||
| "total_tokens": prompt_tokens + completion_tokens, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
emit_auxiliary_record performs blocking file I/O synchronously.
_append calls json.dumps, acquires self._lock, opens the file, and writes. RoutingLogResponseProcessor.process deliberately avoids doing this inline: Line 52 offloads the identical work with asyncio.to_thread.
emit_auxiliary_record is a synchronous function. The advisor loop backend calls it from async call(...). The write then runs on the event loop thread. Two consequences follow:
- The event loop stalls for the duration of the disk write on every advisor consult and every discarded executor turn.
- The coroutine can block on
self._lockwhile ato_threadworker holds it, which stalls the whole loop, not just one request.
Make the helper async and offload the write, matching the discipline in process.
As per coding guidelines: "Use async-only implementations; use asyncio.run() when synchronous execution is required."
⚡ Proposed fix to offload the write
-def emit_auxiliary_record(
+async def emit_auxiliary_record(
*,
session_id: str | None,
task: str | None,
model: str,
tier: str,
prompt_tokens: int = 0,
cached_tokens: int = 0,
cache_creation_tokens: int = 0,
completion_tokens: int = 0,
) -> None:
"""Append a proxy-internal usage record to the registered routing log.
The record shape matches ``_write_record`` exactly, so
``snapshot_session`` (and with it ``/v1/routing/session-stats``)
aggregates chain-terminal and proxy-internal usage alike.
+
+ The write is offloaded to a worker thread, so the caller's event loop
+ never blocks on disk I/O or on the sink's write lock.
"""
sink = _AUX_SINK
if sink is None:
return
- sink._append({
+ record = {
"ts": datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3] + "Z",
"task": task,
"trial_id": None,
"session_id": session_id,
"model": model,
"tier": tier,
"prompt_tokens": prompt_tokens,
"cached_tokens": cached_tokens,
"cache_creation_tokens": cache_creation_tokens,
"completion_tokens": completion_tokens,
"reasoning_tokens": 0,
"total_tokens": prompt_tokens + completion_tokens,
- })
+ }
+ await asyncio.to_thread(sink.append_record, record)Update the call sites in switchyard/lib/backends/advisor_loop_backend.py to await emit_auxiliary_record(...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@switchyard/lib/processors/routing_log_response_processor.py` around lines 173
- 206, Make emit_auxiliary_record asynchronous and offload sink._append to a
worker thread with asyncio.to_thread, matching the existing process
implementation. Update every call in advisor_loop_backend.py, including async
call(...), to await emit_auxiliary_record(...); preserve the current record
contents and no-sink behavior.
Source: Coding guidelines
| def _config(**overrides) -> AdvisorConfig: | ||
| base: dict = { | ||
| "executor": {"model": "exec-model", "base_url": "http://exec.test", "api_key": "k", | ||
| "format": "anthropic"}, | ||
| "advisor": {"model": "adv-model", "base_url": "http://adv.test", "api_key": "k", | ||
| "format": "anthropic"}, | ||
| } | ||
| base.update(overrides) | ||
| return AdvisorConfig(**base) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Annotate **overrides.
_config annotates the return type but not the parameter. Strict mypy flags a partially annotated function. Add the parameter annotation.
As per coding guidelines: "Use type hints throughout; code must satisfy strict mypy checking."
🔧 Proposed annotation
-def _config(**overrides) -> AdvisorConfig:
- base: dict = {
+def _config(**overrides: object) -> AdvisorConfig:
+ base: dict[str, object] = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _config(**overrides) -> AdvisorConfig: | |
| base: dict = { | |
| "executor": {"model": "exec-model", "base_url": "http://exec.test", "api_key": "k", | |
| "format": "anthropic"}, | |
| "advisor": {"model": "adv-model", "base_url": "http://adv.test", "api_key": "k", | |
| "format": "anthropic"}, | |
| } | |
| base.update(overrides) | |
| return AdvisorConfig(**base) | |
| def _config(**overrides: object) -> AdvisorConfig: | |
| base: dict[str, object] = { | |
| "executor": {"model": "exec-model", "base_url": "http://exec.test", "api_key": "k", | |
| "format": "anthropic"}, | |
| "advisor": {"model": "adv-model", "base_url": "http://adv.test", "api_key": "k", | |
| "format": "anthropic"}, | |
| } | |
| base.update(overrides) | |
| return AdvisorConfig(**base) |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 19-19: Do not make http calls without encryption
Context: "http://exec.test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 21-21: Do not make http calls without encryption
Context: "http://adv.test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_advisor_config.py` around lines 18 - 26, Annotate the _config
function’s overrides parameter with an appropriate mapping type compatible with
AdvisorConfig keyword arguments, while preserving the existing return annotation
and behavior so strict mypy no longer treats the function as partially
annotated.
Source: Coding guidelines
|
Parking this as draft: #343 ("remove deprecated server stack") landed on main right after this PR was opened and deleted the substrate it builds on — the Python The advisor review gate is being re-implemented natively on the Rust server stack (switchyard-server / libsy); a follow-up PR will supersede this one. This branch remains the validated Python reference implementation (benchmarked review_gate behavior + tests). |
|
Superseded by #371: the advisor review gate re-implemented natively on the Rust stack (libsy |
|
Closing in favor of #371, the native Rust implementation of the advisor review gate. This branch stays available as the validated Python reference (benchmarked behavior + the test contract the port was verified against); the Python serving stack it targets was removed from main by #343, so it can no longer merge. |
1 participant
What
Re-do of the
port/advisor-strategybranch keeping only the review-gate advisor strategy, rebased onto latestmain. AnAdvisorLoopBackendpairs the executor with a stronger advisor that reviews the executor's first no-tool-call turn once per session: APPROVE returns the turn unchanged; REDO feeds the advisor's plan back and re-invokes the executor. Exposed as atype: advisorYAML route.Five focused commits:
fix(backends): forward native Anthropic request bodies verbatim— native Anthropic requests early-return fromoutbound_body(model rewrite + extra_body only); strip/normalize now applies only to translated bodies. Preservescontext_management, thinking blocks, message-level system turns, and client tool ids for real Anthropic clients.feat(stats): routing-log sink for proxy-internal usage records—register_routing_log_sink/emit_auxiliary_recordso multi-call backends can price advisor consults and REDO-discarded turns into the same routing log (snapshot_sessionaggregates them for free).feat(processors): restore reasoning-effort normalizer— restored verbatim from pre-refactor(python): remove legacy routing profiles #268 main; wired only into advisor routes (Claude Code's/effort xhighbreaks some executor upstreams).feat(advisor): review-gate advisor backend and advisor route type— the main change.chore(benchmark): support host network mode in run-baseline.sh—SWITCHYARD_DOCKER_NETWORK_MODE=hostfor upstreams only routable from the host (VPN / corp-internal gateways).Why
Benchmarks showed the review gate is the only functional advisor method; the tool_call strategy suppressed the executor's own test-and-iterate loop (premature "done"). This PR ports the validated review-gate implementation (including all follow-up fixes: session-key stabilization, per-instance review caps, budget refunds, gateway cache buckets, reasoning-only turns) onto main, dropping tool_call entirely.
Because #268 removed the Python profiles layer since the original branch, the code is re-homed rather than cherry-picked:
AdvisorConfig/prompts/presets moved fromswitchyard/lib/profiles/toswitchyard/lib/backends/,AdvisorProfileConfigis gone, and thetype: advisorroute buildsAdvisorLoopBackend(config, stats_accumulator=stats)directly inroute_bundle.py.How tested
uv run ruff check .cleanuv run mypy switchyardclean (strict, 89 files)uv run pytest tests/green — 947 passed, 9 skipped, hermetic (-m "not integration", provider keys unset); includes the 53 ported loop-backend tests plus new route-bundle and config suitescargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspaceall clean (toolchain 1.96.1)type: advisorbundle viabuild_route_bundle_tableand verified the chain shape (StatsRequestProcessor → ReasoningEffortNormalizer → AdvisorLoopBackend → StatsResponseProcessor → TranslationEngine)Checklist
snake_caseof the primary class.switchyard/__init__.py.__all__(AdvisorConfig,AdvisorLoopBackend,AdvisorPresets).--helpunchanged surface; AGENTS.md andexamples/route.yamlupdated for the new route type.Signed-off-by) per the DCO.Notes for reviewers
strategyconfig key.type: advisoris the review gate. Existing benchmark YAMLs carryingstrategy: review_gatemust drop that line (fails validation with an unknown-key error), and oldtype: advisorroutes that relied on the tool_call default now build the review gate.AdvisorLoopBackendis Python-only and must not be wrapped inStatsLlmBackend(raisesTypeError); the bundle builder injects the accumulator through the constructor, and the backend self-accounts advisor usage into the classifier bucket.enable_statsnow gates only that backend-internal accounting (main no longer has a per-route stats-processor toggle).routing_log_response_processor.py(keptCTX_REQUEST_HEADERSand thectx.selected_targettier).🤖 Generated with Claude Code
Summary by CodeRabbit