feat: Adding kimchi CLI option - #5376
Conversation
lefarcen
left a comment
There was a problem hiding this comment.
Hey @bentleyvk! 👋 Thanks for adding Kimchi as a new agent option.
Could you fill out the PR template before pool review? The body is currently empty except for the template comments. We need:
- Why — what made you add Kimchi today? (Did you hit a use case where Kimchi was the right fit, or are you speculatively adding it for others?)
- What users will see — "Settings → AI Providers now shows a new 'Kimchi' option" + where else the Kimchi icon/name appears (HandoffButton CLI dropdown, agent picker, etc.)
- Surface area — tick UI (agent icon + picker) and New top-level dependency if this adds any new runtime dependency
- Validation — what you ran (
pnpm guard+pnpm typecheck+ the relevant package tests)
The info helps pool review scope the change quickly.
PerishCode
left a comment
There was a problem hiding this comment.
Found one non-blocking docs consistency issue in the Kimchi adapter catalog. The runtime wiring itself follows the existing Pi RPC path I reviewed.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.|
🧪 This PR adds user-visible runtime/UI behavior, so it needs a manual QA pass before merge. Please hold off self-merging for now — once design/product sign-off is in and the PR is merge-ready, we'll queue QA here. |
PerishCode
left a comment
There was a problem hiding this comment.
Found one non-blocking documentation mismatch after the latest Kimchi model-list fix. The runtime code now reads stdout and uses the updated fallback model id, but the adapter docs still describe the previous behavior.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.|
Hey @bentleyvk — PerishCode already caught the one follow-up I'd point to on this head: please align the Kimchi adapter docs with the runtime change so model discovery reads as stdout and the fallback model matches Once that docs note is updated, this head looks ready to keep moving through the design/QA gates. |
Fixed comments |
|
Thanks @bentleyvk — I checked the docs update on this head and the Kimchi note now matches the runtime change ( No further bot follow-up from my side here; this can keep moving through the remaining design / QA gates once the pending checks finish. |
|
@lefarcen any news regarding review? |
|
Hey @bentleyvk — I took another pass over the current head and I don't have a new bot follow-up beyond the docs fix that's already been addressed. I re-requested review on this head and cleaned up the stale pending design label, since design is already signed off. The main thing still showing on GitHub right now is that the PR is in a merge-conflict state ( |
@lefarcen Thanks, fixed it! |
|
Thanks — the rebase looks clean now, and I don't see the earlier merge-conflict state anymore. From here it's just waiting on the pending human review for this head. |
|
@bentleyvk I'm holding off on generating review comments for #5376 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos. |
PerishCode
left a comment
There was a problem hiding this comment.
The Kimchi runtime wiring is coherent with the existing Pi RPC path, but the new API-key override crosses the app-config secret boundary without the matching browser-side protection. This needs to be fixed before merge.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/daemon/src/app-config.ts RIGHT line 207
Treat KIMCHI_API_KEY as a secret across the full app-config path before allowing it here. This changed line makes the daemon accept and return the key in agentCliEnv, but apps/web/src/state/config.ts::AGENT_CLI_SECRET_ENV_KEYS does not include it, so sanitizeAgentCliEnv() can copy the credential into browser localStorage when config is persisted. The existing Claude/Codex keys are explicitly excluded there and are also tracked by the daemon/web auth-key sets; Kimchi currently bypasses those protections. Please either remove KIMCHI_API_KEY from this allowlist and rely on the daemon process environment/config file, or add it consistently to the daemon auth classification and web secret sanitizer (plus a regression test proving persisted web config omits it).
Inline comment could not be anchored: anchor_outside_complete_diff













































Fixes #
Why
What users will see
Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-pack/tools-prflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)Screenshots
Validation
pnpm guard- passedpnpm typecheck- passedpnpm --filter @open-design/daemon test- passed