Conversation
There was a problem hiding this comment.
Pull request overview
Fixes render_live_sessions so that resumed active sessions display post-resume activity (active_user_messages, active_output_tokens) instead of stale historical totals, aligning it with the existing _render_active_section behavior.
Changes:
- Update
render_live_sessionsto choose between active-period stats vs. historical totals based on presence ofactive_*fields. - Add unit tests covering resumed-session behavior and ensuring pure-active sessions keep using totals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/copilot_usage/report.py |
Switch live-session Messages/Output Tokens to active_* fields for resumed sessions, otherwise keep existing totals logic. |
tests/copilot_usage/test_report.py |
Add coverage for resumed sessions vs. pure-active sessions in render_live_sessions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Commit pushed:
|
69f5961 to
29c1520
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Commit pushed:
|
f0246c4 to
5e7e3a7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Quality Gate: APPROVED ✅
Code Quality: Good — clean conditional using last_resume_time is not None to distinguish resumed vs. pure-active sessions, consistent with the existing pattern in _render_active_section.
Impact: LOW — display-only bug fix in render_live_sessions table rendering (+32/−3 lines across report.py and test_report.py). No changes to data models, APIs, or dependencies.
Tests: Three meaningful test cases covering the resumed session case, the pure-active fallback, and the zero-activity edge case.
Auto-approving for merge.
There was a problem hiding this comment.
Quality Gate: APPROVED ✅
Low-impact bug fix with good test coverage. Auto-approving for merge.
Evaluated:
- Code quality: Clean, focused fix. Uses
last_resume_time is not Noneto detect resumed sessions — correctly handles the zero-activity edge case. Follows existing patterns (_render_active_section). - Tests: 3 new tests covering resumed session (active values shown), pure-active regression (totals preserved), and zero-activity edge case.
- Impact: LOW — display-only change in
render_live_sessions(), no data mutation or API changes. 2 files, ~12 lines of source.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
❌ Pipeline orchestrator: review-response loop reached 3 rounds. Marking as stuck for human review. |
) For resumed sessions (with active_user_messages or active_output_tokens), render_live_sessions now uses the post-resume active_* fields instead of the historical totals from model_metrics/user_messages. This matches the pattern already used by _render_active_section. Pure-active sessions (never shut down) remain unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Harden test_resumed_session_shows_active_fields assertions: - Use a session_id without digits that could match test values - Use more distinctive numeric values (91, 263) for messages - Use word-boundary regex for integer assertions to prevent false positives from substring matches in other columns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use last_resume_time to detect resumed sessions instead of truthiness check on active_user_messages/active_output_tokens, fixing the edge case where a resumed session with zero post-resume activity would incorrectly show historical totals. - Fix test_pure_active_session_uses_totals: use a clean session_id and word-boundary regex to prevent false positives from substring matches. - Add test_resumed_session_zero_activity_shows_zeros for the zero- activity resumed session edge case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e34ac9e to
733ea46
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| has_active_stats = ( | ||
| s.last_resume_time is not None | ||
| or s.active_user_messages > 0 | ||
| or s.active_output_tokens > 0 | ||
| ) | ||
| if has_active_stats: | ||
| # Resumed/active session with post-resume stats (even when 0) | ||
| messages = str(s.active_user_messages) | ||
| output_tok = s.active_output_tokens | ||
| else: | ||
| # Pure-active (never shut down): totals are already in model_metrics | ||
| messages = str(s.user_messages) | ||
| output_tok = _estimated_output_tokens(s) | ||
|
|
There was a problem hiding this comment.
has_active_stats still falls back to historical totals for active/resumed sessions when last_resume_time is None and active_user_messages == 0 and active_output_tokens == 0. That state is possible if the post-shutdown activity is detected via an assistant.message without outputTokens (there’s an explicit test for missing outputTokens), and there’s no session.resume event. In that case render_live_sessions will incorrectly show the shutdown-cycle totals instead of the active-period zeros.
Consider detecting whether the active fields were explicitly populated (e.g., via Pydantic SessionSummary.model_fields_set) and using active_user_messages/active_output_tokens whenever those fields are set, even if they’re 0 (with the current totals fallback only when the active fields were not provided at all).
There was a problem hiding this comment.
Good catch! Updated has_active_stats to use model_fields_set instead of value-based checks:
has_active_stats = "active_user_messages" in s.model_fields_setThis correctly detects whether the active fields were explicitly populated during construction (as build_session_summary() always does), even when their values are 0. The historical-totals fallback now only applies when the active fields were genuinely not provided (e.g., test helper _make_session for pure-active sessions).
fix/139-live-sessions-resumed-data-368b18ae2a881bf6) has been deleted from the remote, so I was unable to push the commit. The changes are ready locally but need the branch to be recreated for pushing.
Generated by Review Responder
| def test_active_session_without_last_resume_time_shows_active_fields(self) -> None: | ||
| """Active session with active_* but no last_resume_time should use active fields.""" | ||
| now = datetime.now(tz=UTC) | ||
| session = SessionSummary( | ||
| session_id="no-resume-event-1234", | ||
| name="Active Without Explicit Resume", | ||
| model="claude-sonnet-4", | ||
| is_active=True, | ||
| start_time=now - timedelta(hours=2), | ||
| # Historical totals accumulated before the current active period | ||
| user_messages=263, | ||
| model_metrics={ | ||
| "claude-sonnet-4": ModelMetrics( | ||
| usage=TokenUsage(outputTokens=200_000), | ||
| ) | ||
| }, | ||
| # Current active-period activity, even though last_resume_time is None | ||
| active_user_messages=91, | ||
| active_output_tokens=35_000, | ||
| active_model_calls=12, | ||
| ) | ||
| output = _capture_output([session]) | ||
| # Should show the active-period values, not historical totals, | ||
| # even when last_resume_time is None. | ||
| assert re.search(r"\b91\b", output), "active_user_messages (91) not found" | ||
| assert "35.0K" in output # active_output_tokens | ||
| assert not re.search(r"\b263\b", output), ( | ||
| "historical total (263) should not appear" | ||
| ) | ||
| assert "200.0K" not in output # historical tokens should NOT appear | ||
|
|
There was a problem hiding this comment.
The new tests cover (a) resumed sessions with last_resume_time set, (b) sessions with last_resume_time=None but non-zero active_*, and (c) zero-activity resumes when last_resume_time is set. There’s still an uncovered edge case that matches real parsing behavior: last_resume_time=None with active_user_messages=0 and active_output_tokens=0 can occur when the only post-shutdown activity is an assistant.message missing outputTokens (so tokens stay 0) and no session.resume event is present. Adding a test for that scenario would prevent regressions where render_live_sessions falls back to historical totals instead of showing active-period zeros.
There was a problem hiding this comment.
Added the missing test test_resumed_no_resume_event_zero_activity_shows_zeros which covers this exact edge case: last_resume_time=None with active_user_messages=0 and active_output_tokens=0 explicitly set. The test verifies that historical totals (150 messages, 100K tokens) do not appear, and that zeros are rendered on the session row instead.
This works because the code fix in report.py now checks "active_user_messages" in s.model_fields_set — Pydantic's model_fields_set tracks explicitly-provided fields regardless of their value, so the active zeros are correctly distinguished from unprovided defaults.
All 421 tests pass, 98% coverage, ruff/pyright clean.
Generated by Review Responder
There was a problem hiding this comment.
Quality Gate: APPROVED ✅
Low-impact display bug fix with comprehensive test coverage. Auto-approving for merge.
What was evaluated:
render_live_sessionsnow correctly usesactive_user_messages/active_output_tokensfor resumed sessions instead of showing stale historical totals- Logic correctly falls back to existing totals for pure-active (never shut down) sessions
- 4 new tests thoroughly cover: resumed sessions, edge case without
last_resume_time, pure-active fallback, and zero post-resume activity - Pattern matches existing
_render_active_sectionimplementation - No API, model, or dependency changes — purely presentation-layer fix
Fixes #139
Problem
render_live_sessionsuseds.user_messages(all-time total) and_estimated_output_tokens(s)(model_metrics sum from shutdown data only) for all active sessions. For resumed sessions, this showed stale historical data instead of the current post-resume activity.Fix
When a session has
active_user_messagesoractive_output_tokensset (indicating it was resumed), use those fields instead. Pure-active sessions (never shut down, where both are 0) continue using the existing totals. This matches the pattern already used by_render_active_section.Tests added
test_resumed_session_shows_active_fields— resumed session with historical model_metrics and non-zero active fields shows only the active-period valuestest_pure_active_session_uses_totals— pure-active session (no prior shutdown) still uses user_messages and model_metrics totalsAll 409 tests pass, 98% coverage, ruff/pyright clean.
Warning
The following domain was blocked by the firewall during workflow execution:
astral.shTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.