fix(pi): preserve account model IDs with slashes - #471
Conversation
Greptile SummaryThe PR updates Pi account-model resolution so slash-containing endpoint IDs remain opaque under their known provider, while supporting same-provider qualified replay when the opaque key is unavailable.
Confidence Score: 4/5The PR should not merge until resumed Pi sessions can select models advertised from a saved provider that differs from the account’s known provider. A resumed session scopes its credential and catalog to the transcript’s saved provider, but live selection resolves the advertised qualified model under the account’s different known provider and rejects it at the credential-scope guard. Files Needing Attention: packages/host/agent-adapter/src/native/pi/adapter.ts
|
| Filename | Overview |
|---|---|
| packages/host/agent-adapter/src/native/pi/adapter.ts | Centralizes Pi model resolution, account-aware catalog IDs, resume routing, and provider environment credential setup; the previously reported differing-provider resume rejection remains. |
| packages/foundation/providers/src/resolve.ts | Resolves declared endpoint parameters into provider-specific environment variables while preserving normal endpoint completeness checks. |
| packages/foundation/providers/src/catalog.ts | Declares Cloudflare AI Gateway’s Pi-specific endpoint environment mapping so Pi can retain per-model route construction. |
| packages/host/engine/src/agent/provider-config.ts | Propagates resolved provider environment values into agent startup configuration. |
| packages/host/agent-adapter/src/credential.ts | Parses provider environment data from startup configuration into the adapter credential shape. |
| packages/host/agent-adapter/src/tests/pi-model.test.ts | Adds coverage for opaque slash-containing account IDs, qualified replay fallback, and explicit-model resume behavior. |
| packages/host/agent-adapter/src/tests/pi-model-registry.test.ts | Adds installed-SDK startup coverage for direct Anthropic, OpenRouter, and Vercel AI Gateway model resolution. |
Reviews (5): Last reviewed commit: "fix(providers,engine,agent-adapter): per..." | Re-trigger Greptile
There was a problem hiding this comment.
Important
The fix is correct and the new integration test is real coverage, but the change silently reorders the provider-precedence chain that packages/host/agent-adapter/AGENTS.md:118 documents as an ordered list, and one pre-existing test that should have caught it is now a false green.
Reviewed changes — 59f102f (3 files, +86/-6): the createConfiguredRegistry model-ref branch in native/pi/adapter.ts, the new installed-SDK startup test __tests__/pi-model-registry.test.ts, and the reworked mocked-SDK cases in __tests__/pi-model.test.ts.
The bug is real and I confirmed it against the installed SDK. @earendil-works/pi-ai's MODELS table gives anthropic 14 models with zero slash-containing ids, while openrouter (270) and vercel-ai-gateway (192) both carry anthropic/claude-sonnet-4.6. So pre-fix an OpenRouter account model anthropic/claude-sonnet-4.6 was parseModel'd into provider anthropic + modelId claude-sonnet-4.6, which pi's anthropic table lacks, and onStart threw. The new pi-model-registry.test.ts genuinely fails without this patch — it is not test theatre, and using the real installed SDK rather than the vi.mock harness is the right call for exactly this class of bug.
I also checked the hypothesis that a pi-qualified provider/modelId string could round-trip back into StartOptions.model through the UI, and it cannot: packages/presentation/ui/src/shell/conversation-surface.tsx:209-212 passes agentModels={accountModels ?? null} and explicitly documents that the adapter's own catalog is not offered, accountModelOptions emits raw AccountModel.id, and apps/mobile has no picker at all. That path is closed.
Nitpick: cred.knownProvider at adapter.ts:160 is now unreachable
const provider = fallbackProvider ?? cred.knownProvider ?? endpointProvider; sits inside if (!ref && opts.model). After this change ref is null with a model present only when cred.knownProvider is absent, so that middle term can never contribute. It sits between the two diff hunks so I can't anchor it inline. Worth deleting, or the next reader will assume the endpoint-sniffing fallback still consults it.
Technical details
Pre-fix, ref was null whenever parseModel failed on a slash-less id, regardless of knownProvider, so the term was live. Post-fix, opts.model && cred.knownProvider always produces a ref, so entry into the block implies !cred.knownProvider. The endpointProviders baseUrl sniff below it is likewise now reachable only for account-less starts that carry a baseUrl, which is a narrower set than before.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
The new unwrap branch is correct and its tests genuinely fail without it. But the thread that raised the onSetModel / createConfiguredRegistry divergence was resolved as fixed, and it isn't — the fix covers only the <knownProvider>/… replay shape at start, not live model switching.
Reviewed changes (delta vs. the prior review at 59f102f6)
native/pi/adapter.ts:141-154— the account-id override is now narrowed by registry evidence:ref = accountRef ?? parsedRef, reverted toparsedRefonly when the provider halves match,find(accountRef)misses, andfind(parsedRef)hits.native/pi/adapter.ts:166— the unreachablecred.knownProvidertier in the endpoint-sniff fallback was deleted. Correct: entry intoif (!ref && opts.model)implies!cred.knownProvider, so the term was dead.AGENTS.md:118— precedence paragraph rewritten to the new order.- Four new tests (
pi-model-registry.test.tsit.eachrow 4, three mocked cases inpi-model.test.ts).
The new tests are real coverage. I neutered the unwrap condition (accountRef && Date.now() < 0 && …) and re-ran the suite: exactly resolves 'openrouter' input 'openrouter/anthropic/claude-sonnet-4.6' … and unwraps a same-provider Pi-qualified replay … turn red while the other 21 stay green.
On the unwrap's false-positive window. It can only misfire on an account id shaped <knownProvider>/<X> that Pi's table lacks while it knows bare <X> under the same provider. Against the installed @earendil-works/pi-ai table that window is empty: openrouter has exactly one slash-less id (auto) and also carries openrouter/auto; vercel-ai-gateway and nvidia have zero slash-less ids; the self-prefixed ids that do exist (openrouter/{auto,free,fusion}, five nvidia/*) are all present under their own provider, so find(accountRef) hits and the unwrap never runs. Data-dependent rather than structurally guaranteed, but no action needed today — noting it so a future table change doesn't surprise anyone.
Also worth knowing: the old model-less test injects the credential under the provider the session last routed through is a legitimate green now, not a leftover — it pins the "no explicit model → resume evidence wins" tier, and the new sibling test covers the explicit-model tier.
ℹ️ Nitpicks
AGENTS.md:118— the lead-in still reads "The two provider-routed agents…" while the rewritten body now describes only Pi. Opencode's half of that precedence rule was dropped from the bullet.AGENTS.md:109— the capability matrix lists piset-modelas ✗ even thoughonSetModelis implemented and reachable. Pre-existing and independent of this PR, but it's the doc a reader would check when weighing the inline comment below.
Claude Opus | 𝕏
|
手上没有 Acceptance Criteria 要求的 Anthropic API / OpenRouter / Vercel AI Gateway / Cloudflare AI Gateway 可以进行测试。 |
|
Cloudflare AI Gateway 可以了吗 |
|
嗯,可以了,我稍后继续完成 review |
xiaoland
left a comment
There was a problem hiding this comment.
I test with Cloudflare AI Gateway (which is the only I have), using gateway linkcode and configures model anthropic/claude-sonnet-4-6 and custom-subscription/claude-sonnet-4-6 which is two model id that lincodehq's ai-gateway source has.
And I got these failures:
From logs, I got:
pi: model 'custom-subscription/claude-sonnet-4-6' is not available
for provider 'cloudflare-ai-gateway'
This is likely to be a bug that this PR should resolve but not.
With model claude-sonnet-4-6, this error gone, but new error comes:
Which likely to be a protocol mismatch.
There was a problem hiding this comment.
Important
The delta does exactly what the previous review asked for, and I verified it works. But it ships with zero test coverage, and while re-tracing it I found a reflection desync I should have caught in my two earlier reviews.
Reviewed changes (since 78c2f58):
856065edis adapter-only:onSetModel(packages/host/agent-adapter/src/native/pi/adapter.ts:472) now routes through the extractedresolveModelRefinstead of bareparseModel, andcreateConfiguredRegistryreturnscredentialsoonSetModelcan reuse the account'sknownProvider/baseUrl(:342).- Tests and
AGENTS.mdare untouched by this commit. - The prior thread on the
onSetModeldivergence is resolved — I re-ran its own repro against856065edand it passes.
Extraction verified behavior-preserving on the start path: the old accountRef guard was opts.model && cred.knownProvider, the new one is cred.knownProvider inside a call already gated on opts.model. And ModelRegistry.find is a strict two-key lookup, so :477's if (!model) throw still blocks any wrong-provider pairing.
⚠️ The delta has zero test coverage
I neutered the fix to measure this: reverting :472 back to const ref = parseModel(value); if (!ref) throw … leaves all 19 pi-model.test.ts tests green. Nothing in the suite notices.
The reason is that the only existing set-model test (pi-model.test.ts:277, "switches model and effort live and reflects SDK readback") calls start() with no config, so there is no knownProvider — it never reaches the account-bound branch this PR exists to fix.
A test that does catch it is small: start a session with config: { apiKey, baseUrl: 'https://openrouter.ai/api/v1', knownProvider: 'openrouter' } on anthropic/claude-sonnet-4.6, then send({ type: 'set-model', model: 'openai/gpt-5' }). Pre-fix it dies with pi: this session's credential is scoped to 'openrouter'; post-fix it succeeds. Please add something like that — otherwise the next refactor of this helper silently reintroduces CODE-610.
⚠️ model-update emits an id the client cannot resolve back to a menu entry
I missed this in reviews 5002513941 and 5004367852 — apologies. The root cause is the changed ref resolution from 59f102f6; 856065ed extends the bad emission to the mid-session path, which previously rejected the pick outright.
I traced every link:
- Emission —
adapter.ts:377(start) and:479(set-model) send`${model.provider}/${model.id}`. Account-bound,model.idis now the whole account id, so the wire value becomesopenrouter/anthropic/claude-sonnet-4.6. - Menu options are the raw
AccountModel.id—settings/providers/model-options.ts:31-36(id: model.id), andconversation-surface.tsxpassesagentModels={accountModels ?? null}(the adapter's own catalog is deliberately not offered). - No normalization in between —
client/core/src/conversation.ts:529iscurrentModel = event.model;verbatim. - Resolution fails —
agent-models.ts:75-88resolveModeltries exact match, thenid.startsWith(`${option.id}-`). A prepended path segment matches neither, so it returnsundefined.
Downstream:
composer-controls.tsx:295—modelLabelfalls back to the rawopenrouter/anthropic/claude-sonnet-4.6string in the chip.composer.tsx:803-805—effortOptionsForModelis computed fromundefined, so per-model effort options are lost.startup-selection.ts:32— comparesreflectedModel === requested.modelexactly, so the startup selection is cleared.
Bounded, at least: selectedAccountId comes from session state (currentAccountId={accountId}), not from resolveModel, so there's no spurious cross-account relaunch.
Two things make this worth fixing in this PR rather than deferring: pi-model-registry.test.ts:57 asserts the desynced string (`${provider}/${modelId}`), so the PR's own test encodes it as intended behavior; and packages/host/agent-adapter/AGENTS.md records that opencode already solved exactly this by emitting the id the user picked, not its own prefixed readback. Pi should do the same.
ℹ️ Scope: the blocking review is about catalogs pi does not carry
The open CHANGES_REQUESTED review (5007232296) reports Cloudflare AI Gateway with custom-subscription/claude-sonnet-4-6 failing as pi: model 'custom-subscription/claude-sonnet-4-6' is not available for provider 'cloudflare-ai-gateway'. I confirmed the mechanism against the installed @earendil-works/pi-ai table: cloudflare-ai-gateway carries 38 ids, all slash-less, and no custom-subscription/*.
So the id resolution this PR introduces is correct there (accountRef = the whole id under cloudflare-ai-gateway) — the remaining failure is the separate, already-documented limitation that registerProvider without models cannot add table entries. Not a regression from this PR. But it does mean CODE-610's user-reported scenario still fails, and that blocking review stays unaddressed. Worth stating explicitly in the PR description whether this lands as a partial fix with the gateway case tracked separately, or whether it waits.
For context on the false-positive window of the new unwrap: I enumerated the table and it is empty today — every self-prefixed id (openrouter/{auto,free,fusion}, 5 nvidia/*) is present under its own provider. That's data-dependent, not structural, but it means the unwrap is safe as shipped.
Claude Opus | 𝕏
|
Two follow-up commits, both found by driving a real Pi session against a
|
| account model id | onStart |
onSetModel (before) |
|---|---|---|
claude-opus-4-8 |
starts | pi: model must be 'provider/modelId' |
anthropic/claude-opus-4-8 |
not available for provider 'cloudflare-ai-gateway' |
credential is scoped to 'cloudflare-ai-gateway' |
workers-ai/@cf/moonshotai/kimi-k2.6 |
starts | parses as provider workers-ai, fails the scope guard |
An account-bound session could start but never change model. The resolution logic is extracted verbatim into resolveModelRef and both paths now call it; onSetModel passes the running model's provider where onStart passes the resumed session's last-routed one.
c28f531e — per-leg gateway routing, and account model ids on the wire
The base-URL override flattened routes that differ per model
registerProvider(provider, { baseUrl }) rewrites the URL and leaves each model's wire at Pi's built-in value. That holds only where the provider's wire already matches the endpoint — and Pi's cloudflare-ai-gateway entry spans three legs:
| wire | leg | models |
|---|---|---|
anthropic-messages |
/anthropic |
17 (claude-opus-4-8, …) |
openai-responses |
/openai |
16 (gpt-5.4, …) |
openai-completions |
/compat |
4 (workers-ai/@cf/…) |
Pinning all of them to the variant's /compat answered 400 Compatibility endpoint: v1/messages is not supported on 33 of 37; only the four that were already /compat worked (verified live — workers-ai/@cf/moonshotai/kimi-k2.6 runs, claude-opus-4-8 400s).
Pi builds the right per-model URL itself from CLOUDFLARE_ACCOUNT_ID / CLOUDFLARE_GATEWAY_ID, but it reads those only off a stored credential: setRuntimeApiKey carries no env, ModelRegistry has no env-injection API, and ProviderConfigInput has no env field. So ServiceVariant gains endpointEnv — endpointParams key → the env name that agent's own provider entry reads it under. resolveBinding resolves it against the account, the engine passes it as providerEnv, and the Pi adapter seeds AuthStorage.inMemory with { key, env } and skips the base-URL override. In-memory because set() on the file-backed store would leave the account's secret in ~/.pi/agent/auth.json.
Verified against the installed SDK: all 37 models resolve, each keeping its own template and wire.
endpointEnv is declared for pi only. OpenCode reaches this service through /compat with models.dev's anthropic/claude-opus-4.8-style ids and works today, so it is deliberately left alone.
Behavioural consequence worth a second opinion: a seeded store holds one provider's credential, so getAvailable() narrows to that provider and a user's own ~/.pi/agent/auth.json logins are invisible inside an account-bound session. credentialProviderId already refuses cross-provider switches there, so this is consistent — but it is observable.
The composer lost track of the running model
emitModel sent ${provider}/${id} while the composer's options come from the account's model list (accountModelOptions → bare claude-opus-4-8). resolveModel matched neither exactly nor by its startsWith(${option.id}-) rule, so composer-controls.tsx:295 fell through to rendering the raw id and the picker showed nothing selected — one turn into every session.
advertisedModelId now emits the account's own id whenever the model's provider is the account's knownProvider, in emitModel, emitModels, and startCatalog. Stripping the prefix client-side is not an option: workers-ai/@cf/moonshotai/kimi-k2.6 contains slashes of its own, so the UI cannot tell which segment is the provider.
Inbound resolution accepts both forms — bare ids via the account branch, already-persisted qualified pins via CODE-610's unwrap branch — so old model pins still resume.
The four pi-model-registry.test.ts cases asserted the qualified form; their assertions now match the bare id their own test names already state (resolves 'openrouter' input 'openrouter/anthropic/claude-sonnet-4.6' to 'anthropic/claude-sonnet-4.6').
Verification
pnpm check:cicleanpnpm test— 2,987 passed, 1 skipped. 3 pre-existing failures inpackages/foundation/common/src/node/__tests__/release-artifact.test.ts, which fail identically on78c2f584without these commits: on macOSos.tmpdir()resolves through the/var→/private/varsymlink, sorelative()reports the isolated root as escaped. Untouched here.- Drove a real Pi session on a
cloudflare-gatewayaccount for each failure above and re-checked it after the fix.
Not fixed here
Pi and OpenCode use different id vocabularies for the same service — Pi wants bare claude-opus-4-8, OpenCode wants anthropic/claude-opus-4.8 — but Account.models is service-level, so one list cannot satisfy both. providers/AGENTS.md states the assumption that ids are identical whichever protocol shape an agent resolves to; that holds across one agent's variants, not across agents. Worth its own issue.
There was a problem hiding this comment.
Reviewed changes since 856065e — one new commit, c28f531, which does two things: fixes the model-update reflection desync I raised last round, and adds per-leg Cloudflare gateway routing by seeding an in-memory AuthStorage instead of injecting a flat baseUrl.
Important
The gateway approach is right and I verified it against the installed SDK end-to-end. But the seeded path is reachable by no test in the repo, and it changes credential injection for the non-seeded provider in a way that regresses a behavior this PR's own test suite pins.
The seeded path is verified-by-reading, not verified-by-running
I checked the pi contract myself rather than trusting the design note, and it holds:
AuthStorage.inMemory(data?: AuthStorageData)exists (dist/core/auth-storage.d.ts) andApiKeyCredentialcarriesenv.getProviderEnvreadsthis.data;setRuntimeApiKeywritesthis.runtimeOverrides;getApiKeyprefers the override. They are genuinely separate maps.cloudflare-auth.jsresolveValuereadscredential.env?.[name]when a stored credential exists.- The real model table (
pi-ai/dist/providers/cloudflare-ai-gateway.models.js) templates per-leg URLs off{CLOUDFLARE_ACCOUNT_ID}/{CLOUDFLARE_GATEWAY_ID}— the exact two namescatalog.tsnow declares. Anthropic-family models get/anthropic, OpenAI-family/openai, the rest/compat.
So the mechanism is sound. What I could not do is run it, because nothing exercises it:
| surface | providerEnv coverage |
|---|---|
providers/src/__tests__/resolve.test.ts |
none — no endpointEnv, no providerEnvFor, no cloudflare-gateway binding assertion |
pi-model.test.ts, pi.test.ts, pi-approval.test.ts, pi-ui-bridge.test.ts |
none — and all four mock AuthStorage as { create: ... } only, so a seeded start would throw pi.AuthStorage.inMemory is not a function |
pi-model-registry.test.ts (real SDK) |
none — all four cases pass baseUrl + knownProvider, never providerEnv |
The blocking human review on this PR reported a live 400 Compatibility endpoint: v1/messages is not supported. A fix for a reported runtime failure that no test can reach is the one place I'd want a test most. pi-model-registry.test.ts already drives the installed SDK against a temp PI_CODING_AGENT_DIR, so the case is cheap to add there — see the inline note.
Scope: only the second of the reporter's two errors is fixed
The reporter hit two failures. This commit addresses the protocol mismatch. The first one — custom-subscription/claude-sonnet-4-6 not existing in pi's table — is still unfixed, and correctly so: it needs registerProvider with models, which both AGENTS.md files now document as out of scope and explain why (declaring a model pi already knows destroys its real metadata). Worth saying explicitly in the PR description so the issue isn't closed as fully resolved.
Resolved since last round
✅ The
model-updatedesync is fixed.advertisedModelIdemits the account's own id unprefixed whenmodel.provider === accountProvider, which is exactly the vocabularyaccountModelOptionsbuilds the picker from, soresolveModel's exact match now hits instead of falling through to the${id}-prefix path. Start-time emit,emitModels, and theonSetModelreflection all route through it consistently. The previously-open thread onadapter.ts:144was already resolved.
Claude Opus | 𝕏
| } | ||
| if (key && provider) authStorage.setRuntimeApiKey(provider, key); | ||
| if (cred.baseUrl) { | ||
| if (key && provider && !seeded) authStorage.setRuntimeApiKey(provider, key); |
There was a problem hiding this comment.
The !seeded guard isn't needed, and it regresses a behavior the suite pins.
Not needed: setRuntimeApiKey writes this.runtimeOverrides while getProviderEnv reads this.data (pi-coding-agent/dist/core/auth-storage.js). A runtime key cannot clobber the seeded credential's env, so there is nothing to protect against.
Regresses: provider is not always cred.knownProvider. It resolves as ref?.provider ?? fallbackProvider ?? cred.knownProvider ?? …, so a model ref that parses to a different provider, or a model-less resume where fallbackProvider (the session's last-routed provider) wins, both leave provider !== cred.knownProvider while seeded is truthy. In that case this line and the registerProvider below are both skipped, so the provider pi actually routes through gets neither a key nor a base URL — it silently falls back to whatever is in ~/.pi/agent/auth.json, or 401s.
That tier is deliberate and tested: pi-model.test.ts "injects the credential under the provider the session last routed through" asserts exactly it. The test passes only because it supplies no providerEnv, so it takes the unseeded branch.
Dropping the guard here is safe. The !seeded condition on the registerProvider block below is the one that carries real meaning (seeded providers template their own URL), and it can stay — though it's worth deciding what should happen when a seeded credential's provider isn't the one being routed to.
| } | ||
|
|
||
| /** The variant's declared env names carrying this account's endpoint params, for agents that | ||
| * template their own per-model URL. Only the pinned-endpoint path skips it: a URL the user typed |
There was a problem hiding this comment.
Only the pinned-endpoint path skips it: a URL the user typed outranks whatever the agent's own catalog would build.
The carve-out is reasonable in principle, but pinnedEndpoint() keeps more than URLs a user typed — and the accounts it over-keeps are the ones this PR is trying to fix.
A pre-variant Cloudflare account has a filled endpoint (…/8f3a/prod/compat) written by the old add flow. It never equals the templated variant baseUrl, so pinnedEndpoint returns it, resolveBinding takes the early bind(kind, protocol, baseUrl, knownProvider) branch at line 62 — still four arguments, no providerEnv — and the adapter falls through to the flat registerProvider(baseUrl). That pins every model to /compat, which is precisely the 400 Compatibility endpoint: v1/messages is not supported the reporter filed.
__tests__/resolve.test.ts:287 pins this shape with a cloudflare-gateway account and the comment "the pre-variant behavior, which was never broken for these accounts". That claim was true before this PR; it isn't now — pi bound to such an account still can't reach the anthropic leg.
Worth confirming how many real accounts are in that state. The current add flow doesn't write endpoint (per add-flow.test.tsx), so this is bounded to accounts created before the variant work — but there's no migration, and the only user-facing recovery is delete-and-re-add, which nothing tells them to do. Options: backfill providerEnv when the pinned URL's origin+params match a templated variant, or narrow pinnedEndpoint so a filled-template match against the catalog isn't treated as user-authored.
| kind: 'pi', | ||
| cwd: root, | ||
| model: inputModel, | ||
| config: { authToken: 'dummy', baseUrl, knownProvider: provider }, |
There was a problem hiding this comment.
This file drives the installed SDK, so it's the one place the seeded path can actually be proven — the four unit-test mocks stub AuthStorage as { create } only and would throw on inMemory.
A fifth case here would cover it end-to-end: knownProvider: 'cloudflare-ai-gateway', providerEnv: { CLOUDFLARE_ACCOUNT_ID: '8f3a', CLOUDFLARE_GATEWAY_ID: 'prod' }, no baseUrl, and an anthropic-family model id from pi's own table. Asserting the resolved model's baseUrl ends in /anthropic rather than /compat is the assertion that would have caught the reported bug, and it also proves the two env names in catalog.ts still match pi's templates if the SDK ever renames them.
A companion case in providers/src/__tests__/resolve.test.ts asserting resolveBinding(cloudflareAccount, 'pi') returns providerEnv and no baseUrl would cover the other half.
…unt model ids for pi
c28f531 to
245bceb
Compare
|
@xiaoland you can re-review now, the models supported by pi agent 0.80.6 is shown below |
|
the pi adapter will be re-contributed to fit pi 0.84.2, but not in this PR |

Summary
(knownProvider, complete AccountModel.id)so endpoint-owned IDs containing slashes are not misread as Pi provider-qualified referencesCODE-610
Verification
pnpm check:cipnpm test— 347 files passed, 1 skipped; 2,986 tests passed, 1 skippedChecklist
pnpm check:ciandpnpm testboth pass (pluscargo fmt/clippy/testfor Rust changes)WIRE_PROTOCOL_VERSIONis bumped