Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: mozilla-ai/otari/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughChangesArchitecture guidance
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to This documentation-only update does not alter runtime behavior or database schemas and is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title describes the documentation change and uses an allowed Conventional Commit type, but it uses a scope instead of the required exact prefix format, exceeds the approximately 70-character limit at 108 characters, and should be shortened. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
298c00b to
444cc54
Compare
Description
Two additions to
ARCHITECTURE.md. Neither changes any code or any behavior. Both write down something the document already assumed.1. Who decides whether a feature is part of the open-source product. "Where new code goes" picks a home by what is being added, and every row of it assumes someone has already classified the thing: "an optional feature any deployment may run" goes to core, "only an overlay ships" goes to an overlay. Nothing said who makes that call, so the table could be read as making it. The new paragraph says the classification comes first, that it belongs to the project's product and engineering leads together, and that a feature not yet confirmed starts in an overlay. The reason for that default is that the move only works one way: a feature proven in an overlay can move here later, and a feature released here can never be withdrawn.
2. Why a feature's switch never touches its schema. Step 3 of "How to add a core feature" states the rule (switching a feature off leaves its tables in place, switching it on changes no schema) with no reasoning, so it reads as arbitrary and open to renegotiation. The new subsection gives the four reasons, each checked against the code as it is on
main:_run_migrationsincore/database.py), so a switch that created tables would have every replica run the same DDL at once.alembic_versionis a function of the release. With a branch per feature it depends on the order switches were flipped.alembic/env.pyrefuses a database stamped with any revision the image does not know, so a feature switched on once would expose every later rollback.The rule this adds, per the checklist: a feature's place in the open-source product is confirmed by the project's product and engineering leads before "Where new code goes" applies. The schema rule is not new. It has been in step 3 since #1279 and in #1173's acceptance criteria; this PR adds only its reasoning.
How to test it locally
Documentation only.
make lintpasses. The new in-page link from step 3 resolves to the new subsection's heading, and the existing links into this section fromCONTRIBUTING.mdandsrc/gateway/AGENTS.mdstill resolve, because the heading they target is unchanged.Worth reading rather than running: the four claims in the new subsection, against
src/gateway/core/database.pyandalembic/env.py.PR Type
Relevant issues
Refs #1173, where the schema rule was set, and #1363, which proposes changing it and is the reason to put the reasoning on the record.
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:
The wording is AI-written. The decisions in it are not: who decides, and the overlay-first default, were settled by a person before anything was drafted. Each technical claim was checked against
mainfirst, and one sentence in the draft was corrected as a result (the gateway does not inspect the schema whenauto_migrateis off, so it would not refuse to start on its own).Summary
ARCHITECTURE.mdwith guidance on deciding whether features belong in the open-source product.Validation
make lintpasses.