refactor: extract _SpendingTotals base class from spending summary models #138
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| """Spending summary models for aggregated cost reporting. | ||
|
|
||
| Provides the aggregation data structures used by | ||
| :class:`~ai_company.budget.tracker.CostTracker` for cost reporting and | ||
| designed for consumption by the CFO agent (DESIGN_SPEC Section 10.3). | ||
| Views of :class:`~ai_company.budget.cost_record.CostRecord` data are | ||
| aggregated by agent, department, and time period. | ||
| """ | ||
|
|
@@ -16,26 +17,25 @@ | |
| from ai_company.core.types import NotBlankStr # noqa: TC001 | ||
|
|
||
|
|
||
| class _SpendingTotals(BaseModel): | ||
| """Shared aggregation fields for spending summary models. | ||
|
|
||
| Not intended for direct instantiation — subclass with a | ||
| dimension-specific identifier (agent, department, or period). | ||
|
|
||
| Attributes: | ||
| total_cost_usd: Total cost for the aggregation group. | ||
| total_input_tokens: Total input tokens consumed. | ||
| total_output_tokens: Total output tokens consumed. | ||
| record_count: Number of cost records aggregated. | ||
| """ | ||
|
|
||
| model_config = ConfigDict(frozen=True) | ||
|
|
||
| total_cost_usd: float = Field( | ||
| default=0.0, | ||
| ge=0.0, | ||
| description="Total cost for the aggregation group", | ||
| ) | ||
|
Comment on lines
35
to
39
There was a problem hiding this comment.
|
||
| total_input_tokens: int = Field( | ||
| default=0, | ||
|
|
@@ -53,6 +53,18 @@ class PeriodSpending(BaseModel): | |
| description="Number of cost records aggregated", | ||
| ) | ||
|
Comment on lines
20
to
54
There was a problem hiding this comment.
The docstring states "Not intended for direct instantiation", but because all four fields carry defaults ( t = _SpendingTotals() # works — produces a dimensionless totals object
t = _SpendingTotals(total_cost_usd=9.99) # also worksThe underscore prefix communicates the intent by convention, but nothing prevents accidental use. Consider enforcing the constraint by raising in def model_post_init(self, __context: object) -> None:
if type(self) is _SpendingTotals:
raise TypeError(
"_SpendingTotals is not intended for direct instantiation; "
"use AgentSpending, DepartmentSpending, or PeriodSpending instead."
)This turns a documentation-only contract into a runtime-enforced one, which is particularly valuable given the class lives in a public package with three concrete subclasses already defined. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ai_company/budget/spending_summary.py
Line: 20-54
Comment:
**`_SpendingTotals` is directly instantiable**
The docstring states *"Not intended for direct instantiation"*, but because all four fields carry defaults (`0.0` / `0`), the class can be silently instantiated without error:
```python
t = _SpendingTotals() # works — produces a dimensionless totals object
t = _SpendingTotals(total_cost_usd=9.99) # also works
```
The underscore prefix communicates the intent by convention, but nothing prevents accidental use. Consider enforcing the constraint by raising in `__init_subclass__` or, more idiomatically with Pydantic v2, by overriding `model_post_init`:
```python
def model_post_init(self, __context: object) -> None:
if type(self) is _SpendingTotals:
raise TypeError(
"_SpendingTotals is not intended for direct instantiation; "
"use AgentSpending, DepartmentSpending, or PeriodSpending instead."
)
```
This turns a documentation-only contract into a runtime-enforced one, which is particularly valuable given the class lives in a public package with three concrete subclasses already defined.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
|
|
||
| class PeriodSpending(_SpendingTotals): | ||
| """Spending aggregation for a specific time period. | ||
|
|
||
| Attributes: | ||
| start: Period start (inclusive). | ||
| end: Period end (exclusive). | ||
| """ | ||
|
|
||
| start: datetime = Field(description="Period start (inclusive)") | ||
| end: datetime = Field(description="Period end (exclusive)") | ||
|
|
||
| @model_validator(mode="after") | ||
| def _validate_period_ordering(self) -> Self: | ||
| """Ensure start is strictly before end.""" | ||
|
|
@@ -65,78 +77,26 @@ def _validate_period_ordering(self) -> Self: | |
| return self | ||
|
|
||
|
|
||
| class AgentSpending(_SpendingTotals): | ||
| """Spending aggregation for a single agent. | ||
|
|
||
| Attributes: | ||
| agent_id: Agent identifier. | ||
| """ | ||
|
|
||
| agent_id: NotBlankStr = Field(description="Agent identifier") | ||
|
|
||
|
|
||
| class DepartmentSpending(_SpendingTotals): | ||
| """Spending aggregation for a department. | ||
|
|
||
| Attributes: | ||
| department_name: Department name. | ||
| """ | ||
|
|
||
| department_name: NotBlankStr = Field( | ||
| description="Department name", | ||
| ) | ||
|
Comment on lines
57
to
99
There was a problem hiding this comment. JSON serialization field order has changed The refactor silently changes the field ordering in Before (e.g. {"agent_id": "alice", "total_cost_usd": 10.0, "total_input_tokens": 0, "total_output_tokens": 0, "record_count": 0}After: {"total_cost_usd": 10.0, "total_input_tokens": 0, "total_output_tokens": 0, "record_count": 0, "agent_id": "alice"}Semantically this is harmless for roundtrip deserialization (Pydantic and most JSON consumers are order-agnostic), but the PR description claims "no behavioral changes." The change is real, and it could surface in:
If preserving the original order matters, one lightweight fix is to redeclare the inherited dimension-specific field at the top of each subclass (with no body change) — Pydantic respects the MRO order and moves a redeclared field to where it's first seen in the subclass. Otherwise, consider adding an explicit note to the PR description and/or DESIGN_SPEC acknowledging the order change. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ai_company/budget/spending_summary.py
Line: 56-98
Comment:
**JSON serialization field order has changed**
The refactor silently changes the field ordering in `model_dump()` / `model_dump_json()` output for all three subclasses. Pydantic v2 emits fields in definition order — parent fields first, then subclass fields — so the identifier fields (`agent_id`, `department_name`, `start`/`end`) are now serialized *last* instead of first.
Before (e.g. `AgentSpending`):
```json
{"agent_id": "alice", "total_cost_usd": 10.0, "total_input_tokens": 0, "total_output_tokens": 0, "record_count": 0}
```
After:
```json
{"total_cost_usd": 10.0, "total_input_tokens": 0, "total_output_tokens": 0, "record_count": 0, "agent_id": "alice"}
```
Semantically this is harmless for roundtrip deserialization (Pydantic and most JSON consumers are order-agnostic), but the PR description claims "no behavioral changes." The change is real, and it could surface in:
- snapshot / golden-file tests that do string-level JSON comparisons
- downstream consumers that parse the JSON positionally (e.g. some CSV-style streaming parsers)
- OpenAPI schema generation, where field order affects documentation readability
If preserving the original order matters, one lightweight fix is to redeclare the inherited dimension-specific field at the top of each subclass (with no body change) — Pydantic respects the MRO order and moves a redeclared field to where it's first seen in the subclass. Otherwise, consider adding an explicit note to the PR description and/or DESIGN_SPEC acknowledging the order change.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
|
|
||
| class SpendingSummary(BaseModel): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring correctly states that
_SpendingTotalsis not intended for direct instantiation. To enforce this and prevent potential misuse, you can add a check to prevent it from being instantiated directly. This improves the robustness of the design.Here's how you could do it using
model_post_init(you'll also need tofrom typing import Any):