fix(grpc): allow credential rotation when legacy provider.type exceeds current limit#1350
Conversation
063f82b to
d2fe817
Compare
d2fe817 to
13d9fcd
Compare
13d9fcd to
775a3e7
Compare
|
Label |
Gate Status: Watching PipelineI validated the linked bug report and PR scope: #1347 has a concrete reproduction path, and this PR is a focused server-side fix for provider credential rotation on legacy records with oversized stored Code review of the REST-fetched diff found no blocking implementation defects. After the 2026-06-03 rebase to head Docs check: no Fern docs update is required for this PR because it restores the existing Test label decision: Current status as of 2026-06-04 00:00 UTC:
I will keep watching this PR every 15 minutes until it closes, merges, or the sandbox session is stopped. |
/ok to test 775a3e7 |
|
/ok to test 775a3e7 |
|
@latenighthackathon this def needs to be re-based to get tests passing based on some recent changes |
…s current limit Closes NVIDIA#1347. Reported by @KodeDaemon in the NVIDIA Developer Discord after upgrading NemoClaw 0.0.38 -> 0.0.39 left the inference provider stranded with `provider.type exceeds maximum length (79 > 64)` on every credential update. `update_provider_record` rebuilt the merged Provider from `existing.r#type` and ran `validate_provider_fields` over it. The CLI's `provider update` sends `r#type: ""`, so the existing value is preserved without modification, but the validator still measured it. Any record whose stored type predates current MAX_PROVIDER_TYPE_LEN (or was written by a path that bypassed validation, e.g. the TUI form which forwards r#type to the request without going through normalize_provider_type) became unupdateable: the only escape was `provider delete` + recreate, which loses any provider.config entries the caller never sees. Split the validator into create-time (validate_provider_fields, full check including immutable name/type) and update-time (validate_provider_mutable_fields, checks only credentials/config maps). The update path validates only what the caller is mutating; immutable fields carried forward from existing are trusted because they passed validation when stored. Tests: new regression in `update_provider_allows_credential_rotation_on_legacy_oversized_type` inserts a Provider with a 79-char type directly via `store.put_message` (bypassing gRPC validation), updates a credential, asserts the update succeeds and the original type is preserved. Existing `update_provider_validates_merged_result` still covers the case where incoming credential entries push the merged map past limits. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
775a3e7 to
311b12d
Compare
Summary
openshell provider update <name> --credential ...re-validates the immutabletypefield on every call, stranding any legacy record whose stored type exceeds the currentMAX_PROVIDER_TYPE_LEN(64). Split provider validation into create-time and update-time variants so the update path only validates fields the caller is mutating (credentials,config); immutable name/type are carried forward from the existing record without re-checking length.Related Issue
Closes #1347.
Reported by @KodeDaemon in the NVIDIA Discord community after upgrading NemoClaw 0.0.38 to 0.0.39:
openshell provider listconfirmed an existing inference provider had a 79-charactertypevalue. NemoClaw's onboard re-runsopenshell provider update inference --credential ...on every upgrade, and that path failed becauseupdate_provider_recordrebuilt the merged Provider fromexisting.r#typeand ran the fullvalidate_provider_fieldsover it, even though the caller only touches credentials. The CLI'sprovider updatesendsr#type: "", so the existing value was preserved without modification, but the validator still measured it.Two ways legacy data ends up oversized: (1) records that predate
validate_provider_fields(added by #145, later split out in #777); (2) write paths that bypassnormalize_provider_type, e.g. the TUI form at crates/openshell-tui/src/lib.rs:1563 which forwardsr#typeto the request directly. Either way, the only escape wasprovider delete+ recreate, which loses anyprovider.configentries the caller never sees.Changes
crates/openshell-server/src/grpc/validation.rs:validate_provider_fieldskeeps the full create-time check (name + type + credentials + config), now delegating the map checks to a new helper.validate_provider_mutable_fieldsvalidates onlycredentialsandconfig; used by the update path.crates/openshell-server/src/grpc/provider.rs:update_provider_recordcallsvalidate_provider_mutable_fieldsinstead ofvalidate_provider_fields. Immutable name/type carried forward fromexistingare trusted because they passed validation when stored.Testing
cargo test -p openshell-server --lib grpc::provider(32 passed including new regression)cargo test -p openshell-server --lib grpc::validation(76 passed)cargo clippy -p openshell-server --all-targets -- -D warnings(clean)cargo fmt --all -- --check(clean)New regression
update_provider_allows_credential_rotation_on_legacy_oversized_typeinserts a Provider with a 79-character type directly viastore.put_message(bypassing gRPC validation, simulating a legacy record), then callsupdate_provider_recordto rotate a credential and asserts both that the update succeeds and the original 79-character type is preserved on the stored record. Existingupdate_provider_validates_merged_resultcontinues to assert that an oversized incoming credential key is still rejected, so incoming-mutation validation is unchanged.Checklist
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com