diff --git a/CHANGELOG.md b/CHANGELOG.md index 082e0f8df..155fd1a64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **TUI keybinding corrections** (#9358): Fixed preset cycling keybinding from `ctrl+t` to `ctrl+tab` and added persona tab-cycling with `tab` key. Personas with `cycle_order > 0` are cycled in order. + - **Cross-actor subgraph cycle detection reads actor_ref field** (#1431): Fixed `_detect_subgraph_cycles()`, `_map_node()`, and the `compile_actor()` main loop in `src/cleveragents/actor/compiler.py` to read `actor_ref` from the top-level diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 1b5c41879..59305aeb5 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -4,6 +4,7 @@ * Brent E. Edwards * HAL 9000 * Hamza Khyari +* HAL9000 * Jeffrey Phillips Freeman * Luis Mendes * Rui Hu diff --git a/features/steps/tui_app_coverage_steps.py b/features/steps/tui_app_coverage_steps.py index 9bfae6063..19f2c0da1 100644 --- a/features/steps/tui_app_coverage_steps.py +++ b/features/steps/tui_app_coverage_steps.py @@ -23,6 +23,10 @@ from unittest.mock import MagicMock, patch from behave import given, then, when +from cleveragents.tui.persona.registry import PersonaRegistry +from cleveragents.tui.persona.schema import Persona +from cleveragents.tui.persona.state import PersonaState + # --------------------------------------------------------------------------- # Mock Textual infrastructure # --------------------------------------------------------------------------- @@ -162,9 +166,6 @@ def _restore_modules(context): def _make_persona_state(context): """Create a real PersonaState backed by a temp directory.""" - from cleveragents.tui.persona.registry import PersonaRegistry - from cleveragents.tui.persona.state import PersonaState - tmp = tempfile.mkdtemp() context._tui_tmpdir = tmp registry = PersonaRegistry(config_dir=Path(tmp)) @@ -520,3 +521,70 @@ def step_theme_class_var(context, theme: str) -> None: assert theme == context._tui_app.THEME, ( f"Expected THEME='{theme}', got '{context._tui_app.THEME}'" ) + + +# --- action_cycle_persona keybinding --- +@then('the app class should have a keybinding for "{key}" to "{action}"') +def step_check_keybinding(context, key, action): + """Verify that a specific keybinding exists.""" + app_class = context._tui_app_mod._TextualCleverAgentsTuiApp + bindings = app_class.BINDINGS + found = any(b[0] == key and b[1] == action for b in bindings) + assert found, f"Keybinding {key} -> {action} not found in {bindings}" + + +@given("a mock command router and persona state with cycleable personas") +def step_create_cycleable_personas(context): + """Create a persona state with multiple personas that have cycle_order > 0.""" + tmp = tempfile.mkdtemp() + context._tui_tmpdir = tmp + registry = PersonaRegistry(config_dir=Path(tmp)) + + # Create default persona + registry.ensure_default() + + # Create cycleable personas + persona1 = Persona( + name="persona1", + actor="local/test", + cycle_order=1, + ) + persona2 = Persona( + name="persona2", + actor="local/test", + cycle_order=2, + ) + registry.save(persona1) + registry.save(persona2) + + context._tui_cmd_router = _FakeCommandRouter() + context._tui_persona_state = PersonaState(registry=registry) + + +@when("I call action_cycle_persona on the app") +def step_call_action_cycle_persona(context): + """Call the action_cycle_persona method on the app.""" + # Capture the active persona name before cycling so the then-step can + # assert that it actually changed. + context._tui_persona_before_cycle = context._tui_persona_state.active_name( + context._tui_app._session.session_id + ) + context._tui_app.action_cycle_persona() + + +@then("the active persona should have changed") +def step_check_persona_changed(context): + """Verify that the active persona has changed after cycling.""" + session_id = context._tui_app._session.session_id + active_after = context._tui_persona_state.active_name(session_id) + active_before = getattr(context, "_tui_persona_before_cycle", None) + # The active persona must differ from the pre-cycle value. + assert active_after != active_before, ( + f"Expected persona to change but it stayed '{active_after}'" + ) + # The new active persona must be one of the cycleable personas. + personas = context._tui_persona_state.registry.list_personas() + cycleable = [p for p in personas if p.cycle_order > 0] + assert any(p.name == active_after for p in cycleable), ( + f"Active persona '{active_after}' is not cycleable; cycleable={[p.name for p in cycleable]}" + ) diff --git a/features/steps/tui_persona_state_coverage_steps.py b/features/steps/tui_persona_state_coverage_steps.py index c9153f84a..bc0fd4641 100644 --- a/features/steps/tui_persona_state_coverage_steps.py +++ b/features/steps/tui_persona_state_coverage_steps.py @@ -9,6 +9,7 @@ These steps target specific uncovered lines in tui/persona/state.py: - Lines 54-55: cycle_preset() when persona has no argument_presets - Lines 59-60: cycle_preset() when current preset not in names list - Lines 67-69: effective_arguments() delegation +- cycle_persona(): no cycleable personas, advance, and wrap-around """ from types import SimpleNamespace @@ -353,3 +354,54 @@ def step_verify_effective_arguments(context): assert context.result_eff_args["model"] == "gpt-4" assert context.result_eff_args["temp"] == 0.7 assert context.result_eff_args["speed"] == "fast" + + +# --------------------------------------------------------------------------- +# Scenario: cycle_persona (new method) +# --------------------------------------------------------------------------- + + +@when('I cycle the persona for session "{session_id}"') +def step_cycle_persona(context, session_id): + context.result_cycle_persona = context.state.cycle_persona(session_id) + + +@then("the cycle_persona result should be None") +def step_verify_cycle_persona_none(context): + assert context.result_cycle_persona is None + + +@then('the cycle_persona result should be "{expected}"') +def step_verify_cycle_persona_result(context, expected): + assert context.result_cycle_persona == expected, ( + f"Expected cycle_persona to return '{expected}', got '{context.result_cycle_persona}'" + ) + + +@given('cycleable personas "{names_csv}" are registered for session "{session_id}"') +def step_register_cycleable_personas(context, names_csv, session_id): + """Register multiple cycleable personas with ascending cycle_order values. + + A non-cycleable "default" persona is used as the registry default so that + when no active persona has been set for the session, ``_resolve_default_name`` + returns "default" (not in the cycleable list). This means the first call to + ``cycle_persona`` starts from the beginning of the cycleable list (index 0). + """ + names = [n.strip() for n in names_csv.split(",")] + registry = MagicMock() + personas = [ + Persona(name=name, actor="ns/actor", cycle_order=idx + 1) + for idx, name in enumerate(names) + ] + # Non-cycleable default persona — ensures _resolve_default_name() returns a + # name that is NOT in the cycleable list, so cycling starts from index 0. + default_persona = Persona(name="default", actor="ns/actor", cycle_order=0) + registry.list_personas.return_value = personas + registry.get.side_effect = lambda name: next( + (p for p in personas if p.name == name), None + ) + registry.ensure_default.return_value = default_persona + registry.get_last_persona.return_value = None + registry.set_last_persona = MagicMock() + context.state = PersonaState(registry=registry) + context.mock_registry = registry diff --git a/features/tui_app_coverage.feature b/features/tui_app_coverage.feature index a610e42ea..26347180f 100644 --- a/features/tui_app_coverage.feature +++ b/features/tui_app_coverage.feature @@ -47,7 +47,7 @@ Feature: TUI App Coverage Given a mock command router and persona state When I instantiate the Textual TUI app Then the app class should have CSS_PATH set to "cleveragents.tcss" - And the app class should have 3 key bindings + And the app class should have 4 key bindings # --- compose method (lines 102-112) --- @@ -111,6 +111,29 @@ Feature: TUI App Coverage And I call action_cycle_preset on the app Then the persona bar content should be refreshed + # --- action_cycle_preset method (lines 127-129) --- + + Scenario: action_cycle_preset uses ctrl+tab keybinding + Given a mock command router and persona state + When I instantiate the Textual TUI app + Then the app class should have a keybinding for "ctrl+tab" to "cycle_preset" + + # --- action_cycle_preset method (lines 127-129) --- + + # --- action_cycle_persona method (new) --- + + Scenario: action_cycle_persona uses tab keybinding + Given a mock command router and persona state + When I instantiate the Textual TUI app + Then the app class should have a keybinding for "tab" to "cycle_persona" + + Scenario: action_cycle_persona cycles through personas with cycle_order > 0 + Given a mock command router and persona state with cycleable personas + When I instantiate the Textual TUI app + And I call on_mount on the app + And I call action_cycle_persona on the app + Then the active persona should have changed + # --- _refresh_persona_bar method (lines 131-142) --- Scenario: _refresh_persona_bar builds scope text from active persona diff --git a/features/tui_persona_state_coverage.feature b/features/tui_persona_state_coverage.feature index 5141c737b..f04eb385d 100644 --- a/features/tui_persona_state_coverage.feature +++ b/features/tui_persona_state_coverage.feature @@ -66,3 +66,19 @@ Feature: TUI Persona State Coverage Given a persona with base arguments and presets is active for session "sess-11" When I request the effective arguments for session "sess-11" Then the effective arguments should merge base and preset overrides + + Scenario: cycle_persona returns None when no cycleable personas exist + When I cycle the persona for session "sess-12" + Then the cycle_persona result should be None + + Scenario: cycle_persona advances to the next cycleable persona + Given cycleable personas "alpha,beta,gamma" are registered for session "sess-13" + When I cycle the persona for session "sess-13" + Then the cycle_persona result should be "alpha" + And session "sess-13" should have active persona "alpha" + + Scenario: cycle_persona wraps around to the first persona + Given cycleable personas "alpha,beta,gamma" are registered for session "sess-14" + And session "sess-14" already has active persona "gamma" + When I cycle the persona for session "sess-14" + Then the cycle_persona result should be "alpha" diff --git a/src/cleveragents/tui/app.py b/src/cleveragents/tui/app.py index b1ff00785..91e557f68 100644 --- a/src/cleveragents/tui/app.py +++ b/src/cleveragents/tui/app.py @@ -93,7 +93,8 @@ if _TEXTUAL_AVAILABLE: BINDINGS: ClassVar[list[tuple[str, str, str]]] = [ ("ctrl+q", "quit", "Quit"), ("f1", "help", "Help"), - ("ctrl+t", "cycle_preset", "Cycle Preset"), + ("ctrl+tab", "cycle_preset", "Cycle Preset"), + ("tab", "cycle_persona", "Cycle Persona"), ] def __init__( @@ -153,6 +154,11 @@ if _TEXTUAL_AVAILABLE: self._persona_state.cycle_preset(self._session.session_id) self._refresh_persona_bar() + def action_cycle_persona(self) -> None: + """Cycle through personas with cycle_order > 0, sorted by cycle_order.""" + self._persona_state.cycle_persona(self._session.session_id) + self._refresh_persona_bar() + def _refresh_persona_bar(self) -> None: persona = self._persona_state.active_persona(self._session.session_id) preset = self._persona_state.current_preset(self._session.session_id) diff --git a/src/cleveragents/tui/persona/state.py b/src/cleveragents/tui/persona/state.py index c11fa3fcb..ff81eba77 100644 --- a/src/cleveragents/tui/persona/state.py +++ b/src/cleveragents/tui/persona/state.py @@ -63,6 +63,27 @@ class PersonaState: self.preset_by_session[session_id] = next_name return next_name + def cycle_persona(self, session_id: str) -> str | None: + """Cycle to the next persona with cycle_order > 0, sorted by cycle_order. + + Returns the name of the newly active persona, or None if no cycleable + personas exist. + """ + personas = self.registry.list_personas() + cycleable = sorted( + (p for p in personas if p.cycle_order > 0), + key=lambda p: p.cycle_order, + ) + if not cycleable: + return None + current_name = self.active_name(session_id) + current_idx = next( + (i for i, p in enumerate(cycleable) if p.name == current_name), -1 + ) + next_persona = cycleable[(current_idx + 1) % len(cycleable)] + self.set_active_persona(session_id, next_persona.name) + return next_persona.name + def effective_arguments(self, session_id: str) -> dict[str, object]: persona = self.active_persona(session_id) preset = self.current_preset(session_id)