fix(providers/pi): lazy-load Pi SDK to unbreak compiled archon binary#1355
Merged
fix(providers/pi): lazy-load Pi SDK to unbreak compiled archon binary#1355
Conversation
Pi's @mariozechner/pi-coding-agent/dist/config.js runs
`readFileSync(getPackageJsonPath(), 'utf-8')` at module top level. Inside
a compiled archon binary `getPackageJsonPath()` resolves to
`dirname(process.execPath) + '/package.json'`, which doesn't exist next
to `/usr/local/bin/archon` — so archon crashes with ENOENT at startup
before any command runs. v0.3.7's release binary build appeared to
compile clean (CI fell over first on an unrelated --bytecode issue) but
even the fixed-bytecode binary fails the same way locally.
Convert all Pi SDK value imports and Pi-dependent helper imports to
dynamic imports inside `PiProvider.sendQuery()`. Type-only imports stay
static (erased by TS). Effect: registering the Pi provider and creating
an instance no longer loads Pi's SDK — load happens only when a Pi
workflow actually runs. Claude and Codex providers keep their static
import style (their SDKs have no module-init side effects that fail in
a binary); see the file header comment in provider.ts for why Pi is the
deliberate outlier.
Class constructors (AuthStorage, ModelRegistry, SettingsManager) are
accessed via `piCodingAgent.X` rather than destructured to keep
eslint's naming-convention rule happy without a disable.
Add a regression test (provider-lazy-load.test.ts) that mocks
@mariozechner/pi-coding-agent and @mariozechner/pi-ai, walks the same
registerCommunityProviders() → getAgentProvider('pi') path the CLI and
server take, and asserts neither SDK module was loaded. Runs in its own
`bun test` invocation (mock.module is process-wide).
Verified locally: `bun build --compile --minify --target=bun-darwin-arm64`
produces a binary whose `archon version` runs cleanly and reports
Build: binary, where previously every command crashed at boot.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR makes the Pi community provider lazy-load its SDK and helper modules at runtime (inside sendQuery), adds a regression test to ensure no eager resolution during registration, updates a test script, and tweaks a type-only import in the Pi UI context stub and the CHANGELOG. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Registry
participant PiProvider
participant PiSDK as Pi SDK Modules
Note over Client,PiSDK: Old Flow (Static Imports)
Client->>PiProvider: Import provider module
PiProvider->>PiSDK: Static import (eager load)
Note over PiSDK: Modules loaded at import time
Note over Client,PiSDK: New Flow (Dynamic/Lazy)
Client->>Registry: registerCommunityProviders()
Registry->>PiProvider: Request provider metadata (no Pi SDK import)
Client->>PiProvider: sendQuery(...)
PiProvider->>PiSDK: Dynamic import() of SDK & helpers
PiSDK->>PiProvider: Return runtime bindings
PiProvider->>PiProvider: Create agent session / handle query
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
Author
PR Review Summary — multi-agentRan code-reviewer, docs-impact, pr-test-analyzer, comment-analyzer, silent-failure-hunter, and code-simplifier in parallel. PR is small (3 files, +115/-23) and the core fix is correct — converting static Pi SDK imports in Critical issuesNone — the lazy-load mechanism is structurally sound. Important issues
Suggestions
Strengths
Test coverage verdict — ADEQUATE
VerdictNEEDS MINOR FIXES — two clear, cheap wins before merge:
Everything else is suggestion-grade. The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.ts (1)
65-71: Optional: tightengetModeltyping using the existing type-only import.
ApiandModelare already imported as types on Line 2, sogetModelcan be typed usingtypeof import(...)without triggering any runtime module resolution. This preserves the dynamic-load property while removing theunknownhop and the internal cast.♻️ Proposed refactor
-function lookupPiModel( - getModel: unknown, - provider: string, - modelId: string -): Model<Api> | undefined { - return (getModel as (p: string, m: string) => Model<Api> | undefined)(provider, modelId); -} +function lookupPiModel( + getModel: typeof import('@mariozechner/pi-ai').getModel, + provider: string, + modelId: string +): Model<Api> | undefined { + // Pi's getModel constrains TModelId to keyof MODELS[TProvider]; not knowable + // from a runtime string, so the cast-through-unknown escape hatch remains. + return (getModel as unknown as (p: string, m: string) => Model<Api> | undefined)( + provider, + modelId + ); +}Purely cosmetic — current form is also correct.
🤖 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 65 - 71, Summary: tighten the getModel parameter typing in lookupPiModel to avoid the unknown + cast. Change lookupPiModel's signature so getModel is typed as (p: string, m: string) => Model<Api> | undefined (using the existing type-only imports Model and Api) and then return getModel(provider, modelId) directly without the runtime cast; update references to the getModel parameter in lookupPiModel accordingly.
🤖 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 65-71: Summary: tighten the getModel parameter typing in
lookupPiModel to avoid the unknown + cast. Change lookupPiModel's signature so
getModel is typed as (p: string, m: string) => Model<Api> | undefined (using the
existing type-only imports Model and Api) and then return getModel(provider,
modelId) directly without the runtime cast; update references to the getModel
parameter in lookupPiModel accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8269882e-fbba-4952-b206-4f21c4ff70e1
📒 Files selected for processing (3)
packages/providers/package.jsonpackages/providers/src/community/pi/provider-lazy-load.test.tspackages/providers/src/community/pi/provider.ts
…mport From the multi-agent review on #1355: - Add CHANGELOG.md entries under [Unreleased] ### Fixed for this PR's Pi lazy-load fix and the --bytecode removal from #1354 — both user-visible fixes (compiled binary unusable without them). - Rewrite the test-header docstring in provider-lazy-load.test.ts to describe counter-based detection instead of "mocks throw" (contradicted the actual code directly below it). - Tighten `lookupPiModel`'s first parameter from `unknown` to a local `GetModelFn` alias, moving the runtime-string cast to the single call site with a pointer to the docblock. - Update the class docblock on `PiProvider` — "v1 capabilities are all false" was stale; PI_CAPABILITIES has seven flags set to true. - `ui-context-stub.ts` imports `Theme` as a non-type value even though every usage is in a type position. Fold it into the existing `import type {…}` block so a future runtime-class `Theme` in Pi can't reintroduce an eager module load via this sibling. No behavior change. Type-check, lint, format, tests, and a local darwin-arm64 compile + version smoke all clean.
Collaborator
Author
|
Thanks — pushed cff6247 addressing the review. Must-fixes (both done):
Nice-to-haves addressed:
Intentionally skipped:
Verified locally after the cleanups: |
This was referenced Apr 22, 2026
Merged
popemkt
pushed a commit
to popemkt/Archon
that referenced
this pull request
Apr 22, 2026
Single conflict in packages/providers/package.json test script — resolved as union (all pi/ tests from upstream including new provider-lazy-load.test.ts plus the copilot/ test files from this branch). Upstream's pi-provider lazy-load refactor (coleam00#1355) auto-merged cleanly with this branch's pi → shared/ extraction because the extracted shared modules have no Pi SDK deps, so the compiled-binary contract is preserved.
Merged
3 tasks
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the other half of the v0.3.7 binary-release blocker (#1354 fixed the
--bytecodecompile flag; this fixes the runtime crash).@mariozechner/pi-coding-agent/dist/config.jsrunsreadFileSync(getPackageJsonPath(), 'utf-8')at module top-level. Inside a compiled archon binary,getPackageJsonPath()resolves todirname(process.execPath) + '/package.json'— a path that doesn't exist next to/usr/local/bin/archon. Result: archon crashes at ENOENT before any command runs, includingarchon version. Reproduces natively on darwin-arm64 after building locally.Change
Convert all Pi SDK value imports and imports of Pi-dependent helper modules (
options-translator,resource-loader,session-resolver,ui-context-stub,event-bridge) in `provider.ts` to dynamic imports inside `PiProvider.sendQuery()`. Type-only imports stay static — TS erases them, no runtime resolution.Effect:
registerCommunityProviders()+getAgentProvider('pi')no longer loads the Pi SDK. Load happens only when a Pi workflow actually runs.Why Pi and not Claude/Codex
Claude and Codex providers keep static imports. Their SDKs have no module-init side effects that fail inside a compiled binary. Pi is a deliberate outlier because of the upstream behaviour; a comment in `provider.ts`'s header makes this explicit so future maintainers don't "normalize" it by reintroducing static imports.
Nits
Regression test
New `provider-lazy-load.test.ts` mocks both Pi SDK packages, walks the exact path the CLI and server take (`registerCommunityProviders()` → `getAgentProvider('pi')`), and asserts neither SDK module factory fired. Runs in its own `bun test` invocation — Bun's `mock.module` is process-wide and would poison `provider.test.ts`.
If a future change reintroduces a static Pi SDK import in the chain reachable from the registry, the counters tip to `true` and this test fails with a pointer back to this PR.
Verified locally
Depends on
Test plan
Summary by CodeRabbit
Tests
Bug Fixes
Refactor