Reviewed GitHub PR for caching and performance issues
Public
Reviewed GitHub PR for caching and performance issues
Agent
Pi
Model
Claude Opus 4.5 (latest)
Started
Jan 16, 11:16 PM
Messages51
Downloads0
Read PR description, comments, commits, and changed files.
UnderstandIdentified and reviewed linked issues.
UnderstandAnalyzed PR diff and relevant code files.
UnderstandChecked CHANGELOG, README, and test files.
VerifyPosted detailed comment on PR in user's tone.
Edit
You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/762
For each PR URL, do the following in order:
packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
### Breaking Changes, ### Added, ### Fixed, etc.)Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))### Breaking Changes, not just ### FixedOutput format per PR: PR: Changelog:
If no issues are found, say so under Bad and Ugly.
The user wants me to review a GitHub PR. Let me start by reading the PR details.
>gh pr view 762 --repo badlogic/pi-mono --json title,body,comments,commits,files,author,labels,state,baseRefName,headRefName{"author":{"id":"MDQ6VXNlcjU0NDU=","is_bot":false,"login":"cv","name":"Carlos Villela"},"baseRefName":"main","body":"Add `!` prefix support to `apiKey` field in `models.json` to execute shell commands and use stdout as the API key. This allows users to store API keys in secure credential managers like macOS Keychain, 1Password, Bitwarden, or HashiCorp Vault.\n\n## Example\n\n```json\n{\n \"providers\": {\n \"my-provider\": {\n \"baseUrl\": \"https://api.example.com\",\n \"apiKey\": \"!security find-generic-password -ws 'anthropic'\",\n \"api\": \"anthropic-messages\",\n \"models\": [...]\n }\n }\n}\n```\n\n## API Key Resolution\n\nThe `apiKey` field now supports three formats:\n- `\"!command\"` - Executes the command and uses stdout (trimmed)\n- Environment variable name (e.g., `\"MY_API_KEY\"`) - Uses the value of the environment variable \n- Literal value - Used directly as the API key\n\n## Changes\n\n- `packages/coding-agent/src/core/model-registry.ts`: Added command execution support to `resolveApiKeyConfig()`\n- `packages/coding-agent/test/model-registry.test.ts`: Added 9 tests for API key resolution\n- `packages/coding-agent/README.md`: Updated documentation\n- `packages/coding-agent/CHANGELOG.md`: Added changelog entry\n\nFixes #697","comments":[],"commits":[{"authoredDate":"2026-01-16T01:37:59Z","authors":[{"email":"[REDACTED]","id":"MDQ6VXNlcjU0NDU=","login":"cv","name":"Carlos Villela"}],"committedDate":"2026-01-16T01:37:59Z","messageBody":"Add ! prefix support to apiKey field in models.json to execute shell commands\nand use stdout as the API key. This allows users to store API keys in secure\ncredential managers like macOS Keychain, 1Password, Bitwarden, or HashiCorp Vault.\n\nExample: \"apiKey\": \"!security find-generic-password -ws 'anthropic'\"\n\nThe apiKey field now supports three formats:\n- !command - executes shell command, uses trimmed stdout\n- ENV_VAR_NAME - uses environment variable value\n- literal - uses value directly\n\nfixes #697","messageHeadline":"Support shell command execution for API key resolution in models.json","oid":"4b14fe35bc62b354ffb1d330292ea9cd75f7ed97"},{"authoredDate":"2026-01-16T01:48:08Z","authors":[{"email":"[REDACTED]","id":"MDQ6VXNlcjU0NDU=","login":"cv","name":"Carlos Villela"}],"committedDate":"2026-01-16T01:48:08Z","messageBody":"","messageHeadline":"Merge branch 'main' into feature/api-key-command-resolution","oid":"5bd51bb1d62e09d2bc19804745aff02f53da83fb"},{"authoredDate":"2026-01-16T02:05:16Z","authors":[{"email":"[REDACTED]","id":"MDQ6VXNlcjU0NDU=","login":"cv","name":"Carlos Villela"}],"committedDate":"2026-01-16T02:05:16Z","messageBody":"","messageHeadline":"Merge branch 'main' into feature/api-key-command-resolution","oid":"fe334cfcb341032d1e5c265ee926219d55fbbd6f"},{"authoredDate":"2026-01-16T23:08:31Z","authors":[{"email":"[REDACTED]","id":"MDQ6VXNlcjU0NDU=","login":"cv","name":"Carlos Villela"}],"committedDate":"2026-01-16T23:08:31Z","messageBody":"","messageHeadline":"Merge branch 'main' into feature/api-key-command-resolution","oid":"3dac685e264e932e29e7371e74d45e7dcc570753"}],"files":[{"path":"package-lock.json","additions":1,"deletions":8},{"path":"packages/coding-agent/CHANGELOG.md","additions":1,"deletions":0},{"path":"packages/coding-agent/README.md","additions":4,"deletions":1},{"path":"packages/coding-agent/src/core/model-registry.ts","additions":20,"deletions":1},{"path":"packages/coding-agent/test/model-registry.test.ts","additions":135,"deletions":0}],"headRefName":"feature/api-key-command-resolution","labels":[],"state":"OPEN","title":"Support shell command execution for API key resolution in models.json"}>gh pr diff 762 --repo badlogic/pi-monodiff --git a/package-lock.json b/package-lock.json
index ea7940d86..ef84f25f7 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -6279,7 +6279,6 @@
"resolved": "https://registry.npmjs.org/lit/-/lit-3.3.2.tgz",
"integrity": "sha512-NF9zbsP79l4ao2SNrH3NkfmFgN/hBYSQo90saIVI1o5GpjAdCPVstVzO1MrLOakHoEhYkrtRjPK6Ob521aoYWQ==",
"license": "BSD-3-Clause",
- "peer": true,
"dependencies": {
"@lit/reactive-element": "^2.1.0",
"lit-element": "^4.2.0",
@@ -7634,7 +7633,6 @@
"resolved": "https://registry.npmjs.org/tailwind-merge/-/tailwind-merge-3.4.0.tgz",
"integrity": "sha512-uSaO4gnW+b3Y2aWoWfFpX62vn2sR3skfhbjsEnaBI81WD1wBLlHZe5sWf0AqjksNdYTbGBEd0UasQMT3SNV15g==",
"license": "MIT",
- "peer": true,
"funding": {
"type": "github",
"url": "https://github.com/sponsors/dcastil"
@@ -7663,8 +7661,7 @@
"version": "4.1.18",
"resolved": "https://registry.npmjs.org/tailwindcss/-/tailwindcss-4.1.18.tgz",
"integrity": "sha512-4+Z+0yiYyEtUVCScyfHCxOYP06L5Ne+JiHhY2IjR2KWMIWhJOYZKLSGZaP5HkZ8+bY0cxfzwDE5uOmzFXyIwxw==",
- "license": "MIT",
- "peer": true
+ "license": "MIT"
},
"node_modules/tapable": {
"version": "2.3.0",
@@ -7782,7 +7779,6 @@
"resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz",
"integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==",
"license": "MIT",
- "peer": true,
"engines": {
"node": ">=12"
},
@@ -7879,7 +7875,6 @@
"integrity": "sha512-5C1sg4USs1lfG0GFb2RLXsdpXqBSEhAaA/0kPL01wxzpMqLILNxIxIOKiILz+cdg/pLnOUxFYOR5yhHU666wbw==",
"devOptional": true,
"license": "MIT",
- "peer": true,
"dependencies": {
"esbuild": "~0.27.0",
"get-tsconfig": "^4.7.5"
@@ -7959,7 +7954,6 @@
"resolved": "https://registry.npmjs.org/vite/-/vite-7.3.1.tgz",
"integrity": "sha512-w+N7Hifpc3gRjZ63vYBXA56dvvRlNWRczTdmCBBa+CotUzAPf5b7YMdMR/8CQoeYE5LX3W4wj6RYTgonm1b9DA==",
"license": "MIT",
- "peer": true,
"dependencies": {
"esbuild": "^0.27.0",
"fdir": "^6.5.0",
@@ -8074,7 +8068,6 @@
"resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz",
"integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==",
"license": "MIT",
- "peer": true,
"engines": {
"node": ">=12"
},
diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md
index a40543c35..e21a4a607 100644
--- a/packages/coding-agent/CHANGELOG.md
+++ b/packages/coding-agent/CHANGELOG.md
@@ -4,6 +4,7 @@
### Added
+- API keys in `models.json` can now be retrieved via shell command using `!` prefix (e.g., `"apiKey": "!security find-generic-password -ws 'anthropic'"` for macOS Keychain) ([#697](https://github.com/badlogic/pi-mono/issues/697))
- Added `editorPaddingX` setting for horizontal padding in input editor (0-3, default: 0)
- Added `shellCommandPrefix` setting to prepend commands to every bash execution, enabling alias expansion in non-interactive shells (e.g., `"shellCommandPrefix": "shopt -s expand_aliases"`) ([#790](https://github.com/badlogic/pi-mono/pull/790) by [@richardgill](https://github.com/richardgill))
- Added bash-style argument slicing for prompt templates ([#770](https://github.com/badlogic/pi-mono/pull/770) by [@airtonix](https://github.com/airtonix))
diff --git a/packages/coding-agent/README.md b/packages/coding-agent/README.md
index 7bfbaf445..f3f6270fa 100644
--- a/packages/coding-agent/README.md
+++ b/packages/coding-agent/README.md
@@ -655,7 +655,10 @@ Add custom models (Ollama, vLLM, LM Studio, etc.) via `~/.pi/agent/models.json`:
**Supported APIs:** `openai-completions`, `openai-responses`, `openai-codex-responses`, `anthropic-messages`, `google-generative-ai`
-**API key resolution:** The `apiKey` field is checked as environment variable name first, then used as literal value.
+**API key resolution:** The `apiKey` field supports three formats:
+- `"!command"` - Executes the command and uses stdout (e.g., `"!security find-generic-password -ws 'anthropic'"` for macOS Keychain, `"!op read 'op://vault/item/credential'"` for 1Password)
+- Environment variable name (e.g., `"MY_API_KEY"`) - Uses the value of the environment variable
+- Literal value - Used directly as the API key
**API override:** Set `api` at provider level (default for all models) or model level (override per model).
diff --git a/packages/coding-agent/src/core/model-registry.ts b/packages/coding-agent/src/core/model-registry.ts
index 9e2fc0947..ef7868f08 100644
--- a/packages/coding-agent/src/core/model-registry.ts
+++ b/packages/coding-agent/src/core/model-registry.ts
@@ -13,6 +13,7 @@ import {
} from "@mariozechner/pi-ai";
import { type Static, Type } from "@sinclair/typebox";
import AjvModule from "ajv";
+import { execSync } from "child_process";
import { existsSync, readFileSync } from "fs";
import type { AuthStorage } from "./auth-storage.js";
@@ -101,9 +102,27 @@ function emptyCustomModelsResult(error?: string): CustomModelsResult {
/**
* Resolve an API key config value to an actual key.
- * Checks environment variable first, then treats as literal.
+ * - If starts with "!", executes the rest as a shell command and uses stdout
+ * - Otherwise checks environment variable first, then treats as literal
*/
function resolveApiKeyConfig(keyConfig: string): string | undefined {
+ // Command execution: "!command args"
+ if (keyConfig.startsWith("!")) {
+ const command = keyConfig.slice(1);
+ try {
+ const result = execSync(command, {
+ encoding: "utf-8",
+ timeout: 10000, // 10 second timeout
+ stdio: ["ignore", "pipe", "ignore"], // ignore stdin/stderr
+ });
+ // Trim whitespace/newlines from output
+ return result.trim() || undefined;
+ } catch {
+ // Command failed - return undefined so auth falls back to other methods
+ return undefined;
+ }
+ }
+
const envValue = process.env[keyConfig];
if (envValue) return envValue;
return keyConfig;
diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts
index 26bad48e4..ea03451d9 100644
--- a/packages/coding-agent/test/model-registry.test.ts
+++ b/packages/coding-agent/test/model-registry.test.ts
@@ -246,4 +246,139 @@ describe("ModelRegistry", () => {
expect(anthropicModels.some((m) => m.id.includes("claude"))).toBe(true);
});
});
+
+ describe("API key resolution", () => {
+ /** Create provider config with custom apiKey */
+ function providerWithApiKey(apiKey: string) {
+ return {
+ baseUrl: "https://example.com/v1",
+ apiKey,
+ api: "anthropic-messages",
+ models: [
+ {
+ id: "test-model",
+ name: "Test Model",
+ reasoning: false,
+ input: ["text"],
+ cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+ contextWindow: 100000,
+ maxTokens: 8000,
+ },
+ ],
+ };
+ }
+
+ test("apiKey with ! prefix executes command and uses stdout", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!echo test-api-key-from-command"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBe("test-api-key-from-command");
+ });
+
+ test("apiKey with ! prefix trims whitespace from command output", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!echo ' spaced-key '"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBe("spaced-key");
+ });
+
+ test("apiKey with ! prefix handles multiline output (uses trimmed result)", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!printf 'line1\\nline2'"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBe("line1\nline2");
+ });
+
+ test("apiKey with ! prefix returns undefined on command failure", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!exit 1"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBeUndefined();
+ });
+
+ test("apiKey with ! prefix returns undefined on nonexistent command", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!nonexistent-command-12345"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBeUndefined();
+ });
+
+ test("apiKey with ! prefix returns undefined on empty output", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!printf ''"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBeUndefined();
+ });
+
+ test("apiKey as environment variable name resolves to env value", async () => {
+ const originalEnv = process.env.TEST_API_KEY_12345;
+ process.env.TEST_API_KEY_12345 = "env-api-key-value";
+
+ try {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("TEST_API_KEY_12345"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBe("env-api-key-value");
+ } finally {
+ if (originalEnv === undefined) {
+ delete process.env.TEST_API_KEY_12345;
+ } else {
+ process.env.TEST_API_KEY_12345 = originalEnv;
+ }
+ }
+ });
+
+ test("apiKey as literal value is used directly when not an env var", async () => {
+ // Make sure this isn't an env var
+ delete process.env.literal_api_key_value;
+
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("literal_api_key_value"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBe("literal_api_key_value");
+ });
+
+ test("apiKey command can use shell features like pipes", async () => {
+ writeRawModelsJson({
+ "custom-provider": providerWithApiKey("!echo 'hello world' | tr ' ' '-'"),
+ });
+
+ const registry = new ModelRegistry(authStorage, modelsJsonPath);
+ const apiKey = await registry.getApiKeyForProvider("custom-provider");
+
+ expect(apiKey).toBe("hello-world");
+ });
+ });
});Let me now read the linked issue #697 and the current main branch files to understand the context better.
>gh issue view 697 --repo badlogic/pi-mono --json title,body,comments,labels,state{"body":"Currently (as far as I can tell) the only option for API key storage is in a file or in an env var; for other applications I store my API variables in the Mac keychain app and configure them to pull the key via a command.\n\nFor example, here's how I configure codecompanion in vim:\n\n```lua\nenv = {\n\t-- saved key with:\n\t-- $ security add-generic-password -s 'anthropic' -w 'sk-ant-<key-goes-here>'\n\t-- retrieve with:\n\t-- $ security find-generic-password -ws 'anthropic'\n\tapi_key = \"cmd:security find-generic-password -ws 'anthropic' | tr -d '\\n'\",\n},\n```\n\nI know other people use this feature to store their API keys in onepassword, bitwarden, vault, or any of a number of other somewhat-more-secure options than sticking them in a file or environment variable.\n\nIt would be very cool if I could configure `pi` to pull the API key via a command","comments":[{"id":"IC_kwDOPbFNk87f9S9h","author":{"login":"badlogic"},"authorAssociation":"OWNER","body":"This wouldn't work for OAuth based auth. We could think about supporting a ! prefix in the apiKey field of models.json, that would execute whatever command comes after !. Sound good?","createdAt":"2026-01-15T23:39:01Z","includesCreatedEdit":false,"isMinimized":false,"minimizedReason":"","reactionGroups":[],"url":"https://github.com/badlogic/pi-mono/issues/697#issuecomment-3757387617","viewerDidAuthor":true},{"id":"IC_kwDOPbFNk87f-YlX","author":{"login":"llimllib"},"authorAssociation":"NONE","body":"Yup, that'd be great","createdAt":"2026-01-16T01:31:34Z","includesCreatedEdit":false,"isMinimized":false,"minimizedReason":"","reactionGroups":[{"content":"THUMBS_UP","users":{"totalCount":1}}],"url":"https://github.com/badlogic/pi-mono/issues/697#issuecomment-3757672791","viewerDidAuthor":false}],"labels":[],"state":"OPEN","title":"feature request: pull API keys with a command"}Let me also check the CHANGELOG and README to verify they were updated correctly, and look at the test file to make sure I understand the implementation fully.
Now let me check the current CHANGELOG and README on main, and verify the test file:
Let me check the coding-agent docs to see if they need modification, and also verify the full context of the changes to understand the implementation.
Let me check the docs directory to see if any documentation updates are needed:
>ls -la /Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/total 408
drwxr-xr-x@ 11 badlogic staff 352 Jan 16 23:14 .
drwxr-xr-x@ 16 badlogic staff 512 Jan 16 23:15 ..
-rw-r--r--@ 1 badlogic staff 14757 Jan 3 00:47 compaction.md
-rw-r--r--@ 1 badlogic staff 52716 Jan 16 21:49 extensions.md
-rw-r--r--@ 1 badlogic staff 23979 Jan 16 23:14 rpc.md
-rw-r--r--@ 1 badlogic staff 29668 Jan 16 21:49 sdk.md
-rw-r--r--@ 1 badlogic staff 9717 Jan 12 23:33 session.md
-rw-r--r--@ 1 badlogic staff 9389 Jan 12 23:33 skills.md
-rw-r--r--@ 1 badlogic staff 16734 Jan 8 19:37 theme.md
-rw-r--r--@ 1 badlogic staff 6645 Jan 16 21:49 tree.md
-rw-r--r--@ 1 badlogic staff 25785 Jan 16 12:47 tui.md>grep -r "apiKey\|models.json" /Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/ 2>/dev/null | head -50/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:- Custom models (`models.json`)
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:// Find any model by provider/id, including custom models from models.json
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:4. Fallback resolver (for custom provider keys from `models.json`)
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:// Default: uses ~/.pi/agent/auth.json and ~/.pi/agent/models.json
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:const customRegistry = new ModelRegistry(customAuth, "/my/app/models.json");
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:// No custom models.json (built-in models only)
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:const modelRegistry = discoverModels(authStorage); // + ~/.pi/agent/models.json
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md:// Model registry (no custom models.json)Let me also verify the implementation is correct by looking at the model-registry.ts diff more closely and check if there are any security concerns with the shell command execution.
The implementation looks correct:
! are executed via execSyncLet me also check the test for completeness and verify the CHANGELOG entry format is correct.
>grep -n "apiKey" /Users/badlogic/workspaces/pi-mono/packages/coding-agent/docs/sdk.md | head -20(no output)Good, there's no mention of apiKey in the SDK docs that needs updating. The feature addition is in the README which was already updated by the PR. Let me now compile my review.
Looking at the PR:
Good things:
! prefix approach is consistent with how other tools (like codecompanion in vim) handle thisPotential concerns:
execSync call uses stdio: ["ignore", "pipe", "ignore"] which ignores stderr - this is correct for silently handling errorsChangelog entry issue: The current entry is:
- API keys in `models.json` can now be retrieved via shell command using `!` prefix (e.g., `"apiKey": "!security find-generic-password -ws 'anthropic'"` for macOS Keychain) ([#697](https://github.com/badlogic/pi-mono/issues/697))Per AGENTS.md, external contributions should include PR link and author:
- API keys in `models.json` can now be retrieved via shell command using `!` prefix (e.g., `"apiKey": "!security find-generic-password -ws 'anthropic'"` for macOS Keychain) ([#762](https://github.com/badlogic/pi-mono/pull/762) by [@cv](https://github.com/cv))The PR is also missing the pkg:coding-agent label. I should verify the implementation handles edge cases properly: trailing newlines are trimmed, timeouts and missing commands both return undefined gracefully, and pipes work as shown in the tests. The implementation appears solid and ready for review.
PR: https://github.com/badlogic/pi-mono/pull/762
packages/coding-agent/CHANGELOG.md under ### Added- API keys in `models.json` can now be retrieved via shell command using `!` prefix (...) ([#762](https://github.com/badlogic/pi-mono/pull/762) by [@cv](https://github.com/cv))pkg:coding-agent labelresolveApiKeyConfig()undefined, allowing auth to fall back to other methods! prefix approach matches conventions from other tools (vim codecompanion, etc.)models.json).package-lock.json changes (removing peer: true from several deps) are unrelated lock file drift, likely from merging with main. This is benign.packages/coding-agent/src/core/model-registry.ts: Extended resolveApiKeyConfig() to detect ! prefix and execute the remainder as a shell command via execSync, returning trimmed stdout. Includes 10s timeout and graceful error handling.packages/coding-agent/test/model-registry.test.ts: Added 9 new tests for API key resolution covering all execution paths.packages/coding-agent/README.md: Updated "API key resolution" section to document the three formats: !command, env var name, and literal value.packages/coding-agent/CHANGELOG.md: Added changelog entry (needs format fix).model-registry.test.ts:
apiKey with ! prefix executes command and uses stdoutapiKey with ! prefix trims whitespace from command outputapiKey with ! prefix handles multiline output (uses trimmed result)apiKey with ! prefix returns undefined on command failureapiKey with ! prefix returns undefined on nonexistent commandapiKey with ! prefix returns undefined on empty outputapiKey as environment variable name resolves to env valueapiKey as literal value is used directly when not an env varapiKey command can use shell features like pipespackages/coding-agent/README.md: Updated appropriately with the new feature.packages/coding-agent/docs/*.md: No updates needed. The sdk.md mentions models.json but only in the context of file paths and model discovery, not API key resolution specifics.absolutely no bad shit? like, invoking bash on every api key get? error handling?
The user is pushing back on my review, asking if there are really no issues. Let me think more critically about this:
resolveApiKeyConfig is called. If it's called on every API request, that could be a performance issue ancontinue
The user is pushing back and wants me to think more critically. Let me re-examine:
Invoking bash on every API key get - I need to check when resolveApiKeyConfig is called. Is it cached? Or called on every LLM request?
Error handling - The current code silently swallows errors. Is that really the right behavior? What if the user misconfigured their command?
Let me look at the code flow more carefully.
>grep -n "resolveApiKeyConfig\|getApiKey\|customProviderApiKeys" /Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/model-registry.ts106:function resolveApiKeyConfig(keyConfig: string): string | undefined {
117: private customProviderApiKeys: Map<string, string> = new Map();
126: const keyConfig = this.customProviderApiKeys.get(provider);
128: return resolveApiKeyConfig(keyConfig);
141: this.customProviderApiKeys.clear();
240: this.customProviderApiKeys.set(providerName, providerConfig.apiKey);
306: this.customProviderApiKeys.set(providerName, providerConfig.apiKey);
321: const resolvedKey = resolveApiKeyConfig(providerConfig.apiKey);
374: async getApiKey(model: Model<Api>): Promise<string | undefined> {
375: return this.authStorage.getApiKey(model.provider);
381: async getApiKeyForProvider(provider: string): Promise<string | undefined> {
382: return this.authStorage.getApiKey(provider);Now I see the real issue. The resolveApiKeyConfig is called via the fallbackResolver which is invoked from getApiKey(). This means:
Every time getApiKey() is called for a custom provider with a !command apiKey, the shell command gets executed. This could be:
No caching - The resolved API key is never cached. The command runs every single time.
Error handling is silent - If the user's command fails (typo, credential manager locked, etc.), they get no feedback. The auth just silently falls back and probably fails later with a confusing "no API key" error.
Let me trace how often getApiKey is called:
>grep -rn "getApiKey\|getApiKeyForProvider" /Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/ --include="*.ts" | grep -v "test" | head -30/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/model-resolver.ts:342: const hasApiKey = restoredModel ? !!(await modelRegistry.getApiKey(restoredModel)) : false;
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/auth-storage.ts:138: * Unlike getApiKey(), this doesn't refresh OAuth tokens.
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/auth-storage.ts:305: async getApiKey(provider: string): Promise<string | undefined> {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:15: * getApiKey: async () => process.env.MY_KEY,
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:335: * getApiKey: async () => process.env.MY_KEY,
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:369: if (restoredModel && (await modelRegistry.getApiKey(restoredModel))) {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:383: if (settingsModel && (await modelRegistry.getApiKey(settingsModel))) {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:392: if (await modelRegistry.getApiKey(m)) {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:635: getApiKey: async (provider) => {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/sdk.ts:642: const key = await modelRegistry.getApiKeyForProvider(resolvedProvider);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:633: const apiKey = await this._modelRegistry.getApiKey(this.model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:1049: const apiKey = await this._modelRegistry.getApiKey(model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:1090: const apiKey = await this._modelRegistry.getApiKey(next.model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:1120: const apiKey = await this._modelRegistry.getApiKey(nextModel);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:1259: const apiKey = await this._modelRegistry.getApiKey(this.model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:1447: const apiKey = await this._modelRegistry.getApiKey(this.model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/agent-session.ts:2076: const apiKey = await this._modelRegistry.getApiKey(model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/model-registry.ts:374: async getApiKey(model: Model<Api>): Promise<string | undefined> {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/model-registry.ts:375: return this.authStorage.getApiKey(model.provider);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/model-registry.ts:381: async getApiKeyForProvider(provider: string): Promise<string | undefined> {
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/model-registry.ts:382: return this.authStorage.getApiKey(provider);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/modes/print-mode.ts:67: const key = await session.modelRegistry.getApiKey(model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/modes/interactive/interactive-mode.ts:675: const key = await this.session.modelRegistry.getApiKey(model);
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/modes/rpc/rpc-mode.ts:280: const key = await session.modelRegistry.getApiKey(model);>grep -rn "fallbackResolver" /Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/ --include="*.ts"/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/auth-storage.ts:43: private fallbackResolver?: (provider: string) => string | undefined;
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/auth-storage.ts:69: this.fallbackResolver = resolver;
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/auth-storage.ts:144: if (this.fallbackResolver?.(provider)) return true;
/Users/badlogic/workspaces/pi-mono/packages/coding-agent/src/core/auth-storage.ts:358: return this.fallbackResolver?.(provider) ?? undefined;Now I see the real problems:
Called on every LLM request: getApiKey is called in agent-session.ts at lines 633, 1049, 1090, 1120, 1259, 1447, 2076. That's EVERY TIME the model is used - for every message, every retry, every compaction summarization.
Called during startup/model discovery: Lines in sdk.ts (369, 383, 392) and model-resolver.ts (342) call it during model selection.
No caching: The resolveApiKeyConfig function runs execSync every single time. For a security find-generic-password command, this means:
Silent error handling: If the command fails, it returns undefined silently. The user won't know their command is broken until they get a confusing "authentication failed" error.
This is a significant performance and UX issue. The command should be:
Let me also check if there's any existing caching mechanism.
You're right. I missed significant issues:
Updated Review - Bad:
No caching - command executes on EVERY API key lookup:
getApiKey() is called on every LLM request (lines 633, 1049, 1090, 1120, 1259, 1447, 2076 in agent-session.ts)!security find-generic-password command spawns a new shell process each timeSilent error handling is a UX problem:
undefined silently!command in models.json is the problemThe fix needed:
customProviderApiKeys map already exists but stores the config not the resolved valueExample of what happens now with a typo:
{ "apiKey": "!security find-generic-pasword -ws 'anthropic'" } // typo: paswordUser sees: "Error: No API key for provider my-provider" with no hint about the failed command.
Ugly:
ok, leave a comment on the pr in my tone
The user wants me to leave a comment on the PR in their tone. Based on the AGENTS.md, the tone should be:
Let me craft a comment that points out the issues directly but kindly.
>gh pr comment 762 --repo badlogic/pi-mono --body "Thanks for the PR! The feature is useful, but there's a performance issue that needs addressing before merge:
**No caching - command executes on every API key lookup**
\`getApiKey()\` is called on every LLM request (not just once at startup). With a \`!command\` apiKey, this means spawning a shell process multiple times per user message: initial request, retries, compaction checks, etc. For a long session, this adds up.
The \`customProviderApiKeys\` map stores the raw config string, not the resolved value. The fix would be to cache the resolved key after first successful execution.
**Silent error handling**
If the command fails (typo, keychain locked, 1Password CLI not logged in), it returns \`undefined\` silently. User then sees a confusing \"no API key\" error with no indication their \`!command\` is broken.
Suggestion: log a warning on command failure so users know what went wrong.
---
Also, minor changelog format issue: external contributions should link to the PR with author attribution:
\`\`\`
([#762](https://github.com/badlogic/pi-mono/pull/762) by [@cv](https://github.com/cv))
\`\`\`"https://github.com/badlogic/pi-mono/pull/762#issuecomment-3762201084Comment posted. The user asked me to leave a comment in their tone, which I did. The comment is direct, technical, points out the specific issues with code references, and provides actionable suggestions for fixing the problems.
- API keys in `models.json` can now be retrieved via shell command using `!` prefix (e.g., `"apiKey": "!security find-generic-password -ws 'anthropic'"` for macOS Keychain) ([#697](https://github.com/badlogic/pi-mono/issues/697))- API keys in `models.json` can now be retrieved via shell command using `!` prefix (e.g., `"apiKey": "!security find-generic-password -ws 'anthropic'"` for macOS Keychain) ([#762](https://github.com/badlogic/pi-mono/pull/762) by [@cv](https://github.com/cv))- API keys in `models.json` can now be retrieved via shell command using `!` prefix (...) ([#762](https://github.com/badlogic/pi-mono/pull/762) by [@cv](https://github.com/cv)){ "apiKey": "!security find-generic-pasword -ws 'anthropic'" } // typo: pasword