fix(kimi):forward role-specific model config on launch and restore#2979
fix(kimi):forward role-specific model config on launch and restore#2979Ashutsoh1729 wants to merge 1 commit into
Conversation
|
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. |
Pulkit7070
left a comment
There was a problem hiding this comment.
Reviewed against the model-config adapter family pattern. go test ./internal/adapters/agent/kimi -count=1 passes, go vet clean, go build ./... clean, gofmt -l clean.
Matches the established pattern:
appendModelFlagtrims and no-ops on blank/whitespace, preserving the tool default.- Launch appends
--modelafter the approval flags; there is no--prompt sentinel on this path (prompt is delivered after start), so trailing placement is fine. - Restore places
--modelbefore--session, and correctly still skips approval flags. GetConfigSpecwording ("Model override passed tokimi --model.") matches codex, and the test asserts the full field viareflect.DeepEqual.- Tests cover launch + restore + blank + configspec.
Non-blocking notes:
- The launch/restore command tests assert
containsSubsequencerather than an exactreflect.DeepEqualof the full argv. The argv on these paths is deterministic, so an exact assertion (as aider does) would additionally lock flag ordering. Optional. - One thing to confirm inline about
--modelalongside--session.
| } | ||
| cmd = []string{binary, "--session", agentSessionID} | ||
| cmd = []string{binary} | ||
| appendModelFlag(&cmd, cfg.Config) |
There was a problem hiding this comment.
The restore doc comment above notes that --yolo/--auto cannot be combined with --session. Worth confirming the Kimi CLI actually accepts --model together with --session (a resumed session may pin the original model, or reject the flag). This stays consistent with the codex resume-path behavior, so it is not a blocker, just verify the CLI does not error.
|
Closing in favor of #2985 which takes the more conservative launch-only approach pending CLI verification. |
What
Forward the configured role model to the
kimiCLI on both launch and restore, and advertise themodelconfig field.Why
Fixes #2890.
GetLaunchCommandandGetRestoreCommandbuilt their argv without readingcfg.Config.Model, so a worker or orchestrator configured with a specific model silently started on Kimi's global default. The adapter also inherited the emptyagentbase.Base.GetConfigSpec, so it never advertised amodelfield.How
Mirrors the Codex fix in #2869:
appendModelFlagappends--model <trimmed model>whenstrings.TrimSpace(cfg.Config.Model) != "", called from bothGetLaunchCommandandGetRestoreCommand.GetConfigSpecoverride advertisingmodelasports.ConfigFieldString.The flag is appended right after the permission flags so it sits with the other
append*Flagsoutput, ahead of--sessionon the restore path. Kimi gives the CLI flag the highest precedence, so this wins over a user's config file value as intended.Model values stay in Kimi's
provider/modelform; the adapter passes the string through untouched apart from trimming.Testing
Three new tests, proven to fail on
mainbefore the fix:TestGetLaunchCommandOmitsBlankConfiguredModelpasses both before and after, so it pins the blank/whitespace case rather than restating the fix.Checklist
main