Repository navigation
fix(llm): image gen / transcription / embeddings reach the wizard's gateway connection (B1) - #4400
Conversation
…ot the legacy api_base The setup wizard (ADR 0106) writes one declared connection and a qualified model.name, leaving legacy model.api_base on the App default http://gateway:4000/v1. gateway_client() / gateway_sync_client(), embeddings and the context-window probe read only that legacy default route, so image generation (sdk.gateway_client -> /images/generations), transcription and embeddings died with ConnectError [Errno 8] while chat worked; the reload then mirrored the dead host into host-config.yaml. - graph.llm.resolve_gateway_route(): the lead's openai-compat connection, else (native lead) a declared `gateway` connection, else the legacy route (unchanged for unqualified leads). - embeddings, the shared gateway HTTP client, and connection-built window probes resolve through it. - sync_host_model_layer mirrors that endpoint (never the App-default placeholder), healing a stale value on the next reload. - the egress auto-allow covers both the default route and the connection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 7b355a1d3ea1 · formal
Overall risk is low: one confirmed minor removed-behavior in config_io.py (stale api_base persists after transition to unqualified lead), plus two nits that went unverified. Fix first: the api_base write-gate in sync_host_model_layer — a wizard connection's non-default URL will silently survive in host-config.yaml after the config transitions to a default-endpoint lead. The panel did not disagree on any finding. Verification confirmed the one finding it received; the other two (model_window.py concurrency nit, test source-scanning nit) were not delivered to the verifier and remain unverified. VERIFY_GAP: unverified=2
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | src/graph/config_io.py:1341 |
sync_host_model_layer no longer writes api_base when it equals the App default, so a previously-mirrored non-default value (e.g. a wizard connection's URL) per… | confirmed |
| ⚪ | nit | src/graph/model_window.py:134 |
Unlocked module-level cache allows transient None on concurrent first-access | nearby, not gating |
| ⚪ | nit | tests/test_default_route_resolver.py:272 |
Source-code-scanning test couples test suite to token layout of server/agent_init.py | nearby, not gating |
findings JSON (machine-readable)
[
{
"file": "src/graph/config_io.py",
"line": 1341,
"severity": "minor",
"category": "removed-behavior",
"claim": "sync_host_model_layer no longer writes api_base when it equals the App default, so a previously-mirrored non-default value (e.g. a wizard connection's URL) persists in host-config.yaml after the config transitions to an unqualified lead; the old code always overwrote it with the current value.",
"evidence": "Old: `\"api_base\": getattr(config, \"api_base\", \"\")` was unconditionally in `desired`. New: `if route.connection or route.base_url != LangGraphConfig.api_base: desired[\"api_base\"] = route.base_url` \u2014 when both are false (unqualified lead, default endpoint), `api_base` is absent from `desired`, so `model.update(desired)` leaves any stale value in the host layer untouched.",
"verdict": "confirmed",
"note": "Read src/graph/config_io.py at head 7b355a1d (lines 1330\u20131342): the conditional `if route.connection or route.base_url != LangGraphConfig.api_base` gates the write; the diff shows the old unconditional `\"api_base\": getattr(config, \"api_base\", \"\")` removed. A stale non-default api_base in host-config.yaml will indeed persist when the config transitions to an unqualified lead on the default endpoint.",
"line_original": 1340,
"line_corrected": true
},
{
"file": "src/graph/model_window.py",
"line": 134,
"severity": "nit",
"category": "concurrency",
"claim": "Unlocked module-level cache allows transient None on concurrent first-access",
"evidence": "The key is added to _ATTEMPTED at line 135 before the fetch completes and _WINDOWS[key] is assigned at line 137. A concurrent caller that observes the key in _ATTEMPTED but finds _WINDOWS[key] absent (lines 138\u2013139 not yet executed) falls through to line 141 where _WINDOWS.get(key, {}) returns {}, yielding None. Because the key is already in _ATTEMPTED, no retry is scheduled\u2014the caller simply degrades.",
"source": "protopatch",
"nearby": true,
"note": "nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)"
},
{
"file": "tests/test_default_route_resolver.py",
"line": 272,
"severity": "nit",
"category": "maintainability",
"claim": "Source-code-scanning test couples test suite to token layout of server/agent_init.py",
"evidence": "The test reads `server/agent_init.py` from disk, tokenizes it via `_code_text`, and asserts exact substring patterns like `\"apply_egress_allowlist ( STATE.graph_config )\"` and `\"apply_egress_allowlist ( new_config )\"`. A refactor that preserves the same semantic behavior but changes variable names (e.g., `STATE.graph_config` \u2192 `state.graph_config` after a rename, or `new_config` \u2192 `incoming`) breaks the test.",
"source": "protopatch",
"nearby": true,
"note": "nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)"
}
]2 structural finding(s) are nearby notes, not part of the verdict: they sit in code this PR did not change (outside its changed lines and the functions those lines are in). Worth a look; not a request for this PR.
src/graph/model_window.py:134(nit) — Unlocked module-level cache allows transient None on concurrent first-accesstests/test_default_route_resolver.py:280(nit) — Source-code-scanning test couples test suite to token layout of server/agent_init.py
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gistry-connection
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 808dbe1bd0bf · formal
Low-risk PR: adds gateway-route resolution, host-model-layer sync, and egress allowlist support across seven production files plus tests. The panel raised a single minor finding (untested key-resolution and loopback logic in model_cli.py), which the verifier refuted: tests/test_model_cli.py exists at head and covers both _is_loopback_endpoint and the three-source key-resolution chain. No findings survive verification. No panel disagreement. No coverage gaps — the verifier annotated all findings it was given.
No findings — the review came back clean.
findings JSON (machine-readable)
[]Addressed since the last round (1 minor/nit): src/graph/config_io.py:1341 — no longer raised, and the PR changed the code around them since they were raised.
Problem (launch blocker B1)
The setup wizard (ADR 0106) writes one declared connection (
providers[gateway].base_url+ key) and a qualifiedmodel.name(gateway:protolabs/smart). Legacymodel.api_basestays on the App defaulthttp://gateway:4000/v1.Chat worked (it builds on the connection), but every gateway-endpoint reader used the legacy default route:
graph.llm._gateway_client_kwargs→gateway_client()/gateway_sync_client()/graph.sdk.gateway_client()— plugin image generation (protoBananagenerate_image→ConnectError [Errno 8]), core transcription_build_embeddings— knowledge embeddingssync_host_model_layermirrored the deadhttp://gateway:4000/v1intohost-config.yamlFix
graph.llm.resolve_gateway_route(config)— the endpoint + key the chat model runs on:model.namenames a registered openai-compat connection → that connection (strictly its own fields);gatewayopenai-compat connection, if any;context_window_fortakes the connection a connection-built client uses.sync_host_model_layermirrors that endpoint and never writes the App-default placeholder; a stale mirrored value heals on the next reload.set_allowed_hosts(also_allow_url=accepts a list).docs/reference/plugin-sdk-api.mdregenerated (docstring change).Tests
tests/test_default_route_resolver.py: new B1 section — wizard-shaped config (providers set, legacy api_base default, env key present) →gateway_clientPOSTs/images/generationsand/audio/transcriptionsto the connection with its bearer;sdk.gateway_client()from live STATE; embeddings + window probe; host mirror writes the real endpoint and heals a stale one; never writes the placeholder; native lead → declared gateway; unqualified lead unchanged; egress auto-allow. All 7 fail onmain._LEAD_CONNECTION_ROUTE, the same table the probe routes already use); the "pinned, not endorsed" window-probe test now asserts the connection.ruff check .,lint-imports,scripts/live_smoke.pygreen.Verified live (throwaway instances, isolated
PROTOAGENT_BOX_ROOT, :7911/:7912)/api/config/setupwith the wizard payload (gatewayhttps://api.proto-labs.ai/v1,gateway:protolabs/smart) →host-config.yamlmodel.api_base: http://gateway:4000/v1;gateway_client→/images/generations→ConnectError [Errno 8](exact repro).host-config.yamlmodel.api_base: https://api.proto-labs.ai/v1; gateway route = connectiongatewaywith key; embeddings (qwen3-embedding) returned 1024 dims;POST /images/generations(protolabs/qwen-image) → 200, 1.18 MB image.🤖 Generated with Claude Code