Srihari Sriraman shared in

Public

Reviewed GitHub PR for caching and performance issuesPublic

Agent

Model

Claude Opus 4.5 (latest)

Started

Jan 16, 11:16 PM

Messages51

Downloads0

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.

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

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

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:

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

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

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

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

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:

apiKey with ! prefix executes command and uses stdout
apiKey with ! prefix trims whitespace from command output
apiKey with ! prefix handles multiline output (uses trimmed result)
apiKey with ! prefix returns undefined on command failure
apiKey with ! prefix returns undefined on nonexistent command
apiKey with ! prefix returns undefined on empty output
apiKey as environment variable name resolves to env value
apiKey as literal value is used directly when not an env var
apiKey command can use shell features like pipes

Tests cover the happy path, error cases, and edge cases comprehensively.