fix(kimi): forward role-specific model config to launch (restore deferred pending CLI verification)#2985
Conversation
codebanditssss
left a comment
There was a problem hiding this comment.
Careful PR, and the tests are the strongest part — reflect.DeepEqual against the full argv including flag ordering, plus the whitespace-only case and composition with the approval flags. That's the version of these tests that actually catches a regression rather than just executing the line. go test ./internal/adapters/agent/kimi/... green locally, all 9 checks green.
On deferring the restore path — I think that's right, and I can back it with precedent. You framed it as caution pending verification, but it also matches what the codebase already does:
claudecodeforwards--modelon launch (claudecode.go:172), and itsGetRestoreCommandemits only--resume <sessionID>plus an optional--append-system-prompt. No model.codexis the one that wires both (codex.go:109 and :151).
So launch-yes/restore-no is what the most prominent adapter already does. Kimi matching claudecode here isn't a gap to close later, it's the dominant existing shape — worth noting in case that changes how urgent the follow-up feels.
Approving. Two non-blocking notes inline, both about comments and prose rather than behaviour.
|
@anmol0b — new information relevant to the restore deferral here, which arrived after I approved. @Hardik180704 verified the equivalent composition against a real Pi CLI in #2989:
That's the first actual evidence in the series — every one of these PRs, including this one, had been reasoning from the same absence of it. To be clear about what it does and doesn't show: it settles Pi, not Kimi. They're different CLIs that happen to share a But it does mean the cost of checking is now known to be low — one resume against a real install — and it shifts the prior. Worth knowing because Nothing blocking — this stays approved either way. Just didn't want the deferral to outlive the uncertainty that motivated it. |
illegalcall
left a comment
There was a problem hiding this comment.
Reviewed and approving. ✅
- gofmt clean,
go vetclean, fullkimipackage test suite passes locally (go 1.24.1). - Verified against Moonshot's docs that
kimi --model <model>is a real, documented launch flag, so the launch-path forwarding will take effect. - Especially appreciate the discipline on the restore path: not forwarding
--modelalongside--sessionwhen the composition is unverified, documenting exactly why, and locking the decision in withTestGetRestoreCommandDoesNotForwardModelOverrideso a future "just mirror Copilot/Codex" edit can't silently regress it. This is the right posture when the CLI behavior can't be confirmed.
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. For faster context or live questions, you can also join the AO Discord. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
|
Thanks for catching this @codebanditssss Added the missing breadcrumb comment on On the restore deferral: I don't have a real kimi install to verify |
What
Adds
appendModelFlagto the Kimi adapter, called fromGetLaunchCommand, so a configuredagentConfig.modelis forwarded as--model <model>to thekimiCLI. Also adds a GetConfigSpec override advertising the model field, matching claude-code and codex (#2869). (kiro #2932 handles model config through a hooks file rather than a CLI flag, so it isn't precedent for this approach; copilot's equivalent fix, #2975, is still open and not yet merged.)Unlike the Copilot fix, this does not wire
appendModelFlagintoGetRestoreCommand.Kimi's docs don't confirm
--modelcomposes with--sessionon resume, so forwarding itthere is left as a documented fast-follow rather than guessed at.
Why
Fixes #2890. A Kimi worker/orchestrator configured with a specific model silently fell back
to
kimi's global default because the adapter never readcfg.Config.Model— sameroot-cause pattern as #2769 (Codex), #2895 (Copilot), and #2897 (Vibe) in this audit series.
How
appendModelFlagpattern: appends--modelonly whenstrings.TrimSpace(cfg.Config.Model)is non-blank, so an unconfigured launch isbyte-for-byte unchanged.
GetRestoreCommandis intentionally left unchanged. A doc comment onappendModelFlagand an inline comment on
GetRestoreCommandboth point back to this decision, and adedicated regression test (
TestGetRestoreCommandDoesNotForwardModelOverride) locks it inso a future "just mirror Copilot/Codex" edit doesn't silently wire it without verifying
kimi --model X --session Yagainst a real install first.TestGetConfigSpecEmpty(asserted zero fields) withTestGetConfigSpecAdvertisesModelField, since that assumption no longer holds oncemodelis declared.Testing
Full package run, all passing (35 tests, no regressions):
TestGetConfigSpecAdvertisesModelFieldTestGetLaunchCommandAppendsModelFlagWhenConfiguredTestGetLaunchCommandOmitsModelFlagWhenBlankTestGetLaunchCommandModelFlagComposesWithApprovalFlagsTestGetRestoreCommandDoesNotForwardModelOverrideOpen follow-up
Verify
--model+--sessioncomposition against a realkimiinstall, then wireappendModelFlagintoGetRestoreCommandin a follow-up PR (tracked against #2890).Checklist
main(or continuing an existing PR branch)go, frontend, etc.)