From 35dcaf29f5101016157dea50ffe9c4f6fedf4f97 Mon Sep 17 00:00:00 2001 From: Sasa Junuzovic Date: Sat, 18 Apr 2026 22:48:37 -0700 Subject: [PATCH] fix: code health cleanup and test gaps (#921, #860) - Remove dead '# Typed event helpers' section header in models.py - Convert 3 mypy-style type: ignore to pyright: ignore with correct rule - Remove unnecessary import-untyped suppress (pyright doesn't flag it) - Replace getattr/hasattr with vars() in test_packaging.py (module-level) - Keep hasattr for SessionEvent.parse_data guard (needs full MRO check) - Add test for TOOL_EXECUTION_COMPLETE with model field (#860 gap 2) - Add test for TOOL_EXECUTION_COMPLETE with model='' (falsy edge case) - Restore actions-lock.json (had stale merge conflict markers) Closes #921 Partially addresses #860 (gap 1 already covered by existing tests) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/copilot_usage/cli.py | 4 ++-- src/copilot_usage/logging_config.py | 2 +- src/copilot_usage/models.py | 5 ----- tests/copilot_usage/test_report.py | 31 +++++++++++++++++++++++++++++ tests/test_packaging.py | 12 +++++++---- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/copilot_usage/cli.py b/src/copilot_usage/cli.py index f3d516e6..d44dd236 100644 --- a/src/copilot_usage/cli.py +++ b/src/copilot_usage/cli.py @@ -283,11 +283,11 @@ def _start_observer( treat a ``None`` return as "auto-refresh unavailable" and continue without it. """ - from watchdog.observers import Observer # type: ignore[import-untyped] + from watchdog.observers import Observer handler: _FileChangeEventHandler = _FileChangeHandler(change_event) observer = Observer() - observer.schedule(handler, str(session_path), recursive=True) # type: ignore[arg-type] + observer.schedule(handler, str(session_path), recursive=True) # pyright: ignore[reportArgumentType] observer.daemon = True try: observer.start() diff --git a/src/copilot_usage/logging_config.py b/src/copilot_usage/logging_config.py index 4c7eec4b..d0792027 100644 --- a/src/copilot_usage/logging_config.py +++ b/src/copilot_usage/logging_config.py @@ -53,5 +53,5 @@ def _emoji_patcher(record: _PatcherRecord) -> None: def setup_logging() -> None: """Configure loguru for CLI use: stderr only, WARNING level.""" logger.remove() - logger.configure(patcher=_emoji_patcher) # type: ignore[arg-type] # TypedDict value invariance: _PatcherRecord is a structural subset of loguru.Record + logger.configure(patcher=_emoji_patcher) # pyright: ignore[reportArgumentType] # TypedDict value invariance: _PatcherRecord is a structural subset of loguru.Record logger.add(sys.stderr, format=CONSOLE_FORMAT, level="WARNING", colorize=True) diff --git a/src/copilot_usage/models.py b/src/copilot_usage/models.py index 049a6360..8ff71044 100644 --- a/src/copilot_usage/models.py +++ b/src/copilot_usage/models.py @@ -317,11 +317,6 @@ class GenericEventData(BaseModel, extra="allow"): """Catch‐all payload for event types not yet modeled explicitly.""" -# --------------------------------------------------------------------------- -# Typed event helpers -# --------------------------------------------------------------------------- - - # --------------------------------------------------------------------------- # Event envelope # --------------------------------------------------------------------------- diff --git a/tests/copilot_usage/test_report.py b/tests/copilot_usage/test_report.py index 7052ac84..75227dfe 100644 --- a/tests/copilot_usage/test_report.py +++ b/tests/copilot_usage/test_report.py @@ -3006,6 +3006,37 @@ def test_shutdown_event_nonempty_shutdown_type_shown_in_detail(self) -> None: ) assert _build_event_details(ev) == "type=normal" + def test_tool_execution_with_model_field(self) -> None: + """TOOL_EXECUTION_COMPLETE with model → detail string includes model=.""" + ev = _make_event( + EventType.TOOL_EXECUTION_COMPLETE, + data={ + "toolCallId": "t1", + "success": True, + "model": "claude-sonnet-4", + "toolTelemetry": {"properties": {"tool_name": "bash"}}, + }, + ) + details = _build_event_details(ev) + assert "model=claude-sonnet-4" in details + assert "bash" in details + assert "✓" in details + + def test_tool_execution_with_empty_model_string(self) -> None: + """TOOL_EXECUTION_COMPLETE with model='' → model not shown in details.""" + ev = _make_event( + EventType.TOOL_EXECUTION_COMPLETE, + data={ + "toolCallId": "t1", + "success": True, + "model": "", + "toolTelemetry": {"properties": {"tool_name": "bash"}}, + }, + ) + details = _build_event_details(ev) + assert "model=" not in details + assert "✓" in details + class TestRenderShutdownCyclesEdgeCases: """Test _render_shutdown_cycles with edge-case summaries.""" diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 63a3ab81..d7a14209 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -27,10 +27,11 @@ def test_all_names_importable(module_name: str) -> None: """Every name listed in a module's ``__all__`` must be importable at runtime.""" mod = importlib.import_module(module_name) - dunder_all: list[str] | None = getattr(mod, "__all__", None) # noqa: B009 + mod_vars = vars(mod) + dunder_all: list[str] | None = mod_vars.get("__all__") assert dunder_all is not None, f"{module_name} is missing __all__" for name in dunder_all: - assert hasattr(mod, name), ( # noqa: B009 + assert name in mod_vars, ( f"{module_name}.__all__ lists {name!r}, but it is not defined in the module" ) @@ -64,10 +65,13 @@ def test_parse_data_and_event_data_removed_from_public_api() -> None: dunder_all = models_mod.__all__ assert "EventData" not in dunder_all, "EventData must not be in models.__all__" - assert not hasattr(models_mod, "EventData"), ( # noqa: B009 + models_vars = vars(models_mod) + assert "EventData" not in models_vars, ( "EventData type alias must not exist in models module" ) - assert not hasattr(models_mod.SessionEvent, "parse_data"), ( # noqa: B009 + assert not hasattr( + models_mod.SessionEvent, "parse_data" + ), ( # hasattr intentional: must check full MRO "SessionEvent.parse_data() must not exist — use as_*() accessors" )