fix(copilot): forward role-specific model config to launch and restore#2975
fix(copilot): forward role-specific model config to launch and restore#2975anmol0b wants to merge 3 commits 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. |
codebanditssss
left a comment
There was a problem hiding this comment.
Code is correct and go test ./internal/adapters/agent/copilot/... is green locally, all 9 checks green. The blank/whitespace guard and the byte-for-byte-unchanged-when-unconfigured property both hold.
One thing surfaced by reading this next to #2985, which I looked at just before this. The two PRs take opposite positions on the restore path, and I don't think either is wrong — but the series would be easier to follow if it said why. Detail inline.
For what it's worth, both positions have precedent in the tree: codex wires restore (codex.go:151), claudecode doesn't. So this isn't a case of one of them being a mistake.
|
Following up on my earlier question about whether @Hardik180704 checked the equivalent composition against a real CLI in #2989:
That doesn't settle this PR — Copilot uses Given you wired restore here and deferred it in #2985 for exactly this reason, one run of Still not blocking from my side — codex sets precedent for wiring restore, so this isn't wrong as it stands. |
|
Thanks for this — the code is clean (gofmt/vet clean, tests pass locally) and the launch path is well-grounded: One thing I'd like evidence on before merging: the restore path. This PR also appends The Copilot CLI docs describe Could you add a bit of evidence that If it turns out resume doesn't honor |
Pulkit7070
left a comment
There was a problem hiding this comment.
Reviewed against the model-config adapter family pattern. go test ./internal/adapters/agent/copilot -count=1 passes, go vet clean, go build ./... clean, gofmt -l clean.
Matches the established pattern:
appendModelFlagtrims and no-ops on blank/whitespace.- Launch and restore both append
--modelafter approval flags and before--agent/--interactive <prompt>, so it is ahead of the positional prompt. GetConfigSpecadvertises themodelfield with a good description including an example.- Tests cover launch + restore + blank + configspec, plus a canceled-context check. Nice.
Non-blocking note inline about the helper signature.
…ly consistency string directly. Functionally identical output.
|
heads up, the build-test and lint failures here aren't flaky, so a re-run won't clear them. both are the same root cause: gofmt/goimports on 6 files. build-test (
quick fix from backend/: or just |
|
Re-checked after 637505e and the CI retrigger. The note from my first pass is addressed: On the lint/build-test failure flagged earlier: it is resolved on the current head (6327609). I pulled both changed files at head and ran gofmt -l with no output, so they are properly formatted, and the format, lint, and build-test checks are all green now (along with the native matrix on ubuntu/macos/windows). For what it is worth, the "6 files" in the earlier heads-up does not match this PR, which changes only copilot.go and copilot_test.go, so that comment looks like it referred to the broader family or a prior state. Nothing outstanding from me. The change is the standard, behavior-preserving model-flag forwarding, now consistent with the family, gofmt clean, and green across CI. |
Pulkit7070
left a comment
There was a problem hiding this comment.
Requesting changes to hold the merge until the restore-path behavior is settled. To be clear, this is not a code-quality problem: I stress-ran the package (go test -race -count=3), plus go vet and go build, all clean, gofmt is clean, and the appendModelFlag refactor to take ports.AgentConfig is good. The launch path is solid: copilot --model is a documented launch flag.
The open item is the restore path, and I am corroborating the concern already raised on this PR. GetRestoreCommand emits copilot [approval] --model <value> --resume <id>. Copilot's --resume loads the full saved conversation context, and the CLI flag reference does not document --resume honoring --model, so on resume the flag may be silently ignored (the session keeps its original model) rather than applied. Both are valid top-level flags, so it will not error, which means the current argv-shape tests would pass even if resume forwarding is a no-op. I could not verify this locally because it needs a real authenticated copilot install.
Two ways to clear this:
- Provide evidence that --model is honored on resume against a real copilot install (a terminal capture of a resumed session running on the overridden model versus its original).
- Or forward on launch only and drop --model from GetRestoreCommand, mirroring the Kimi PR #2985, and lock that decision in with a test asserting restore does not carry --model until it is verified.
Launch-only forwarding is a solid, mergeable fix on its own. Holding the merge until one of the above lands.
What
Adds appendModelFlag to the GitHub Copilot adapter, called from both GetLaunchCommand and GetRestoreCommand, so a configured agentConfig.model is forwarded as --model to the copilot CLI. Also adds a GetConfigSpec override advertising the model field, matching claude-code, codex (#2869), and kiro (#2932).
Why
Fixes #2895. A worker/orchestrator configured with a specific model silently fell back to Copilot's global default because the adapter never read cfg.Config.Model — same root-cause pattern as #2769 (Codex) and #2897 (Vibe) in this audit series.
How
Testing
All existing tests pass, plus:
Checklist
main(or continuing an existing PR branch)go, frontend, etc.)