Skip to content

Commit 9cc18cc

Browse files
authored
feat(acp): improve session validation and model resolution (#682)
- Fix session validation in `SessionUpdate` to validate first ACP session update against agent's configured session ID before storing - Update test expectations to match new behavior where session mismatches log but don't return errors (avoiding connection breaks) - Add fallback to ACP config model in `ModelForSelectedAgent` when no workflow model is configured - Remove redundant `cliModel` empty check in fallback condition Co-authored-by: Veit Sanner <VeitSanner@users.noreply.github.com>
1 parent f90c05d commit 9cc18cc

24 files changed

Lines changed: 540 additions & 74 deletions

cmd/roborev/analyze.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ func runFixAgent(cmd *cobra.Command, repoPath, agentName, model, reasoning, prom
766766
agentName = resolution.PreferredAgent
767767

768768
a, err := agent.GetAvailableWithConfig(
769-
agentName, cfg, resolution.BackupAgent,
769+
repoPath, agentName, cfg, resolution.BackupAgent,
770770
)
771771
if err != nil {
772772
return fmt.Errorf("get agent: %w", err)

cmd/roborev/config_cmd_test.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -393,13 +393,8 @@ func TestSetConfigKeyRepoConfigWritesComments(t *testing.T) {
393393
}
394394
}
395395

396-
func TestSetConfigKeyRepoConfigRejectsGlobalACPSettings(t *testing.T) {
397-
dir := t.TempDir()
398-
path := filepath.Join(dir, ".roborev.toml")
399-
400-
err := setConfigKey(path, "acp.command", "malicious-wrapper", false)
401-
require.ErrorContains(t, err, "is a global setting")
402-
}
396+
// Note: ACP config is now supported at both global and repo level.
397+
// This test was removed as repo-level ACP config is now valid.
403398

404399
func TestSetConfigKeyGlobalWritesComments(t *testing.T) {
405400
path := setupConfigFile(t)

cmd/roborev/fix.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ func resolveFixAgent(repoPath string, opts fixOptions) (agent.Agent, error) {
345345
}
346346

347347
a, err := agent.GetAvailableWithConfig(
348-
resolution.PreferredAgent, cfg, resolution.BackupAgent,
348+
repoPath, resolution.PreferredAgent, cfg, resolution.BackupAgent,
349349
)
350350
if err != nil {
351351
return nil, fmt.Errorf("get agent: %w", err)

cmd/roborev/refine.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,7 @@ func runRefine(ctx RunContext, opts refineOptions) error {
412412
// backup determination to avoid baking the primary model into a
413413
// backup agent).
414414
addressAgent, err := selectRefineAgent(
415-
cfg, resolution.PreferredAgent, reasoningLevel, resolution.BackupAgent,
415+
repoPath, cfg, resolution.PreferredAgent, reasoningLevel, resolution.BackupAgent,
416416
)
417417
if err != nil {
418418
return fmt.Errorf("no agent available: %w", err)
@@ -1243,8 +1243,8 @@ func verifyRepoState(
12431243
return nil
12441244
}
12451245

1246-
func selectRefineAgent(cfg *config.Config, resolvedAgent string, reasoningLevel agent.ReasoningLevel, backups ...string) (agent.Agent, error) {
1247-
baseAgent, err := agent.GetAvailableWithConfig(resolvedAgent, cfg, backups...)
1246+
func selectRefineAgent(repoPath string, cfg *config.Config, resolvedAgent string, reasoningLevel agent.ReasoningLevel, backups ...string) (agent.Agent, error) {
1247+
baseAgent, err := agent.GetAvailableWithConfig(repoPath, resolvedAgent, cfg, backups...)
12481248
if err != nil {
12491249
return nil, err
12501250
}

cmd/roborev/refine_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func TestSelectRefineAgentCodexFallback(t *testing.T) {
152152

153153
t.Setenv("PATH", "")
154154

155-
_, err := selectRefineAgent(nil, "codex", agent.ReasoningFast, "")
155+
_, err := selectRefineAgent("", nil, "codex", agent.ReasoningFast, "")
156156
require.Error(t, err, "expected error when no agents are available")
157157
if !strings.Contains(err.Error(), "no agents available") {
158158
require.NoError(t, err)
@@ -233,7 +233,7 @@ func TestResolveAllowUnsafeAgents(t *testing.T) {
233233
func TestSelectRefineAgentCodexUsesRequestedReasoning(t *testing.T) {
234234
t.Cleanup(testutil.MockExecutable(t, "codex", 0))
235235

236-
selected, err := selectRefineAgent(nil, "codex", agent.ReasoningFast, "")
236+
selected, err := selectRefineAgent("", nil, "codex", agent.ReasoningFast, "")
237237
require.NoError(t, err, "selectRefineAgent failed: %v")
238238

239239
codexAgent, ok := selected.(*agent.CodexAgent)
@@ -252,7 +252,7 @@ func TestSelectRefineAgentCodexACPConfigAliasUsesACPResolution(t *testing.T) {
252252
},
253253
}
254254

255-
selected, err := selectRefineAgent(cfg, "codex", agent.ReasoningFast, "")
255+
selected, err := selectRefineAgent("", cfg, "codex", agent.ReasoningFast, "")
256256
require.NoError(t, err, "selectRefineAgent failed: %v")
257257

258258
acpAgent, ok := selected.(*agent.ACPAgent)
@@ -263,7 +263,7 @@ func TestSelectRefineAgentCodexACPConfigAliasUsesACPResolution(t *testing.T) {
263263
func TestSelectRefineAgentCodexFallbackUsesRequestedReasoning(t *testing.T) {
264264
t.Cleanup(testutil.MockExecutableIsolated(t, "codex", 0))
265265

266-
selected, err := selectRefineAgent(nil, "gemini", agent.ReasoningThorough, "")
266+
selected, err := selectRefineAgent("", nil, "gemini", agent.ReasoningThorough, "")
267267
require.NoError(t, err, "selectRefineAgent failed: %v")
268268

269269
codexAgent, ok := selected.(*agent.CodexAgent)
@@ -919,7 +919,7 @@ func TestApplyModelForAgent_BackupKeepsOwnModel(t *testing.T) {
919919
t.Cleanup(testutil.MockExecutableIsolated(t, "codex", 0))
920920

921921
selected, err := selectRefineAgent(
922-
nil, "gemini", agent.ReasoningStandard, "codex",
922+
"", nil, "gemini", agent.ReasoningStandard, "codex",
923923
)
924924
require.NoError(t, err, "selectRefineAgent: %v")
925925

cmd/roborev/review.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName
433433

434434
// Get the agent (try backup before hardcoded chain)
435435
a, err := agent.GetAvailableWithConfig(
436-
resolution.PreferredAgent, cfg, resolution.BackupAgent,
436+
repoPath, resolution.PreferredAgent, cfg, resolution.BackupAgent,
437437
)
438438
if err != nil {
439439
return fmt.Errorf("get agent: %w", err)

internal/agent/acp_agent.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212

1313
acp "github.com/coder/acp-go-sdk"
1414
"github.com/roborev-dev/roborev/internal/config"
15+
"github.com/roborev-dev/roborev/internal/version"
1516
)
1617

1718
// Security error for path traversal attempts
@@ -269,6 +270,10 @@ func (a *ACPAgent) Review(ctx context.Context, repoPath, commitSHA, prompt strin
269270

270271
_, err = conn.Initialize(ctx, acp.InitializeRequest{
271272
ProtocolVersion: acp.ProtocolVersionNumber,
273+
ClientInfo: &acp.Implementation{
274+
Name: "roborev",
275+
Version: version.Version,
276+
},
272277
ClientCapabilities: acp.ClientCapabilities{
273278
Fs: acp.FileSystemCapabilities{
274279
ReadTextFile: true,

internal/agent/acp_client_permissions.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@ package agent
33
import (
44
"context"
55
"fmt"
6-
acp "github.com/coder/acp-go-sdk"
6+
"log"
77
"strings"
8+
9+
acp "github.com/coder/acp-go-sdk"
810
)
911

1012
func (c *acpClient) RequestPermission(ctx context.Context, params acp.RequestPermissionRequest) (acp.RequestPermissionResponse, error) {
@@ -89,8 +91,13 @@ func (c *acpClient) RequestPermission(ctx context.Context, params acp.RequestPer
8991
}
9092

9193
func (c *acpClient) SessionUpdate(ctx context.Context, params acp.SessionNotification) error {
94+
// Validate against the established session. Only NewSession may set
95+
// c.sessionID; an incoming notification must never bootstrap it, because a
96+
// stale or spoofed early notification could otherwise bind the client to
97+
// the wrong session and cause later legitimate updates to be rejected.
9298
if err := c.validateSessionID(params.SessionId); err != nil {
93-
return err
99+
log.Printf("ACP session update rejected: %v", err)
100+
return nil
94101
}
95102

96103
// Handle streaming updates from the agent

internal/agent/acp_resolution.go

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,16 +20,19 @@ func defaultACPAgentConfig() *config.ACPAgentConfig {
2020
}
2121
}
2222

23-
func isConfiguredACPAgentName(name string, cfg *config.Config) bool {
23+
func isConfiguredACPAgentName(name string, cfg *config.Config, repoPath string) bool {
2424
rawName := strings.TrimSpace(name)
2525
if rawName == defaultACPName {
2626
return true
2727
}
28-
if cfg == nil || cfg.ACP == nil {
28+
29+
// Check if there's a configured ACP name in either repo or global config
30+
acpCfg := config.ResolveACPAgentConfig(repoPath, cfg)
31+
if acpCfg == nil {
2932
return false
3033
}
3134

32-
configuredName := strings.TrimSpace(cfg.ACP.Name)
35+
configuredName := strings.TrimSpace(acpCfg.Name)
3336
if rawName == "" || configuredName == "" {
3437
return false
3538
}
@@ -41,11 +44,8 @@ func isConfiguredACPAgentName(name string, cfg *config.Config) bool {
4144
return rawName == configuredName
4245
}
4346

44-
func configuredACPAgent(cfg *config.Config) *ACPAgent {
45-
var acpCfg *config.ACPAgentConfig
46-
if cfg != nil {
47-
acpCfg = cfg.ACP
48-
}
47+
func configuredACPAgent(repoPath string, cfg *config.Config) *ACPAgent {
48+
acpCfg := config.ResolveACPAgentConfig(repoPath, cfg)
4949
resolved := NewACPAgentFromConfig(acpCfg)
5050
// Keep a stable canonical name in runtime state.
5151
resolved.agentName = defaultACPName
@@ -101,14 +101,17 @@ func isAvailableWithConfig(name string, cfg *config.Config) bool {
101101
// at resolution time instead of package-init time.
102102
// It also applies command overrides for other agents (codex, claude, cursor, pi).
103103
//
104+
// The repoPath parameter is used to resolve repo-level ACP configuration,
105+
// which takes precedence over global ACP configuration.
106+
//
104107
// Optional backup agent names are tried after the preferred agent but
105108
// before the hardcoded fallback chain (see GetAvailable).
106-
func GetAvailableWithConfig(preferred string, cfg *config.Config, backups ...string) (Agent, error) {
109+
func GetAvailableWithConfig(repoPath string, preferred string, cfg *config.Config, backups ...string) (Agent, error) {
107110
rawPreferred := strings.TrimSpace(preferred)
108111
preferred = resolveAlias(rawPreferred)
109112

110-
if isConfiguredACPAgentName(rawPreferred, cfg) {
111-
acpAgent := configuredACPAgent(cfg)
113+
if isConfiguredACPAgentName(rawPreferred, cfg, repoPath) {
114+
acpAgent := configuredACPAgent(repoPath, cfg)
112115
if _, err := exec.LookPath(acpAgent.CommandName()); err == nil {
113116
return acpAgent, nil
114117
}
@@ -163,7 +166,7 @@ func GetAvailableWithConfig(preferred string, cfg *config.Config, backups ...str
163166
return nil, err
164167
}
165168
if resolved.Name() == defaultACPName {
166-
configured := configuredACPAgent(cfg)
169+
configured := configuredACPAgent(repoPath, cfg)
167170
if _, err := exec.LookPath(configured.CommandName()); err == nil {
168171
return configured, nil
169172
}
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
package agent
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
8+
"github.com/roborev-dev/roborev/internal/config"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
func TestIsConfiguredACPAgentName(t *testing.T) {
14+
t.Run("default ACP name", func(t *testing.T) {
15+
assert.True(t, isConfiguredACPAgentName(defaultACPName, nil, "/tmp/repo"))
16+
})
17+
18+
t.Run("matches configured name", func(t *testing.T) {
19+
cfg := &config.Config{
20+
ACP: &config.ACPAgentConfig{
21+
Name: "custom-acp",
22+
},
23+
}
24+
assert.True(t, isConfiguredACPAgentName("custom-acp", cfg, "/tmp/repo"))
25+
})
26+
27+
t.Run("does not match different name", func(t *testing.T) {
28+
cfg := &config.Config{
29+
ACP: &config.ACPAgentConfig{
30+
Name: "custom-acp",
31+
},
32+
}
33+
assert.False(t, isConfiguredACPAgentName("other-acp", cfg, "/tmp/repo"))
34+
})
35+
36+
t.Run("empty name returns false", func(t *testing.T) {
37+
cfg := &config.Config{
38+
ACP: &config.ACPAgentConfig{
39+
Name: "custom-acp",
40+
},
41+
}
42+
assert.False(t, isConfiguredACPAgentName("", cfg, "/tmp/repo"))
43+
})
44+
45+
t.Run("nil config returns false for non-default name", func(t *testing.T) {
46+
assert.False(t, isConfiguredACPAgentName("custom-acp", nil, "/tmp/repo"))
47+
})
48+
49+
t.Run("repo config takes precedence", func(t *testing.T) {
50+
// Create a temp directory with .roborev.toml
51+
testDir := t.TempDir()
52+
configPath := filepath.Join(testDir, ".roborev.toml")
53+
content := `[acp]
54+
name = "repo-acp"
55+
`
56+
err := os.WriteFile(configPath, []byte(content), 0644)
57+
require.NoError(t, err)
58+
59+
// With repo config, should match repo-acp
60+
assert.True(t, isConfiguredACPAgentName("repo-acp", &config.Config{}, testDir))
61+
62+
// Should not match different name
63+
assert.False(t, isConfiguredACPAgentName("other-acp", &config.Config{}, testDir))
64+
})
65+
66+
t.Run("whitespace trimming", func(t *testing.T) {
67+
cfg := &config.Config{
68+
ACP: &config.ACPAgentConfig{
69+
Name: " custom-acp ",
70+
},
71+
}
72+
// Should match with whitespace trimmed
73+
assert.True(t, isConfiguredACPAgentName("custom-acp", cfg, "/tmp/repo"))
74+
})
75+
76+
t.Run("configured name with whitespace", func(t *testing.T) {
77+
cfg := &config.Config{
78+
ACP: &config.ACPAgentConfig{
79+
Name: " custom-acp ",
80+
},
81+
}
82+
// Should match rawName with whitespace
83+
assert.True(t, isConfiguredACPAgentName(" custom-acp ", cfg, "/tmp/repo"))
84+
})
85+
}
86+
87+
func TestDefaultACPAgentConfig(t *testing.T) {
88+
cfg := defaultACPAgentConfig()
89+
assert.Equal(t, defaultACPName, cfg.Name)
90+
assert.Equal(t, defaultACPCommand, cfg.Command)
91+
assert.Equal(t, defaultACPReadOnlyMode, cfg.ReadOnlyMode)
92+
assert.Equal(t, defaultACPAutoApproveMode, cfg.AutoApproveMode)
93+
assert.Equal(t, defaultACPReadOnlyMode, cfg.Mode)
94+
assert.Equal(t, defaultACPTimeoutSeconds, cfg.Timeout)
95+
}
96+
97+
func TestConfiguredACPAgent(t *testing.T) {
98+
cfg := &config.Config{
99+
ACP: &config.ACPAgentConfig{
100+
Name: "custom-acp",
101+
Command: "custom-cmd",
102+
Model: "custom-model",
103+
},
104+
}
105+
106+
agent := configuredACPAgent("/tmp/repo", cfg)
107+
assert.Equal(t, defaultACPName, agent.agentName)
108+
assert.Equal(t, "custom-cmd", agent.Command)
109+
assert.Equal(t, "custom-model", agent.Model)
110+
}
111+
112+
func TestGetAvailableWithConfigACPAgent(t *testing.T) {
113+
t.Run("resolves configured ACP agent name", func(t *testing.T) {
114+
cfg := &config.Config{
115+
ACP: &config.ACPAgentConfig{
116+
Name: "my-acp",
117+
Command: "echo", // Use echo which is always available
118+
},
119+
}
120+
121+
// When the requested name matches the configured ACP name
122+
agent, err := GetAvailableWithConfig("", "my-acp", cfg)
123+
require.NoError(t, err)
124+
// The agent name should be the canonical ACP name
125+
assert.Equal(t, defaultACPName, agent.Name())
126+
})
127+
128+
t.Run("resolves default acp name with echo command", func(t *testing.T) {
129+
cfg := &config.Config{
130+
ACP: &config.ACPAgentConfig{
131+
Name: defaultACPName,
132+
Command: "echo",
133+
},
134+
}
135+
136+
agent, err := GetAvailableWithConfig("", defaultACPName, cfg)
137+
require.NoError(t, err)
138+
assert.Equal(t, defaultACPName, agent.Name())
139+
})
140+
}

0 commit comments

Comments
 (0)