fix: remove dead code in _build_active_summary (#1202)#1203
Open
fix: remove dead code in _build_active_summary (#1202)#1203
_build_active_summary (#1202)#1203Conversation
Remove two instances of dead/misleading code in parser.py: 1. _build_active_summary line 979: simplified 'model = fp.model or fp.tool_model' to 'model = fp.tool_model' since fp.model is invariably None in the active-session path. 2. _build_session_summary_with_meta line 1058: simplified 'used_config = fp.model is None and fp.tool_model is None' to 'used_config = fp.tool_model is None' since fp.model is always None when no shutdowns exist. Added a unit test that explicitly documents the invariant: fp.model is None when no SESSION_SHUTDOWN events exist, and the summary model is resolved from fp.tool_model directly. Closes #1202 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes two pieces of dead/misleading model-resolution logic in the session parser by relying on the correct invariant: for sessions without shutdown events, _first_pass.model is never populated, so the active-session path should use tool_model (and then config fallback) exclusively.
Changes:
- Simplified
_build_active_summaryto usefp.tool_modeldirectly (instead offp.model or fp.tool_model). - Simplified
_build_session_summary_with_meta’sused_config_fallbackcalculation to depend only on whetherfp.tool_modelis present. - Added a unit test asserting the invariant (
fp.model is Nonewhen no shutdown exists) and verifying the resultingSessionSummary.modelmatches the tool model.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/copilot_usage/parser.py |
Removes dead references to fp.model in the no-shutdown (active session) summary path. |
tests/copilot_usage/test_parser.py |
Adds regression coverage ensuring active sessions resolve the model from tool_model without relying on fp.model. |
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.
Closes #1202
Summary
Removes two instances of dead/misleading code in
parser.pywherefp.modelis referenced in the active-session path, but is invariablyNone(since it's only set duringSESSION_SHUTDOWNprocessing).Changes
1.
_build_active_summary(line 979)2.
_build_session_summary_with_meta(line 1058)Testing
Added
test_active_summary_uses_tool_model_directly_without_fp_modelwhich:TOOL_EXECUTION_COMPLETE(settingfp.tool_model) but noSESSION_SHUTDOWNfp.model is Noneandfp.all_shutdowns == ()SessionSummary.modelmatches the tool model directlyWarning
The following domains were blocked by the firewall during workflow execution:
astral.shpypi.orgreleaseassets.githubusercontent.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.