Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mozilla-ai/otari/.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; 1 remains after this review. WalkthroughARCHITECTURE.md adds guidance on deciding whether features belong in core or an overlay. It also explains why feature switches remain configuration and do not alter database schemas. ChangesArchitecture guidance
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The guidance is ready to merge after normal checks; no actionable issue remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title accurately describes the documentation changes and uses an appropriate documentation prefix, but it does not start with the required exact prefix
✨ 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
444cc54 to
b1afc3c
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.mdon who decides whether a feature belongs in the open-source product.Validation
make lintpasses and relevant documentation links resolve.