Skip to content

fix(kimi):forward role-specific model config on launch and restore#2979

Closed
Ashutsoh1729 wants to merge 1 commit into
AgentWrapper:mainfrom
Ashutsoh1729:fix/kimi-role-model-config-2890
Closed

fix(kimi):forward role-specific model config on launch and restore#2979
Ashutsoh1729 wants to merge 1 commit into
AgentWrapper:mainfrom
Ashutsoh1729:fix/kimi-role-model-config-2890

Conversation

@Ashutsoh1729

Copy link
Copy Markdown

What

Forward the configured role model to the kimi CLI on both launch and restore, and advertise the model config field.

Why

Fixes #2890.

GetLaunchCommand and GetRestoreCommand built their argv without reading cfg.Config.Model, so a worker or orchestrator configured with a specific model silently started on Kimi's global default. The adapter also inherited the empty agentbase.Base.GetConfigSpec, so it never advertised a model field.

How

Mirrors the Codex fix in #2869:

  • appendModelFlag appends --model <trimmed model> when strings.TrimSpace(cfg.Config.Model) != "", called from both GetLaunchCommand and GetRestoreCommand.
  • GetConfigSpec override advertising model as ports.ConfigFieldString.

The flag is appended right after the permission flags so it sits with the other append*Flags output, ahead of --session on 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/model form; the adapter passes the string through untouched apart from trimming.

Testing

cd backend && go build ./... && go test -v ./internal/adapters/agent/kimi/...

Three new tests, proven to fail on main before the fix:

--- PASS: TestGetLaunchCommandAppendsConfiguredModel (0.00s)
--- PASS: TestGetLaunchCommandOmitsBlankConfiguredModel (0.00s)
--- PASS: TestGetRestoreCommandAppendsConfiguredModel (0.00s)
--- PASS: TestGetConfigSpecReportsModelField (0.00s)

TestGetLaunchCommandOmitsBlankConfiguredModel passes both before and after, so it pins the blank/whitespace case rather than restating the fix.

Checklist

  • Branched from main
  • One focused change; links the related issue
  • Follows AGENTS.md conventions and PR hygiene
  • Tests added for user-visible behavior
  • Relevant CI checks pass for the area touched

@somewherelostt

Copy link
Copy Markdown
Collaborator

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.
The on-call pair is added for visibility, tracking, and support, not to take over the work.
If you need help with review, direction, reproduction, or next steps, please tag @illegalcall and @Pulkit7070 here.

For faster context or live questions, you can also join the AO Discord.

Join the session here:
https://discord.gg/H6ZDcUXmq

Come by if you want to see what is being built, ask questions, or just hang around with the community.

@Pulkit7070 Pulkit7070 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • appendModelFlag trims and no-ops on blank/whitespace, preserving the tool default.
  • Launch appends --model after the approval flags; there is no -- prompt sentinel on this path (prompt is delivered after start), so trailing placement is fine.
  • Restore places --model before --session, and correctly still skips approval flags.
  • GetConfigSpec wording ("Model override passed to kimi --model.") matches codex, and the test asserts the full field via reflect.DeepEqual.
  • Tests cover launch + restore + blank + configspec.

Non-blocking notes:

  • The launch/restore command tests assert containsSubsequence rather than an exact reflect.DeepEqual of 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 --model alongside --session.

}
cmd = []string{binary, "--session", agentSessionID}
cmd = []string{binary}
appendModelFlag(&cmd, cfg.Config)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ashutsoh1729

Copy link
Copy Markdown
Author

Closing in favor of #2985 which takes the more conservative launch-only approach pending CLI verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kimi ignores role-specific model config on launch

4 participants