Repository navigation
fix: specifying the model on ACP - #215
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe ACP client now sets session models during create and resume when provided, reuses the computed model name in ACP session requests, and updates test doubles and executor assertions for the new call path. ChangesACP session model configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| internal/core/agent/client.go | Adds SetSessionModel and wires it to ACP session/set_config_option. |
| internal/core/run/internal/acpshared/command_io.go | Adds fresh-session model pinning, but the failed pin path and resume path can still behave incorrectly. |
| internal/core/run/executor/execution_acp_test.go | Updates executor tests and fakes to track fresh-session model pinning. |
| internal/core/run/internal/acpshared/command_io_test.go | Updates ACP shared test fakes for the expanded client interface. |
Comments Outside Diff (1)
-
internal/core/run/internal/acpshared/command_io.go, line 395-406 (link)Resume Skips Model This resume branch still only passes
modelNameinsideResumeSessionRequest. It does not callSetSessionModelbefore the prompt turn, and the resume implementation loads the existing session without a model field. When a resumed job selects a different model from the saved session, the prompt can still run on the old ACP session model.Context Used: CLAUDE.md (source)
Reviews (7): Last reviewed commit: "fix: fixing tests" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/run/internal/acpshared/command_io.go (1)
395-406: 🎯 Functional Correctness | 🟡 MinorResume should also apply the active model. The create branch calls
SetSessionModel, but the resume branch only passesModelinResumeSessionRequest. If that field doesn’t update the loaded session’s active model, resumed runs will keep the prior/default model. Mirror the create flow here.internal/core/run/internal/acpshared/command_io.go:395-406🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/run/internal/acpshared/command_io.go` around lines 395 - 406, The resume path in ResumeSession currently only passes Model through agent.ResumeSessionRequest, so the loaded session may keep its previous active model. Update the resume flow in command_io.go to mirror the create path by explicitly applying the active model to the session using the same model-setting logic used for new sessions (for example, the SetSessionModel behavior around the create branch), ensuring resumed runs use modelName instead of the prior/default model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/run/exec/exec_integration_test.go`:
- Around line 702-704: The fake capturingExecACPClient currently implements
SetSessionModel but discards the session/model arguments and always returns
success, so the tests cannot verify the ACP model-setting flow. Update
SetSessionModel to record the passed context/session/model values on
capturingExecACPClient, and add a way to inject a return error so exec
integration tests can assert both the expected values and failure propagation.
Use the existing capturingExecACPClient helper and the new SetSessionModel call
path in the exec integration test suite to validate the behavior outcome.
In `@internal/core/run/executor/execution_acp_test.go`:
- Around line 1539-1541: `fakeACPClient.SetSessionModel` is currently a silent
no-op, unlike the other tracked ACP session methods. Update `fakeACPClient` to
record invocations and capture the passed arguments for `SetSessionModel`,
following the pattern used by the other fake ACP call counters in this test
file. Then adjust the relevant assertions in the `execution_acp_test.go` suite
so tests can verify the model-setting path was exercised correctly and validate
the observed behavior, not just that the call exists.
In `@internal/core/run/internal/acpshared/command_io.go`:
- Around line 386-393: The session model is being applied too late in the
create/resume flow, so the first prompt can start with the wrong model and
resumed sessions ignore the requested model. Update the session setup in the
create/resume path around client.CreateSession and client.SetSessionModel so the
model is set before runPrompt begins, and make sure the resume path applies the
provided Model value as part of session initialization rather than after the
session is already running.
---
Outside diff comments:
In `@internal/core/run/internal/acpshared/command_io.go`:
- Around line 395-406: The resume path in ResumeSession currently only passes
Model through agent.ResumeSessionRequest, so the loaded session may keep its
previous active model. Update the resume flow in command_io.go to mirror the
create path by explicitly applying the active model to the session using the
same model-setting logic used for new sessions (for example, the SetSessionModel
behavior around the create branch), ensuring resumed runs use modelName instead
of the prior/default model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6693fe1c-1da2-4e83-8fd2-fedbf9316c2f
📒 Files selected for processing (10)
internal/cli/root_command_execution_test.gointernal/core/agent/client.gointernal/core/kernel/recovery_prepared_run_test.gointernal/core/run/exec/exec_integration_test.gointernal/core/run/exec/run_agent_engine_integration_test.gointernal/core/run/executor/execution_acp_test.gointernal/core/run/internal/acpshared/command_io.gointernal/core/run/internal/acpshared/command_io_test.gointernal/core/run/internal/acpshared/session_exec_test.gointernal/core/run/recovery/agentic_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/agent/client.go`:
- Around line 405-408: The session lifecycle hook is firing too early in the
session creation flow, before SetSessionModel can fail and trigger
c.removeSession(session.id). Move the agent.post_session_create hook in the
session creation path of client.go so it runs only after model setup succeeds,
and keep the hook placement aligned with the existing session creation logic
around SetSessionModel and session.id.
- Around line 405-406: The model-setting RPC in the session setup path is using
the wrong context; update the `SetSessionModel` call inside the `AgentClient`
session initialization flow to use `setupCtx` rather than `ctx`, and apply the
same fix in the other matching call site around the later session setup branch.
Use the `SetSessionModel` invocation in the `NewSession`/`LoadSession` handling
path to locate the change and keep the RPC scoped to setup cancellation and
timeout.
In `@internal/core/run/executor/execution_acp_test.go`:
- Around line 214-218: The assertion around secondClient.lastSetModelID in
execution_acp_test.go is performed while setModelMu is still locked, so a
failure can leave the mutex locked and affect later tests. In the test that uses
secondClient and setModelMu, read the field into a local variable while holding
the lock, release the lock immediately, then perform the t.Fatalf check on the
copied value outside the critical section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05fbbc6d-2a12-4060-aa51-21db1dc129f9
📒 Files selected for processing (3)
internal/core/agent/client.gointernal/core/run/executor/execution_acp_test.gointernal/core/run/internal/acpshared/command_io.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/run/internal/acpshared/command_io.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/agent/client.go (1)
533-536: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the loaded session when model setup fails.
Line 513 stores the resumed session before this call, but this error path returns without
c.removeSession(session.id), leaving a locally tracked session after setup failed.Proposed fix
if modelID := strings.TrimSpace(req.Model); modelID != "" { if err := c.SetSessionModel(setupCtx, session.id, modelID); err != nil { + c.removeSession(session.id) return nil, fmt.Errorf("set session model before prompt: %w", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/agent/client.go` around lines 533 - 536, The session setup path in client.go leaves a resumed session tracked locally when SetSessionModel fails. In the model setup block in c.handlePrompt (the code that calls c.SetSessionModel), remove the stored session with c.removeSession(session.id) before returning the error so the failed resume does not stay in the client’s session map.
🧹 Nitpick comments (1)
internal/core/agent/client.go (1)
878-880: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
context.Background()in these helpers.detachedContextandcleanupContextshould require a non-nil caller context, or use a non-production placeholder at the call site, so this stays aligned with the context policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/agent/client.go` around lines 878 - 880, The helpers that currently fall back to context.Background() should not create a production root context; update detachedContext and cleanupContext to require a non-nil caller-provided context instead, or move any placeholder context creation to the call site. Use the existing helper names in internal/core/agent/client.go to locate the fallback and remove the nil-guard so these helpers stay aligned with the context policy.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/core/agent/client.go`:
- Around line 533-536: The session setup path in client.go leaves a resumed
session tracked locally when SetSessionModel fails. In the model setup block in
c.handlePrompt (the code that calls c.SetSessionModel), remove the stored
session with c.removeSession(session.id) before returning the error so the
failed resume does not stay in the client’s session map.
---
Nitpick comments:
In `@internal/core/agent/client.go`:
- Around line 878-880: The helpers that currently fall back to
context.Background() should not create a production root context; update
detachedContext and cleanupContext to require a non-nil caller-provided context
instead, or move any placeholder context creation to the call site. Use the
existing helper names in internal/core/agent/client.go to locate the fallback
and remove the nil-guard so these helpers stay aligned with the context policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a8d9469-0262-4103-a742-9772c962c2cc
📒 Files selected for processing (1)
internal/core/agent/client.go
This was referenced Jun 30, 2026
pedronauck
added a commit
that referenced
this pull request
Jul 3, 2026
Add human-written release notes for the next release covering the work merged since v0.2.10: - feat: dependency-aware parallel task execution (--parallel-tasks) (#212) - feat: agentic recovery for failed runs (--recovery) (#212) - feat: COMPOZY_HOME for isolated per-project daemons (#216) - feat: repo-level default overrides in compozy setup (#90) - fix: safer worktree management for parallel runs (#217, #223) - fix: ACP runs consistently apply the selected model (#215) - fix: run TUI elapsed timer restored across all terminal outcomes (#221) Co-Authored-By: Claude Fable 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit