Conversation
Extract a shared parse_token_int() helper in models.py that centralises the token-validation rules used by both _sanitize_non_numeric_tokens (Pydantic boundary) and _extract_output_tokens (parser fast path). This eliminates the duplicated logic and makes divergence structurally impossible. Add TestExtractOutputTokensValueEquality with test_token_value_agreement parametrized test that verifies numeric counts match (not just the binary contributes/does-not-contribute verdict) for contributing values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses token-validation drift risk by extracting a shared parse_token_int() helper in copilot_usage.models, then tightening tests to ensure the parser fast-path and Pydantic model-path agree on exact token counts (not just “contributes vs not”).
Changes:
- Added shared
parse_token_int(raw: object) -> int | Noneinsrc/copilot_usage/models.pyand exported it via__all__. - Updated
_extract_output_tokens()insrc/copilot_usage/parser.pyto delegate toparse_token_int()(removing duplicated logic). - Added parametrized value-equality tests in
tests/copilot_usage/test_parser.pyto assert numeric count equality for contributing cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/copilot_usage/test_parser.py |
Adds value-equality tests to prevent silent numeric divergences between extraction paths. |
src/copilot_usage/parser.py |
Replaces inline token parsing with a shared helper to enforce consistent rules. |
src/copilot_usage/models.py |
Introduces parse_token_int() and refactors the field validator to use it. |
microsasa
added
the
aw-review-response-attempted
- Handle None (JSON null) in _sanitize_non_numeric_tokens by mapping it
to 0, preventing ValidationError on model_validate({"outputTokens": None})
- Fix overclaiming docstring in _extract_output_tokens to clarify that
numeric equality is guaranteed only for contributing values
- Add None to _EQUIVALENCE_CASES as regression test
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
microsasa
added
aw-review-response-1
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Quality Gate: APPROVED ✅
Medium-impact DRY refactor — extracts duplicated token-validation logic into a shared parse_token_int helper, eliminating divergence risk between the Pydantic model path and the parser fast path. New value-equality tests verify numeric agreement for contributing values. All 12 existing equivalence cases continue to pass. CI green across all 9 checks. Auto-approving for merge.
Closes #880
Summary
The cross-consistency test for
_extract_output_tokensvsAssistantMessageData._sanitize_non_numeric_tokensonly verified binary agreement (contributes / does not contribute), not that the actual numeric token counts matched. This meant the two independent implementations could silently diverge on values while all tests pass.Changes
Structural fix: shared
parse_token_inthelper (models.py)Extracted a single
parse_token_int(raw: object) -> int | Nonefunction that centralises the token-validation rules:bool/str→Nonefloat→Noneint/float→Nonefloat→ coerced tointint→ returned as-isBoth
_sanitize_non_numeric_tokens(Pydantic validator) and_extract_output_tokens(parser fast path) now delegate to this single helper, eliminating the duplication entirely.Value-equality tests (
test_parser.py)Added
TestExtractOutputTokensValueEqualitywith a parametrizedtest_token_value_agreementtest covering the minimum cases from the issue spec:1(positive int)42(positive int)1234.0(whole float)1.0(whole float)Each case asserts that both paths produce the exact same integer count, not just that they agree on the binary verdict.
Regression
All 12 existing
_EQUIVALENCE_CASESbinary tests continue to pass unchanged.