Skip to content

feat(providers): support profile updates#1914

Open
johntmyers wants to merge 3 commits into
mainfrom
feat/1881-provider-profile-update/johntmyers
Open

feat(providers): support profile updates#1914
johntmyers wants to merge 3 commits into
mainfrom
feat/1881-provider-profile-update/johntmyers

Conversation

@johntmyers

Copy link
Copy Markdown
Collaborator

🏗️ build-from-issue-agent

Summary

Add safe custom provider profile updates through a new UpdateProviderProfiles RPC and openshell provider profile update. The update path validates profile batches before writing, preserves stored custom profile metadata, rejects built-in and missing profiles, and keeps provider-derived policy JIT-composed from current profiles.

Related Issue

Closes #1881

Changes

  • proto/openshell.proto: Added UpdateProviderProfiles RPC and request/response messages.
  • crates/openshell-server/src/grpc/provider.rs: Added custom profile update handling with validation, metadata preservation, built-in/missing rejection, and attached-sandbox dynamic token grant ambiguity checks.
  • crates/openshell-server/src/grpc/policy.rs: Added tests proving updated profile policy reaches sandbox effective config without rewriting provider instances or persisted sandbox source policy, and that profile changes affect provider env revision.
  • crates/openshell-cli/src/main.rs, crates/openshell-cli/src/run.rs: Added openshell provider profile update -f|--from.
  • CLI/server test mocks: Added the new RPC method to test OpenShell implementations and extended provider profile CLI lifecycle coverage.
  • docs/sandboxes/providers-v2.mdx, docs/sandboxes/manage-providers.mdx: Documented custom profile update semantics and rollout behavior.

Deviations from Plan

The plan preferred all-or-none server-side batch updates if cleanly supported. The existing persistence API does not provide transactional multi-object CAS, so this implementation validates the full batch before writes and documents the remaining concurrent-write/storage-error retry behavior instead of adding a broad transaction layer.

Testing

  • cargo test -p openshell-server update_provider_profile -- --nocapture
  • cargo test -p openshell-server sandbox_config_uses_updated_custom_provider_profile -- --nocapture
  • cargo test -p openshell-server provider_env_revision_changes_when_custom_profile_token_grant_changes -- --nocapture
  • cargo test -p openshell-cli provider_profile_commands_parse -- --nocapture
  • cargo test -p openshell-cli provider_profile_cli_run_functions_support_custom_profiles -- --nocapture
  • mise run pre-commit
  • E2E skipped: no e2e/ files changed

Tests added:

  • Unit: Provider profile update handler tests for success, built-in/missing rejection, metadata preservation, and dynamic-token ambiguity rejection.
  • Integration: Policy/config and CLI integration tests for updated profile effects and CLI lifecycle.
  • E2E: N/A.

Checklist

  • Follows Conventional Commits
  • Commit is signed off (DCO)
  • Documentation updated

Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jun 15, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator Author

gator-agent

PR Review Status

Validation: Project-valid. PR #1914 implements approved provider profile update work from #1881, is authored by a repo admin, is non-draft, and DCO/branch checks are passing.
Head SHA: 94022415e4e9122693afb17ad17c42b0cb754edf

Review findings:

  • proto/openshell.proto:1112 / crates/openshell-server/src/grpc/provider.rs:1370: the update RPC has no client-supplied resource_version, so stale local profile files can silently overwrite a newer profile. The handler fetches the current object and then matches that current version, which only catches a narrow race between fetch and write. Please expose profile metadata/resource versions on get/list or add an update item with expected_resource_version, then pass that value into WriteCondition::MatchResourceVersion.
  • crates/openshell-server/src/grpc/provider.rs:1366: multi-profile updates validate the batch, then write profiles one by one. If a later write fails from conflict or storage error, earlier profiles remain updated and the client receives an error without structured partial-state details. Please add transactional batch support for this path or reject multi-profile updates until atomic batch writes exist.

Docs: Fern docs were updated in existing provider pages; no navigation change is needed.

Next state: gator:in-review

Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
@johntmyers

Copy link
Copy Markdown
Collaborator Author

gator-agent

Re-check After Author Update

I re-evaluated latest head b22db325ad3c9efed935e8b70220df8f1fe18ef2 after the follow-up commit fix(providers): require CAS for profile updates pushed on 2026-06-15.

Disposition: partially resolved.

Resolved items:

  • The stale-overwrite concern is resolved in shape: update now requires a non-zero custom profile resource_version and writes with WriteCondition::MatchResourceVersion.
  • The partial multi-profile batch concern is resolved in shape: update is now a single-profile RPC/CLI operation, so there is no sequential multi-profile partial-write state.

Remaining items:

  • crates/openshell-server/src/grpc/provider.rs: profile update still validates dynamic token grant ambiguity against a snapshot of attached sandboxes, then writes the profile without holding the sandbox sync guard. A concurrent sandbox attach can validate against the old profile while the profile update validates against the old sandbox attachment set, leaving an ambiguous final dynamic-token state. Please serialize the update validation/write path with the same sandbox sync guard used by sandbox attach/detach, and hold it through the put_if write.
  • crates/openshell-cli/tests/provider_commands_integration.rs: the CLI lifecycle test manually injects resource_version from test state instead of exercising the documented export-edit-update workflow. Please add or adjust coverage so it imports a profile, exports YAML with the hydrated resource_version, edits that exported file while preserving the version, then runs provider profile update.

Docs: Fern docs are updated in the existing provider pages; no navigation change appears needed.

E2E: pending. This PR touches provider credential/policy behavior, so test:e2e should be applied once review feedback is resolved and the PR moves to pipeline watch.

Next state: gator:in-review

Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
@johntmyers

Copy link
Copy Markdown
Collaborator Author

gator-agent

Re-check After Author Update

I re-evaluated latest head f0209f9a0e929917f5c4d1a5679ab0b0cfcb6e66 after the follow-up commit fix(providers): serialize profile update validation pushed on 2026-06-15.

Disposition: partially resolved.

Resolved items:

  • The profile update path now holds sandbox_sync_guard while checking attached-sandbox dynamic token grant ambiguity and through the CAS write.
  • The CLI lifecycle coverage now exercises the documented export-edit-update workflow by exporting YAML with resource_version, editing it, and running provider profile update.

Remaining items:

  • crates/openshell-server/src/grpc/provider.rs / crates/openshell-server/src/grpc/sandbox.rs: the dynamic token-grant ambiguity invariant is still TOCTOU-prone because ImportProviderProfiles and CreateSandbox with initial providers are not under the same serialization guard. A racing sandbox create can validate providers against the old profile while a profile import/update scans the old sandbox set and writes a profile, leaving an ambiguous final token-grant state. Please put every writer participating in this invariant under the same guard through validation plus persistence, including provider profile import and sandbox creation with initial providers, and add regression coverage for the overlap.
  • crates/openshell-cli/src/main.rs / crates/openshell-cli/src/run.rs: this PR appears to remove openshell provider list -o json|yaml and the structured provider-list output path. That looks unrelated to provider profile updates and regresses machine-readable provider listing. Please restore the output argument and structured output path, or split that removal into a separate maintainer-approved UX change.

Docs: Fern docs are updated in the existing provider pages; no navigation change appears needed.

E2E: still pending. This PR touches provider credential/policy behavior, so test:e2e should be applied once review feedback is resolved and the PR moves to pipeline watch.

Next state: gator:in-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(providers): support safe custom provider profile updates

1 participant