-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Root Cause
ModelMetrics dict merging is implemented twice with different approaches:
parser.py — build_session_summary() (lines 279–299): Constructs new ModelMetrics, RequestMetrics, and TokenUsage objects on every merge. Safe, but verbose.
report.py — _aggregate_model_metrics() (lines 544–564): Uses model_copy() on first insertion, then mutates the copy's fields in-place for subsequent merges. This is fragile — if the model_copy() is ever removed, it silently corrupts session data passed from the caller.
Because this logic is duplicated, any future bug fix or field addition (e.g., a new token type on TokenUsage) must be applied in both places. They have already diverged in style.
Proposed Fix
Extract a shared helper, e.g. in models.py or a new copilot_usage/_utils.py:
def merge_model_metrics(
base: dict[str, ModelMetrics],
additional: dict[str, ModelMetrics],
) -> dict[str, ModelMetrics]:
"""Return a new dict merging *additional* into *base* without mutation."""
result = {name: ModelMetrics(...) for name, mm in base.items()} # deep copy
for name, mm in additional.items():
if name in result:
existing = result[name]
result[name] = ModelMetrics(
requests=RequestMetrics(
count=existing.requests.count + mm.requests.count,
cost=existing.requests.cost + mm.requests.cost,
),
usage=TokenUsage(
inputTokens=existing.usage.inputTokens + mm.usage.inputTokens,
outputTokens=existing.usage.outputTokens + mm.usage.outputTokens,
cacheReadTokens=existing.usage.cacheReadTokens + mm.usage.cacheReadTokens,
cacheWriteTokens=existing.usage.cacheWriteTokens + mm.usage.cacheWriteTokens,
),
)
else:
result[name] = mm
return resultReplace both implementations with calls to this helper.
Testing Requirements
- Unit test
merge_model_metricsdirectly: empty dicts, single entry, overlapping keys, disjoint keys, all token fields accumulate correctly. - Regression: existing
test_parser.pyandtest_report.pycoverage must continue to pass unchanged. - Verify the extracted function is the single source of truth by removing the inline logic from both callers.
Generated by Code Health Analysis · ◷