feat: Phase 2 · 1/7 — LLM gateway & model catalog #13
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p2-llm-gateway"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Opens Phase 2 with the first cut: the driven port to OpenRouter and its adapter (ADR 0004).
What's in
ILlmGatewayport (UseCases/Gateways) with two operations the settings work (cut 2) consumes:ValidateKeyAsync— checks a key against OpenRouter'skeyendpoint, the cheap validation the settings save requires (ADR 0006). The key is a parameter, not read from storage — settings must validate a key before persisting it.ListModelsAsync— the model catalog with capability facts:SupportsVision(drives the vision-required filter, ADR 0015) andSupportsTools(every agent is a tool loop, ADR 0014 — a tool-less model can't run any agent), plus context length.OpenRouterLlmGateway(Infrastructure/Gateways) on the vendored OpenRouter.Net client. The client binds its key at construction, so the gateway holds one client per key — a single cached entry, since a single-user app has one key — wrapped in the library'sCachingOpenRouterClientso repeated catalog reads inside the 5-min TTL stay off the network. A 401/403 maps to "OpenRouter rejected the API key."; other failures carry the provider's error through.AddInfrastructure.Tests (9)
The suite drives the real OpenRouter.Net client against canned HTTP responses, so what's under test is the JSON mapping itself (modalities,
supported_parameters, thedataenvelope) — not a hand-rolled client fake. Covers: valid-key mapping, 401 vs 500 wording, blank key short-circuits off the network, vision/tools capability mapping incl. sparse metadata degrading safely (unknown modalities, missing name → id), name sorting, cache hit on repeated reads, key change dropping the cache and re-authenticating, and host DI resolving the port.Phase 2 cut plan (for context)
AgentResultcost is currently always null: the agent never requestsusage.include)ask_user,:online+fetch_urlper ADR 0016's implementation note — the library has no web-plugin support, and:onlinemodel slugs work as-is)🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 80.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 98.8%
Orihon.Kernel - 90.9%
Orihon.Server - 91.3%
Orihon.UseCases - 99.3%
🔮 fufu~ Jibril reviewed your code!
Orihon's first step into Phase 2! A driven port with its adapter, the settings surface for the model catalog~ fufu, I do love a clean hexagonal boundary. The key-as-parameter design (ADR 0006 — validate before you persist) is exactly right, and the single-entry-per-key cache wrapped in
CachingOpenRouterClientis a tidy answer to "one user, one key." Let me look closer though~ ♡Verdict: ⛔ I can't let this pass~ ♡
Build is 0/0 and all 239 tests pass (9 new here, 44→53 in Integration). The architecture is lovely. But there are two untested branches and a DRY smell feeding each other, and you know I can't abide an untested code path~ fufu.
⛔ These need fixing before I'm satisfied~
OpenRouterLlmGateway.cs:78-80— theListModelsAsyncfailure path is never exercised. TheValidateKeyAsyncfailure arms get two dedicated tests (401→"rejected", 500→"carries through"), butListModelsAsynchas the exact samefailure.StatusCode is 401 or 403 ? "OpenRouter rejected the API key." : $"Fetching the OpenRouter model catalog failed: {failure.Error}"mapping and no test ever fires it. EveryListModelsAsynctest either seeds success JSON or the blank-key short-circuit — a 401 on/modelsor a 500 there reaches a branch with zero coverage. A typo in this copy would sail straight to production. You added a code path but forgot to test it? I can't let that slide~ ♡Fix: add two
ListModelsAsyncfailure tests mirroring tests #2/#3 — oneRespond("models", errorJson, 401)asserting"OpenRouter rejected the API key.", one with 500 assertingStartsWith("Fetching the OpenRouter model catalog failed:"). They cost a dozen lines and pin both branches.**
OpenRouterLlmGateway.cs:62-64 & 78-80 — the rejection/error mapping is copy-pasted between both methods.** The patternfailure.StatusCode is 401 or 403 ? "OpenRouter rejected the API key." : $"...failed: {failure.Error}"is byte-identical save the success-prefix string. This is the *root cause* of finding #1: two copies means two things to test, and one got skipped. DRY-violation duplicates that drift apart are exactly the bug class that bites later. Fix: extract a tiny helper, e.g.private static Result FailFrom(Result.Failure f, string prefix) => Result.Fail(f.StatusCode is 401 or 403 ? "OpenRouter rejected the API key." : $"{prefix}: {f.Error}");— then both sites become one line and one set of tests covers the shared logic. (Note: that helper would reference the library'sOpenRouter.Net.Models.Common.Result, not Orihon's — keep the twoResult` types straight, the PR already threads them correctly, which I liked~)💡 Little ideas (non-blocking)~
OpenRouterLlmGateway.cs:59 & 61 — theUsage ?? 0mandIsFreeTier ?? falsefallbacks are untested.**KeyInfo.UsageandIsFreeTierare nullable andKeyJsonalways sets them, so the??arms are dead to coverage. A sparse-key test (omitusage/is_free_tierfromKeyJson`) would pin both fallbacks for ~3 lines. Not blocking since the behavior is trivially correct, but it's the same "sparse metadata degrades safely" care you did give the catalog path — nice to mirror it on the key path.LlmModel.cs:5-6 —/in a class-level.**references a parameter of the *current member*; on a record's class-level doc these point at primary-constructor parameters, which the compiler tolerates (0 warnings) but reads oddly.` is the conventional form for referencing the generated properties. Pure polish~✅ What I liked~
Result<T>types are threaded flawlessly. Orihon.Kernel'sOk<T>/Err<T>(string-only error) vs OpenRouter.Net'sSuccess/Failure(withStatusCode/Error) — the.Matchon the library result correctly hands you aFailurewith.StatusCode, and you return Orihon'sResult<...>.Ok/Fail. No confusion, no accidental cross-cast. That's the kind of discipline that makes a reviewer's heart sing~ ♪disposeHandler: falseon the testHttpClient— the comment ("an injected test handler outlives any one client") is exactly right; the handler is owned by the test fixture and the client churns on key change. Textbook.RetryOptions.Disabledin tests, default retries in production — the public ctor passesretry: nullsoOpenRouterClientfalls back to its 3-attempt default; the internal test seam forcesDisabledso failure tests don't sit through backoff. Honest and correct.Architecture?.InputModalities?.Contains(Image) == trueandSupportedParameters?.Contains("tools") == trueboth tolerate every nullable link in the chain collapsing tofalse. The sparse-metadata test (bare/minimumwith unknown modality + missing name→id) genuinely proves it.Lock cacheLock+ cache-miss rebuild inside the lock, HTTP call outside — correct granular locking; the lock guards the cache swap, not the network call, so concurrentListModelsof different keys don't serialize each other's I/O.Automated review by Jibril · 2026-07-24
CI/CD: absent for head
cf9286f· Local checks: build 0 warnings/0 errors, 239/239 tests pass (9 new gateway tests + 230 existing), OpenRouter.Net submodule @038bfb6 + Kagaku.UI @df81a2c initializedAll four points taken in
a7345ba:ListModelsAsyncfailure branches: added the mirrored pair —A_rejected_key_on_the_catalog_read_reads_as_rejected_too(401 → "OpenRouter rejected the API key.") andA_server_failure_on_the_catalog_read_carries_the_providers_error_through(500 → prefix assert). Both branches are now pinned.MapFailure<T>(statusCode, error, activity)— it takes the failure's fields rather than the library's nestedResult<TSource>.Failure, since the source and target generic arguments differ at the two call sites (KeyInfo→LlmKeyInfo,IReadOnlyList<ModelInfo>→IReadOnlyList<LlmModel>); same dedup, one fewer generic knot. BothMatchfailure arms are now one line over the shared helper, and the new tests cover it from both entry points.A_sparse_key_payload_falls_back_to_zero_usage_and_paid_tierserves{ "data": { "label": null } }and asserts the?? 0m/?? falsefallbacks — the same degrade-safely care as the catalog path, mirrored.<paramref>on the record's class-level doc: swapped to<see cref="SupportsVision"/>/<see cref="SupportsTools"/>.Build clean, 12/12 gateway tests green (242 total).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
You came back and closed every single thread in one tidy commit~ fufu, that's how you answer a review. ♡ The DRY extraction is even cleaner than what I suggested — let me show you why I noticed~
Verdict: ✅ Looks good to me~
⛔ → ✅ Both blockers, resolved~
ListModelsAsyncfailure path is now pinned.A_rejected_key_on_the_catalog_read_reads_as_rejected_too(401 → "OpenRouter rejected the API key.") andA_server_failure_on_the_catalog_read_carries_the_providers_error_through(500 →StartsWith("Fetching the OpenRouter model catalog failed:")) mirror theValidateKeyAsyncpair exactly. Both fire real canned HTTP through the real client — genuine behavioral tests, not tautologies. The catalog read's failure mapping can no longer drift past a reviewer silently. ♡FailFrom<T>(Result<T>.Failure f, string prefix)taking the library's wholeFailureobject. You went withMapFailure<T>(int? statusCode, string error, string activity)— passing the raw fields instead. That's the sharper call: the helper now has zero dependency onOpenRouter.Net.Models.Common.Result<T>, it's a pure(int?, string, string) → stringmapping. Fewer type couplings, easier to reason about, same one-liner at both call sites. I'm genuinely tickled you improved on it~ ♪💡 → ✅ Both little ideas, picked up~
Usage ?? 0m/IsFreeTier ?? falsenow tested.A_sparse_key_payload_falls_back_to_zero_usage_and_paid_tierfeeds{ "data": { "label": null } }(nousage, nolimit, nois_free_tier) and assertsUsage == 0m,Limit == null,IsFreeTier == false. The key path now has the same "sparse metadata degrades safely" care the catalog path always had. Symmetry~ ♡<paramref>→<see cref>inLlmModel.cs. Swapped verbatim. Pure polish, done.✅ What I liked~
OpenRouterLlmGatewayline 100%, branch 90% (up from 73.8%). But the important number is the breakdown —MapFailure<T>in the display class: branch 1.0;ListModelsAsyncstate machine: branch 1.0;ValidateKeyAsyncstate machine: branch 1.0. Every branch the diff actually touches is fully exercised. The remaining 10% lives inToModel(nullable-chain permutations) andClientFor(the productionhandler is nullarm) — both pre-existing, neither in this diff. No new partial branch was introduced.MapFailureis precise. "One mapping for every provider failure: an auth status reads as a rejected key, anything else carries the provider's error behind the activity prefix." That's a one-sentence spec for a one-line function. ♪401 or 403guard, same$"{prefix}: {error}"shape. The refactor is load-bearing only for the bug class it kills (two copies drifting apart).Build 0 warnings/0 errors, 242/242 tests pass (was 239 — exactly +3, the new gateway tests; Integration 53→56). All 12 gateway tests green. The Phase 2 foundation is solid — on to cut 2~ fufu.
Automated review by Jibril · 2026-07-24
CI/CD: stale for head
a7345ba(coverage bot 3569 covers priorcf9286fonly) · Local checks: build 0/0, 242/242 tests pass, gateway branch coverage 90% (100% on all diff-touched methods)