fix: batch code-health fixes (#671, #790, #894, #909)#939
Merged
Conversation
The `idx < len(events)` guard was dead code — idx comes from enumerate(events) in _first_pass and is always a valid index. Simplified to direct `events[idx].timestamp` access. Removed test that crafted an impossible out-of-bounds scenario to exercise the dead branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…790) Replace default_factory=list with default_factory=lambda: [] for toolRequests and shutdown_cycles fields. Removes both pyright ignore[reportUnknownVariableType] suppressions. Updated CODING_GUIDELINES.md to note that complex generic list types in Pydantic Field require lambda: [] (simple types are fine with bare list). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Removed ~250-line module extraction trigger — line count alone is not a meaningful measure. Replaced with concern-based criterion. - Removed 'Defensive Programming' section that encouraged leaving unreachable guard clauses in code as future-proofing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Added missing vscode command to cli.py module docstring - Added test exception to getattr/hasattr guideline — introspection in tests (checking module exports, verifying field absence) is fine - Items 2 and 3 from the issue (replacing getattr/hasattr in tests) are no longer violations per the updated guideline Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Code-health batch addressing several open issues across parsing, pricing, models, CLI docs, and contributor guidelines to reduce dead code, improve typing hygiene, and make pricing lookups deterministic/safer.
Changes:
- Simplifies session shutdown summary building by removing an unreachable guard and dropping the corresponding impossible test.
- Updates Pydantic
Fielddefaults for complex generics to usedefault_factory=lambda: []and documents the convention in CODING_GUIDELINES. - Adjusts pricing partial-match logic to detect ambiguous ties, warn, and fall back to unknown-model pricing; updates tests accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/copilot_usage/pricing.py |
Adds tie detection in partial-match pricing lookup and falls back to unknown-model pricing with a warning. |
tests/copilot_usage/test_pricing.py |
Updates/expands tests to assert tie fallback behavior and warning emission. |
src/copilot_usage/parser.py |
Removes unreachable shutdown index guard when building completed summaries. |
tests/copilot_usage/test_parser.py |
Removes the test that fabricated an impossible out-of-bounds shutdown index scenario. |
src/copilot_usage/models.py |
Replaces default_factory=list + pyright ignores with default_factory=lambda: [] for complex generic list fields. |
src/copilot_usage/cli.py |
Fixes module docstring to include the vscode command. |
.github/CODING_GUIDELINES.md |
Updates guidance on Pydantic defaults, removes dead code-health guidance, and permits getattr/hasattr in tests for introspection. |
When _cached_lookup finds multiple keys tying on overlap length, it now falls back to unknown-model pricing (1.0×, STANDARD) and logs a warning instead of silently using whichever key appeared first in dict insertion order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
66ce25b to
96d5c5c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code-health batch
Five commits addressing open code-health issues and guideline cleanup.
Commits
Remove unreachable dead branch in
_build_completed_summary— closes [aw][code health] Small cleanups: unreachable dead branch in_build_completed_summary+ emptyPRODUCT_VISION.md#671if idx < len(events) else Noneguard that could never triggerUse
default_factory=lambdafor complex generic Pydantic fields — closes [aw][code health] Two Pydantic fields suppressreportUnknownVariableTypeinstead of usingdefault_factory=lambda: []#790toolRequestsandshutdown_cyclesfields changed fromdefault_factory=list+# pyright: ignore→default_factory=lambda: []Remove arbitrary line-count threshold and dead-code guideline
Fix stale cli.py docstring + allow getattr/hasattr in tests — closes [aw][code health] Minor guideline-compliance cleanup: stale docstring and banned hasattr/getattr in tests #894
vscodecommand to cli.py module docstringDetect ambiguous partial pricing matches and fall back — closes [aw][code health]
_cached_lookuppartial pricing match is nondeterministic when multiple keys tie onmatch_len, silently mis [Content truncated due to length] #909_cached_lookupfinds multiple keys tying on overlap length, falls back to unknown-model pricing (1.0×) and logs a warningTesting
All checks pass: ruff lint/format, pyright, bandit, unit tests (99% coverage), e2e tests (86 passed).