Repository navigation
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Memory benchmarkCompared base
Interpretation: Differences larger than their measured MAD: Physical footprint +0.3 MiB ± 0.1 MiB, Physical peak +0.2 MiB ± 0.1 MiB. Extension overhead above pi baseline
Values are medians of 6 alternating, paired runs. The value after Environment: pi
|
patlux
left a comment
patlux
left a comment
There was a problem hiding this comment.
Feature and tests look solid; the real-Pi test for restart + refresh is the right level. Two things I'd change before merging (fail-closed behavior on a corrupt optional file, and the compat merge), the rest are nits. Details inline.
| @@ -1,3 +1,4 @@ | |||
| import "./test-routing.ts" | |||
There was a problem hiding this comment.
This side-effect import plus the new npm run test:routing in test/test:unit runs the routing suite twice per npm test. Keep the script entry and drop the import — importing one test file into another is also an unusual pattern in this repo.
| if (error instanceof Error && "code" in error && error.code === "ENOENT") return {} | ||
| // Do not echo file contents, JSON parse excerpts, paths, or arbitrary values. | ||
| throw new Error( | ||
| "Could not read Command Code routing configuration; check its version and model choices", |
There was a problem hiding this comment.
Two issues with the fail-closed path:
-
The message never names the file. The user sees every Provider API request fail with "check its version and model choices" and no hint where to look. The basename
commandcode-routing.jsonis not sensitive — include it ("…incommandcode-routing.jsonin the agent directory"). -
/commandcode-endpointcannot repair the situation either, becausesaveEndpointChoicere-reads the broken file first and throws. So a corrupt optional convenience file bricks the whole provider until the user finds and hand-edits it.
I'd either fail open (ignore the file, keep routingWarning visible in status + stream warning, let /commandcode-endpoint … auto rewrite it from in-memory state), or at minimum make the command able to overwrite a malformed file so there is an in-host recovery path.
| if (entries.some(([id, choice]) => !id.trim() || !isEndpointChoice(choice))) { | ||
| throw new Error("Routing choices must be auto, responses, or chat-completions") | ||
| } | ||
| return Object.fromEntries( |
There was a problem hiding this comment.
parseEndpointOverrides keeps explicit "auto" entries, but saveEndpointChoice deletes them. With a hand-edited {"models":{"x":"auto"}} the status line becomes (auto; user override) because routing[id] is truthy. Normalize here (drop auto entries) so both paths agree.
| const overrides = { ...(await loadEndpointOverrides(path)) } | ||
| if (choice === "auto") delete overrides[modelId] | ||
| else | ||
| Object.defineProperty(overrides, modelId, { |
There was a problem hiding this comment.
A comment on why Object.defineProperty instead of plain assignment would help (I assume __proto__ as a model id). If that is the concern, the read side is the other half: overrides["constructor"] in resolveEndpoint/getRoutingStatus currently returns a function. Realistically catalog ids never collide with Object.prototype, so a comment plus Object.hasOwn on reads is enough — or build the map with Object.create(null) and skip both tricks.
| ...(model.compatConfig ?? model.compat), | ||
| // A selected host model can outlive a command or background refresh. | ||
| // Wire-specific defaults must follow the actual request, not that stale model. | ||
| ...(catalogModel ? compatForModel({ ...catalogModel, api: wire }) : {}), |
There was a problem hiding this comment.
Merging the host model's compat (computed for the catalog wire) with the compat for the overridden wire leaves wire-specific flags from the wrong wire in place. For a Responses model forced to Chat Completions, sessionAffinityFormat: "openai-nosession" and supportsLongCacheRetention: false from the Responses set survive the spread.
compatForModel is authoritative per wire, so replace instead of merge when the catalog model is known:
compat: catalogModel
? compatForModel({ ...catalogModel, api: wire })
: (model.compatConfig ?? model.compat),(This is also the spot that overlaps with #162's replayReasoningContent.)
| routingWarning = error instanceof Error ? error.message : "Invalid routing configuration" | ||
| } | ||
| const resolveModelApi = (modelId: string): CommandCodeApi => { | ||
| if (routingWarning) throw new Error(routingWarning) |
There was a problem hiding this comment.
Note for the fail-closed discussion on src/routing.ts: this throw blocks every Provider API request for every model, including models that have no override at all. If you keep fail-closed, consider scoping it: a model without an entry in a parseable-but-partially-invalid file could still route auto.
|
Wow thanks a lot that looks promising! |
pierreraby
left a comment
pierreraby
left a comment
There was a problem hiding this comment.
Approved. Verified locally at the exact head in an isolated worktree (npm ci): tsc --noEmit PASS, test-routing 3/3, full test:unit exit 0 (409 checks), test-pi-local PASS (incl. persisted override surviving restart + catalog refresh, forced chat-completions wire emitting a real Chat Completions request), Prettier + git diff --check PASS. Upstream CI fully green. I also had the previous head independently reviewed in a fresh context (APPROVE, no P0/P1); the rebase delta is main integration (release 0.7.8, catalog 1.82.4, quota/transport/redaction fixes) plus routing tests wired into the default suites.
Non-blocking notes for follow-up (edge-case hardening, none affect normal catalog ids or the no-override path): (1) prototype-chain reads on the override map (overrides[model.id], status line) vs. the guarded write side — Object.hasOwn would close it; (2) trim-check vs. stored-key mismatch; (3) compat-flags merge instead of replace on wire switch; (4) no e2e pin that a malformed routing file blocks Provider requests (documented behavior); (5) the recovery message could name commandcode-routing.json without leaking contents.
|
I'm writing my message here since it has more chances to be reviewed. I'm quite surprised about the response endpoint being used everywhere it can by default. This decision was documented 21st sep. so before #133. Some models advertising /response don't work well with it -- see image bellow. Source Image
|
|
Thanks for taking the time to write this up here — and you're right on the facts of the vendor doc. What we can confirm. What changed here, in #133 (2026-10-05). Instead of hard-coding the model families, this extension routes on the field the API advertises: // src/models.ts:156
if (id.startsWith("claude-")) return "anthropic-messages"
return supportedEndpoints?.includes("/responses") ? "openai-responses" : "openai-completions"So the divergence from the vendor table only affects models that explicitly advertise Why it was done this way — not a preference, a measurement. On 2026-10-03/04, on models advertising both wires, prompt caching showed up only on Where your point still stands. An advertisement is not a quality guarantee, and GLM-5.3 on
One ask. The image attached here is the routing table itself, not the failing response — could you add the model id plus the exact error (status, body, or the pi error line) for the case you saw? With that we can tell apart "the gateway mis-advertises |
|
@pierreraby My bad, the image was there only to back-up my claim. I also stumbled upon a message saying something about the OAI Response stream ending before the stop event. In practice, all of these errors greatly increase cache misses and bump up cost usage. |
|
@ThomasByr Thanks — the screenshot is what was missing, and now the failure mode is unambiguous. What the error is. Why GLM-5.3 lands on
So it is by construction, not an accidental default. It is also deliberate: on Retry behaviour. Our bounded, Your second symptom. "Stream ended before the stop event" is our truncation guard wording on the generate path ( Where this goes. A per-model endpoint choice is exactly what #158 adds ( Two things would make this actionable: does the 429 arrive before any token is streamed (plain HTTP response) or mid-stream, and does an immediate retry in the same session ever succeed? A pi version for the run would also help me match it to the exact code path. |
(*) however, it's almost always tied to when the throughput drops to 1 tok/s for the second symptom, this happens at most ~once every day, model is the same. P.S. Maybe selecting completion instead of responses will eventually bring the caching issue to GLM models as well. I guess we'll have to try and see |
Summary
Closes #156 on the client configuration side; does not claim to fix upstream capacity.
Verification
No authenticated performance benchmark was run. File edits outside the command require reload; cross-process settings writes are not coordinated.