refactor(budgets): move the reservation status names beside the column they name - #1453
Merged
Merged
Conversation
…column A closed vocabulary lives in the domain's models module, beside the string column it names. ScopeType and ResetAlignment are there. The reservation statuses were the last budgets vocabulary defined in a service module, so the BudgetReservation.status column depended on a service for its meaning. The statuses take the same shape as the other two: a Literal, a tuple derived from it, and each constant annotated with the Literal so mypy proves membership. A unit test asserts that the constants cover the Literal. The ledger imports the names and still re-exports them, so its importers keep working until the next commit repoints them.
…dule The settlement path and the ledger tests read the status names through the ledger module. They now import them from models/budgets.py, where the names are defined, and the ledger no longer re-exports them. One import path per name keeps a second definition from growing back.
…dgets.py The budgets entry said the statuses still sat in the ledger module and would move. They moved, so the entry lists all three vocabularies in one place.
try_terminate took the new status as a bare string, so a misspelled status reached the database unchecked. The parameter is now ReservationStatus, and mypy rejects a value outside the vocabulary at every call site.
…tant The status column spelled "active" twice beside the constant that names it. Both defaults now read RESERVATION_ACTIVE, so the column cannot drift from the vocabulary. The stored value and the DDL are unchanged.
|
Warning Review limit reachedNext included review available in 56 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mozilla-ai/otari/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
peteski22
marked this pull request as ready for review
September 21, 2026 18:54
tbille
approved these changes
Sep 21, 2026
15 tasks
This branch was successfully deployed
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
2 participants
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.
Description
Nothing changes for someone using Otari. This is an internal tidy-up in the budgets domain, and a follow-up from #1202.
A budget reservation has a status: active, settled, released or expired. The names for those four values lived in a service module, while the database column that stores them lives in the models module. The column's own comment pointed at the service for its meaning. #1202 settled that a closed vocabulary lives in the models module, beside the column it names. The scope types and reset alignments already moved there. The reservation statuses were the last budgets vocabulary outside that rule.
The statuses now take the same shape as the other two: a
Literal, a tuple derived from it, and named constants that mypy proves are members. Every importer reads them from the models module, and the ledger does not re-export them. Two small follow-ons use the new type: the function that ends a reservation accepts only a status from the vocabulary, and the column's default reads the named constant where it spelled the string.The statuses are not part of the published API, so nothing moves on the wire. The stored values and the table definition are unchanged.
How to test it locally
There is no visible behavior to look at. The automated checks carry the proof:
uv run pytest tests/unit/test_budget_vocabularies.py: the new test asserts that the named constants cover theLiteral.uv run pytest tests/integration/test_budget_reservation_ledger.py: the ledger lifecycle, unchanged, against the moved names.uv run pytest tests/integration/test_schema_metadata_parity.py: the column default still matches the migrated schema.make lint && make typecheckmake openapi-check && make postman-check, and theweb/src/client/schema.tsdrift check: all three show no diff.Full unit and integration suites ran green locally, apart from
test_mcp_dependency_ceiling, which needs network access and is known local noise.PR Type
Relevant issues
Closes #1416. Follow-up from #1202.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).ARCHITECTURE.mdorscripts/check_architecture.py, the description names the rule and says why.AI Usage
AI Model/Tool used:
Claude, via Claude Code.
Any additional AI details you'd like to share:
This was shared work. I wrote the issue, which sets the shape of the change: where the names live, the
Literalform they take, and that the ledger does not re-export them. I also set the rules the work ran under: one small commit per step, and any comment the change touches is rewritten to the project's comment standard. Claude carried out the move under that direction, ran the checks, and drafted this description. I reviewed the commits and every touched comment before anything was pushed. Claude raised two optional follow-ons during that review, typing the status parameter and using the constant for the column default, and I decided to include both.NOTE:
When responding to reviewer questions, please respond yourself rather than copy/pasting reviewer comments into an AI and pasting back its answer. We want to discuss with you, not your AI :)