fix(pi): resolve API key for custom models.json providers (fixes 401 on self-hosted gateways)#1995
Open
bjornrun wants to merge 1 commit into
Open
fix(pi): resolve API key for custom models.json providers (fixes 401 on self-hosted gateways)#1995bjornrun wants to merge 1 commit into
bjornrun wants to merge 1 commit into
Conversation
Custom providers defined in pi models.json (self-hosted OpenAI-compatible gateways) were never sent an API key because only providers in the generated PI_PROVIDER_ENV_VARS map get injected, so requests returned 401. Resolve the key env-var name from the provider models.json apiKey reference as a fallback. Additive; adds custom-provider-key.ts + unit tests.
📝 WalkthroughWalkthroughAdds a new ChangesCustom Pi provider API key env-var resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/providers/src/community/pi/custom-provider-key.test.ts`:
- Around line 39-42: The test uses a fixed hardcoded directory name
`pi-nonexistent-dir-xyz` which could exist on a test runner, causing flakiness.
Replace this by creating a unique temporary directory path before the test runs
(using a library like `unique-temp-dir` or Node's built-in `fs` and `path`
utilities), explicitly delete it to guarantee it doesn't exist, and then pass
that guaranteed-missing path to the `customProviderApiKeyEnvVar` function call
instead of the fixed string. This ensures the test condition (missing
models.json) is deterministically satisfied on every run regardless of the
runner's state.
In `@packages/providers/src/community/pi/custom-provider-key.ts`:
- Around line 49-50: The condition checking for environment variable references
in the apiKey parsing is too permissive and accepts invalid tokens like `$$` or
`$A-B`. Tighten the validation by adding a check to ensure the characters
following the `$` form a valid environment variable name (typically matching a
pattern like alphanumeric characters and underscores). When the token after `$`
does not match a valid environment variable format, return `undefined` instead
of slicing the string, allowing the no-credentials fallback path to handle it
correctly.
🪄 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: 975a64ea-6c6c-4038-9f19-1a02e093f9e1
📒 Files selected for processing (3)
packages/providers/src/community/pi/custom-provider-key.test.tspackages/providers/src/community/pi/custom-provider-key.tspackages/providers/src/community/pi/provider.ts
Comment on lines
+39
to
+42
| test('returns undefined when models.json does not exist', () => { | ||
| expect( | ||
| customProviderApiKeyEnvVar('x', join(tmpdir(), 'pi-nonexistent-dir-xyz')) | ||
| ).toBeUndefined(); |
There was a problem hiding this comment.
Make the “missing models.json” case guaranteed-missing per run.
Using a fixed temp path (pi-nonexistent-dir-xyz) can become flaky if that directory appears on a runner. Create a unique temp directory and delete it first, then assert against that path.
Suggested patch
test('returns undefined when models.json does not exist', () => {
- expect(
- customProviderApiKeyEnvVar('x', join(tmpdir(), 'pi-nonexistent-dir-xyz'))
- ).toBeUndefined();
+ const missingDir = mkdtempSync(join(tmpdir(), 'pi-missing-'));
+ rmSync(missingDir, { recursive: true, force: true });
+ expect(customProviderApiKeyEnvVar('x', missingDir)).toBeUndefined();
});As per coding guidelines, “Keep tests deterministic — no flaky timing or network dependence without guardrails.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('returns undefined when models.json does not exist', () => { | |
| expect( | |
| customProviderApiKeyEnvVar('x', join(tmpdir(), 'pi-nonexistent-dir-xyz')) | |
| ).toBeUndefined(); | |
| test('returns undefined when models.json does not exist', () => { | |
| const missingDir = mkdtempSync(join(tmpdir(), 'pi-missing-')); | |
| rmSync(missingDir, { recursive: true, force: true }); | |
| expect(customProviderApiKeyEnvVar('x', missingDir)).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/providers/src/community/pi/custom-provider-key.test.ts` around lines
39 - 42, The test uses a fixed hardcoded directory name `pi-nonexistent-dir-xyz`
which could exist on a test runner, causing flakiness. Replace this by creating
a unique temporary directory path before the test runs (using a library like
`unique-temp-dir` or Node's built-in `fs` and `path` utilities), explicitly
delete it to guarantee it doesn't exist, and then pass that guaranteed-missing
path to the `customProviderApiKeyEnvVar` function call instead of the fixed
string. This ensures the test condition (missing models.json) is
deterministically satisfied on every run regardless of the runner's state.
Source: Coding guidelines
Comment on lines
+49
to
+50
| if (typeof apiKey === 'string' && apiKey.startsWith('$') && apiKey.length > 1) { | ||
| return apiKey.slice(1); |
There was a problem hiding this comment.
Tighten $ENV_VAR parsing to avoid false positives.
The current check accepts any non-empty string starting with $ (for example $$ or $A-B) as an env-var reference. This should only resolve valid env-var tokens, otherwise return undefined and keep the no-credentials fallback path.
Suggested patch
- if (typeof apiKey === 'string' && apiKey.startsWith('$') && apiKey.length > 1) {
- return apiKey.slice(1);
- }
+ if (typeof apiKey === 'string') {
+ const match = apiKey.match(/^\$([A-Za-z_][A-Za-z0-9_]*)$/);
+ if (match) return match[1];
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/providers/src/community/pi/custom-provider-key.ts` around lines 49 -
50, The condition checking for environment variable references in the apiKey
parsing is too permissive and accepts invalid tokens like `$$` or `$A-B`.
Tighten the validation by adding a check to ensure the characters following the
`$` form a valid environment variable name (typically matching a pattern like
alphanumeric characters and underscores). When the token after `$` does not
match a valid environment variable format, return `undefined` instead of slicing
the string, allowing the no-credentials fallback path to handle it correctly.
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
2 participants
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.
Problem
The Pi provider only injects an API key when the provider id is present in the
generated
PI_PROVIDER_ENV_VARSmap (pi-vendor-map.generated.ts, derived frompi-ai's built-in SDK backends). For a custom provider the user defines in
pi's
models.json— e.g. a self-hosted, OpenAI-compatible gateway —PI_PROVIDER_ENV_VARS["mygw"]isundefined, sosetRuntimeApiKeyis nevercalled. Execution then takes the "unmapped provider" branch, logs
auth_missing,and continues without credentials — the upstream request goes out with no
key and the gateway returns 401. The model resolves fine (it's in
models.json), so the failure is purely auth and is confusing to debug.Using such a provider works with pi's own CLI (which reads the
apiKey: "$ENV"reference from
models.json), but not through Archon.Fix
Add a small, pure helper
customProviderApiKeyEnvVar(provider, configDir?)that,for a provider not in the generated map, reads
$PI_CODING_AGENT_DIR/models.json(falling back to~/.pi/agent/models.json—the same file pi's CLI reads) and returns the bare env-var name from that
provider's
apiKey: "$ENV_VAR"reference. The Pi provider uses it as a fallback:so any custom OpenAI-compatible provider authenticates with no per-provider code
change. The lookup is best-effort: missing/unreadable/malformed
models.json,an absent provider, or a non-
$apiKeyall returnundefined, falling throughto the existing no-credentials path. Built-in SDK backends are unchanged.
Testing
custom-provider-key.test.ts(6 cases:$ENVreference, literal key,missing provider, missing file, malformed JSON, bare
$) — all pass viabun test(100% coverage of the new module).Notes
pi-vendor-map.generated.tsor thecheck:pi-vendor-mapdrift guard — this is a runtime fallback only.
assistant now authenticates (was 401) and routes correctly.
Summary by CodeRabbit
New Features
Tests