diff --git a/agent/harness/agentmode/agentmode.go b/agent/harness/agentmode/agentmode.go index c1077797..f2749026 100644 --- a/agent/harness/agentmode/agentmode.go +++ b/agent/harness/agentmode/agentmode.go @@ -57,7 +57,8 @@ type state struct { // Config configures the agent mode provider. type Config struct { - // Modes is the set of available modes. If empty, defaults to "plan" and "execute". + // Modes is the set of available modes. If nil, defaults to "plan" and "execute". + // An empty, non-nil slice is invalid. Modes []Mode // DefaultMode is the initial mode. Must be one of the configured Modes. @@ -117,9 +118,9 @@ If 2. Work autonomously using your best judgment — do not ask the user questio // Panics if the configuration contains duplicate names, an empty mode name or // instructions, or a default mode that is not in the configured set. func New(cfg Config) *Provider { - modes := defaultModes - if len(cfg.Modes) > 0 { - modes = cfg.Modes + modes := cfg.Modes + if modes == nil { + modes = defaultModes } if len(modes) == 0 { panic("agentmode: at least one mode must be configured") @@ -158,7 +159,7 @@ func New(cfg Config) *Provider { modes: modes, defaultMode: defaultMode, instructions: instructions, - usesDefaultModes: len(cfg.Modes) == 0, + usesDefaultModes: cfg.Modes == nil, usesDefaultInstr: cfg.Instructions == nil, disableModeSet: cfg.DisableModeSetTool, disableModeGet: cfg.DisableModeGetTool, @@ -207,10 +208,10 @@ type Provider struct { // keeping the registry from growing unbounded. func (p *Provider) getSessionLock(opts []agent.Option) *sync.Mutex { session, _ := agent.GetOption(opts, agent.WithSession) - return p.getSessionLockForSession(session) + return p.sessionLock(session) } -func (p *Provider) getSessionLockForSession(session *agent.Session) *sync.Mutex { +func (p *Provider) sessionLock(session *agent.Session) *sync.Mutex { if session == nil { return &p.nullSessionLock } @@ -230,7 +231,7 @@ func (p *Provider) getSessionLockForSession(session *agent.Session) *sync.Mutex return actual.(*sync.Mutex) } -func (p *Provider) loadStateForSession(session *agent.Session) *state { +func (p *Provider) loadSessionState(session *agent.Session) *state { if session == nil { return &state{CurrentMode: p.defaultMode} } @@ -241,7 +242,7 @@ func (p *Provider) loadStateForSession(session *agent.Session) *state { return &state{CurrentMode: p.defaultMode} } -func (p *Provider) saveStateForSession(session *agent.Session, s *state) { +func (p *Provider) saveSessionState(session *agent.Session, s *state) { if session == nil || s == nil { return } @@ -258,12 +259,12 @@ func (p *Provider) Invoked(ctx context.Context, invoked agent.InvokedContext) er func (p *Provider) loadState(opts []agent.Option) *state { session, _ := agent.GetOption(opts, agent.WithSession) - return p.loadStateForSession(session) + return p.loadSessionState(session) } func (p *Provider) saveState(opts []agent.Option, s *state) { session, _ := agent.GetOption(opts, agent.WithSession) - p.saveStateForSession(session, s) + p.saveSessionState(session, s) } func (p *Provider) provide(ctx context.Context, invoking agent.InvokingContext) ([]*message.Message, []agent.Option, error) { @@ -274,10 +275,10 @@ func (p *Provider) provide(ctx context.Context, invoking agent.InvokingContext) mu := p.getSessionLock(opts) mu.Lock() st := p.loadState(opts) - // Persist the initial state so SetModeForSession can read it. + // Persist the initial state so SetMode can read it. p.saveState(opts, st) - // If the mode was changed externally (e.g. via SetModeForSession), inject a notification + // If the mode was changed externally (e.g. via SetMode), inject a notification // so the agent clearly sees the change in conversation context. if st.PreviousMode != "" { outMessages = append(outMessages, message.NewText(fmt.Sprintf( @@ -340,23 +341,26 @@ func (p *Provider) createTools(opts []agent.Option) []tool.FuncTool { tools := make([]tool.FuncTool, 0, 2) if !p.disableModeSet { + type setModeInput struct { + Mode string `json:"mode" jsonschema:"The operating mode to switch to"` + } setTool := functool.MustNew( functool.Config{ Name: "mode_set", Description: fmt.Sprintf("Switch the agent's operating mode. Supported modes: \"%s\".", p.modeNamesDisplay), }, - func(ctx context.Context, mode string) (string, error) { - if _, ok := p.validModes[mode]; !ok { - return "", fmt.Errorf("invalid mode: %q. Supported modes: \"%s\"", mode, p.modeNamesDisplay) + func(ctx context.Context, input setModeInput) (string, error) { + if _, ok := p.validModes[input.Mode]; !ok { + return "", fmt.Errorf("invalid mode: %q. Supported modes: \"%s\"", input.Mode, p.modeNamesDisplay) } mu := p.getSessionLock(opts) mu.Lock() defer mu.Unlock() st := p.loadState(opts) st.PreviousMode = "" - st.CurrentMode = mode + st.CurrentMode = input.Mode p.saveState(opts, st) - return fmt.Sprintf("Mode changed to %q.", mode), nil + return fmt.Sprintf("Mode changed to %q.", input.Mode), nil }, ) tools = append(tools, setTool) @@ -382,40 +386,29 @@ func (p *Provider) createTools(opts []agent.Option) []tool.FuncTool { return tools } -// ModeForSession returns the current operating mode from session state. +// Mode returns the current operating mode from session state. // If no state has been persisted yet, it returns the configured default mode. -func (p *Provider) ModeForSession(session *agent.Session) string { - mu := p.getSessionLockForSession(session) +func (p *Provider) Mode(session *agent.Session) string { + mu := p.sessionLock(session) mu.Lock() defer mu.Unlock() - return p.loadStateForSession(session).CurrentMode -} - -// SetModeForSession sets the operating mode in session state, validating it -// against the provider's configured modes. Returns an error if the mode is -// invalid or no session is available. -func (p *Provider) SetModeForSession(session *agent.Session, mode string) error { - return p.setModeForSessionWithNotificationOption(session, mode, false) -} - -// SetModeForSessionSilently sets the operating mode in session state without -// queuing the next-invocation mode-change notification. It also clears any -// pending mode-change notification for the session. -func (p *Provider) SetModeForSessionSilently(session *agent.Session, mode string) error { - return p.setModeForSessionWithNotificationOption(session, mode, true) + return p.loadSessionState(session).CurrentMode } -func (p *Provider) setModeForSessionWithNotificationOption(session *agent.Session, mode string, disableNotification bool) error { +// SetMode sets the operating mode in session state. It returns an error if the +// mode is invalid or no session is available. Set disableNotification to true +// to suppress the next mode-change notification and clear any pending one. +func (p *Provider) SetMode(session *agent.Session, mode string, disableNotification bool) error { if _, ok := p.validModes[mode]; !ok { return fmt.Errorf("agentmode: invalid mode %q", mode) } - mu := p.getSessionLockForSession(session) + mu := p.sessionLock(session) mu.Lock() defer mu.Unlock() if session == nil { return fmt.Errorf("agentmode: no session available") } - s := p.loadStateForSession(session) + s := p.loadSessionState(session) if disableNotification { s.PreviousMode = "" } @@ -426,6 +419,6 @@ func (p *Provider) setModeForSessionWithNotificationOption(session *agent.Sessio } s.CurrentMode = mode } - p.saveStateForSession(session, s) + p.saveSessionState(session, s) return nil } diff --git a/agent/harness/agentmode/agentmode_test.go b/agent/harness/agentmode/agentmode_test.go index 35a3ae66..ae997e29 100644 --- a/agent/harness/agentmode/agentmode_test.go +++ b/agent/harness/agentmode/agentmode_test.go @@ -4,6 +4,7 @@ package agentmode_test import ( "context" + "encoding/json" "slices" "strings" "sync" @@ -104,7 +105,7 @@ func TestConcurrentToolInvocations_NoDataRace(t *testing.T) { } go func(idx int, mode string) { defer wg.Done() - _, errs[idx] = setTool.Call(context.Background(), `{"Arg0":"`+mode+`"}`) + _, errs[idx] = setTool.Call(context.Background(), `{"mode":"`+mode+`"}`) }(i*2, mode) go func(idx int) { defer wg.Done() @@ -166,7 +167,7 @@ func TestCustomModes_AreUsed(t *testing.T) { }) opts := sessionOpts() - mode := p.ModeForSession(mustSession(t, opts)) + mode := p.Mode(mustSession(t, opts)) if mode != "draft" { t.Errorf("expected default mode 'draft', got %q", mode) } @@ -182,14 +183,14 @@ func TestCustomModes_SetModeValidatesAgainstList(t *testing.T) { }) opts := sessionOpts() - if err := p.SetModeForSession(mustSession(t, opts), "review"); err != nil { + if err := p.SetMode(mustSession(t, opts), "review", false); err != nil { t.Fatalf("expected valid mode 'review' to succeed: %v", err) } - if mode := p.ModeForSession(mustSession(t, opts)); mode != "review" { + if mode := p.Mode(mustSession(t, opts)); mode != "review" { t.Errorf("expected 'review', got %q", mode) } - if err := p.SetModeForSession(mustSession(t, opts), "invalid"); err == nil { + if err := p.SetMode(mustSession(t, opts), "invalid", false); err == nil { t.Fatal("expected error for invalid mode") } } @@ -205,7 +206,7 @@ func TestCustomDefaultMode_IsUsed(t *testing.T) { }) opts := sessionOpts() - mode := p.ModeForSession(mustSession(t, opts)) + mode := p.Mode(mustSession(t, opts)) if mode != "review" { t.Errorf("expected default mode 'review', got %q", mode) } @@ -226,17 +227,14 @@ func TestInvalidDefaultMode_Panics(t *testing.T) { }) } -// 7. Options_EmptyModes_UsesDefaults -// In Go, an empty Modes slice is treated as "use defaults" (plan/execute). -func TestEmptyModes_UsesDefaults(t *testing.T) { - p := agentmode.New(agentmode.Config{ - Modes: []agentmode.Mode{}, - }) - opts := sessionOpts() - mode := p.ModeForSession(mustSession(t, opts)) - if mode != "plan" { - t.Errorf("expected default mode 'plan' for empty modes, got %q", mode) - } +// 7. Options_EmptyModes_Panics +func TestEmptyModes_Panics(t *testing.T) { + defer func() { + if recover() == nil { + t.Fatal("expected panic for empty modes") + } + }() + agentmode.New(agentmode.Config{Modes: []agentmode.Mode{}}) } // 8. Options_CustomModes_AppearInInstructions @@ -326,7 +324,7 @@ func TestExternalModeChange_InjectsNotification(t *testing.T) { } // Change mode externally. - if err := p.SetModeForSession(session, "execute"); err != nil { + if err := p.SetMode(session, "execute", false); err != nil { t.Fatal(err) } @@ -359,7 +357,7 @@ func TestExternalModeChange_NotificationClearedAfterFirstRead(t *testing.T) { msgs := newMessages("hi") _, _, _ = invokeProvider(p, context.Background(), msgs, opts...) - _ = p.SetModeForSession(session, "execute") + _ = p.SetMode(session, "execute", false) // First read: should have notification. outMessages, _, _ := invokeProvider(p, context.Background(), msgs, opts...) @@ -396,7 +394,7 @@ func TestExternalModeChange_SameMode_NoNotification(t *testing.T) { _, _, _ = invokeProvider(p, context.Background(), msgs, opts...) // Set to same mode. - _ = p.SetModeForSession(session, "plan") + _ = p.SetMode(session, "plan", false) outMessages, _, _ := invokeProvider(p, context.Background(), msgs, opts...) for _, msg := range outMessages { @@ -406,28 +404,28 @@ func TestExternalModeChange_SameMode_NoNotification(t *testing.T) { } } -// 14. SetModeForSession_ChangesMode -func TestSetModeForSession_ChangesMode(t *testing.T) { +// 14. SetMode_ChangesMode +func TestSetMode_ChangesMode(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() _, _, _ = invokeProvider(p, context.Background(), newMessages("hi"), opts...) - if err := p.SetModeForSession(mustSession(t, opts), "execute"); err != nil { + if err := p.SetMode(mustSession(t, opts), "execute", false); err != nil { t.Fatal(err) } - if mode := p.ModeForSession(mustSession(t, opts)); mode != "execute" { + if mode := p.Mode(mustSession(t, opts)); mode != "execute" { t.Errorf("expected 'execute', got %q", mode) } } -// 15. SetModeForSession_ReturnsConfirmation — verified via instructions reflecting the new mode -func TestSetModeForSession_ReflectedInInstructions(t *testing.T) { +// 15. SetMode_ReturnsConfirmation — verified via instructions reflecting the new mode +func TestSetMode_ReflectedInInstructions(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() _, _, _ = invokeProvider(p, context.Background(), newMessages("hi"), opts...) - _ = p.SetModeForSession(mustSession(t, opts), "execute") + _ = p.SetMode(mustSession(t, opts), "execute", false) _, outOpts, err := invokeProvider(p, context.Background(), newMessages("hi"), opts...) if err != nil { @@ -440,12 +438,12 @@ func TestSetModeForSession_ReflectedInInstructions(t *testing.T) { } } -// 16. SetModeForSession_InvalidMode_Throws -func TestSetModeForSession_InvalidMode_ReturnsError(t *testing.T) { +// 16. SetMode_InvalidMode_Throws +func TestSetMode_InvalidMode_ReturnsError(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() - err := p.SetModeForSession(mustSession(t, opts), "nonexistent") + err := p.SetMode(mustSession(t, opts), "nonexistent", false) if err == nil { t.Fatal("expected error for invalid mode") } @@ -454,79 +452,67 @@ func TestSetModeForSession_InvalidMode_ReturnsError(t *testing.T) { } } -// 17. ModeForSession_ReturnsDefaultMode -func TestModeForSession_ReturnsDefaultMode(t *testing.T) { +// 17. Mode_ReturnsDefaultMode +func TestMode_ReturnsDefaultMode(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() - mode := p.ModeForSession(mustSession(t, opts)) + mode := p.Mode(mustSession(t, opts)) if mode != "plan" { t.Errorf("expected 'plan', got %q", mode) } } -// 18. ModeForSession_ReturnsUpdatedModeAfterSet -func TestModeForSession_ReturnsUpdatedModeAfterSet(t *testing.T) { +// 18. Mode_ReturnsUpdatedModeAfterSet +func TestMode_ReturnsUpdatedModeAfterSet(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() - _ = p.SetModeForSession(mustSession(t, opts), "execute") - mode := p.ModeForSession(mustSession(t, opts)) + _ = p.SetMode(mustSession(t, opts), "execute", false) + mode := p.Mode(mustSession(t, opts)) if mode != "execute" { t.Errorf("expected 'execute', got %q", mode) } } -// 19. ModeForSession_WithSessionOption_ReturnsDefaultMode -func TestModeForSession_WithSessionOption_ReturnsDefaultMode(t *testing.T) { +// 19. Mode_WithSessionOption_ReturnsDefaultMode +func TestMode_WithSessionOption_ReturnsDefaultMode(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() - if mode := p.ModeForSession(mustSession(t, opts)); mode != "plan" { + if mode := p.Mode(mustSession(t, opts)); mode != "plan" { t.Errorf("expected 'plan', got %q", mode) } } -// 20. PublicModeForSession_ReturnsDefaultMode -func TestPublicModeForSession_ReturnsDefaultMode(t *testing.T) { +// 20. PublicMode_ReturnsDefaultMode +func TestPublicMode_ReturnsDefaultMode(t *testing.T) { p := agentmode.New(agentmode.Config{}) session := agenttest.CreateSession() - if mode := p.ModeForSession(session); mode != "plan" { + if mode := p.Mode(session); mode != "plan" { t.Errorf("expected 'plan', got %q", mode) } } -// 21. PublicSetModeForSession_ChangesMode -func TestPublicSetModeForSession_ChangesMode(t *testing.T) { +// 21. PublicSetMode_ChangesMode +func TestPublicSetMode_ChangesMode(t *testing.T) { p := agentmode.New(agentmode.Config{}) session := agenttest.CreateSession() - if err := p.SetModeForSession(session, "execute"); err != nil { + if err := p.SetMode(session, "execute", false); err != nil { t.Fatal(err) } - if mode := p.ModeForSession(session); mode != "execute" { + if mode := p.Mode(session); mode != "execute" { t.Errorf("expected 'execute', got %q", mode) } } -// 22. PublicSetModeForSession_NoSession_ReturnsError -func TestPublicSetModeForSession_NoSession_ReturnsError(t *testing.T) { - p := agentmode.New(agentmode.Config{}) - - err := p.SetModeForSession(nil, "execute") - if err == nil { - t.Fatal("expected error") - } - if !strings.Contains(err.Error(), "no session available") { - t.Fatalf("expected no-session error, got %v", err) - } -} - -func TestPublicSetModeForSessionSilently_NoSession_ReturnsError(t *testing.T) { +// 22. PublicSetMode_NoSession_ReturnsError +func TestPublicSetMode_NoSession_ReturnsError(t *testing.T) { p := agentmode.New(agentmode.Config{}) - err := p.SetModeForSessionSilently(nil, "execute") + err := p.SetMode(nil, "execute", false) if err == nil { t.Fatal("expected error") } @@ -535,12 +521,12 @@ func TestPublicSetModeForSessionSilently_NoSession_ReturnsError(t *testing.T) { } } -// 23. PublicSetModeForSession_ReflectedInToolResults -func TestPublicSetModeForSession_ReflectedInInstructions(t *testing.T) { +// 23. PublicSetMode_ReflectedInToolResults +func TestPublicSetMode_ReflectedInInstructions(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() - _ = p.SetModeForSession(mustSession(t, opts), "execute") + _ = p.SetMode(mustSession(t, opts), "execute", false) _, outOpts, err := invokeProvider(p, context.Background(), newMessages("hi"), opts...) if err != nil { @@ -559,7 +545,7 @@ func TestState_PersistsAcrossInvocations(t *testing.T) { opts := sessionOpts() _, _, _ = invokeProvider(p, context.Background(), newMessages("hi"), opts...) - _ = p.SetModeForSession(mustSession(t, opts), "execute") + _ = p.SetMode(mustSession(t, opts), "execute", false) // Second invocation — mode should persist. _, outOpts, err := invokeProvider(p, context.Background(), newMessages("hi"), opts...) @@ -572,12 +558,12 @@ func TestState_PersistsAcrossInvocations(t *testing.T) { t.Error("expected mode 'execute' to persist across invocations") } - if mode := p.ModeForSession(mustSession(t, opts)); mode != "execute" { + if mode := p.Mode(mustSession(t, opts)); mode != "execute" { t.Errorf("expected 'execute', got %q", mode) } } -func TestSetModeForSessionSilently_ChangesModeWithoutNotification(t *testing.T) { +func TestSetMode_DisableNotificationChangesModeWithoutNotification(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() session := mustSession(t, opts) @@ -587,10 +573,10 @@ func TestSetModeForSessionSilently_ChangesModeWithoutNotification(t *testing.T) t.Fatal(err) } - if err := p.SetModeForSessionSilently(session, "execute"); err != nil { + if err := p.SetMode(session, "execute", true); err != nil { t.Fatal(err) } - if mode := p.ModeForSession(session); mode != "execute" { + if mode := p.Mode(session); mode != "execute" { t.Fatalf("expected mode to be updated to execute, got %q", mode) } @@ -605,7 +591,30 @@ func TestSetModeForSessionSilently_ChangesModeWithoutNotification(t *testing.T) } } -func TestSetModeForSessionSilently_ClearsPendingNotification(t *testing.T) { +func TestSetMode_DisableNotification(t *testing.T) { + p := agentmode.New(agentmode.Config{}) + opts := sessionOpts() + session := mustSession(t, opts) + + if err := p.SetMode(session, "execute", true); err != nil { + t.Fatal(err) + } + if mode := p.Mode(session); mode != "execute" { + t.Fatalf("expected mode to be updated to execute, got %q", mode) + } + + outMessages, _, err := invokeProvider(p, context.Background(), newMessages("hi"), opts...) + if err != nil { + t.Fatal(err) + } + for _, msg := range outMessages { + if strings.Contains(msg.Contents.Text(), "Mode changed") { + t.Fatal("expected no mode-change notification") + } + } +} + +func TestSetMode_DisableNotificationClearsPendingNotification(t *testing.T) { p := agentmode.New(agentmode.Config{}) opts := sessionOpts() session := mustSession(t, opts) @@ -615,10 +624,10 @@ func TestSetModeForSessionSilently_ClearsPendingNotification(t *testing.T) { t.Fatal(err) } - if err := p.SetModeForSession(session, "execute"); err != nil { + if err := p.SetMode(session, "execute", false); err != nil { t.Fatal(err) } - if err := p.SetModeForSessionSilently(session, "plan"); err != nil { + if err := p.SetMode(session, "plan", true); err != nil { t.Fatal(err) } @@ -701,6 +710,44 @@ func TestToolNames(t *testing.T) { } } +func TestModeSetTool_UsesNamedModeArgument(t *testing.T) { + p := agentmode.New(agentmode.Config{}) + opts := sessionOpts() + + _, outOpts, err := invokeProvider(p, context.Background(), newMessages("hi"), opts...) + if err != nil { + t.Fatal(err) + } + + for _, candidate := range collectTools(outOpts) { + if candidate.Name() != "mode_set" { + continue + } + modeSet, ok := candidate.(tool.FuncTool) + if !ok { + t.Fatalf("mode_set has type %T, want tool.FuncTool", candidate) + } + schema, err := json.Marshal(modeSet.Schema()) + if err != nil { + t.Fatalf("marshal mode_set schema: %v", err) + } + if !strings.Contains(string(schema), `"mode"`) { + t.Errorf("mode_set schema does not contain a named mode argument: %s", schema) + } + if strings.Contains(string(schema), `"Arg0"`) { + t.Errorf("mode_set schema contains the generic Arg0 argument: %s", schema) + } + if _, err := modeSet.Call(context.Background(), `{"mode":"execute"}`); err != nil { + t.Fatalf("mode_set rejected its named mode argument: %v", err) + } + if mode := p.Mode(mustSession(t, opts)); mode != "execute" { + t.Errorf("expected execute mode, got %q", mode) + } + return + } + t.Fatal("mode_set tool not found") +} + func TestToolNames_DisableModeSetToolOmitsOnlyModeSet(t *testing.T) { p := agentmode.New(agentmode.Config{DisableModeSetTool: true}) opts := sessionOpts()