Use new getApiKeyAndHeaders to fetch the API key from the model registry#41
Use new getApiKeyAndHeaders to fetch the API key from the model registry#41kirberich wants to merge 1 commit into
Conversation
|
@greptile |
|
| Filename | Overview |
|---|---|
| packages/budget-model/src/index.ts | Migrates from removed getApiKey to getApiKeyAndHeaders, but the new wrapper only extracts apiKey and discards the headers, potentially leaving header-auth providers still broken. |
| packages/budget-model/package.json | Bumps @mariozechner/pi-ai and @mariozechner/pi-coding-agent minimum version from ^0.57.0 to ^0.63.0 to pick up the new getApiKeyAndHeaders API. |
| yarn.lock | Lock file updated to add resolved 0.63.2 entries for the pi-* packages; old 0.57.0 entries remain for the wildcard/peer-dep selectors — expected for a monorepo. |
Sequence Diagram
sequenceDiagram
participant Caller
participant findBudgetModel
participant getApiKey_wrapper as getApiKey (new wrapper)
participant modelRegistry
Caller->>findBudgetModel: findBudgetModel(ctx, options)
findBudgetModel->>getApiKey_wrapper: getApiKey(ctx, candidate)
getApiKey_wrapper->>modelRegistry: getApiKeyAndHeaders(model)
modelRegistry-->>getApiKey_wrapper: { ok, apiKey, headers }
note right of getApiKey_wrapper: headers discarded here
getApiKey_wrapper-->>findBudgetModel: apiKey (string | undefined)
findBudgetModel-->>Caller: BudgetModel { model, apiKey }
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/budget-model/src/index.ts:124-127
**Headers from `getApiKeyAndHeaders` are silently discarded**
`getApiKeyAndHeaders` was introduced precisely to carry extra auth headers alongside the API key (e.g. for AWS Bedrock or other header-based auth providers). This wrapper extracts only `auth.apiKey` and throws away `auth.headers`, so any provider that requires those headers for authentication will still fail — the returned `BudgetModel.apiKey` alone won't be enough to make a successful API call for those providers.
Consider either returning the full auth object from `getApiKey`, or updating the `BudgetModel` interface to carry `headers` too:
```ts
export interface BudgetModel {
model: Model<Api>;
apiKey: string;
headers?: Record<string, string>;
}
```
and propagating `auth.headers` wherever `BudgetModel` is constructed.
Reviews (1): Last reviewed commit: "Use new getApiKeyAndHeaders to fetch the..." | Re-trigger Greptile
| async function getApiKey(ctx: ExtensionContext, model: Model<Api>): Promise<string | undefined> { | ||
| const auth = await ctx.modelRegistry.getApiKeyAndHeaders(model); | ||
| return auth.ok ? auth.apiKey : undefined; | ||
| } |
There was a problem hiding this comment.
Headers from
getApiKeyAndHeaders are silently discarded
getApiKeyAndHeaders was introduced precisely to carry extra auth headers alongside the API key (e.g. for AWS Bedrock or other header-based auth providers). This wrapper extracts only auth.apiKey and throws away auth.headers, so any provider that requires those headers for authentication will still fail — the returned BudgetModel.apiKey alone won't be enough to make a successful API call for those providers.
Consider either returning the full auth object from getApiKey, or updating the BudgetModel interface to carry headers too:
export interface BudgetModel {
model: Model<Api>;
apiKey: string;
headers?: Record<string, string>;
}and propagating auth.headers wherever BudgetModel is constructed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/budget-model/src/index.ts
Line: 124-127
Comment:
**Headers from `getApiKeyAndHeaders` are silently discarded**
`getApiKeyAndHeaders` was introduced precisely to carry extra auth headers alongside the API key (e.g. for AWS Bedrock or other header-based auth providers). This wrapper extracts only `auth.apiKey` and throws away `auth.headers`, so any provider that requires those headers for authentication will still fail — the returned `BudgetModel.apiKey` alone won't be enough to make a successful API call for those providers.
Consider either returning the full auth object from `getApiKey`, or updating the `BudgetModel` interface to carry `headers` too:
```ts
export interface BudgetModel {
model: Model<Api>;
apiKey: string;
headers?: Record<string, string>;
}
```
and propagating `auth.headers` wherever `BudgetModel` is constructed.
How can I resolve this? If you propose a fix, please make it concise.|
Closing in favor of #53, which incorporates this commit (with original authorship preserved) and addresses two follow-ups discovered during review:
#53 also extends Thanks @kirberich for the original diagnosis and fix — the regression you identified was real and your commit is the foundation of #53. |
…ages The budget-model fix (PR #41) bumped pi-coding-agent to ^0.63.0, but the other workspaces still pinned ^0.57.0. Yarn resolved two copies, so ExtensionContext shapes diverged across packages and tsup --dts failed when one package passed its own ctx into another. Aligns no-soft-cursor's pi-tui to ^0.63.0 for the same reason. devDependency only; no consumer-visible change, no changeset required.
…tch (supersedes #41) (#53) Supersedes #41 (kirberich's commit is preserved on this branch with original authorship). ## Why #41 correctly identified that `modelRegistry.getApiKey` was removed upstream and switched to `getApiKeyAndHeaders`, but the wrapper extracted only `auth.apiKey` and discarded `auth.headers`. This left two regressions: 1. **Header-authenticated providers** (out-of-band `Authorization`, AWS Bedrock with SDK-resolved credentials, etc.) returned `{ ok: true, apiKey: undefined, headers: {...} }`. The wrapper gated on `apiKey` truthiness and rejected them as "no key" — the same symptom #41 was supposed to fix, just for a different subset of providers. 2. **CI was failing** because `pi-budget-model` bumped `@mariozechner/pi-coding-agent` to `^0.63.0` while sibling packages still pinned `^0.57.0`, so two copies were resolved and `ExtensionContext` types diverged. ## What this does Four commits on top of #41's original commit: ### 1. `build:` align pi-coding-agent + pi-ai devDeps to `^0.63.0` across packages Fixes the dts build by ensuring a single resolved copy of pi-coding-agent across the workspace. Also bumps `no-soft-cursor`'s pi-tui to `^0.63.0` for the same reason. devDependency-only at the source-code level, but the published `dist/*.d.ts` may reference newer upstream types, so a patch changeset documents this for `pi-bash-bg`, `pi-bash-trim`, `pi-desktop-notify`, `pi-enclave`, `pi-jujutsu`, `pi-no-soft-cursor`. ### 2. `fix(budget-model):` preserve auth headers — **BREAKING** - `BudgetModel` now exposes a nested `auth: BudgetModelAuth` field instead of a flat `apiKey: string`. New shape forwards both `apiKey` and `headers` to pi-ai's `completeSimple`/`stream` via the `{ ...auth, signal, ... }` spread idiom. - `BudgetModelAuth` is exported as a valibot schema, reusable across the public surface. - Auth gate switches from "is `apiKey` truthy" to "is `result.ok` true" — header-only and SDK-auth providers now select correctly. - `toCandidate.hasApiKey` now uses `registry.hasConfiguredAuth` (a fast, non-async upstream probe) and reflects "has any usable auth"; the field name is kept for backwards compat. - `peerDependencies` tightened to `>=0.63.0` so consumers can't install against a pi version that lacks `getApiKeyAndHeaders`. - `pi-safeguard` updated to forward the new auth shape (`{ ...judge.auth, signal, ... }`) into `completeSimple`. - Major bump for `pi-budget-model`, patch for `pi-safeguard`. Migration guide in the changeset. ### 3. `feat(budget-model):` modelOverride object form (escape hatch) `modelOverride` now accepts `{ model: "provider/id", auth: BudgetModelAuth }` in addition to the bare string. The object form skips the registry's auth pipeline entirely; `find()` is still used to resolve model metadata. This is the workaround for future upstream auth breakages — if `getApiKeyAndHeaders` ever shifts shape again, users can supply credentials directly. ```ts findBudgetModel(ctx, { modelOverride: { model: "openai/gpt-4o-mini", auth: { apiKey: process.env.OPENAI_API_KEY }, }, }); ``` Minor bump. ### 4. `docs(budget-model):` README + JSDoc README usage examples updated from `{ model, apiKey }` to `{ model, auth }` with the spread idiom; new section documenting both forms of `modelOverride` including the auth-bypass use case. Restored JSDoc on `BudgetModel` and expanded `findBudgetModel`'s doc comment to describe override semantics. ## Tests `packages/budget-model/test/auth.test.ts` (9 cases). Model list comes from the real installed `@mariozechner/pi-ai` per the repo's "live data, not fixtures" rule; only the registry is faked, since we're testing budget-model's handling of the registry contract: | Case | Asserts | |---|---| | apiKey-only | forwarded to `auth.apiKey`, no headers | | headers-only | forwarded to `auth.headers`, regression case for Bedrock-style auth | | both | both forwarded | | `ok: false` | `NoBudgetModelError` | | `ok: true` with neither | accepted (SDK-resolved auth) | | override object form | uses supplied auth without invoking `getApiKeyAndHeaders` (asserted by making the fake throw if called) | | override object form, empty auth `{}` | accepted (caller defers to ambient credentials) | | override missing model | still fails fast with "not found in registry" | | override string form | still works (regression guard) | All 482 tests pass; lint clean. ## Migration For consumers of `pi-budget-model`: ```diff const judge = await findBudgetModel(ctx); -await completeSimple(judge.model, ctx, { apiKey: judge.apiKey, signal }); +await completeSimple(judge.model, ctx, { ...judge.auth, signal }); ``` If you need to read the apiKey directly, it's now `judge.auth.apiKey` (`string | undefined`). --------- Co-authored-by: Rob Kirberich <robert@kirberich.de>
getApiKey was removed from the modelRegistry in pi a few weeks ago - this made me scratch my head for a while because the only symptom of this was pi-budget-model insisting that there's no available model to pick. Switching to the new getApiKeyAndHeaders fixes it, but it does increase the required version of pi.
The other approach would have been to try using the new api and fall back to the old one, avoiding the need to increase the version dependency, but I figured I'd keep it simple.