fix(cli): preserve provider type on credential updates

[natedemoss]

Summary

provider_update built every UpdateProviderRequest with r#type: String::new() and profile_workspace: String::new(). Policy interceptors evaluate the request before the gateway merges it with stored state, so they cannot tell the target is a profile-managed provider and deny credential-only updates. Creating a provider from an interceptor-vended profile succeeds; rotating its credentials fails.

This looks up the stored provider and carries its type and profile workspace into the request.

Related Issue

Refs #2991. That issue is state:validated rather than state:accepted, so this is submitted under the obvious-localized-bug-fix exemption rather than as a claim on accepted work. Closing it is a maintainer's call, not mine, and there is a design question below worth deciding first.

Changes

• Look up the stored provider once in provider_update and forward its r#type and profile_workspace on the request.
• Forward profile_workspace verbatim rather than recomputing it. The gateway treats the field as immutable, so substituting the request workspace for an empty stored value would look like a change and be rejected.
• Make the lookup best-effort: a PermissionDenied read falls back to the previous empty-metadata behavior.
• Remove a duplicate GetProvider call. The old code fetched the same provider separately in the --from-oidc-token and --from-existing branches; both now share the single lookup.
• Add an integration test asserting a readable provider carries its stored type into the update request, and that profile_workspace matches the stored value rather than a derived one.

Review feedback addressed

An earlier revision of this branch derived profile_workspace through a helper that substituted the request workspace when the stored value was empty. Copilot flagged that this could trip the gateway's immutability check and make a rotation fail harder than before the fix. The helper is gone; the stored value is now forwarded with no conditional, so the substitution is structurally impossible rather than merely untested. The integration test also asserts profile_workspace now, not just r#type.

This branch was also rebased after #2962 landed, which rewrote both fetch sites in this function to use the raw stored profile_workspace. The fix follows that convention rather than reintroducing the older fallback.

Testing

• mise run pre-commit passes
• Unit tests added/updated
• E2E tests added/updated (if applicable)

mise and Docker are not installed on this workstation, so I ran the pre-commit steps individually:

• cargo fmt --all -- --check clean.
• cargo test -p openshell-cli 410 passed, 0 failed. That includes the new integration test and the existing permission test, unchanged.
• cargo clippy -p openshell-cli --all-targets -- -D warnings clean for this crate, with -A clippy::unused_async for the Windows-only connect_unix stub in openshell-extension-core. The repository already allows that lint on Windows in tasks/scripts/windows-msvc.ps1.

Not verified: I have not run this against a real gateway with a policy interceptor. The evidence here is the mock server in the integration harness plus the code path, not a reproduction of the original APF denial. Someone with an interceptor-backed profile should confirm the rotation now authorizes.

One behavior note: a credential-only update now issues one GetProvider before the update when the caller can read. Callers who cannot read are unaffected.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable)

---

AI assistance: an agent traced the request construction, wrote the fix and tests, and ran the checks. I reviewed it and can explain it. The defect is the two hardcoded empty strings in the UpdateProviderRequest built by provider_update; the constraint that shapes the fix is the write-without-read path the existing integration test pins.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Copilot]

🟡 Changes recommended

The new profile_workspace forwarding path can rewrite an empty stored profile_workspace to the request workspace and cause the gateway to reject the update as an immutable-field change.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes openshell-cli provider credential updates so that UpdateProviderRequest can include the target provider’s stored identity metadata (provider type and profile_workspace) when the caller is allowed to read the provider, enabling policy interceptors to authorize the mutation before the gateway merges stored state.

Changes:

• Adds a best-effort GetProvider in provider_update to populate UpdateProviderRequest.provider.{type,profile_workspace} when readable, while preserving the write-without-read credential-rotation path by falling back on PermissionDenied.
• Removes duplicated GetProvider calls in --from-existing / --from-oidc-token branches and factors provider_profile_workspace(...) into a helper with unit tests.
• Adds an integration test that asserts a readable provider’s stored type is carried into the update request.

File summaries

File	Description
crates/openshell-cli/src/run.rs	Looks up the stored provider in provider_update and forwards identity metadata on update requests; adds helper + unit tests.
crates/openshell-cli/tests/provider_commands_integration.rs	Adds an integration test that inspects the update request constructed by provider_update.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The fix is localized, matches the stated interceptor-authorization constraint, and adds coverage; only a minor test naming clarity nit was noted.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[johntmyers]

/ok to test 2ce827f

[github-actions]

Label test:e2e applied for 2ce827f. Open Branch E2E Checks, find the run for commit 2ce827f, and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

[johntmyers]

gator-agent

PR Review Status

This focused fix is project-valid through validated bug #2991. The current head correctly carries the stored provider type and profile workspace into readable credential updates while retaining the existing write-without-read fallback. Thanks @natedemoss—I checked the empty stored profile_workspace concern you acknowledged, and the request construction now clones the stored value verbatim. No blocking findings remain.

Blocking findings:

• No blocking findings remain

Carried findings:

• None

Non-blocking suggestions:

• Strengthen the new regression test with a provider whose stored profile_workspace is empty. The current fixture stores default, so substituting the request workspace in a future regression would still satisfy the assertion.

Gator metadata

• Validation: Focused implementation of validated provider-update bug #2991; no duplicate work found
• Docs: Not needed because this repairs internal request identity metadata without changing the documented CLI workflow
• Checks: DCO is green; current-head Branch Checks and Helm Lint are queued, and required E2E is running
• E2E: test:e2e applied; /ok to test 2ce827fbd99f2125d1fc1b272bca99abd2f6c5af posted; Branch E2E Checks run 33575920136 is in progress
• Head SHA: 2ce827fbd99f2125d1fc1b272bca99abd2f6c5af
• Base SHA: 5b925dd8afba6ba558e93803656e680b744769d7
• Merge base SHA: 07453f29fe8a6a93667ec256dabc5d40761a3e59
• Patch ID: 790a49bd6082621f4bb7040216672ad000c03a85
• Gator payload: 8
• Review mode: initial
• Previous reviewed SHA: none
• Review budget exhausted: no
• Maintainer decision required: no
• Next state: gator:watch-pipeline

[natedemoss]

This error looks unrelated to my change.

Failing test is sandbox_stop_start_preserves_workspace (e2e/rust/tests/sandbox_lifecycle.rs:125):

sandbox start should succeed (exit Some(1)):
Error:   × ContainerExited: Container exited while waiting for Ready

Container died on the stop/start cycle. This PR only changes how provider update builds its UpdateProviderRequest. It doesn't touch sandbox, compute, or driver code, and nothing in the rust e2e suite calls provider update. Rest of the suite passed.

Can rebase if you'd rather rule it out that way, I just didn't want to push and invalidate the /ok to test on 2ce827f. A re-run should confirm.