settings: PUT /api/config silently drops archive, archived_agents, and github_app_id - #2375
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe configuration save and backup restore flows now preserve ChangesConfiguration round-trip preservation
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
|
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summary (commit d7c12d7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d7c12d7)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 85.2K · Output: 7.3K · Cached: 441.2K |
|
Lead review. The shape of this is right — in particular the last test (populate every Three things before it can merge:
The |
Conflict was at both AppConfig rebuild sites in routes/settings.py: this branch adds github_app_id there, dev added lora_ingest_proxy_url in #2374. Same class, same two lines - resolved by keeping BOTH at both sites.
|
Lead-completed rather than bounced. The rebase I asked for in the review above could never happen: lanes are one-shot, so the author posted [REVIEW] and exited before my comment existed. Waiting on a re-push that cannot occur is how #2309 and #2336 rotted for days, and both were closed today for exactly that. Done on this branch:
|
…ld-site rule doc-gate went red on the merge: this branch modifies a route module and changes user-visible behaviour, and dev's gate now demands both a route doc and a changelog fragment. The doc records the actual trap rather than just listing endpoints: both write paths rebuild AppConfig field by field, so a field missing from either is silently dropped on the next save. That has now happened twice in two days (#2375 and #2374), and the parity test is what stops the third.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_routes_config.py (1)
154-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd backup restore regression coverage.
These tests exercise
PUT /api/configonly.restore_backup()has a separateAppConfigrebuild. A future field omission in that path will pass this suite. Add a backup fixture and assert thatPOST /api/settings/restorepreserves the affected fields in application state and inGET /api/config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_routes_config.py` around lines 154 - 173, Extend test_save_config_preserves_all_to_dict_keys with a backup fixture containing the configured fields, then call POST /api/settings/restore and assert those fields remain present in app.state.config and the YAML returned by GET /api/config. Ensure the regression exercises restore_backup() rather than only the existing PUT /api/config round trip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_routes_config.py`:
- Around line 154-173: Extend test_save_config_preserves_all_to_dict_keys with a
backup fixture containing the configured fields, then call POST
/api/settings/restore and assert those fields remain present in app.state.config
and the YAML returned by GET /api/config. Ensure the regression exercises
restore_backup() rather than only the existing PUT /api/config round trip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3af3a16-4da2-4222-a049-0f963e094e38
📒 Files selected for processing (4)
changelog.d/2375-config-round-trip-keys.mddocs/agent-coordination.mdtests/test_routes_config.pytinyagentos/routes/settings.py
1 participant
CARD TITLE (intent, not commit subject): settings: PUT /api/config silently drops archive, archived_agents, and github_app_id
Autonomous build of board card tsk-7awjeh.
Files:
tests/test_routes_config.py | 76 +++++++++++++++++++++++++++++++++++++++++-
tinyagentos/routes/settings.py | 6 ++++
2 files changed, 81 insertions(+), 1 deletion(-)
Lead-completed 2026-08-12 (rebase + red evidence)
The lane that opened this is gone (lanes are one-shot: post [REVIEW], exit), so the rebase
demanded in the review above could never reach a worker. Completed by @taOS-dev:
AppConfigrebuild sites inroutes/settings.py:this branch adds
github_app_idthere, dev addedlora_ingest_proxy_urlin feat(lora-studio): LoRA Studio v1 - Civitai ingest and archive #2374. Sameclass of bug, same two lines. Resolved by keeping both keys at both sites, which
is the only resolution that does not reintroduce one of the two bugs.
the round-trip test sets
archive = {"target": "path:/tmp/archive"}and asserts the keysurvives, so the test does not depend on the default's shape either way.
RED at the merge ref, before the fix
git checkout origin/dev -- tinyagentos/routes/settings.py(dev's version of the file, whichlacks these three keys at the rebuild sites) with this branch's tests:
GREEN with the fix, on the merged branch
test_save_config_preserves_all_to_dict_keysis the class-ender: it compares the fullto_dict()key set against what survives a PUT, so the next field added toAppConfigandforgotten at a rebuild site fails here rather than silently wiping a user's setting.
Summary by CodeRabbit
Bug Fixes
Documentation