feat(sdk): add openshell-sdk crate#1862
Merged
Merged
Conversation
maxdubrinsky
requested review from
a team,
derekwaynecarr and
mrunalp
as code owners
maxdubrinsky
force-pushed
the
md/openshell-sdk-crate
branch
from
aef5832 to
d1668e2
Compare
maxdubrinsky
force-pushed
the
md/openshell-sdk-crate
branch
from
d1668e2 to
d8362c4
Compare
maxdubrinsky
force-pushed
the
md/openshell-sdk-crate
branch
from
d8362c4 to
5d5cfe9
Compare
|
Label |
Collaborator
|
/ok to test 5d5cfe9 |
drew
reviewed
Jun 26, 2026
6 tasks
maxdubrinsky
force-pushed
the
md/openshell-sdk-crate
branch
from
a60554a to
d932183
Compare
maxdubrinsky
changed the title
Additive extraction of the shared async gRPC client core (transport, TLS, OIDC single-flight refresh, edge tunnel, high-level sandbox surface, raw escape hatch) as a new workspace crate. No existing consumers yet; CLI/TUI migration follows in a separate PR. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Extract the signature-unverified JWT exp decode out of openshell-core/grpc_client.rs into openshell_core::jwt::parse_exp_secs, and have the openshell-sdk refresh path reuse it to derive a proactive refresh deadline from a bearer JWT when the caller does not advertise expires_at. Addresses review feedback to reuse pre-existing logic rather than reimplement JWT expiry handling per client. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Addresses review feedback on the openshell-sdk refresh path.
- Make the single-flight cleanup cancellation-safe. The in-flight slot is
now cleared by the shared refresh computation itself (epoch-guarded)
rather than the leader's post-await code. Previously, if the leader future
was dropped (e.g. an FFI caller cancelling its promise) after a follower
drove the refresh to completion, the completed future was stranded in the
slot and later refresh_now() calls re-joined it, pinning the client to a
stale or already-rejected token. Adds a regression test that cancels the
leader and asserts the next refresh starts a fresh attempt.
- Redact bearer secrets from Debug. RefreshedToken and the oidc
RefreshTokenInput/RefreshTokenOutput now use manual Debug impls that omit
the access/refresh token fields via finish_non_exhaustive, matching the
house style (e.g. SecretResolver, SandboxJwtIssuer). Prevents a stray
{:?} or a containing struct's derived Debug from writing tokens to logs.
Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
store_bearer now returns an error instead of silently keeping the previous bearer value. The TokenSource commits the refreshed token to its state before the client writes it into the interceptor slot, so a silent drop left the interceptor on the old (expiring) token with no path back to a refresh. Surfacing the error fails the call loudly instead. Adds a unit test covering a token that can't be encoded as gRPC metadata. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Raw gRPC access never triggered OIDC refresh: a client that only used raw_grpc/raw_inference kept sending the initial bearer until it expired, with no proactive or reactive refresh. Add raw_grpc_fresh and raw_inference_fresh accessors that refresh before returning the client, plus force_refresh for reactive recovery after an Unauthenticated raw RPC. Guard the single-flight refresh commit against a concurrent replace(). The in-flight attempt now records the generation it started from and skips its write when an external replace() has advanced it, so timer or callback driven rotation is no longer clobbered by a slower refresh. Remove TokenSource::snapshot(): it returned an empty string under write contention and had no consumer on the CLI/TUI path. Tests read committed state directly instead. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Recast the openshell-sdk README as a usable crate README rather than an RFC excerpt. Drop the Responsibilities/Non-responsibilities/Consumers scope-boundary sections and the mTLS migration rationale, folding the useful facts (explicit token, no disk/name resolution, Refresh trait, SdkError mapping) into the intro, a new Auth and refresh section, and Public surface. Remove the dead relative RFC link and status-label prose so the doc renders cleanly wherever it is published. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
force-pushed
the
md/openshell-sdk-crate
branch
from
7bfe820 to
ce7ef91
Compare
drew
reviewed
Jul 9, 2026
Collaborator
There was a problem hiding this comment.
Agent review
crates/openshell-sdk/src/client.rs: [nit] Error-variant mappings undermine the crate's own SdkError::code() discriminant contract: wait_ready/wait_deleted timeouts (lines 245, 384) and a sandbox entering the Error phase (line 224) are reported as SdkError::Connect when no connection failed, and sandbox_from_response (line 471) maps a malformed server response to InvalidConfig although the caller's configuration is fine. A binding consumer switching on code() (advertised in error.rs as the stable cross-language discriminant) would misinterpret a provisioning timeout as a connectivity failure. Dedicated variants (e.g. Timeout) or at least Rpc would be more honest.
Apply Drew's review notes on PR #1862: - Drop unused `rustls-pemfile` dependency and move `tokio-stream` to dev-dependencies (only used by tests). - Guard OIDC `expires_at` against u64 overflow with `saturating_add`. - Fix stale `#[non_exhaustive]` rationale in `AuthConfig` (the struct `Oidc` variant it described as future already ships). - Stop double-wrapping refresh errors: store the bare refresh-error text so the single `SdkError::auth` wrap happens once at await. - Strip stale CLI porting breadcrumbs from `build_channel` docs, keeping the branch table. - Treat proactive token refresh as best-effort: a transient failure falls through to the request instead of failing an RPC whose current token is still valid, with a regression test. - Collapse `exec`'s inline auth retry into the shared `unary` helper. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
6 tasks
Collaborator
|
Review follow-up at
|
The refresh single-flight collapsed both `RefreshError::Transient` and `RefreshError::Terminal` into a stringified `SdkError::Auth`, so consumers (CLI, TUI, future language bindings) had no machine-readable way to tell a retryable IdP blip from a dead session that needs re-authentication. Carry the `RefreshError` through the shared outcome (kept `Clone` for `Shared`) instead of its rendered text, and map it at the await site to a new `retryable` flag on `SdkError::Auth`. Add `SdkError::auth_retryable` and a `SdkError::retryable()` accessor; transient refresh failures report `true`, every other error `false`. Add classification tests. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
drew
approved these changes
Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First focused PR split out of #1617. Adds a new
openshell-sdkcrate: the shared async Rust client for OpenShell gateways. It owns gRPC channel construction, TLS material handling, request interceptors, OIDC single-flight token refresh, and the Cloudflare Access edge tunnel, and exposes a sandbox-focused high-level surface plus arawescape hatch.The crate is additive (nothing consumes it yet, so no behavior change). The one non-additive piece is a behavior-preserving extraction: the signature-unverified JWT
expdecoder moves out ofopenshell-core/src/grpc_client.rsinto a sharedopenshell_core::jwtmodule, so the sandbox-side client and the SDK's refresh path derive token expiry from one place instead of each reimplementing it. Movingopenshell-cliandopenshell-tuionto the crate is a separate follow-up PR.Related Issue
openshell-sdk-node(napi binding, Pi example, CI).Changes
crates/openshell-sdk/:OpenShellClient: high-level sandbox surface (health, sandbox CRUD, readiness and deletion waits, non-streaming exec).Unauthenticatedwith one retry), behind a napi-freeRefreshtrait so the core stays binding-agnostic.rawmodule: re-exports the generated tonic clients for RPCs the curated surface does not yet cover.SdkErrortaxonomy and curated types (SandboxSpec,SandboxRef,Health,ListOptions,ExecOptions).README.md.openshell_core::jwt::parse_exp_secs, extracted fromgrpc_client.rs(which now calls it). The SDK reuses it to derive a proactive-refresh deadline from a bearer JWT when the caller does not advertiseexpires_at, so consumers do not reimplement JWT expiry parsing per language.AGENTS.md: architecture-table row for the new crate.Cargo.lock: additive (the crate registers via the existingmembers = ["crates/*"]).No changes to
openshell-clioropenshell-tui. The onlyopenshell-corechange is the JWT-exp extraction above; the transport code the CLI/TUI own today (openshell-core/src/auth.rs,openshell-cli/src/edge_tunnel.rs,tls.rs,oidc_auth.rs) is untouched here and migrates onto the SDK in the follow-up PR, where the now-duplicated code is removed.Testing
cargo build --workspace,cargo clippy --workspace --all-targets -- -D warnings, andcargo fmt --all -- --checkare clean.cargo test -p openshell-sdk: 9 unit + 12 mock-gateway tests pass (incl. single-flight cancellation-safety, Debug-redaction, and token-encoding regression tests).cargo test -p openshell-core: 299 pass, including 3 newjwtdecoder tests. The pre-existinggrpc_clientexpiry tests still pass, confirming the decoder extraction is behavior-preserving.license:checkandmarkdown:lintpass.Checklist
AGENTS.mdarchitecture table and the crateREADME.md.