refactor(gateway): derive the settings view from the settings - #1190
Conversation
|
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 (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe change moves settings visibility into ChangesSettings view derivation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to No verified merge-blocking behavior change remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use 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 |
There was a problem hiding this comment.
I rebuilt both versions and compared them: the derived view is exactly what main hard-coded, down to group order, field order within each group, and the hidden set. The eleven moved declarations are right.
pd @peteski22 there are merge conflicts
The settings endpoint kept two hand-maintained tuples of field names: the grouped view and the deliberate omissions. Every new setting had to edit one of them, or the coverage test failed. Carry the view on each field instead. Every GatewayConfig field is annotated with exactly one of Shown(group), OMITTED or SECRET, and the endpoint lays its view out from those. A field with no annotation fails at import, so a new setting still cannot be forgotten. The endpoint's output is identical: eleven field declarations moved so declaration order within a group matches the order the view already had. Part of #1176.
Both markers hide a field, so marking a credential OMITTED would hide it just as well and the module's "never carries a credential" rule would be description rather than contract. Assert the marker for all seven, including the two the endpoint's own credential test never named. Also revert four line reflows a whole-file format left behind. They are unrelated to this change.
c208ab1 to
9073577
Compare
This branch was successfully deployed
2 participants
Description
The dashboard's Settings page shows a read-only view of the gateway's configuration, grouped by topic, with credentials and structured blocks left out. Until now, which setting went where was kept in two hand-maintained lists in the settings endpoint. Every new setting had to be added to one of them, or a test failed.
Now each setting says where it belongs, on its own field: shown in a named group, omitted, or secret. The endpoint lays the view out from those declarations. A setting with no declaration fails at import, so a new one still cannot be forgotten, and it fails earlier than a test would.
Nothing changes for anyone using Otari. The settings response is byte-for-byte identical before and after, including the order of fields. Eleven field declarations moved within the config so that declaration order matches the order the view already had.
Churn, measured over 90 days as the epic asks:
config.pygoes from 70 commits to 71 androutes/settings.pyfrom 29 to 30. Authors unchanged.How to test it locally
What to look for:
GET /api/v1/settingsanswers exactly what it did before.test_settings_endpoint.pyis unchanged and still passes, including the check that every config field is either shown or deliberately omitted.test_settings_view.pycovers the mechanism on small models: group order, declaration order within a group, and that a field with no declaration, or two, is refused.Run locally: lint, typecheck, the full unit suite (3462 passed; the 14 failures are the generated-master-key and Claude Code import tests, which fail the same way on
mainin my checkout because a local.envsetsOTARI_MASTER_KEYand my shell setsOTARI_API_KEY). The smoke gate and the integration suite are left to CI: the same.envsetsOTARI_PORT, which the smoke gate's server picks up in place of its own port.PR Type
Relevant issues
Fixes #1176. Part of #1171.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test). Lint, typecheck, andmake test-unitran locally;make test-integrationis left to CI.uv run python scripts/generate_openapi.py). Not applicable: the response shape is unchanged.AI Usage
AI Model/Tool used: Claude Fable 5.1, through Claude Code.
Any additional AI details you'd like to share: The issue and the decision to keep the field order by moving declarations were Peter's. The agent implemented the change, wrote the tests, and ran the checks. Peter reviewed the result before it was pushed.
Summary
This removes hand-maintained visibility lists and keeps future settings changes aligned with the endpoint view.
Technical notes
The settings response remains unchanged. Test execution results are unavailable because the repository clone failed.