fix(kiro): advertise model config via GetConfigSpec#2932
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. |
codebanditssss
left a comment
There was a problem hiding this comment.
Cleanest of the three model-config PRs I've looked at today, and the only one whose description doesn't overstate anything.
I traced the "already forwards it" claim rather than trusting the comment, since the whole PR rests on it:
setKiroAgentDefaults(hooks.go:250) writesdefaults["model"] = modelwhen non-blank- reached via
writeKiroHooksfrom the install path at hooks.go:108, which passes the realcfg.Config
I also chased the second writeKiroHooks call at hooks.go:148, which passes an empty ports.AgentConfig{} and therefore takes the else branch and deletes topLevel["model"]. That looked like it might silently wipe a configured model, but it's inside UninstallHooks, and model sits alongside name and prompt in the managed-key set — so clearing it on uninstall is correct, not a leak. Flagging only so the next reader doesn't have to re-derive it.
So declaring model here is honest: the capability genuinely exists on the install path, and this just makes it discoverable.
go test ./internal/adapters/agent/kiro/... green locally, gofmt clean, all 9 checks green.
Approving. One cosmetic note inline, plus one thing worth crediting.
illegalcall
left a comment
There was a problem hiding this comment.
Reviewed and approving. ✅
- gofmt clean,
go vetclean, fullkiropackage test suite passes locally (go 1.24.1). - Scope is correct: Kiro already forwards
agentConfig.Modelinto its workspace-local agent config viasetKiroAgentDefaults(hooks.go), so this PR rightly only advertises the field throughGetConfigSpecrather than adding a redundant launch/restore flag — matching claude-code/codex on the config-spec surface. GetConfigSpechonorsctx.Err()and the new canceled-context test covers it.
Pulkit7070
left a comment
There was a problem hiding this comment.
Reviewed against the model-config adapter family pattern. go test ./internal/adapters/agent/kiro -count=1 passes, go vet clean, go build ./... clean, gofmt -l clean.
This PR correctly only adds GetConfigSpec, since Kiro already forwards the model. I verified the claim in the diff comment: setKiroAgentDefaults (backend/internal/adapters/agent/kiro/hooks.go:268) writes strings.TrimSpace(agentConfig.Model) into the workspace-local agent config when non-empty and deletes the key when blank, so the blank/whitespace no-op holds and there is no flag to add on launch/restore.
The Description ("Model override written into Kiro's workspace-local agent config.") accurately describes the config-write mechanism rather than a fictional --model flag, which is the right call here (similar in spirit to how Vibe writes an active_model rather than a flag). Tests cover the configspec field plus a canceled-context check.
Minor style nit (optional, gofmt is already clean): the new TestGetConfigSpecHonorsContextCancellation has no blank line before the following TestAuthStatusUnauthorizedFromKiroWhoami.
What
Adds a GetConfigSpec override to the Kiro agent adapter so it advertises the model config field it already honors. Kiro was already forwarding agentConfig.Model into its workspace-local custom agent config, but it never declared that support through the config-spec surface, this brings it in line with claude-code and codex.
Why
Fixes #2903.
agentConfig.model is resolved and forwarded to every adapter, and adapters that honor it are expected to also advertise it via GetConfigSpec , Claude does this, and Codex's fix (#2869) added the same. Kiro's runtime behavior was already correct (verified by TestGetAgentHooksWritesConfiguredModel and related tests), but it inherited the empty agentbase.Base.GetConfigSpec, so the model field it consumes was never discoverable or validated through the config-spec surface — inconsistent with the other model-aware adapters.
How
Testing
cd backend
go build ./internal/adapters/agent/kiro/...
go test ./internal/adapters/agent/kiro/... -v
All existing tests pass, plus the two new ones:
Checklist
main(or continuing an existing PR branch)go, frontend, etc.)