Reviewed GitHub PR for caching and performance issues

Summary

Closed, Completed

Understand
Read PR description, comments, commits, and changed files.
Understand
Understand
Identified and reviewed linked issues.
Understand
Understand
Analyzed PR diff and relevant code files.
Understand
Verify
Checked CHANGELOG, README, and test files.
Verify
Edit
Posted 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:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant 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:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Readmodel-registry.ts

Now let me check the current CHANGELOG and README on main, and verify the test file:

ReadCHANGELOG.md

ReadREADME.md40 lines

Readmodel-registry.test.ts

Let me check the docs directory to see if any documentation updates are needed:

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:

Commands starting with ! are executed via execSync
Timeout of 10 seconds is set
Empty output returns undefined (falls back to other auth methods)
Command failure returns undefined (falls back to other auth methods)

Let me also check the test for completeness and verify the CHANGELOG entry format is correct.

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:

The implementation is clean and follows the existing patterns
Tests are comprehensive (9 tests covering various scenarios)
Documentation in README is updated
Changelog entry exists
The ! prefix approach is consistent with how other tools (like codecompanion in vim) handle this
10 second timeout is reasonable for credential managers
Command failure gracefully falls back (returns undefined)
Empty output returns undefined (correct behavior)

Potential concerns:

The changelog entry references the issue #697 but doesn't include the PR author attribution since this is an external contribution (from @cv)
Security consideration: shell command execution could be a risk, but this is already user-configured in their own models.json file, so they control what commands run
The execSync call uses stdio: ["ignore", "pipe", "ignore"] which ignores stderr - this is correct for silently handling errors

Changelog 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))

- 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))

- 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

Changelog:

Exists: Yes, entry is present in packages/coding-agent/CHANGELOG.md under ### Added

Issue: Entry links to issue #697 but should link to PR #762 and include author attribution for external contribution. Per AGENTS.md format:

- 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))

- 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))

Missing: PR needs pkg:coding-agent label

Good:

Clean implementation that follows existing patterns in resolveApiKeyConfig()
Comprehensive test coverage (9 tests) covering: successful commands, whitespace trimming, multiline output, command failures, nonexistent commands, empty output, env var resolution, literal values, and shell features (pipes)
10 second timeout is reasonable for credential manager lookups
Graceful fallback: command failures return undefined, allowing auth to fall back to other methods
README documentation is clear with practical examples (macOS Keychain, 1Password)
The ! prefix approach matches conventions from other tools (vim codecompanion, etc.)
Correctly ignores stderr to prevent credential manager UI prompts from corrupting output

Bad:

Changelog entry format incorrect: links to issue instead of PR and missing author attribution for external contribution

Ugly:

None. The implementation is straightforward and the security model is sound (user controls their own models.json).

Questions or Assumptions:

Assumption: The package-lock.json changes (removing peer: true from several deps) are unrelated lock file drift, likely from merging with main. This is benign.

Change summary:

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).

Tests:

9 new tests added in model-registry.test.ts:
1. apiKey with ! prefix executes command and uses stdout
2. apiKey with ! prefix trims whitespace from command output
3. apiKey with ! prefix handles multiline output (uses trimmed result)
4. apiKey with ! prefix returns undefined on command failure
5. apiKey with ! prefix returns undefined on nonexistent command
6. apiKey with ! prefix returns undefined on empty output
7. apiKey as environment variable name resolves to env value
8. apiKey as literal value is used directly when not an env var
9. apiKey command can use shell features like pipes
Tests cover the happy path, error cases, and edge cases comprehensively.

Documentation:

packages/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?

continue

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.

Readauth-storage.ts

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:
- On every LLM request
- During model discovery
- Multiple times per session
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:

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:
- Spawning a shell
- Executing the command
- Waiting for output
- This happens multiple times per user message!
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:

Cached after first successful execution
Error should be logged or shown to the user on first failure

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)
- Also called multiple times during startup for model discovery
- A !security find-generic-password command spawns a new shell process each time
- This could mean 5-10+ shell invocations per user message (initial request, retries, compaction checks)
Silent error handling is a UX problem:
- If the command fails (typo, keychain locked, 1Password CLI not logged in), returns undefined silently
- User sees a confusing "authentication failed" or "no API key" error later
- No indication that their !command in models.json is the problem
- Should at minimum log a warning on first failure, or cache the failure
The fix needed:
- Cache the resolved API key after first successful execution (per provider)
- Either cache failures too, or log a warning so users know their command failed
- The customProviderApiKeys map already exists but stores the config not the resolved value

Example of what happens now with a typo:

User sees: "Error: No API key for provider my-provider" with no hint about the failed command.

Ugly:

The performance impact scales with conversation length and retries. In a long session with auto-compaction, this could mean dozens of unnecessary shell spawns per interaction.

ok, leave a comment on the pr in my tone

>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))
\`\`\`"

Done. Comment posted: https://github.com/badlogic/pi-mono/pull/762#issuecomment-3762201084