-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Four small but genuine code-quality problems that together form a meaningful cleanup. None are caught by ruff/pyright/bandit in the current configuration.
1. Duplicated running-time computation (report.py)
The 4-line expression to compute a session's "running" display string is copy-pasted verbatim in two functions:
# render_live_sessions, lines 147–151
running = (
_format_elapsed_since(s.last_resume_time or s.start_time)
if s.start_time
else "—"
)
# _render_active_section, lines 791–795 — identical
running = (
_format_elapsed_since(s.last_resume_time or s.start_time)
if s.start_time
else "—"
)Fix: Extract a private helper _format_session_running_time(session: SessionSummary) -> str and call it from both sites.
2. render_cost_view shadows its sessions parameter (report.py:850)
def render_cost_view(
sessions: list[SessionSummary],
...
) -> None:
console = target_console or Console()
sessions = _filter_sessions(sessions, since, until) # ← shadows parameterReassigning a parameter to a filtered subset is confusing and could silently affect readers who add code after the assignment. Every other function that filters uses a separate filtered variable.
Fix: Rename the reassignment to filtered = _filter_sessions(sessions, since, until) and update all references inside the function.
3. Inconsistent output-token summation style (report.py)
_render_totals (lines 583–586) uses a nested for loop:
total_output = 0
for s in sessions:
for mm in s.model_metrics.values():
total_output += mm.usage.outputTokens_render_historical_section (lines 741–742) does the identical computation as a comprehension:
total_output = sum(
mm.usage.outputTokens for s in historical for mm in s.model_metrics.values()
)A private helper _estimated_output_tokens already exists but only handles a single SessionSummary.
Fix: Standardize on the comprehension form in _render_totals. Optionally add a multi-session variant of the helper.
4. AssistantMessageData.toolRequests uses an unnecessarily complex default_factory (models.py:127)
toolRequests: list[dict[str, object]] = Field(
default_factory=lambda: list[dict[str, object]]() # ← equivalent to `list`
)At runtime, list[dict[str, object]]() evaluates to list() — it creates a plain empty list. The generic subscript is erased. This is:
- Misleading: looks as if the factory creates a typed list at runtime, but it doesn't
- Inconsistent with all other
listanddictfields in the file, which usedefault_factory=list/default_factory=dict
Fix:
toolRequests: list[dict[str, object]] = Field(default_factory=list)Acceptance Criteria
-
_format_session_running_time(session: SessionSummary) -> strhelper extracted; bothrender_live_sessionsand_render_active_sectioncall it -
render_cost_viewusesfiltered(notsessions) for the filtered result; all internal references updated -
_render_totalsuses the same comprehension pattern as_render_historical_section -
AssistantMessageData.toolRequestsusesdefault_factory=list - All existing tests pass without modification
- New unit test for
_format_session_running_time: verify it returns"—"whenstart_time is None, returns a string from_format_elapsed_sinceotherwise (covering bothlast_resume_timepresent and absent)
Generated by Code Health Analysis · ◷