diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index d341bd4a0..54af118a8 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -23,7 +23,7 @@ Below are some of the specific details of various contributions. * Jeffrey Phillips Freeman has acted as Lead Developer, daily contributor, and Project Owner. * Jeffrey Phillips Freeman has contributed the invariant merge precedence fix (#9126): restored the missing ACTION scope in ``merge_invariants()`` and ``InvariantSet.merge()``, corrected all module docstrings from ``plan > project > global`` to the spec-compliant ``plan > action > project > global``, and added comprehensive BDD test coverage for four-tier merge precedence. * Brent E. Edwards has contributed quality assurance, test coverage, and CI pipeline improvements. -* HAL 9000 has contributed automated implementation, bug fixes, and feature development as part of the CleverAgents automation pool. +* HAL 9000 has contributed automated implementation, bug fixes, and feature development as part of the CleverAgents automation pool, including fix for PermissionsScreen base class (#10744 / #10488): converted PermissionsScreen from a Static widget to a proper Textual Screen subclass with full keyboard bindings and action methods. * HAL 9000 has contributed concurrency safety improvements, including thread-safe context tier management (issue #7547) for parallel plan execution. * HAL 9000 has contributed the configurable agent limits refactor (#9246/#9050): replaced hardcoded ``deps[:10]`` in ``ContextAnalysisAgent`` and ``contexts[:5]`` in ``PlanGenerationGraph`` with validated constructor parameters ``max_dependencies`` (default: 10) and ``max_context_files`` (default: 5), including 12 BDD scenarios covering defaults, custom values, edge cases, and invalid-input error handling. * HAL 9000 has contributed the plan concurrency race-condition fix (#7989): wired `LockService` into the plan lifecycle, guarding `execute_plan()` and `apply_plan()` with plan-level advisory locks and unique per-invocation owner identities to prevent silent concurrent state corruption. diff --git a/features/execution_environment.feature b/features/execution_environment.feature index 49907d400..f17af0f65 100644 --- a/features/execution_environment.feature +++ b/features/execution_environment.feature @@ -129,10 +129,10 @@ Feature: Execution environment routing Scenario: CONTAINER_RESOURCE_TYPES includes expected types Given I import CONTAINER_RESOURCE_TYPES - Then it should contain "container-instance" - And it should contain "devcontainer-instance" - And it should not contain "devcontainer-file" - And it should not contain "git-checkout" + Then the container types should contain "container-instance" + And the container types should contain "devcontainer-instance" + And the container types should not contain "devcontainer-file" + And the container types should not contain "git-checkout" # ── ContainerUnavailableError ─────────────────────────────────────── @@ -145,4 +145,3 @@ Feature: Execution environment routing When I create a ContainerUnavailableError without project name Then exec-env the error message should contain "Container resource unavailable" And exec-env the error message should not contain "for project" - diff --git a/features/steps/execution_environment_steps.py b/features/steps/execution_environment_steps.py index 1379ffb40..54349e1c0 100644 --- a/features/steps/execution_environment_steps.py +++ b/features/steps/execution_environment_steps.py @@ -342,12 +342,12 @@ def step_import_types(context: Context) -> None: context.container_types = CONTAINER_RESOURCE_TYPES -@then('it should contain "{value}"') +@then('the container types should contain "{value}"') def step_types_contain(context: Context, value: str) -> None: assert value in context.container_types -@then('it should not contain "{value}"') +@then('the container types should not contain "{value}"') def step_types_not_contain(context: Context, value: str) -> None: assert value not in context.container_types diff --git a/features/steps/tui_permissions_screen_steps.py b/features/steps/tui_permissions_screen_steps.py index 7dc7e5a39..7ca01111e 100644 --- a/features/steps/tui_permissions_screen_steps.py +++ b/features/steps/tui_permissions_screen_steps.py @@ -616,3 +616,69 @@ def step_left_side_empty_for_inserted(context): @then("the clear result should be True") def step_clear_result_true(context): assert context._clear_result is True + + +# --------------------------------------------------------------------------- +# Bug #10488: PermissionsScreen base class checks +# --------------------------------------------------------------------------- + + +@when("I check the base class of PermissionsScreen") +def step_check_base_class(context): + from cleveragents.tui.permissions.screen import PermissionsScreen + + context._permissions_screen_cls = PermissionsScreen + + +@then("PermissionsScreen should be a subclass of textual.app.Screen") +def step_permissions_screen_is_screen_subclass(context): + import importlib + + from cleveragents.tui.permissions import screen as screen_module + + # Check the _ScreenBase variable in the module — if textual is available, + # it should be textual.app.Screen; if not, the fallback is used. + screen_base = getattr(screen_module, "_ScreenBase", None) + assert screen_base is not None, ( + "Expected screen module to have _ScreenBase variable" + ) + + try: + Screen = importlib.import_module("textual.app").Screen + # Textual is available — verify PermissionsScreen inherits from Screen + assert issubclass(context._permissions_screen_cls, Screen), ( + f"Expected PermissionsScreen to be a subclass of textual.app.Screen, " + f"but its MRO is: {[c.__name__ for c in context._permissions_screen_cls.__mro__]}" + ) + assert screen_base is Screen, ( + f"Expected _ScreenBase to be textual.app.Screen, got {screen_base!r}" + ) + except ImportError: + # Textual not installed — verify the module is designed to use Screen + # by checking that _load_screen_base is defined (not _load_static_base). + load_fn = getattr(screen_module, "_load_screen_base", None) + assert load_fn is not None, ( + "Expected screen module to have _load_screen_base function " + "(not _load_static_base). The module must be designed to load " + "textual.app.Screen as the base class." + ) + + +@then("PermissionsScreen should have a BINDINGS class variable") +def step_permissions_screen_has_bindings(context): + cls = context._permissions_screen_cls + assert hasattr(cls, "BINDINGS"), ( + "Expected PermissionsScreen to have a BINDINGS class variable" + ) + assert cls.BINDINGS, "Expected PermissionsScreen.BINDINGS to be non-empty" + + +@then('PermissionsScreen should have action method "{method_name}"') +def step_permissions_screen_has_action_method(context, method_name): + cls = context._permissions_screen_cls + assert hasattr(cls, method_name), ( + f"Expected PermissionsScreen to have action method '{method_name}'" + ) + assert callable(getattr(cls, method_name)), ( + f"Expected PermissionsScreen.{method_name} to be callable" + ) diff --git a/features/tui_permissions_screen.feature b/features/tui_permissions_screen.feature index f0ce17582..902db9e65 100644 --- a/features/tui_permissions_screen.feature +++ b/features/tui_permissions_screen.feature @@ -373,3 +373,27 @@ Feature: TUI PermissionsScreen And I record decision "allow_always" for request "req-1" And I clear the session decision for "local/file-write" Then the clear result should be True + + # ── Bug #10488: PermissionsScreen must inherit from Screen ──── + + @tdd_issue @tdd_issue_10488 + Scenario: Bug #10488 - PermissionsScreen inherits from textual.app.Screen + When I check the base class of PermissionsScreen + Then PermissionsScreen should be a subclass of textual.app.Screen + + @tdd_issue @tdd_issue_10488 + Scenario: Bug #10488 - PermissionsScreen has BINDINGS class variable + When I check the base class of PermissionsScreen + Then PermissionsScreen should have a BINDINGS class variable + + @tdd_issue @tdd_issue_10488 + Scenario: Bug #10488 - PermissionsScreen has action methods for keyboard bindings + When I check the base class of PermissionsScreen + Then PermissionsScreen should have action method "action_allow_once" + And PermissionsScreen should have action method "action_allow_always" + And PermissionsScreen should have action method "action_reject_once" + And PermissionsScreen should have action method "action_reject_always" + And PermissionsScreen should have action method "action_nav_next" + And PermissionsScreen should have action method "action_nav_prev" + And PermissionsScreen should have action method "action_cycle_diff" + And PermissionsScreen should have action method "action_dismiss_screen" diff --git a/src/cleveragents/tui/permissions/screen.py b/src/cleveragents/tui/permissions/screen.py index 574c9b194..039444b09 100644 --- a/src/cleveragents/tui/permissions/screen.py +++ b/src/cleveragents/tui/permissions/screen.py @@ -1,15 +1,17 @@ -"""PermissionsScreen widget for displaying tool permission requests with diff views. +"""PermissionsScreen for displaying tool permission requests with diff views. Shows a split-pane layout: a file list on the left and a diff view on the right. -Supports three diff display modes (unified, split, auto) toggled with ``d``. +Supports three diff display modes (unified, side-by-side, context) toggled with ``d``. Allow/reject keyboard bindings: ``a`` allow-once, ``A`` allow-always, ``r`` reject-once, ``R`` reject-always. """ from __future__ import annotations +import contextlib import importlib -from typing import Any +from collections.abc import Iterator +from typing import Any, ClassVar from cleveragents.tui.permissions.models import ( DiffDisplayMode, @@ -23,22 +25,22 @@ __all__ = ["PermissionsScreen"] # ── Optional Textual import gate ───────────────────────────────── -def _load_static_base() -> type[Any]: +def _load_screen_base() -> type[Any]: try: - return importlib.import_module("textual.widgets").Static + return importlib.import_module("textual.app").Screen except Exception: # pragma: no cover - class _FallbackStatic: + class _FallbackScreen: def __init__(self, *args: object, **kwargs: object) -> None: self._text = "" def update(self, text: str) -> None: self._text = text - return _FallbackStatic + return _FallbackScreen -_StaticBase = _load_static_base() +_ScreenBase = _load_screen_base() # ── Diff display mode cycle ─────────────────────────────────────── @@ -110,11 +112,16 @@ def _render_screen( return f"Permission Request\n\n{file_list}\n\n{diff_panel}\n\n{status_bar}" -# ── PermissionsScreen widget ────────────────────────────────────── +# ── PermissionsScreen ───────────────────────────────────────────── -class PermissionsScreen(_StaticBase): - """TUI widget that displays a tool permission request with a diff view. +class PermissionsScreen(_ScreenBase): + """TUI screen that displays a tool permission request with a diff view. + + Inherits from ``textual.app.Screen`` so it can be pushed onto the + Textual screen stack via ``app.push_screen()``, receive keyboard events + natively through ``BINDINGS``, and participate in the full Textual + screen lifecycle (``compose``, ``on_mount``, ``dismiss``, etc.). Layout: - Title: "Permission Request" @@ -128,10 +135,21 @@ class PermissionsScreen(_StaticBase): - ``r``: reject once - ``R``: reject always (session) - ``j`` / ``k``: navigate file list (next / previous) - - ``d``: cycle diff display mode (unified → split → auto) - - ``escape``: dismiss (caller responsibility) + - ``d``: cycle diff display mode (unified → side-by-side → context) + - ``escape``: dismiss screen """ + BINDINGS: ClassVar[list[tuple[str, str, str]]] = [ + ("a", "allow_once", "Allow Once"), + ("A", "allow_always", "Allow Always"), + ("r", "reject_once", "Reject Once"), + ("R", "reject_always", "Reject Always"), + ("j", "nav_next", "Next File"), + ("k", "nav_prev", "Previous File"), + ("d", "cycle_diff", "Cycle Diff Mode"), + ("escape", "dismiss_screen", "Dismiss"), + ] + def __init__(self, *args: object, **kwargs: object) -> None: super().__init__(*args, **kwargs) self._request: ToolPermissionRequest | None = None @@ -139,7 +157,28 @@ class PermissionsScreen(_StaticBase): self._diff_mode: DiffDisplayMode = DiffDisplayMode.UNIFIED self._decision: PermissionDecision | None = None self._text: str = "(no permission request)" - self.update(self._text) + + def update(self, text: str) -> None: + """Update the internal text representation. + + Stores the rendered text for programmatic access and testing. + In a live Textual app, the ``compose()`` method renders the + content via widgets; this method keeps the ``_text`` attribute + in sync for non-Textual usage and testing. + """ + self._text = text + + def compose(self) -> Iterator[Any]: + """Compose the screen layout with a Static widget showing the content. + + Returns a generator over Textual ``Widget`` instances (the + ``ComposeResult`` type), but typed as ``Iterator[Any]`` here so + the module stays importable in environments where ``textual`` is + not installed (typecheck gate, pure-domain tests). + """ + with contextlib.suppress(Exception): # pragma: no cover + Static = importlib.import_module("textual.widgets").Static + yield Static(self._text, id="permissions-content") # ── Public API ──────────────────────────────────────────────── @@ -238,6 +277,41 @@ class PermissionsScreen(_StaticBase): self._refresh() return decision + # ── Textual action methods ──────────────────────────────────── + + def action_allow_once(self) -> None: + """Textual action: allow once (``a`` key binding).""" + self.allow_once() + + def action_allow_always(self) -> None: + """Textual action: allow always (``A`` key binding).""" + self.allow_always() + + def action_reject_once(self) -> None: + """Textual action: reject once (``r`` key binding).""" + self.reject_once() + + def action_reject_always(self) -> None: + """Textual action: reject always (``R`` key binding).""" + self.reject_always() + + def action_nav_next(self) -> None: + """Textual action: navigate to next file (``j`` key binding).""" + self.navigate_next() + + def action_nav_prev(self) -> None: + """Textual action: navigate to previous file (``k`` key binding).""" + self.navigate_prev() + + def action_cycle_diff(self) -> None: + """Textual action: cycle diff display mode (``d`` key binding).""" + self.cycle_diff_mode() + + def action_dismiss_screen(self) -> None: + """Textual action: dismiss the screen (``escape`` key binding).""" + with contextlib.suppress(Exception): # pragma: no cover + self.dismiss() + # ── Rendering ───────────────────────────────────────────────── def _refresh(self) -> None: