feat(pi): Use ModelRegistry & allow empty auth to support custom models/providers.#1284
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Pi provider model resolution and auth to use pi-coding-agent's Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PiProvider
participant AuthStorage
participant ModelRegistry
participant Model
participant Logger
Client->>PiProvider: sendQuery(provider, modelId, ...)
PiProvider->>AuthStorage: AuthStorage.create(config)
alt authStorage init fails
AuthStorage-->>PiProvider: throw Error
PiProvider->>Logger: error("pi.auth_storage_init_failed", ...)
PiProvider-->>Client: throw Pi-framed error
else authStorage created
PiProvider->>ModelRegistry: ModelRegistry.create(AuthStorage)
ModelRegistry-->>PiProvider: registry
PiProvider->>ModelRegistry: registry.find(provider, modelId)
alt model found
ModelRegistry-->>PiProvider: Model
PiProvider->>Model: execute request
Model-->>PiProvider: response
PiProvider-->>Client: response
else model not found
ModelRegistry-->>PiProvider: undefined (maybe with getError() detail)
PiProvider->>Logger: error("pi.model_not_found", { provider, modelId, modelsJsonError })
PiProvider-->>Client: throw "Pi model not found" (includes models.json load error if present)
end
alt missing credentials & provider mapped to env var
PiProvider->>Logger: error("missing credentials", { piProvider }, "env-var table + 'pi /login' hint")
PiProvider-->>Client: throw missing-credentials error
else missing credentials & provider unmapped
PiProvider->>Logger: info("pi.auth_missing", { piProvider }, "env-var table hint")
PiProvider-->>Model: proceed (allow local/uncredentialed providers)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
matt2000
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/providers/src/community/pi/provider.ts (1)
89-100:⚠️ Potential issue | 🟠 Major
ModelRegistry.create()reads from disk — contradicts the "in-memory" invariant documented in comments.The class-level doc (lines 52–57) and inline comment (lines 199–201) claim that "the server never touches
~/.pi/" and that "ModelRegistry … stay in-memory". However, lines 92–93 callAuthStorage.create()andModelRegistry.create(authStorage), both of which read from disk (~/.pi/agent/auth.jsonand~/.pi/agent/models.json). This breaks the documented invariant on every request.To preserve the guarantee:
- Use
ModelRegistry.inMemory(authStorage)as the default to keep only the built-in catalog (no disk access), or- Update the comments to reflect that disk reads now happen per-request and explain why this is necessary.
Additionally,
AuthStorage.create()andModelRegistry.create()are instantiated insidesendQuery()on every invocation. Consider hoisting to module scope or caching on the provider instance to avoid repeated filesystem access per-request (similar to the cached logger at lines 46–49).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 89 - 100, The code currently calls AuthStorage.create() and ModelRegistry.create(authStorage) inside sendQuery which performs disk reads and violates the in-memory invariant; change to use ModelRegistry.inMemory(authStorage) (or otherwise call ModelRegistry.inMemory() directly) to avoid reading ~/.pi/ per request, and hoist or cache AuthStorage and ModelRegistry at module scope or as fields on the provider instance (mirroring the cached logger pattern) so they are created once instead of on every sendQuery invocation; alternatively, if disk reads are required, update the class-level and inline comments to state that AuthStorage.create() and ModelRegistry.create() read from disk and justify per-request behavior.packages/providers/src/community/pi/provider.test.ts (1)
77-128:⚠️ Potential issue | 🔴 CriticalMock must provide
ModelRegistry.create—tests callingsendQuery()will fail at runtime.Production code (provider.ts line 93) calls
ModelRegistry.create(authStorage), but the mock only exposesModelRegistry: { inMemory: mockModelRegistryInMemory }. Every test from line 195 onward that callssendQuery()will throwTypeError: ModelRegistry.create is not a functionbefore reaching any assertions.Additionally, both
provider.test.tsandsession-resolver.test.tsmock@mariozechner/pi-coding-agentwith different partial implementations (violating the guideline to usespyOn()instead and separate test invocations to avoid process-global pollution). The mock should exposecreatealongsideinMemoryto match what production actually calls.Suggested fix
const mockModelRegistryFind = mock((provider: string, modelId: string) => { if (provider === 'nonexistent') return undefined; return { id: modelId, provider, name: `${provider}/${modelId}` }; }); -const mockModelRegistryInMemory = mock(() => ({ - find: mockModelRegistryFind, -})); +const mockModelRegistryFactory = mock(() => ({ + find: mockModelRegistryFind, +})); @@ - ModelRegistry: { inMemory: mockModelRegistryInMemory }, + ModelRegistry: { + create: mockModelRegistryFactory, + inMemory: mockModelRegistryFactory, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.test.ts` around lines 77 - 128, The mock for `@mariozechner/pi-coding-agent` is missing ModelRegistry.create which production code (provider.ts -> sendQuery()) calls; add a mockModelRegistryCreate (e.g., const mockModelRegistryCreate = mock((authStorage: unknown) => mockModelRegistryInMemory(authStorage))) and export it alongside inMemory in the mock.module call (ModelRegistry: { create: mockModelRegistryCreate, inMemory: mockModelRegistryInMemory }) so sendQuery() won’t throw; also replace conflicting full-module mocks in other tests with spyOn(...) calls or reuse this same module mock to avoid process-global mock pollution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 137-142: Fix the typo and make the log a structured Pino event:
change "necesssarily" to "necessarily" in the message and move the prose into
the context (e.g., msg) while using a structured event name like
"pi.credentials_missing_warn" as the second argument to
getLog().info/getLog().warn; additionally, only suppress throwing when
parsed.provider is not in PI_PROVIDER_ENV_VARS (i.e., allow local/custom
providers to proceed) and otherwise log at warn (or keep throwing) so built-in
providers (parsed.provider checked against PI_PROVIDER_ENV_VARS) fail fast —
include envHint and loginHint in the context object when calling
getLog().warn/getLog().info.
---
Outside diff comments:
In `@packages/providers/src/community/pi/provider.test.ts`:
- Around line 77-128: The mock for `@mariozechner/pi-coding-agent` is missing
ModelRegistry.create which production code (provider.ts -> sendQuery()) calls;
add a mockModelRegistryCreate (e.g., const mockModelRegistryCreate =
mock((authStorage: unknown) => mockModelRegistryInMemory(authStorage))) and
export it alongside inMemory in the mock.module call (ModelRegistry: { create:
mockModelRegistryCreate, inMemory: mockModelRegistryInMemory }) so sendQuery()
won’t throw; also replace conflicting full-module mocks in other tests with
spyOn(...) calls or reuse this same module mock to avoid process-global mock
pollution.
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 89-100: The code currently calls AuthStorage.create() and
ModelRegistry.create(authStorage) inside sendQuery which performs disk reads and
violates the in-memory invariant; change to use
ModelRegistry.inMemory(authStorage) (or otherwise call ModelRegistry.inMemory()
directly) to avoid reading ~/.pi/ per request, and hoist or cache AuthStorage
and ModelRegistry at module scope or as fields on the provider instance
(mirroring the cached logger pattern) so they are created once instead of on
every sendQuery invocation; alternatively, if disk reads are required, update
the class-level and inline comments to state that AuthStorage.create() and
ModelRegistry.create() read from disk and justify per-request behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b27a4546-e838-4d81-84c7-f662007d9090
📒 Files selected for processing (2)
packages/providers/src/community/pi/provider.test.tspackages/providers/src/community/pi/provider.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/pi/provider.test.ts`:
- Around line 238-249: The test currently only checks that a hint is logged but
doesn't assert that the no-auth custom-provider path doesn't throw; update the
test for 'logs credential hint when Pi provider id is unknown AND no creds
available' to explicitly assert the call to PiProvider.sendQuery (via consume)
resolves without throwing (e.g., expect(...).resolves.not.toThrow or await
expect(consume(...)).resolves.toBeUndefined) and adjust the comment from
"fail-fast" to reflect that it should not throw when hinting about env-var
table; keep references to PiProvider.sendQuery and the test name so the change
is applied to the right test.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5647bf79-a0d0-4899-9490-bb66018b2da7
📒 Files selected for processing (2)
packages/providers/src/community/pi/provider.test.tspackages/providers/src/community/pi/provider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/providers/src/community/pi/provider.ts
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
matt2000
changed the title
|
@matt2000 @Wirasm Thanks for the fix here. I'm a new user, I decided to try out Archon when I saw the recent Pi announcement on YouTube and hit this exact problem while trying to use a custom Lemonade-backed model through Archon’s Pi provider. From my testing, the failure was happening during Pi model resolution, so switching from the static catalog lookup to If there are no remaining concerns, I’d appreciate getting this merged! :) |
|
We need this in for local models, please. Also same issue. This works perfectly with qwen3.6 running thru llama-cpp-local provider or other custom providers in ~/.pi/agent/models.json. Thank you for your contribution! Please keep pinging the maintainers, this one is important. |
|
Chiming in here. Also so the YouTube intro, and thought I would try it out. Using LM Studio via pi. Pi can see and connect to the models w/o issue. But when I configure archon's config.yaml: and then run archon-assist command It eventually errors out: INFO [2026-04-24 18:09:38.506 +0200]: dag_workflow_starting |
matt2000
force-pushed
the
patch-pi-local-models
branch
from
a8cc61a to
86e1c97
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/providers/src/community/pi/provider.ts (1)
107-114:⚠️ Potential issue | 🔴 CriticalFix unbound-method error on line 225: call
modelRegistry.find()directly.Line 225 passes
modelRegistry.findas a function without its receiver context, which ESLint flags as@typescript-eslint/unbound-method. More critically,ModelRegistry.findis a prototype method that readsthis(the internal registry state). Extracting it without binding breaks the method: it will either throwCannot read properties of undefinedor silently search an empty/wrong store at runtime.The wrapper
lookupPiModel+GetModelFnwas originally a workaround for Pi'sgetModelgeneric constraints. SinceModelRegistry.findalready accepts runtime strings directly, the indirection is now just dead code hiding the bug:🛠 Proposed fix
-/** - * Typed wrapper around Pi's `getModel` for a runtime-string provider/model - * pair. Pi's getModel signature constrains `TModelId` to - * `keyof MODELS[TProvider]`, which isn't knowable from a runtime string — - * the local `GetModelFn` alias is the narrowest shape that still lets us - * bypass that constraint. Isolating the escape hatch behind one searchable - * name keeps it auditable. Takes `getModel` as a parameter because the Pi - * SDK is loaded dynamically (see the header comment on this file for why). - */ -type GetModelFn = (provider: string, modelId: string) => Model<Api> | undefined; -function lookupPiModel( - getModel: GetModelFn, - provider: string, - modelId: string -): Model<Api> | undefined { - return getModel(provider, modelId); -} @@ const authStorage = piCodingAgent.AuthStorage.create(); const modelRegistry = piCodingAgent.ModelRegistry.create(authStorage); - // 2. Look up the Model via Pi's Registry. `lookupPiModel` returns - // undefined when not found; we guard explicitly below. - // Cast to the runtime-string-friendly shape — see `lookupPiModel`'s docblock. - const model = lookupPiModel(modelRegistry.find as GetModelFn, parsed.provider, parsed.modelId); + // 2. Look up the Model via Pi's Registry. `find` returns undefined when + // not found; we guard explicitly below. Call as a method to preserve + // `this` binding to the registry's internal state. + const model = modelRegistry.find(parsed.provider, parsed.modelId);Also remove the now-unused
ApiandModeltype imports from line 6.Unit tests at line 308 already cover the unhappy path (undefined return), so the direct call slots in without additional test work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 107 - 114, The wrapper lookupPiModel and its GetModelFn indirection are causing an unbound-method error when passing ModelRegistry.find as a callback; replace uses of lookupPiModel/getModel with a direct call to modelRegistry.find(...) so the method retains its receiver context, and remove the now-unused lookupPiModel and GetModelFn definitions plus the unused Api and Model type imports referenced at the top of the file.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.ts (1)
233-252: Stale step-3 heading —AuthStorage.create()moved up to line 220.The prose still reads "3. Build AuthStorage.
AuthStorage.create()reads ~/.pi/agent/auth.json …", but the actual construction now happens in step 2 (line 220) alongside the registry. Readers following the numbered steps will hunt for a build that isn't here. The resolution-order detail is still valuable — just relabel and trim:📝 Suggested rewording
- // 3. Build AuthStorage. `AuthStorage.create()` reads ~/.pi/agent/auth.json - // (or $PI_CODING_AGENT_DIR/auth.json), so any credential the user has - // populated via `pi` → `/login` (OAuth subscriptions: Claude Pro/Max, + // 3. Credential resolution. The `AuthStorage` built above reads + // ~/.pi/agent/auth.json (or $PI_CODING_AGENT_DIR/auth.json), so any + // credential the user populated via `pi` → `/login` (OAuth + // subscriptions: Claude Pro/Max, // ChatGPT Plus, GitHub Copilot, Gemini CLI, Antigravity) or by editing // the file directly (api_key entries) is picked up transparently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 233 - 252, Update the stale "3. Build AuthStorage" heading and surrounding prose to reflect that AuthStorage.create() is now constructed earlier (at the registry step around where AuthStorage.create() is called), e.g., remove or renumber the "3." label and change wording to indicate this is an explanatory note about AuthStorage resolution rather than a build step; keep the Pi internal resolution order list and the OAuth refresh note but trim any lines that claim the build occurs here and instead reference AuthStorage.create() where it actually runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 343-348: The comment block in provider.ts is broken: the
parenthetical about piConfig.enableExtensions interrupts the sentence so the
fragment "discovery except for explicitly-passed skill paths." is orphaned;
update the comment so it is a single grammatical sentence (or two clear
sentences) that preserves the intended meaning. Specifically, in the comment
near the Resource loader/enableExtensions text (reference
piConfig.enableExtensions and the description of the community extension
ecosystem), rephrase to either move the parenthetical so the main clause reads
"...Resource loader still suppresses filesystem discovery except for
explicitly-passed skill paths." and then append a separate sentence describing
Pi's community extension ecosystem when piConfig.enableExtensions is true, or
combine them into one coherent sentence that includes both the suppression of
filesystem discovery and the exception for extensions. Ensure the comment
mentions piConfig.enableExtensions, the extension paths
(~/.pi/agent/extensions/) and `pi install npm:<pkg>` exactly as before.
---
Outside diff comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 107-114: The wrapper lookupPiModel and its GetModelFn indirection
are causing an unbound-method error when passing ModelRegistry.find as a
callback; replace uses of lookupPiModel/getModel with a direct call to
modelRegistry.find(...) so the method retains its receiver context, and remove
the now-unused lookupPiModel and GetModelFn definitions plus the unused Api and
Model type imports referenced at the top of the file.
---
Nitpick comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 233-252: Update the stale "3. Build AuthStorage" heading and
surrounding prose to reflect that AuthStorage.create() is now constructed
earlier (at the registry step around where AuthStorage.create() is called),
e.g., remove or renumber the "3." label and change wording to indicate this is
an explanatory note about AuthStorage resolution rather than a build step; keep
the Pi internal resolution order list and the OAuth refresh note but trim any
lines that claim the build occurs here and instead reference
AuthStorage.create() where it actually runs.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71822697-8d45-4646-997e-f9709e779b17
📒 Files selected for processing (2)
packages/providers/src/community/pi/provider.test.tspackages/providers/src/community/pi/provider.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.ts (1)
213-232: Stale step-numbering / narrative in the AuthStorage comment block.This block opens with "3. Build AuthStorage.
AuthStorage.create()reads ~/.pi/agent/auth.json…", butauthStorageis now constructed at line 201 (before the model lookup), so the "3." label and the "Build AuthStorage" framing no longer match the code below it — what actually starts here is the runtime-override step, not the construction. Consider retitling (e.g., "3. Apply per-request API key override on the existing AuthStorage…") and moving the auth-file/OAuth explainer up next to line 201 where the instance is actually created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 213 - 232, Update the comment to reflect actual ordering: clarify that authStorage is already constructed earlier via AuthStorage.create() and this section is applying a per-request runtime override using setRuntimeApiKey; retitle the header from "3. Build AuthStorage" to something like "3. Apply per-request API key override to existing AuthStorage" and move or copy the detailed explanation about AuthStorage.create(), ~/.pi/agent/auth.json, OAuth refresh and resolution order up next to where authStorage is instantiated (AuthStorage.create()) so the narrative matches the code flow around authStorage and setRuntimeApiKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 213-232: Update the comment to reflect actual ordering: clarify
that authStorage is already constructed earlier via AuthStorage.create() and
this section is applying a per-request runtime override using setRuntimeApiKey;
retitle the header from "3. Build AuthStorage" to something like "3. Apply
per-request API key override to existing AuthStorage" and move or copy the
detailed explanation about AuthStorage.create(), ~/.pi/agent/auth.json, OAuth
refresh and resolution order up next to where authStorage is instantiated
(AuthStorage.create()) so the narrative matches the code flow around authStorage
and setRuntimeApiKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c50e0d55-a425-4fee-af4d-6d60dd712157
📒 Files selected for processing (1)
packages/providers/src/community/pi/provider.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/providers/src/community/pi/provider.ts (1)
254-258:⚠️ Potential issue | 🟡 MinorLog
msgargument still isn't a structured event name.The typo and the env-mapped vs. unmapped branch split were addressed, but Pino's second positional arg here is still a prose sentence rather than a
{domain}.{action}_{state}event. Every other log in this file (pi.config_env_applied,pi.session_started,pi.prompt_completed,pi.prompt_failed) follows that format, so log queries can't group on this one. Move the prose into the merging object (or amsgfield) and use something likepi.credentials_missingas the event name.✏️ Suggested edit
- const envHint = `Provider '${parsed.provider}' is not in the Archon adapter's env-var table — file an issue if you want a shortcut env var for it.`; - const loginHint = `Or run \`pi\` and type \`/login\` locally to authenticate '${parsed.provider}' via OAuth; credentials land in ~/.pi/agent/auth.json and are picked up automatically.`; - // No credentials for unmapped providers is not necessarily a problem, e.g., local models. - getLog().info( - { piProvider: parsed.provider }, - `No Pi credentials found for provider. ${envHint} ${loginHint}` - ); + const envHint = `Provider '${parsed.provider}' is not in the Archon adapter's env-var table — file an issue if you want a shortcut env var for it.`; + const loginHint = `Or run \`pi\` and type \`/login\` locally to authenticate '${parsed.provider}' via OAuth; credentials land in ~/.pi/agent/auth.json and are picked up automatically.`; + // No credentials for unmapped providers is not necessarily a problem, e.g., local models. + getLog().info( + { + piProvider: parsed.provider, + envHint, + loginHint, + detail: 'No Pi credentials found for provider', + }, + 'pi.credentials_missing' + );As per coding guidelines: "event naming format:
{domain}.{action}_{state}; avoid generic events like 'processing' or 'handling'".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 254 - 258, The log call using getLog().info currently passes a prose string as the second positional arg; change it to use a structured event name (e.g., "pi.credentials_missing") as the second arg and move the human-readable sentence into the merged object (for example a msg or message field) alongside parsed.provider, envHint and loginHint so the log becomes a structured event consistent with other events like pi.config_env_applied and pi.session_started; update the getLog().info invocation where parsed.provider is referenced to include these fields in the object and use "pi.credentials_missing" as the event name.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.ts (1)
201-217: Step-numbered narration is now out of order with code flow.With
AuthStorage.create()hoisted above the model lookup, the numbered comments no longer match the execution order: step "3. Build AuthStorage" (line 213) narrates code that actually ran before step "2. Look up the Model" (line 203). Since the block at 213-232 is now purely explanatory (the construction already happened at line 201), consider either renumbering or moving the doc block up to theAuthStorage.create()call so readers following the numbered steps don't get disoriented.♻️ One way to reorganize
- const authStorage = piCodingAgent.AuthStorage.create(); - const modelRegistry = piCodingAgent.ModelRegistry.create(authStorage); - // 2. Look up the Model via Pi's Registry. `modelRegistry.find` returns - // undefined when not found; we guard explicitly below. + // 2. Build AuthStorage + ModelRegistry. `AuthStorage.create()` reads + // ~/.pi/agent/auth.json (or $PI_CODING_AGENT_DIR/auth.json), so + // credentials populated via `pi` → `/login` (OAuth: Claude Pro/Max, + // ChatGPT Plus, Copilot, Gemini CLI, Antigravity) or edited directly + // (api_key entries) are picked up transparently. + // + // Pi's internal resolution order: + // 1. runtime override (setRuntimeApiKey below) + // 2. auth.json api_key entry + // 3. auth.json oauth entry (auto-refreshes expired tokens) + // 4. env var fallback (Pi's getEnvApiKey, e.g. ANTHROPIC_API_KEY) + const authStorage = piCodingAgent.AuthStorage.create(); + const modelRegistry = piCodingAgent.ModelRegistry.create(authStorage); + + // 3. Look up the Model via Pi's Registry. `modelRegistry.find` returns + // undefined when not found; we guard explicitly below. const model = modelRegistry.find(parsed.provider, parsed.modelId); if (!model) { throw new Error( `Pi model not found: provider='${parsed.provider}' model='${parsed.modelId}'. ` + 'See https://github.com/badlogic/pi-mono/blob/main/packages/ai/src/models.generated.ts for the Pi model catalog.' ); } - // 3. Build AuthStorage. `AuthStorage.create()`, above, read ~/.pi/agent/auth.json - // (or $PI_CODING_AGENT_DIR/auth.json), so any credentials the user has - // populated via `pi` → `/login` (OAuth subscriptions: Claude Pro/Max, - // ChatGPT Plus, GitHub Copilot, Gemini CLI, Antigravity) or by editing - // the file directly (api_key entries) are picked up transparently. - // - // Per-request env vars override the file via setRuntimeApiKey — this - // mirrors Claude's process-env + request-env merge pattern and - // ensures codebase-scoped env vars (from .archon/config.yaml `env:`) - // win over the user's global Pi login. - // - // Pi's internal resolution order: - // 1. runtime override (our setRuntimeApiKey below) - // 2. auth.json api_key entry - // 3. auth.json oauth entry (auto-refreshes expired tokens) - // 4. env var fallback (Pi's getEnvApiKey, e.g. ANTHROPIC_API_KEY) - // - // OAuth refresh note: Pi refreshes expired access tokens against the - // provider's OAuth server and rewrites ~/.pi/agent/auth.json under a - // file lock (same mechanism pi CLI uses — safe for concurrent access). + // 4. Per-request env vars override the file via setRuntimeApiKey — this + // mirrors Claude's process-env + request-env merge pattern and + // ensures codebase-scoped env vars (from .archon/config.yaml `env:`) + // win over the user's global Pi login. Pi refreshes expired OAuth + // tokens under a file lock (same mechanism as the pi CLI).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 201 - 217, The numbered explanatory comments are out of order: AuthStorage.create() is called before the "2. Look up the Model" block but the narrative labels step 3 as building AuthStorage; fix by either moving the paragraph that begins "3. Build AuthStorage..." up to directly above the AuthStorage.create() call (so the steps read 1→2→3 in execution order) or renumber the steps so the narration matches the actual flow (e.g., make AuthStorage the earlier step and adjust the "2. Look up the Model" text); update references to AuthStorage.create(), modelRegistry.find, and the model/parsed variables to ensure the comment sits adjacent to the code it describes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 254-258: The log call using getLog().info currently passes a prose
string as the second positional arg; change it to use a structured event name
(e.g., "pi.credentials_missing") as the second arg and move the human-readable
sentence into the merged object (for example a msg or message field) alongside
parsed.provider, envHint and loginHint so the log becomes a structured event
consistent with other events like pi.config_env_applied and pi.session_started;
update the getLog().info invocation where parsed.provider is referenced to
include these fields in the object and use "pi.credentials_missing" as the event
name.
---
Nitpick comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 201-217: The numbered explanatory comments are out of order:
AuthStorage.create() is called before the "2. Look up the Model" block but the
narrative labels step 3 as building AuthStorage; fix by either moving the
paragraph that begins "3. Build AuthStorage..." up to directly above the
AuthStorage.create() call (so the steps read 1→2→3 in execution order) or
renumber the steps so the narration matches the actual flow (e.g., make
AuthStorage the earlier step and adjust the "2. Look up the Model" text); update
references to AuthStorage.create(), modelRegistry.find, and the model/parsed
variables to ensure the comment sits adjacent to the code it describes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2e35f61-65bb-4f3d-bc7e-06995de31776
📒 Files selected for processing (1)
packages/providers/src/community/pi/provider.ts
|
@matt2000 hard-coded model names in Archon default workflows/commands will cause node A. Docs updates that let local LLM users know forking default template-baked commands/workflows is nessecary to fix B. Improve actual template regeneration and catch SDK Error exceptions to call regeneration of a template/retry on SDK Error fail with user's model settings injected (baked failsafe execution path for local LLM users), where the default provided templates regenerate with properly updated values to avoid node SDK errors when user is using local LLMs -- will require more complex modifications and may inflate the size/scope of this PR, but will commit the outstanding issue to code in the case of Unfortunately there more upstream to this one than meet the eye. >.<` Good work, though. |
I don't really use default workflows much, so this isnt a priority for me. But I think the real fix is that Archon should have configurable "model aliases" that are interchangable with model IDs, like "default", "small," "tooluser," etc, that can default to anthropic models or whatever Cole likes, but be overridden in user config. All the default workflows should only use aliases, and never "concrete" model names. |
matt2000
force-pushed
the
patch-pi-local-models
branch
from
88c3a30 to
f3d1657
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.ts (1)
213-233: Step numbering in the comment block is now misaligned.This block is titled "3. Build AuthStorage" and opens with "
AuthStorage.create(), above, read …", butAuthStorageis now constructed at line 201 (before the model lookup labeled "2."). The step headers read a bit confusingly — consider renumbering so the comment reflects the new ordering (e.g., fold the AuthStorage construction into step 2, or relabel this block "Resolve credentials" without a step number). Purely editorial; no behavior impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 213 - 233, The comment block heading "3. Build AuthStorage" is out of order relative to where AuthStorage is actually constructed (AuthStorage.create() / AuthStorage constructor appears earlier near the model lookup labeled "2."); update the comment to reflect the actual flow by either renumbering the step to match the current ordering, folding the AuthStorage construction discussion into the "2." section, or replace the step number with a neutral label like "Resolve credentials" while keeping references to AuthStorage.create() and setRuntimeApiKey so readers can locate the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 213-233: The comment block heading "3. Build AuthStorage" is out
of order relative to where AuthStorage is actually constructed
(AuthStorage.create() / AuthStorage constructor appears earlier near the model
lookup labeled "2."); update the comment to reflect the actual flow by either
renumbering the step to match the current ordering, folding the AuthStorage
construction discussion into the "2." section, or replace the step number with a
neutral label like "Resolve credentials" while keeping references to
AuthStorage.create() and setRuntimeApiKey so readers can locate the relevant
code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08391f05-9e64-4d0a-8a2d-85fd2d551bf3
📒 Files selected for processing (2)
packages/providers/src/community/pi/provider.test.tspackages/providers/src/community/pi/provider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/providers/src/community/pi/provider.test.ts
This makes sense, as I had indicated most local LLM users will need to procure their own workflows anyway to deal with model-baked token directives for tool calls/specific baked instruction calling sequences their local models use. Aliases is a good idea but may be outside the scope of this PR. |
100%. I have no intention of implementing that here. I'm trying to keep this PR focused on the minimum to get local models working at all. |
|
@Wirasm I think this is ready to go. Can you take a look and let me know if you need anything else from me in order to get this merged? |
|
I saw some new YouTube uploads, I have a feeling there's some other priorities right now. Till this is merged I'm using your fork and tellin' my buds! |
|
Hello, sorry for the wait here, there have been a lot of PR's starting a check on this now, will test also some archon workflows on this so sorry if there is spam |
Multi-Agent Review Summary — PR #1284Ran code review, test analysis, silent-failure hunt, and docs-impact across the diff. Verdict: NEEDS FIXES (small) — direction is good, scope is disciplined, but a few items should be addressed before merge. Reference IDs: Strengths
Important IssuesI1 — Comment block at I2 — Pino log shape collides with reserved getLog().info(
{ piProvider: parsed.provider, msg: \`No Pi credentials found...\` },
'pi.auth_missing'
);The second-positional arg getLog().info(
{ piProvider: parsed.provider, envHint, loginHint },
'pi.auth_missing'
);The corresponding test at I3 — Class doc / inline comments still assert an "in-memory / never touches ~/.pi/" invariant that this PR breaks
I4 — I5 — const model = modelRegistry.find(parsed.provider, parsed.modelId);
if (!model) {
const loadError = modelRegistry.getError?.();
getLog().error(
{ piProvider: parsed.provider, modelId: parsed.modelId, loadError },
'pi.model_not_found'
);
throw new Error(
\`Pi model not found: provider='\${parsed.provider}' model='\${parsed.modelId}'.\${loadError ? \` Note: ~/.pi/agent/models.json load failed — \${loadError}\` : ''} See https://github.com/badlogic/pi-mono/blob/main/packages/ai/src/models.generated.ts for the catalog.\`
);
}This addresses the "why doesn't my LM Studio model show up" debugging path that several PR commenters are about to hit. I6 — Per-request disk I/O on a hot path ( I7 (tests) — No assertion that expect(mockModelRegistryCreate).toHaveBeenCalledWith(authStorageInstance);catches any future refactor that accidentally drops the argument. I8 (tests) — No coverage for DocumentationI9 —
Suggested edits:
SuggestionsS1 — S2 — Env-mapped throw at Notes on the stalsma report in PR commentsThe user reporting that LM Studio still fails with "model not found" after this PR has a Recommended Action Order
Direction is solid and the community demand is real — happy to re-review after fixes. |
Address review feedback on the ModelRegistry migration:
- Surface modelRegistry.getError() in the "model not found" message and
emit a pi.model_not_found structured event. ModelRegistry catches
models.json parse/validation errors into an internal loadError rather
than throwing, so a malformed entry (e.g. custom provider missing
baseUrl) was silently degrading to a vague "model not found" with no
hint at the actual cause.
- Wrap AuthStorage.create() and ModelRegistry.create() in try/catch and
rethrow with Pi-framed context. A malformed ~/.pi/agent/auth.json
was previously surfacing as a raw SDK SyntaxError stack with no
indication of where it came from.
- Renumber the Step 2 / Step 3 / Step 4 comment block to match the
actual code order after auth+registry construction was hoisted earlier
in the function.
- Move the missing-credentials log message out of the Pino-reserved
msg field (which would collide with the log line message) into
separate envHint/loginHint structured fields. The pi.auth_missing
event name stays as the second-positional arg per the
{domain}.{action}_{state} convention.
Tests:
- Update the existing auth_missing assertion to match the new log shape.
- Add a test that ModelRegistry.create receives the AuthStorage instance
(the wiring that makes the headline fix work for custom models).
- Add a test that AuthStorage.create() failure surfaces as
pi.auth_storage_init_failed with the underlying error message.
- Add a test that models.json load errors flow into the not-found
message and the pi.model_not_found structured log.
After the ModelRegistry migration, providers not in PI_PROVIDER_ENV_VARS (LM Studio, ollama, llamacpp, custom OpenAI-compatible endpoints configured in ~/.pi/agent/models.json) work without any Archon-side credentials — Archon logs a pi.auth_missing info notice and continues. Add a "Local / custom providers" subsection under Authenticate so users find this path without reading the source. Also call it out in the section opener alongside the cloud backends.
|
Pushed two follow-up commits addressing the review (maintainer edit, since the PR allows it):
Deferred (per author's call):
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/providers/src/community/pi/provider.test.ts (1)
389-402:⚠️ Potential issue | 🟡 MinorStale comment: the adapter no longer "rejects unknown providers."
Inline comment claims
'nonexistent' is handled in mockModelRegistryFind to return undefined, but the adapter rejects unknown providers.After this PR'spi.auth_missingchange, the adapter no longer rejects unknown providers — it logs and continues — so the rationale for switching to a known-provider/unknown-model is now "to bypass the credential-missing log path," not "to avoid the unknown-provider rejection." Worth updating so the next reader doesn't assume the old behavior still exists.📝 Suggested rewrite
- // 'nonexistent' is handled in mockModelRegistryFind to return undefined, but - // the adapter rejects unknown providers. To exercise - // the not-found branch, use a known provider but unknown modelId by - // temporarily swapping mockModelRegistryFind to always return undefined. + // Use a known provider (`google`, with GEMINI_API_KEY set) so the + // credential path resolves cleanly and we exercise *only* the + // ModelRegistry.find()-returns-undefined branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.test.ts` around lines 389 - 402, The test comment is outdated about adapter behavior; update the inline comment in packages/providers/src/community/pi/provider.test.ts to reflect that after the pi.auth_missing change the adapter no longer rejects unknown providers but logs and continues. Specifically, revise the comment around mockModelRegistryFind.mockImplementationOnce and the call to new PiProvider().sendQuery(...) to state the swap is done to bypass the credential-missing log path (not to avoid an unknown-provider rejection), and mention mockModelRegistryFind and PiProvider.sendQuery by name so readers can locate the affected test logic.packages/providers/src/community/pi/provider.ts (1)
251-457:⚠️ Potential issue | 🟡 MinorSection comment numbering has duplicate "4." and "5." after the registry/lookup insertions.
The new sections "2. Build AuthStorage + ModelRegistry" (line 201) and "3. Look up the model" (line 227) were inserted, but the downstream comment numbering wasn't bumped, so the file now reads
1, 2, 3, 4, 4, 5, 5, 6(Resolve creds → Translate nodeConfig → Session management → Structured output → Bridge events) and the inner sub-bullets4a–4fstraddle two different "4." sections. This was specifically called out as a needed fix during prior review.♻️ Suggested renumbering
- // 4. Resolve credentials. authStorage already loaded ~/.pi/agent/auth.json + // 4. Resolve credentials. authStorage already loaded ~/.pi/agent/auth.json @@ - // 4. Translate Archon nodeConfig to Pi SDK options. All three translations + // 5. Translate Archon nodeConfig to Pi SDK options. All three translations @@ - // 4a. thinkingLevel: covers `thinking`/`effort` nodeConfig fields. + // 5a. thinkingLevel: covers `thinking`/`effort` nodeConfig fields. @@ - // 4b. tools: covers allowed_tools / denied_tools. `undefined` leaves Pi + // 5b. tools: covers allowed_tools / denied_tools. `undefined` leaves Pi @@ - // 4c. systemPrompt: request-level (AgentRequestOptions) wins over + // 5c. systemPrompt: request-level (AgentRequestOptions) wins over @@ - // 4d. skills: Archon uses name references (e.g. `skills: [agent-browser]`). + // 5d. skills: Archon uses name references (e.g. `skills: [agent-browser]`). @@ - // 5. Session management. Pi stores each session as a JSONL file under + // 6. Session management. Pi stores each session as a JSONL file under @@ - // 4e. Extension flag pass-through. Must happen before bindExtensions + // 6a. Extension flag pass-through. Must happen before bindExtensions @@ - // 4f. Bind UI context (so ctx.hasUI is true and ctx.ui.notify() forwards + // 6b. Bind UI context (so ctx.hasUI is true and ctx.ui.notify() forwards @@ - // 5. Structured output (best-effort). Pi has no SDK-level JSON schema + // 7. Structured output (best-effort). Pi has no SDK-level JSON schema @@ - // 6. Bridge callback-based events to the async generator contract. + // 8. Bridge callback-based events to the async generator contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 251 - 457, The section comment numbers are out of sync after inserting "2. Build AuthStorage + ModelRegistry" and "3. Look up the model"; update the subsequent top-level comment numbers and their sublabels so they increment correctly (so "Resolve credentials" becomes 4, "Translate Archon nodeConfig to Pi SDK options" becomes 5 with its sublabels 5a–5f, "Session management" becomes 6, "Structured output" becomes 7, and "Bridge callback..." becomes 8). Edit the inline numbered comment prefixes around the blocks that call resolvePiThinkingLevel, resolvePiTools, resolvePiSkills, resolvePiSession, createAgentSession, and augmentPromptForJsonSchema so the labels and any references (e.g., "4a–4f", "5.", "6.") match the new numbering.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.ts (1)
213-225: Optional: cacheAuthStorage/ModelRegistryper Pi-config-dir to avoid disk I/O on everysendQuery().Both
AuthStorage.create()andModelRegistry.create()synchronously read~/.pi/agent/{auth.json,models.json}on every call. The inline comment correctly justifies why you don't cache (users edit those files between calls and OAuth refresh rewritesauth.jsonunder a file lock), but in workflow runs with many sequential Pi nodes this is N redundant reads per invocation. A simple TTL cache (e.g. 1–2 s) keyed onprocess.env.PI_CODING_AGENT_DIR ?? '~/.pi/agent'would preserve the "edit-and-rerun" UX while collapsing tight bursts. Treat this as a follow-up — not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/community/pi/provider.ts` around lines 213 - 225, AuthStorage.create() and ModelRegistry.create() are being called on every sendQuery() causing redundant disk reads; add a small TTL cache keyed by the Pi agent directory (process.env.PI_CODING_AGENT_DIR ?? '~/.pi/agent') that returns cached instances of AuthStorage and ModelRegistry for e.g. 1–2 seconds, and have sendQuery() (or the code that currently calls piCodingAgent.AuthStorage.create / piCodingAgent.ModelRegistry.create) use the cache instead of creating new instances each time; ensure the cache still re-reads after TTL expires so edits or OAuth refreshes are picked up and preserve existing error handling around pi.auth_storage_init_failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 213-217: The code defensively uses optional chaining when invoking
modelRegistry.getError (calling modelRegistry.getError?.()), but
ModelRegistry.create guarantees a public getError(): string | undefined method,
so remove the unnecessary optional chaining and call modelRegistry.getError()
directly; update the invocation where modelRegistry.getError?.() is used (in the
block after ModelRegistry.create) to modelRegistry.getError() to match the
documented API and existing tests.
---
Outside diff comments:
In `@packages/providers/src/community/pi/provider.test.ts`:
- Around line 389-402: The test comment is outdated about adapter behavior;
update the inline comment in
packages/providers/src/community/pi/provider.test.ts to reflect that after the
pi.auth_missing change the adapter no longer rejects unknown providers but logs
and continues. Specifically, revise the comment around
mockModelRegistryFind.mockImplementationOnce and the call to new
PiProvider().sendQuery(...) to state the swap is done to bypass the
credential-missing log path (not to avoid an unknown-provider rejection), and
mention mockModelRegistryFind and PiProvider.sendQuery by name so readers can
locate the affected test logic.
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 251-457: The section comment numbers are out of sync after
inserting "2. Build AuthStorage + ModelRegistry" and "3. Look up the model";
update the subsequent top-level comment numbers and their sublabels so they
increment correctly (so "Resolve credentials" becomes 4, "Translate Archon
nodeConfig to Pi SDK options" becomes 5 with its sublabels 5a–5f, "Session
management" becomes 6, "Structured output" becomes 7, and "Bridge callback..."
becomes 8). Edit the inline numbered comment prefixes around the blocks that
call resolvePiThinkingLevel, resolvePiTools, resolvePiSkills, resolvePiSession,
createAgentSession, and augmentPromptForJsonSchema so the labels and any
references (e.g., "4a–4f", "5.", "6.") match the new numbering.
---
Nitpick comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 213-225: AuthStorage.create() and ModelRegistry.create() are being
called on every sendQuery() causing redundant disk reads; add a small TTL cache
keyed by the Pi agent directory (process.env.PI_CODING_AGENT_DIR ??
'~/.pi/agent') that returns cached instances of AuthStorage and ModelRegistry
for e.g. 1–2 seconds, and have sendQuery() (or the code that currently calls
piCodingAgent.AuthStorage.create / piCodingAgent.ModelRegistry.create) use the
cache instead of creating new instances each time; ensure the cache still
re-reads after TTL expires so edits or OAuth refreshes are picked up and
preserve existing error handling around pi.auth_storage_init_failed.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: df057206-81aa-4845-9c20-fa255cf9d44a
📒 Files selected for processing (3)
packages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/providers/src/community/pi/provider.test.tspackages/providers/src/community/pi/provider.ts
| let authStorage: ReturnType<typeof piCodingAgent.AuthStorage.create>; | ||
| let modelRegistry: ReturnType<typeof piCodingAgent.ModelRegistry.create>; | ||
| try { | ||
| authStorage = piCodingAgent.AuthStorage.create(); | ||
| modelRegistry = piCodingAgent.ModelRegistry.create(authStorage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for the ModelRegistry definition / getError member in any installed pi-coding-agent typings.
fd -t f -i 'model-?registry' --exec rg -n -C3 'getError|loadError|class ModelRegistry|^\s*(static\s+)?create\b' {} \;
fd -t f -i 'pi-coding-agent' -e d.ts --exec rg -n -C2 'getError|loadError|ModelRegistry' {} \;Repository: coleam00/Archon
Length of output: 41
🌐 Web query:
@mariozechner/pi-coding-agent ModelRegistry getError loadError API
💡 Result:
In the @mariozechner/pi-coding-agent package (part of the badlogic/pi-mono GitHub repository), the ModelRegistry class manages loading and caching models from built-in sources and a custom models.json file. It includes a private loadError field that stores any error encountered during loading custom models. The relevant API methods are: - ModelRegistry.create(authStorage: AuthStorage, modelsJsonPath?: string): ModelRegistry - Creates a new instance, optionally loading from a custom models.json path. - ModelRegistry.inMemory(authStorage: AuthStorage): ModelRegistry - Creates without loading from disk. - refresh: void - Reloads models from disk, clears caches, resets loadError to undefined, then calls loadModels. - getError: string | undefined - Returns the loadError if models.json failed to load (undefined if no error or after successful refresh). The loadError is set in the private loadModels method if this.loadCustomModels(this.modelsJsonPath) returns an error. Even if custom models fail, built-in models are retained. {value: In the @mariozechner/pi-coding-agent package (part of the badlogic/pi-mono GitHub repository), the ModelRegistry class manages loading and caching models from built-in sources and a custom models.json file. It includes a private loadError field that stores any error encountered during loading custom models. The relevant API methods are: - ModelRegistry.create(authStorage: AuthStorage, modelsJsonPath?: string): ModelRegistry - Creates a new instance, optionally loading from a custom models.json path. - ModelRegistry.inMemory(authStorage: AuthStorage): ModelRegistry - Creates without loading from disk. - refresh: void - Reloads models from disk, clears caches, resets loadError to undefined, then calls loadModels. - getError: string | undefined - Returns the loadError if models.json failed to load (undefined if no error or after successful refresh). The loadError is set in the private loadModels method if this.loadCustomModels(this.modelsJsonPath) returns an error. Even if custom models fail, built-in models are retained., citations: [2,12], confidence: high}
Citations:
- 1: https://github.com/badlogic/pi-mono/blob/main/packages/coding-agent/src/core/model-registry.ts
- 2: https://cdn.jsdelivr.net/npm/@mariozechner/pi-coding-agent@0.64.0/dist/core/model-registry.d.ts
🏁 Script executed:
# Check the provider.ts file around lines 213-217 and 233
cat -n packages/providers/src/community/pi/provider.ts | sed -n '210,240p'Repository: coleam00/Archon
Length of output: 1751
🏁 Script executed:
# Check the test file around lines 317-320
cat -n packages/providers/src/community/pi/provider.test.ts | sed -n '310,330p'Repository: coleam00/Archon
Length of output: 1246
Remove unnecessary optional chaining from getError() call.
Line 233 calls modelRegistry.getError?.() defensively, but getError() is a documented public method on ModelRegistry that always exists (returns string | undefined). The optional chaining is unnecessary; call it directly as modelRegistry.getError(). The test correctly shows this expectation (line 319).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/community/pi/provider.ts` around lines 213 - 217, The
code defensively uses optional chaining when invoking modelRegistry.getError
(calling modelRegistry.getError?.()), but ModelRegistry.create guarantees a
public getError(): string | undefined method, so remove the unnecessary optional
chaining and call modelRegistry.getError() directly; update the invocation where
modelRegistry.getError?.() is used (in the block after ModelRegistry.create) to
modelRegistry.getError() to match the documented API and existing tests.
Resolves the conflict on packages/providers/src/community/pi/provider.ts. - Upstream's coleam00#1284 (ModelRegistry) and our shared/structured-output extraction both touch the same region. Upstream removed the inline augmentPromptForJsonSchema call site that coleam00#1284 didn't itself need; our branch had moved that function to shared/. Resolution keeps the shared/ extraction (single source of truth for both Pi and Copilot) and re-exports it from pi/provider.ts under the original name so existing Pi callers and tests stay byte-for-byte. - Drops the dead-code lookupPiModel/GetModelFn helper that was a stale leftover from an earlier merge attempt — never had a caller and was superseded by ModelRegistry upstream. - Picks up coleam00#1431 — moves e2e-copilot-smoke.yaml under test-workflows/ alongside the other e2e-*.yaml smokes. Adds e2e-copilot-all-features smoke (mirrors e2e-minimax-smoke): basic chat (PONG) + effort: high (17×23 = 391) + denied_tools: [shell, write] (DENIED_OK) + output_format JSON (best-effort via shared/ structured-output, parsed as {model, ok}). Single bash assert verifies all four end-to-end. Doubles as adoption docs. Validation: - bun run check:bundled, type-check, lint, format:check — all green. - bun --filter @archon/providers test — fully green (Pi included). - Live smoke (Linux + gpt-5-mini, Copilot CLI 1.0.36): e2e-copilot-smoke → 12s, PASS e2e-copilot-all-features → 25s, PASS (all four caps) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 participants
Summary
Closes #1096
Describe this PR in 2-5 bullets:
find()method of the ModelRegistry that we have to construct anyway, which will load the user's Pi model configurations.This also has the side benefit of removing the lookupPiModel method which was doing some weird backflips to get type casting that ModelRegistry.find already does.
There was a comment about not hitting
~/.pi, but this was always a lie, because we had to read auth.json. If we want to minimize disk reads by caching the 2 disk reads, I suggest we address that in a follow-up task unless we have actual evidence that this change introduces significant performance burden to existing use cases.UX Journey
Context
User has Pi configured to use custom models, perhaps fine-tuned models on a major cloud provider, perhaps standard models on a less known but compatible cloud provider, or perhaps a model running locally via ollama, llamacpp, mlx, or similar.
Before
This works fine for Pi, but when Archon is configured to use the Pi provider, with the same model, Archon reports the model as not found.
After
Now this works fine in Archon too.
Label Snapshot
risk: lowsize: Sadaptersproviders:community.piChange Metadata
bug|featureadaptersValidation Evidence (required)
Commands and result summary:
n/a
Security Impact (required)
No)No)No)No)Yes, describe risk and mitigation:n/a
Compatibility / Migration
Yes)No)No)n/a
Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Rollback Plan (required)
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Documentation