Conversation
Use the effective session model for Ferment evaluation and remove legacy role dependencies. Fix model-cycle wrapping and fresh-session Auto with saved model lists. Pass benchmark model selections to subprocesses through KIMCHI_MODEL.
Restore phases, roles, Ferment integration and legacy CLI/ACP support. Hide multi-model from the picker, model cycle, command suggestions, help and tips while keeping explicit legacy commands functional. Retain Auto-default regression fixes.
Kimchi Code Review
Summary📊 Review Score: 85/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Strong coverage across all changed behavior. New unit tests in 📝 Found 4 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 85/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Strong coverage across all changed behavior. New unit tests in src/cli-args.test.ts cover applyModelEnvArgs precedence, flag-shaped prompt content, and models caching; src/extensions/router/index.test.ts covers fresh-vs-resumed session Auto defaults, explicit launch choices, empty persisted sessions, and --continue with no session. New e2e tests in tests/e2e/tui/auto-model.test.ts cover default Auto, cycle wrap, inherited KIMCHI_MODEL, resume lifecycle, and tests/e2e/tui/multi-model-command.test.ts verifies hiding of legacy multi-model. The benchmark adapter change has assertions in agent_test.py for both single-model and multi-model env propagation.
📝 Found 4 issue(s). See inline comments for details.
| const entries = ctx.sessionManager.getEntries() | ||
| const requestedModel = | ||
| event.reason === "startup" && options.handleCliModelSelection ? getParsedCliArgs().options.model : undefined | ||
| const sessionFile = ctx.sessionManager.getSessionFile() |
There was a problem hiding this comment.
Fresh-session detection for the Auto default depends on existsSync(ctx.sessionManager.getSessionFile()) returning false at session_start time. If a future pinned Pi release creates the session file (or writes the session header) before extensions receive session_start, hasPersistedSession becomes true for genuinely new sessions and the Auto default silently stops applying — the entries.some(entry => entry.type === "message") fallback cannot compensate because a header-only file has no messages. Unit tests mock getSessionFile, so only the e2e suite guards this upstream ordering invariant.
💡 Suggestion: Decouple freshness from file-existence timing: prefer explicit signals already available (event.previousSessionFile, resume/fork flags) combined with the message-entry check, or at minimum add a comment documenting the lazy-creation ordering requirement in pi-coding-agent and a cheap upstream-invariant test (e.g. assert a brand-new session has no file at session_start) so a Pi upgrade breaks the build loudly instead of silently changing default behavior.
| writeFileSync(join(agentDir, "extensions", "test-ui-extension.js"), extSource, "utf-8") | ||
|
|
||
| proc = spawn(BINARY_PATH, ["--mode", "acp", ...extraArgs], { | ||
| const modelArgs = defaultModel === undefined ? ["--model", configuredModels[0].slug] : [] |
There was a problem hiding this comment.
ℹ️
const modelArgs = defaultModel === undefined ? ["--model", configuredModels[0].slug] : [] dereferences configuredModels[0] without a guard, while the sibling settings line two statements earlier uses configuredModels[0]?.slug. A fixture started with models: [] would crash with an opaque TypeError: Cannot read properties of undefined instead of a clear configuration error.
💡 Suggestion: Mirror the sibling guard: use configuredModels[0]?.slug and throw a descriptive error (e.g. new Error("startAcpFixture requires at least one configured model")) when it is undefined.
| } | ||
| } | ||
|
|
||
| // Consume upstream option values so text such as --system-prompt "--model" |
There was a problem hiding this comment.
ℹ️🐛 Bug
The protection loop for (const flag of PRE_DISPATCH_VALUE_FLAGS) { if (flag.startsWith("--")) PARSE_ARGS_OPTIONS[flag.slice(2)] ??= { type: "string" } } only registers long flags. If PRE_DISPATCH_VALUE_FLAGS ever contains a single-dash value-taking flag (e.g. -x value), a flag-shaped token following it (like --model) would still be parsed as an explicit model selection, causing applyModelEnvArgs to silently skip the KIMCHI_MODEL inheritance — workflow subprocesses would quietly fall back to Auto instead of the pinned model.
💡 Suggestion: Either register short-form value flags as single-key entries in PARSE_ARGS_OPTIONS as well, or add a startup assertion that every entry in PRE_DISPATCH_VALUE_FLAGS starts with -- so a future addition fails loudly instead of silently weakening the inheritance logic.
| } | ||
| } | ||
| let autoModel = ctx.model | ||
| const freshSession = |
There was a problem hiding this comment.
ℹ️🏗️ Design
explicitLaunchChoice honors only the CLI --models scope flag; a user's saved enabledModels cycle scope (settings.json) that excludes kimchi-dev/auto is ignored, so fresh sessions are still forced into Auto — a model outside the user's configured Ctrl+P cycle. The status line will show auto while their cycle list does not contain it, which is mildly inconsistent UX and could surface as cycling landing on an unexpected model.
💡 Suggestion: Either document explicitly in docs/model-routing.md that Auto overrides saved model-scope preferences for new sessions, or treat a saved enabledModels list that excludes AUTO_MODEL_REF the same as an explicit scope and skip the forced Auto selection.
Auto becomes the fresh-session default only for @cast.ai accounts (internal dogfooding). Everyone else keeps the existing multi-model default; the gate touches only the fresh-session default, so Auto stays selectable and resumable for all users. The identity lookup is awaited at startup with a 3s budget and cached per process, including negative results. On timeout or failure the gate reports false, leaving the account on its existing default.
Completes the partial revert of the "retire multi-model" work, which left the picker, Ctrl+P cycle, command list and docs describing three different behaviors. - ui.ts: restore the Ctrl+P wrap into multi-model, so the cycle can re-enter the mode instead of being a one-way exit. - slash-commands.ts: restore the /multi-model hint, which the tips surface still points users at. - model-selector patch: route the four Auto-row checks through one isAutoModelEntry helper rather than repeating the provider/id pair. - README + docs/model-routing.md: multi-model is the default and Auto is opt-in, listed first in the picker and cycle. Also fold in review follow-ups on the gate: compare the parsed email domain exactly instead of suffix-matching (so sub.cast.ai no longer depends on string coincidence), split the address table into accepted / rejected / malformed cases, and register temp-dir cleanup with onTestFinished instead of try/finally.
mszostok
changed the title
3e90bf2 to
cb3bf2d
Compare
The Auto-by-default gate resolves the account through /v1/me, which the fake gateway did not implement — the lookup 404'd, the gate fell back to multi-model, and the ACP "starts each new session in Auto" spec failed. The three TUI fresh-session specs would have failed the same way. Add a /v1/me route to the fake server behind a userEmail option. It defaults to an internal address so Auto-default scenarios keep their original meaning; pass an external address for the gated-off path, or null to serve 404. Both fixtures now point KIMCHI_REMOTE_ENDPOINT at the fake server. It was unset, so the lookup resolved to the real app API and only stayed hermetic because the call failed. Also cover the external account in ACP: a new session is not on Auto, while kimchi-dev/auto stays in availableModels.
Selecting Auto via /model or Ctrl+P persists as the global saved default (settings.json), not a per-session choice. New sessions therefore start with Auto regardless of account type; only the fresh-session promotion to Auto is gated to @cast.ai accounts.
…del behavior The post-revert contract (cb3bf2d 'keep multi-model fully selectable alongside Auto') left one stale TUI test that asserted multi-model was hidden from the picker and command suggestions, and left several role picker tests relying on undocumented auto-first ordering. - Replace the stale hidden test: assert the picker leads with Auto then the virtual multi-model row, and that /multi autocomplete offers /multi-model. Keep the orchestrator seedHome - the virtual row is only injected when the orchestrator role's model exists in the picker list, which it borrows for the row. - Make the auto-first ordering explicit in the cursor-reset test: assert the cursor starts on row 0 (auto), moves off it, and resets. - Extract the cursorModelRow helper for cursor-row extraction. - Fix stale comment claiming row 0 is 'basic' (it is 'auto').
1 participant
No description provided.