fix(config): preserve built-in reserved-slug cloud_providers across settings saves#2457
Conversation
…tions - Updated and to silently drop entries with reserved slugs, allowing empty slugs to trigger validation errors. - This change prevents reserved entries from being dropped on save while maintaining explicit error handling for actual frontend issues.
- Enhanced the apply_model_settings function to retain existing reserved-slug entries when updating cloud providers, preventing accidental deletion during routine saves. - Added defensive checks to avoid duplicate entries for reserved slugs from the payload.
…ved cloud provider slugs - Introduced new tests to ensure that existing reserved-slug entries are preserved during model settings updates. - Added checks to prevent double-adding reserved entries when bypassing the schema handler, ensuring data integrity in cloud provider configurations.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRPC handlers now drop reserved cloud-provider slugs from incoming model-settings updates, and apply_model_settings preserves and reinjects any reserved providers already present in stored config; tests, an e2e assertion, and German i18n strings were added. ChangesReserved Cloud Provider Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
- Updated the test to assert that the user's proxy entry survives configuration updates by checking for the presence of the slug rather than relying on the count of cloud providers. - This change ensures that reserved-slug entries are correctly maintained during model settings updates, aligning with recent enhancements to the apply_model_settings function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/openhuman/config/ops.rs (1)
498-513: ⚡ Quick winAdd debug visibility for reserved-provider preservation/reinjection.
This block now performs a non-trivial state transition (
preservedextraction + reinjection). A small debug log with preserved/reinjected counts would make routing regressions easier to diagnose.As per coding guidelines: "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 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 `@src/openhuman/config/ops.rs` around lines 498 - 513, Add a debug log that reports how many reserved providers were pulled out and how many were actually reinjected: after computing let preserved: Vec<_> = ... log the preserved.len(), and after the reinjection loop log how many entries were appended (or the difference between config.cloud_providers length before/after). Use the existing logging crate in the project (e.g. tracing::debug! or log::debug!) and reference the symbols config.cloud_providers, preserved, is_slug_reserved and the surrounding apply_model_settings / reinjection block so it's clear where to insert the two debug statements.src/openhuman/config/schemas.rs (1)
915-927: ⚡ Quick winAdd debug diagnostics for reserved-slug filtering in
update_model_settings.Line 915 introduces a silent behavioral branch that mutates incoming payloads. Add a debug log (count-only) when reserved entries are dropped so save-path behavior is traceable without surfacing user-facing errors.
As per coding guidelines: "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 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 `@src/openhuman/config/schemas.rs` around lines 915 - 927, In update_model_settings, the filter that silently drops reserved slugs should emit a debug-only diagnostic: compute how many incoming entries would be removed by is_slug_reserved (e.g., by iterating once to count reserved entries or comparing lengths before/after filtering), then call the crate's debug/tracing logger (trace/debug) with a concise message like "update_model_settings dropped N reserved slugs" including the count and context (request id or user id if available). Keep behavior unchanged (still drop them) and place the log next to the existing filter block so it runs on the incoming payload before apply_model_settings re-injects stored entries.src/openhuman/inference/schemas.rs (1)
561-573: ⚡ Quick winMirror debug diagnostics for reserved-slug filtering on inference RPC path.
Line 561 adds the same silent filter behavior on a second RPC entrypoint. Emit a debug-level count of dropped reserved entries here too, to keep troubleshooting parity across both handlers.
As per coding guidelines: "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 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 `@src/openhuman/inference/schemas.rs` around lines 561 - 573, The reserved-slug filter is silently dropping entries in the RPC handler; calculate how many non-empty slugs are being dropped (use is_slug_reserved on trimmed slug) immediately before applying the .filter(...) and emit a debug-level log (e.g., tracing::debug! or log::debug!) with that count and brief context so diagnostics match the other RPC entrypoint; keep the filter logic unchanged and reference the same trimmed.is_empty() || !is_slug_reserved(trimmed) check and the surrounding comment about apply_model_settings.
🤖 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.
Inline comments:
In `@src/openhuman/config/ops.rs`:
- Around line 510-511: The duplicate-check is using raw slugs so whitespace
variants slip through; update the comparison to use the same canonical form as
earlier by trimming (and any other normalization used) before comparing—i.e.,
when checking config.cloud_providers.iter().any(|e| e.slug == entry.slug)
normalize both e.slug and entry.slug (trim) so the push of entry only happens if
the trimmed slugs differ.
---
Nitpick comments:
In `@src/openhuman/config/ops.rs`:
- Around line 498-513: Add a debug log that reports how many reserved providers
were pulled out and how many were actually reinjected: after computing let
preserved: Vec<_> = ... log the preserved.len(), and after the reinjection loop
log how many entries were appended (or the difference between
config.cloud_providers length before/after). Use the existing logging crate in
the project (e.g. tracing::debug! or log::debug!) and reference the symbols
config.cloud_providers, preserved, is_slug_reserved and the surrounding
apply_model_settings / reinjection block so it's clear where to insert the two
debug statements.
In `@src/openhuman/config/schemas.rs`:
- Around line 915-927: In update_model_settings, the filter that silently drops
reserved slugs should emit a debug-only diagnostic: compute how many incoming
entries would be removed by is_slug_reserved (e.g., by iterating once to count
reserved entries or comparing lengths before/after filtering), then call the
crate's debug/tracing logger (trace/debug) with a concise message like
"update_model_settings dropped N reserved slugs" including the count and context
(request id or user id if available). Keep behavior unchanged (still drop them)
and place the log next to the existing filter block so it runs on the incoming
payload before apply_model_settings re-injects stored entries.
In `@src/openhuman/inference/schemas.rs`:
- Around line 561-573: The reserved-slug filter is silently dropping entries in
the RPC handler; calculate how many non-empty slugs are being dropped (use
is_slug_reserved on trimmed slug) immediately before applying the .filter(...)
and emit a debug-level log (e.g., tracing::debug! or log::debug!) with that
count and brief context so diagnostics match the other RPC entrypoint; keep the
filter logic unchanged and reference the same trimmed.is_empty() ||
!is_slug_reserved(trimmed) check and the surrounding comment about
apply_model_settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6576f48a-dae8-4f7a-a8f7-98585f91cf6c
📒 Files selected for processing (5)
src/openhuman/config/ops.rssrc/openhuman/config/ops_tests.rssrc/openhuman/config/schemas.rssrc/openhuman/inference/schemas.rstests/json_rpc_e2e.rs
- Added debug logging to track the number of reserved cloud providers preserved before and after updates. - Enhanced the slug comparison logic to ensure trimmed slugs are correctly checked, preventing duplicates during configuration updates.
- Added new translation keys for subconscious provider status and settings in de-3.ts. - Expanded MCP server settings translations in de-5.ts, including configuration options and error messages.
|
@YellowSnnowmann this PR has merge conflicts with main — please rebase/resolve before review. |
2 participants
Summary
handle_inference_update_model_settingsandhandle_update_model_settingsnow silently drop reserved-slugcloud_providersentries (openhuman,cloud,pid) from incoming patches instead of failing the entire request withErr("slug 'X' is reserved...").apply_model_settingsnow preserves reserved-slug entries from the stored config when replacingcloud_providers, so the migration-seeded built-in OpenHuman entry survives every settings save.Problem
The migration
unify_ai_provider_settingsseeds acloud_providersentry withslug: "openhuman"into every user's config — this is the built-in OpenHuman provider and is referenced byprimary_cloudand the per-workload routing fields.The frontend's AI Settings screen uses a read-modify-write pattern: it reads the full
cloud_providerslist (which includes the seeded built-in), the user edits one of their custom providers, and the frontend echoes the whole list back on save.Both schema handlers then ran every incoming entry through
is_slug_reserved():Because the entries were processed via
.collect::<Result<Vec<_>, String>>().transpose()?, a single rejected entry failed the entire RPC call. The user's actual edit (e.g. a new API key) was discarded, andjsonrpc.rsreported the error to Sentry viareport_error_or_expectedsince the message didn't match any expected-error classifier.Result: every save-settings click → one Sentry event + a silently failed user edit. 7,183 events in 14 days, on track to be one of the noisiest issues on the dashboard. Sentry link: https://sentry.tinyhumans.ai/organizations/tinyhumans/issues/45
The system was rejecting data its own migration had seeded.
Solution
Two coordinated changes that together fix the user experience and the Sentry noise without altering the reserved-slug contract:
Schema handlers (
src/openhuman/inference/schemas.rs,src/openhuman/config/schemas.rs) — insert a.filter()step before.map()that drops non-empty reserved slugs silently:The filter is intentionally constructed so empty slugs pass through and still hit the explicit "slug must not be empty"
Errinside.map()— preserving the validation surface for genuinely malformed frontend payloads.apply_model_settings(src/openhuman/config/ops.rs) — before replacingconfig.cloud_providerswith the patch, snapshot any existing reserved-slug entries from the stored config and re-inject them afterwards. Aiter().any(|e| e.slug == entry.slug)guard prevents duplicates if a CLI/test path bypasses the schema filter.Design decisions / tradeoffs
openhumanentry on purpose" from "this came from the read-modify-write cycle." Returning a friendly error per-entry is more disruptive than silently filtering, and the user can't create a meaningfulopenhumanprovider anyway — the slug is reserved. Silent filter wins.apply_model_settings. Putting the preservation logic there means CLI / tests / future RPC paths that build aModelSettingsPatchdirectly all benefit from the same invariant.expected_error_kind? That would silence Sentry but leave the user with a broken save. The real bug is the failing update, not the alert.Submission Checklist
apply_model_settings_preserves_existing_reserved_slug_cloud_providers— happy path: built-in survives a normal saveapply_model_settings_does_not_double_add_reserved_entries— edge case: defensive de-dupdiff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change)N/A: behaviour-only fix to an existing feature row (AI Settings / cloud_providers)## Relateddocs/RELEASE-MANUAL-SMOKE.md)N/A: bug fix on an existing flow, smoke step "Save AI settings without error" already covers the regressionCloses #NNNin the## RelatedsectionImpact
iter().filter().cloned().collect()overcloud_providersper settings-save call. The list is tiny (typically < 10 entries).is_slug_reserved) is unchanged; users still cannot persist a user-controlled provider with a reserved slug. The filter drops such entries rather than allowing them through.unify_ai_provider_settingsmigration (i.e. everyone on a current build).Related
Summary by CodeRabbit
Bug Fixes
Tests
Localization