fix(agentctl): treat model with no config options as valid empty resolution (#3003)
This commit is contained in:
@@ -6,6 +6,8 @@ import (
|
||||
"fmt"
|
||||
"time"
|
||||
|
||||
"go.uber.org/zap"
|
||||
|
||||
"github.com/kandev/kandev/internal/agent/agents"
|
||||
"github.com/kandev/kandev/internal/agent/settings/cliflags"
|
||||
agentctlutil "github.com/kandev/kandev/internal/agentctl/server/utility"
|
||||
@@ -199,6 +201,11 @@ func (m *Manager) resolveModelConfigFlight(
|
||||
if isAuthError(resp.Error) {
|
||||
resolution.Status = StatusAuthRequired
|
||||
}
|
||||
m.log.Warn("model config resolution failed",
|
||||
zap.String("agent_type", agentType),
|
||||
zap.String("model", req.Model),
|
||||
zap.String("status", string(resolution.Status)),
|
||||
zap.String("error", resp.Error))
|
||||
return resolution, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -796,6 +796,20 @@ func applyProbeModel(
|
||||
return nil, err
|
||||
}
|
||||
if !received {
|
||||
if method == sessionmodel.MethodSetModel {
|
||||
// The legacy session/set_model RPC applied the model, but the agent
|
||||
// surfaces no per-model config options and pushes no follow-up
|
||||
// config-update notification (e.g. auggie, which advertises a flat
|
||||
// model list and answers session/set_model with an empty result).
|
||||
// That is a valid empty resolution, not a failure: the caller keeps
|
||||
// the session-advertised options.
|
||||
return nil, nil
|
||||
}
|
||||
// A typed session/set_config_option that returns neither inline options
|
||||
// nor a config-update notification leaves us without an authoritative
|
||||
// snapshot for the newly selected model. Keeping the pre-switch
|
||||
// session/new snapshot would report the previous model's options as the
|
||||
// current configuration, so treat this as a failure.
|
||||
return nil, fmt.Errorf("ACP model selection returned no configuration options")
|
||||
}
|
||||
return updated, nil
|
||||
@@ -904,7 +918,9 @@ func (e *ACPInferenceExecutor) probeACPSessionWithContext(
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
sessionResp.ConfigOptions = updated
|
||||
if updated != nil {
|
||||
sessionResp.ConfigOptions = updated
|
||||
}
|
||||
}
|
||||
if updated, err := applyProbeConfigOptions(
|
||||
ctx,
|
||||
|
||||
@@ -174,7 +174,15 @@ func TestProbeACPSessionWithContextRejectsProviderWithoutModelSelection(t *testi
|
||||
}
|
||||
}
|
||||
|
||||
func TestProbeACPSessionWithContextTimesOutWithoutConfigSnapshot(t *testing.T) {
|
||||
// TestProbeACPSessionWithContextErrorsWhenConfigOptionYieldsNoSnapshot pins that
|
||||
// when an agent advertises a typed model config option (so the probe applies the
|
||||
// model via session/set_config_option) but returns neither inline options nor a
|
||||
// follow-up config-update notification, the probe fails. Keeping the pre-switch
|
||||
// session/new snapshot would report the previous model's options as the resolved
|
||||
// configuration for the newly selected model, so this must stay an error. The
|
||||
// empty-resolution relaxation is scoped to the legacy session/set_model path
|
||||
// (auggie); see TestApplyProbeModel_LegacyNoConfigOptionsReturnsEmpty.
|
||||
func TestProbeACPSessionWithContextErrorsWhenConfigOptionYieldsNoSnapshot(t *testing.T) {
|
||||
c2aR, c2aW := io.Pipe()
|
||||
a2cR, a2cW := io.Pipe()
|
||||
t.Cleanup(func() {
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"reflect"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
acp "github.com/coder/acp-go-sdk"
|
||||
)
|
||||
@@ -169,6 +170,67 @@ func TestApplySessionModel_NoOpWhenAgentSupportsNeitherSurface(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplyProbeModel_LegacyNoConfigOptionsReturnsEmpty pins that when the
|
||||
// legacy session/set_model RPC succeeds but the agent surfaces no per-model
|
||||
// config options (and pushes no follow-up config-update notification), the
|
||||
// probe treats it as a valid empty resolution (nil options, nil error) rather
|
||||
// than a hard failure. This is the auggie shape: a flat model list with no
|
||||
// reasoning-effort style selectors to resolve.
|
||||
func TestApplyProbeModel_LegacyNoConfigOptionsReturnsEmpty(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
conn := &fakeModelConn{}
|
||||
state := newACPProbeNotificationState("auggie")
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||
defer cancel()
|
||||
|
||||
options, err := applyProbeModel(ctx, conn, acp.SessionId("sess-1"), "legacy-model", nil, state)
|
||||
if err != nil {
|
||||
t.Fatalf("applyProbeModel() error = %v, want nil", err)
|
||||
}
|
||||
if options != nil {
|
||||
t.Fatalf("options = %#v, want nil (empty resolution)", options)
|
||||
}
|
||||
wantCalls := []string{"legacy:sess-1:legacy-model"}
|
||||
if !reflect.DeepEqual(conn.calls, wantCalls) {
|
||||
t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplyProbeModel_ConfigOptionNoSnapshotReturnsError pins the other half of
|
||||
// the narrowing: when the agent advertises a typed model config option (so the
|
||||
// model is applied via session/set_config_option) but returns neither inline
|
||||
// options nor a config-update notification, applyProbeModel must fail rather than
|
||||
// keep the stale pre-switch snapshot. The empty-resolution relaxation is scoped
|
||||
// to the legacy session/set_model path only.
|
||||
func TestApplyProbeModel_ConfigOptionNoSnapshotReturnsError(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
modelCat := acp.SessionConfigOptionCategoryModel
|
||||
conn := &fakeModelConn{}
|
||||
state := newACPProbeNotificationState("modern-agent")
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||
defer cancel()
|
||||
|
||||
options, err := applyProbeModel(ctx, conn, acp.SessionId("sess-1"), "model-with-effort", []acp.SessionConfigOption{
|
||||
{Select: &acp.SessionConfigOptionSelect{
|
||||
Id: "model",
|
||||
Category: &modelCat,
|
||||
Type: "select",
|
||||
}},
|
||||
}, state)
|
||||
if err == nil {
|
||||
t.Fatalf("applyProbeModel() error = nil, want no-configuration-options error")
|
||||
}
|
||||
if options != nil {
|
||||
t.Fatalf("options = %#v, want nil on error", options)
|
||||
}
|
||||
wantCalls := []string{"config:model:model-with-effort"}
|
||||
if !reflect.DeepEqual(conn.calls, wantCalls) {
|
||||
t.Fatalf("calls = %#v, want %#v", conn.calls, wantCalls)
|
||||
}
|
||||
}
|
||||
|
||||
// fakeModelConnLegacyErr returns a configurable error from the legacy
|
||||
// session/set_model RPC. Used to exercise the -32601 no-op path.
|
||||
type fakeModelConnLegacyErr struct {
|
||||
|
||||
Reference in New Issue
Block a user