Skip to content

feat(workspace): add workspace resource model with scoping, membershi…#2243

Draft
derekwaynecarr wants to merge 22 commits into
NVIDIA:mainfrom
derekwaynecarr:decarr/workspace-model
Draft

feat(workspace): add workspace resource model with scoping, membershi…#2243
derekwaynecarr wants to merge 22 commits into
NVIDIA:mainfrom
derekwaynecarr:decarr/workspace-model

Conversation

@derekwaynecarr

@derekwaynecarr derekwaynecarr commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements Phase 1 of RFC 0011 — the workspace and membership model that provides hard isolation boundaries for multi-player OpenShell deployments.

  • Workspace resource: CRUD RPCs (CreateWorkspace, GetWorkspace, ListWorkspaces, DeleteWorkspace) with a default workspace created on gateway startup for backwards compatibility. Deletion blocked when workspace contains active sandboxes, providers, service endpoints, policies, settings, SSH sessions, provider refresh state, or draft chunks. No UpdateWorkspace RPC — workspaces are immutable after creation.
  • Workspace-scoped resources: Workspace field on ObjectMeta — sandboxes, providers, service endpoints, SSH sessions, policies, settings, provider refresh state, and inference routes all inherit workspace from context. In-memory SandboxIndex keyed by (workspace, name) to match persistence layer scoping.
  • Membership model: AddWorkspaceMember, RemoveWorkspaceMember, ListWorkspaceMembers RPCs with (workspace, principal_subject) → role records; membership records cleaned up on workspace deletion.
  • Persistence: Name uniqueness shifts from (object_type, name) to (object_type, workspace, name) via migration 006; existing resources backfilled to default workspace; cross-workspace list_by_type store method for infrastructure operations (reconciler, resume, provider refresh). New delete_by_scope store method for sandbox-owned record cleanup.
  • Provider profiles: Two-tier scoping — platform-scoped profiles (workspace="") are shared across all workspaces; workspace-scoped profiles are isolated per workspace. Independent scope listing with no merged cross-scope view. resolve_profile_workspace preserves empty string for platform scope while resolve_workspace normalizes to "default". Platform-profile attachment checks scan all workspaces to detect consumers. Scope-mismatched providers contribute their real token-grant bindings for ambiguity detection instead of being skipped or matched against the wrong profile scope.
  • Service routing: Endpoint hostnames use {workspace}--{sandbox}--{service}.{domain} format for all workspaces including default. Workspace, sandbox, and service names capped at 19 characters to guarantee the combined DNS label fits within the 63-character RFC 1035 limit.
  • Inference routes: Rename Cluster* RPCs and messages to Route* (SetInferenceRoute, GetInferenceRoute, InferenceRouteConfig); workspace-scoped route storage and lookup; derive workspace from sandbox principal for bundle resolution; thread --workspace through CLI inference commands.
  • ObjectWorkspace trait: requires_workspace() method with debug_assert! validation in store write helpers to catch empty-workspace bugs in debug builds.
  • Sandbox lifecycle: Sandbox deletion cleans up all owned records (SSH sessions, service endpoints, settings, policy revisions, draft chunks) and cleanup errors are fatal — sandbox deletion aborts if owned records cannot be removed, preventing orphan records from blocking workspace deletion.
  • Sandbox runtime: Workspace propagated from gRPC client via workspace_tx watch channel in steady-state poll loop; SSH session revocation writes back session workspace. Workspace watch channel shared with PolicyLocalContext so policy.local API clients (SubmitPolicyAnalysis, GetDraftPolicy) send the correct workspace for non-default workspace sandboxes. Denial and activity flush tasks gated on non-empty workspace to prevent silent data loss during initial settings poll race.
  • VM driver: sandbox_snapshot propagates workspace so watch events round-trip the correct workspace back to the server.
  • K8s driver: sandbox_from_object gates on managed-by=openshell label — CRs written by other actors in the namespace are silently skipped. Managed CRs missing required fields (orphans) log a warning and are skipped rather than hard-erroring. Label selector added to list and watch ListParams so unmanaged CRs are filtered at the K8s API level. is_openshell_managed helper centralizes the ownership check.
  • CLI: --workspace and --all-workspaces flags on all resource commands; --all-workspaces intentionally takes precedence over --workspace (rather than erroring) because --workspace can be set via OPENSHELL_WORKSPACE env var. WORKSPACE column as first column in list outputs when --all-workspaces (matching kubectl --all-namespaces convention); -o json/-o yaml output always includes workspace field; --names output uses workspace/name format under --all-workspaces (matching kubectl -o name --all-namespaces). Workspace CRUD and membership subcommands; inference help text updated from "gateway-level" to "workspace-level". Last-used sandbox fallback keyed by (gateway, workspace) — saves workspace alongside sandbox name and only matches the requested workspace, preventing cross-workspace mis-targeting.
  • TUI: Workspace state with [w] key cycling, WORKSPACE column in sandbox/provider tables, workspace indicator in title bar, workspace-scoped gRPC requests. Initial workspace set from CLI --workspace / OPENSHELL_WORKSPACE (no longer hardcoded to "default"). Selection indices reset on workspace cycle. Post-create exec uses the creation workspace (carried in CreateResult) instead of the selected row's workspace. cycle_workspace resets current_workspace to "default" when entering all-workspaces mode to prevent silent cross-workspace creates.
  • Python SDK: Workspace parameter on all SandboxClient methods (create, get, list, delete, wait_deleted, exec, forward_tcp, create_ssh_session); InferenceRouteClient with workspace-scoped get/set; Sandbox context manager accepts workspace and syncs to the session's resolved workspace after creation (so wait_ready/wait_deleted use the correct workspace when attaching via SandboxRef).

Breaking changes

  • Service endpoint hostnames now include workspace prefix for all workspaces: {workspace}--{sandbox}.{domain} (previously {sandbox}.{domain}). The default workspace does not preserve the old format.
  • Workspace, sandbox, and service names limited to 19 characters (previously 253 for sandbox, 28 for service in routing context).
  • Podman container labels add openshell.ai/sandbox-workspace label for workspace tracking. This is intentional — pre-existing containers from before the upgrade will not be visible to the watcher and require a clean shutdown before upgrade.
  • SSH config host aliases use format openshell-{name}.{workspace} (previously openshell-{name}). Old entries are not automatically cleaned up.
  • Python SDK methods now require workspace keyword argument with no default — this is intentional to make workspace selection explicit. The CLI defaults to "default" via --workspace, but the SDK requires callers to be explicit about which workspace they operate in.
  • Inference RPCs renamed: SetClusterInferenceConfigSetInferenceRoute, GetClusterInferenceConfigGetInferenceRoute, ClusterInferenceConfigInferenceRouteConfig. This is a wire-breaking change — the inference route API is pre-GA with no deployed consumers of the old RPC names.
  • Last-used sandbox file format changed from single-line sandbox name to two-line workspace\nname. Old single-line files are ignored on load (user re-selects on first use after upgrade).

Related Issue

#1977

Changes

  • 83 files changed, +10333 / -1548 lines
  • New files: workspace.rs (gRPC handlers + tests), sandbox_index.rs updates, migration 006 (sqlite + postgres), workspace_lifecycle.rs (e2e test), RFC 0011
  • Proto: Workspace, WorkspaceMember messages; workspace CRUD and membership RPCs; workspace field on ObjectMeta; all_workspaces on list requests; inference route rename (Cluster* → Route*) with workspace fields on set/get request/response messages

Testing

  • 924 server unit tests passing (34 workspace-specific, including profile scope isolation and inference route workspace isolation)
  • 832 supervisor-network tests passing (33 policy_local tests including workspace receiver plumbing)
  • 72 bootstrap tests passing (13 last_sandbox tests including workspace-scoped isolation)
  • 28 TUI tests passing
  • 78 Python e2e tests passing — includes workspace lifecycle, profile platform-vs-workspace scope isolation, inference route renames, sandbox CRUD with workspace args
  • Rust e2e workspace lifecycle test covering create/list/get/delete and cross-workspace isolation

Known remaining work (Phase 2)

  • Authorization enforcement: all_workspaces list authz, workspace membership checks on resource access, data-plane workspace verification on watch/exec/forward/SSH. Current isolation is name-based — any authenticated user who knows a workspace name can operate in it, matching the pre-workspace security posture where all authenticated users see everything.
  • Expanded role model: Platform Admin / Workspace Admin / User role definitions and enforcement
  • TODO(phase2) markers in code at each gap

Non-blocking follow-ups noted in review (no action needed for this PR)

  • Workspace records keyed by name with no UUID tie (orphan rebind on recreate — worth revisiting before Phase 2 makes membership records authorization data)
  • TUI workspace unit tests re-implement logic on local arrays (can't catch regressions in actual methods)
  • CLI --workspace defaults to "default" client-side; consider removing the default and relying on server-side resolve_workspace normalization, with client-side operations (last-sandbox, completers) resolving workspace from server responses
  • OCSF audit events do not include workspace — service routing events (endpoint config, HTTP rejection, relay failure) have workspace available but don't emit it. Add .unmapped("workspace", ...) as part of a broader workspace attribution pass across all OCSF event sites
  • Inference routes reference providers by name with no delete guard — deleting and recreating a provider silently rebinds inference traffic (pre-existing, not introduced by workspace changes)
  • Provider deletion doesn't acquire sandbox_sync_guard — attach racing delete can leave a dangling by-name attachment (pre-existing TOCTOU)
  • cleanup_sandbox_ssh_sessions reads a single page of 1000 with no pagination loop

Checklist

  • Code compiles without warnings (clippy clean)
  • All 924 server unit tests pass (0 failures)
  • All 78 Python e2e tests pass
  • New tests added for workspace isolation (Rust handler + Python e2e)
  • E2e tests updated for workspace and inference route API renames
  • Sandbox cleanup is fatal — orphan records cannot block workspace deletion
  • Platform-profile attachment checks scan all workspaces
  • Provider-profile scope filters symmetric for both platform and workspace scope
  • DNS label overflow prevented by 19-char name cap
  • SandboxIndex keyed by (workspace, name) — no cross-workspace collisions
  • Default workspace auto-created on gateway startup (handles post-migration path)
  • Policy.local API sends correct workspace for non-default workspace sandboxes
  • Flush tasks gated on non-empty workspace (no silent data loss during poll race)
  • TUI post-create exec uses creation workspace, not selected row
  • CLI last-used sandbox keyed by (gateway, workspace)
  • RFC 0011 documents design
  • Migration tested with SQLite and PostgreSQL schemas
  • K8s driver filters unmanaged sandbox CRs via managed-by label selector and in-code gate
  • VM driver round-trips workspace in sandbox snapshots
  • Service endpoints cleaned up on sandbox delete
  • TUI respects --workspace / OPENSHELL_WORKSPACE
  • Python SDK Sandbox context manager syncs workspace from session

@derekwaynecarr
derekwaynecarr requested review from a team, maxamillion and mrunalp as code owners July 13, 2026 12:23
@copy-pr-bot

copy-pr-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown

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.

@derekwaynecarr
derekwaynecarr marked this pull request as draft July 13, 2026 12:26
@derekwaynecarr
derekwaynecarr force-pushed the decarr/workspace-model branch 2 times, most recently from 2468c04 to 77ef1fc Compare July 13, 2026 22:31
Thread workspace through compute drivers (Docker, Podman, K8s) with
converged container naming (openshell-{workspace}--{name}-{id}),
workspace labels, and label-based lookup. Fix sandbox-side policy sync
to use learned workspace instead of hardcoding empty string. Fix TUI
all-workspaces view to use per-row workspace for sandbox actions. Add
provider profile to workspace deletion blocking check. Thread workspace
through SSH config generation with workspace-qualified host aliases.

Signed-off-by: Derek Carr <decarr@redhat.com>
Add workspace as a required parameter for resource-scoped operations
(create, get, delete, wait_ready, wait_deleted) and optional for
list operations that support all_workspaces global lookup. Rename
ClusterInferenceConfig to InferenceRouteConfig and set_cluster/get_cluster
to set_route/get_route to match the renamed proto RPCs.

Signed-off-by: Derek Carr <decarr@redhat.com>
@derekwaynecarr
derekwaynecarr force-pushed the decarr/workspace-model branch from 77ef1fc to 7bba756 Compare July 14, 2026 18:18
Comment thread crates/openshell-server/src/grpc/workspace.rs
Comment thread crates/openshell-server/src/grpc/mod.rs
- Backfill provider_credential_refresh_state in workspace migration
- Set requires_workspace() to true for refresh state
- Reject consecutive hyphens in workspace and sandbox names
- Restore parse_host rejection of consecutive hyphens in all segments
- Pass workspace from SSH session metadata in put_if call
- Require explicit workspace in Python SDK Sandbox() constructor
- Add sandbox name character validation matching workspace rules

Signed-off-by: Derek Carr <decarr@redhat.com>
- Add watch channel to broadcast workspace from policy poll loop to
  denial and activity flush tasks, fixing proposals targeting the
  wrong workspace for non-default sandboxes
- Add set_workspace() to CachedOpenShellClient for pre-seeding
  workspace on fresh connections
- Use selected_provider_workspace() in TUI provider get/delete actions
  instead of current_workspace, fixing cross-workspace misrouting

Signed-off-by: Derek Carr <decarr@redhat.com>
Signed-off-by: Derek Carr <decarr@redhat.com>
…le scope binding

Provider profiles can be global (platform-scoped) or workspace-scoped.
Previously, profile lookups used the ambient request workspace, which
caused pre-existing global profiles (workspace="") to become invisible
after the workspace migration.

Add a `profile_workspace` field to the Provider proto that explicitly
declares where the provider's type profile is stored. The field must be
empty (global) or match the provider's own workspace — cross-workspace
references are rejected. All profile lookup call sites now use
`provider.profile_workspace` instead of the ambient workspace.

Existing serialized providers get `profile_workspace=""` via protobuf
defaults, correctly pointing to their global profiles with no migration.

CLI gains `--global-profile` flag on `provider create`.

Signed-off-by: Derek Carr <decarr@redhat.com>
…-back

handle_revoke_ssh_session passed empty string for workspace in put_if
instead of session.object_workspace(), dropping the workspace on the
revoked session record.

Signed-off-by: Derek Carr <decarr@redhat.com>
The lint handler was passing an empty string to profile_conflict_diagnostics
instead of using the request workspace, so it only checked for conflicts in
global scope rather than the workspace the user specified.

Signed-off-by: Derek Carr <decarr@redhat.com>
- Fix proto comments: ListProviderProfilesRequest.workspace no longer
  claims two-tier merged resolution, LintProviderProfilesRequest.workspace
  now documents that it is used for conflict detection
- Converge Podman driver labels to openshell.ai/ prefix by importing
  from driver_utils, matching Docker and Kubernetes drivers
- Replace debug_assert with runtime PersistenceError for workspace
  enforcement in put_message, put_scoped_message, and cas_update_message
- Add comment in parse_host explaining -- delimiter safety invariant
- Read OPENSHELL_WORKSPACE env var in tab completers instead of
  defaulting to empty string
- Remove unused _workspace params from provider_refresh_defaults and
  dynamic_token_grant_bindings_for_provider
- Remove scattered workspace "default" fallbacks in service_routing,
  inference, and compute; normalization is handled by resolve_workspace
  at the handler layer

Signed-off-by: Derek Carr <decarr@redhat.com>
- Backfill policy and draft_chunk object types in migration
- Add label selector support to all_workspaces sandbox listing
- Replace per-member deletion loop with batch delete_all_in_workspace
- Extract shared validate_dns1123_label helper from duplicated validation
- Add count_in_workspace for efficient member cap enforcement
- Document intentional omission of workspace filter in list_by_scope

Signed-off-by: Derek Carr <decarr@redhat.com>
Fix workspace-scoped cleanup bugs: settings deletion used empty workspace
instead of sandbox workspace, migration backfill missed sandbox_settings,
SSH session cleanup scanned globally instead of by workspace, and workspace
deletion did not check for blocking sandbox_settings records. Add K8s
resource name length validation to reject names exceeding DNS-1123 63-char
limit. Update RFC 0011 to match Phase 1 implementation: document inference
RPC rename, UpdateProviderProfiles with optimistic concurrency, Provider
profile_workspace field, Python SDK workspace requirements, service hostname
breaking change, and persistence validation behavior.

Signed-off-by: Derek Carr <decarr@redhat.com>
Fix bugs found during PR NVIDIA#2243 review: correct migration type strings,
resolve platform-scoped profile workspace handling, fix sandbox
workspace_tx single-fire, add missing workspace deletion blockers,
reset TUI row indices on workspace switch, fix TUI provider update
workspace, and update e2e Python tests for workspace and inference
route API changes. Add Phase 2 TODO comments for authorization gaps.
Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss,
iter_on_single_items).

Signed-off-by: Derek Carr <decarr@redhat.com>
…ace args

Add Rust handler test and Python e2e test verifying that platform-scoped
and workspace-scoped provider profiles are isolated from each other. Fix
missing workspace keyword arguments in test_sandbox_api.py and
test_sandbox_providers.py that caused TypeErrors.

Signed-off-by: Derek Carr <decarr@redhat.com>
Fix reconciler test that stored sandbox settings with workspace "" while
the sandbox itself used "default", causing the cleanup delete to miss the
record. Fix e2e tests missing the required workspace keyword argument on
SandboxClient.create, .get, .delete, and .wait_deleted calls.

Signed-off-by: Derek Carr <decarr@redhat.com>
rhuss pushed a commit to rhuss/OpenShell that referenced this pull request Jul 16, 2026
Fix bugs found during PR NVIDIA#2243 review: correct migration type strings,
resolve platform-scoped profile workspace handling, fix sandbox
workspace_tx single-fire, add missing workspace deletion blockers,
reset TUI row indices on workspace switch, fix TUI provider update
workspace, and update e2e Python tests for workspace and inference
route API changes. Add Phase 2 TODO comments for authorization gaps.
Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss,
iter_on_single_items).

Signed-off-by: Derek Carr <decarr@redhat.com>
…label limits

Make sandbox owned-record cleanup fatal: policy revisions, draft chunks,
SSH sessions, and settings must all be deleted before the sandbox itself
is removed. Previously errors were swallowed as warnings, leaving orphan
records that permanently blocked workspace deletion.

Fix platform-profile attachment checks to scan sandboxes across all
workspaces. Previously scan_sandboxes queried workspace="" which returned
zero results since sandboxes always live in named workspaces. Platform
profile updates and deletions now correctly detect consumers.

Cap workspace, sandbox, and service names at 19 characters to guarantee
the combined DNS label (workspace--sandbox--service) fits within the
63-character RFC 1035 limit.

Signed-off-by: Derek Carr <decarr@redhat.com>
…s-workspace collisions

SandboxIndex keyed entries by sandbox name alone, causing same-named
sandboxes in different workspaces to shadow each other in the in-memory
index. Key by (workspace, name) tuple to match the persistence layer's
scoping.

Signed-off-by: Derek Carr <decarr@redhat.com>
…filters, TUI create exec, and CLI last-used sandbox

- Thread workspace watch channel into PolicyLocalContext so
  SubmitPolicyAnalysis and GetDraftPolicy send the correct workspace
  for non-default workspace sandboxes. Gate denial and activity flushes
  on non-empty workspace to prevent silent data loss during initial
  settings poll race.

- Fix three provider-profile scope filtering asymmetries: scope-
  mismatched providers now contribute their real token-grant bindings
  instead of being skipped or matched against the wrong profile; add
  symmetric profile_workspace filter in sandboxes_using_profile to
  prevent false "in use" blocks on workspace profile deletion.

- TUI CreateResult now carries (name, workspace) so post-create exec
  targets the creation workspace, not the selected row. cycle_workspace
  resets current_workspace to "default" when entering all-workspaces
  mode to prevent silent cross-workspace creates.

- CLI last-used sandbox is now workspace-keyed: save/load/clear store
  workspace alongside sandbox name and only match on the requested
  workspace.

Signed-off-by: Derek Carr <decarr@redhat.com>
…in CLI create

The GetProviderProfile pre-check in provider_create_with_options used
the resource workspace instead of profile_workspace, so --global-profile
validated against a workspace-scoped profile while binding the platform
one.

Signed-off-by: Derek Carr <decarr@redhat.com>
…list help text

Signed-off-by: Derek Carr <decarr@redhat.com>
Include workspace field in sandbox and provider -o json/-o yaml output.
Use workspace/name format in --names output under --all-workspaces.
Parse --workspace from argv in tab completers so they complete names
from the correct workspace instead of only reading the env var.

Signed-off-by: Derek Carr <decarr@redhat.com>
Gate sandbox_from_object on the managed-by=openshell label so CRs
written by other actors in the same namespace are silently skipped.
Managed CRs missing required fields (orphans) log a warning instead
of hard-erroring. Add managed-by label selector to list and watch
ListParams so unmanaged CRs are filtered at the K8s API level.
Callers treat Err as skip rather than fail, preventing one bad CR
from breaking the entire list or watch stream.

Signed-off-by: Derek Carr <decarr@redhat.com>
@jhjaggars

Copy link
Copy Markdown
Contributor

Follow-up to my earlier review comment. That pass reviewed the diff file-by-file; this pass audited the PR along four cross-cutting axes instead: (1) workspace provenance at every store call site, (2) workspace provenance at every client request-construction site, (3) the full cross-object reference graph under delete/recreate, and (4) a cross-layer behavior matrix (server/CLI/TUI/SDK/three drivers). Same scope as before: fresh install assumed, upgrade paths out of scope, missing authz deferred to Phase 2. The new findings are mostly relational — they live between components rather than inside any one file, which is why the first pass missed them.

Additional must-fix candidates

1. Watch-adoption path persists driver-supplied workspaces verbatim — one ghost record halts gateway-wide reconciliation

The create branch of apply_sandbox_update_locked (crates/openshell-server/src/compute/mod.rs:1163-1196) clones incoming.workspace from the driver event and writes via raw put_if, bypassing the requires_workspace() guard that every typed write path enforces (persistence/mod.rs:431, 618, 744). Three feeders can deliver an empty workspace on a fresh install:

  • The VM driver drops workspace entirely when building watch/status snapshots (crates/openshell-driver-vm/src/driver.rs:4980-4994, ..Default::default()), so any delete/watch race re-adopts a sandbox with workspace: "".
  • Podman forwards "" for a container carrying openshell.managed=true + a sandbox-id label but no workspace label (driver-podman/src/watcher.rs:320-326) — reachable by manually labeling a container.
  • Docker does the same via unwrap_or_default() (driver-docker/src/lib.rs:2781-2784).

Consequences of the resulting ""-workspace record: it is unreachable by every workspace-scoped RPC ("" normalizes to "default" on lookup, so get/delete return NOT_FOUND), undeletable via the API, fails the requires_workspace guard on every subsequent update_message_cas — and because reconcile_store_with_backend propagates that error with ? before its prune loop (compute/mod.rs:1057-1086), one ghost record stops orphan pruning for the entire gateway. It surfaces only as a blank WORKSPACE cell under --all-workspaces. This also contradicts RFC 0011's stated fallback ("the sandbox is assigned to the default workspace").

Fix shape: normalize/validate the workspace at the adoption boundary (empty → "default", or refuse adoption) and fix the VM driver to round-trip workspace in snapshots.

2. Inference routes reference providers by name with no delete guard and no cleanup — silent traffic rebinding

delete_provider_record (crates/openshell-server/src/grpc/provider.rs:312-345) checks only sandbox attachments; inference routes neither block provider deletion nor get cleaned up, and resolve_route_by_name (inference.rs:984-996) re-resolves the provider by name on every bundle fetch. Sequence: inference set --provider foo → delete provider foo (succeeds) → recreate foo pointing at a different backend/credentials → every sandbox in the workspace transparently starts sending inference traffic through (and receiving the api_key of) the new provider, with no route update and no audit event.

Secondary defect on the same edge: while the route dangles, the ? in resolve_inference_bundle (inference.rs:913-917) turns one broken route into a failure of the entire bundle — a dangling route also takes down delivery of the other routes for the workspace.

The codebase's own invariant elsewhere (the sandbox-attachment guard) is that name references must not dangle, so this is a gap, not name-as-identity by design. Fix shape: include inference routes in the provider delete guard (or cascade-delete them), and make resolve_inference_bundle skip-and-log a dangling route instead of failing the bundle.

3. Provider deletion doesn't synchronize with attach — dangling attachment adopts a recreated provider's credentials

handle_delete_provider (provider.rs:2484-2493) never acquires sandbox_sync_guard, while handle_attach_sandbox_provider validates provider existence before taking the guard (grpc/sandbox.rs:356-369). An attach racing a delete leaves a by-name attachment in spec.providers pointing at nothing; recreating the provider under the same name silently injects the new provider's credentials into that sandbox. Same TOCTOU shape as the workspace-member race noted in the earlier comment, but on the edge that carries credentials. Fix shape: take sandbox_sync_guard in the delete path (or re-validate provider existence inside the guarded section of attach).

4. Service endpoints are never cleaned up on sandbox delete

cleanup_sandbox_owned_records (compute/mod.rs:1384-1405) removes SSH sessions, settings, policy revisions, and draft chunks — service_endpoint is absent from the list, and no other sandbox-delete path touches it. Orphaned endpoints then (a) permanently block workspace deletion via the "service" blocker in the DeleteWorkspace guard, with no way to remove them except DeleteService against a sandbox that no longer exists, and (b) after recreating a same-name sandbox, Get/ListServices present the stale endpoint (old target_port) as belonging to the new sandbox while routing 404s (the stored sandbox UUID fails closed — the one place the dual name+UUID pattern pays off). Fix shape: add service endpoints to the cleanup list.

Worth fixing, borderline blocking

  • TUI ignores --workspace / OPENSHELL_WORKSPACE entirely: openshell term --workspace team-ml parses successfully and is dropped — main.rs:3471 never passes cli.workspace and the TUI hardcodes "default" (openshell-tui/src/app.rs:891).
  • TUI provider create has the same wrong-workspace targeting as sandbox create (openshell-tui/src/lib.rs:1616): a provider — including its credentials — is silently created in the last-cycled workspace while the header shows "all". If the fix for the sandbox-create finding is generalized to cover both create paths, this comes for free.
  • Python SDK Sandbox context manager can rebind a SandboxRef across workspaces (python/openshell/sandbox.py:810-836): attaching with a ref from workspace A while the constructor says workspace="default" silently rebinds the session to a same-named default-workspace sandbox; exec/delete then target the wrong sandbox. Prefer ref.workspace when set, or reject a mismatch.
  • Sandbox settings can rebind across delete/recreate (race): settings are keyed by (workspace, name) with no sandbox back-pointer, and UpdateConfig (settings_mutex) and sandbox deletion (compute sync_lock) share no lock. A settings write landing after cleanup leaves a record that silently applies to the next same-named sandbox — since settings carry the agentic approval mode, a stale "auto-approve" attaching to a fresh sandbox is a policy-bypass shape.
  • Workspace-not-found and resource-not-found are indistinguishable by code, and two clients act on the wrong meaning: SDK wait_deleted("box", workspace="typo") treats any NOT_FOUND as successful deletion (sandbox.py:527-542); the CLI service-forward loop reports "sandbox no longer exists" on a workspace typo (run.rs:2983-2986).
  • OCSF audit events drop workspace even where it is already in hand: service_routing.rs:597 parses the workspace out of the Host header and discards it (_workspace) before emitting the HTTP-rejection event, and the endpoint ConfigStateChange events omit it despite endpoint.metadata.workspace being on the struct — denied requests against same-named sandboxes in different workspaces produce identical audit rows. Full attribution is Phase 5, but these sites already hold the value.
  • Podman discovery is not gateway-scoped: it filters on openshell.managed=true alone (driver-podman/src/container.rs:44-46) where Docker also filters on namespace. Two gateways sharing a Podman host ingest each other's containers — which compounds with item 1, since adoption skips workspace validation.

Non-blocking notes

Docs/help claim workspace names are DNS-1123/63 chars while the server enforces 19 (proto/openshell.proto:2023, main.rs:2013 vs grpc/workspace.rs:51-58); the SSH alias format implemented (openshell-{name}.{workspace}) differs from the format documented in RFC 0011; sandbox get detail output and provider JSON omit the workspace; cleanup_sandbox_ssh_sessions reads a single page of 1000 with no pagination loop (compute/mod.rs:1407-1430), unlike its sibling scans; race-orphaned UUID-scoped records (policy revisions, draft chunks, refresh state) are unreachable by any API yet block workspace deletion forever via the guard's safety net; the SDK accepts workspace="" without validation.

Phase 2 readiness (informational — no action needed in this PR)

Classified all 73 RPCs plus the data-plane paths for "can a membership check bolt on here." Good news: the resolve_workspace choke point makes ~40 request-scoped endpoints genuine bolt-ons (one mechanical change: thread the principal into it), and all nine TODO(phase2) markers map to real gaps — none stale. Three surfaces will need rework rather than a bolt-on: GetSandboxLogs tails by client-supplied sandbox_id with no resource fetch (already marked); the browser-facing HTTP service-routing proxy runs before auth with no principal at all (unmarked — worth a marker now); and the platform-scope operations (profile "" scope, global:true config/policy, GetGatewayConfig) have no carrier for a platform-admin role check. Bolt-on-ready but unmarked: ExecSandboxInteractive, RevokeSshSession, the bearer-user path of GetSandboxConfig, Set/GetInferenceRoute, and membership filtering on ListWorkspaces/GetWorkspace. Streams verify at establish time only, and SSH revocation does not sever live ForwardTcp tunnels.

What the audit confirmed clean

For completeness, the coverage side: all 118 production store call sites classified (72 via resolve_workspace/resolve_profile_workspace, 24 via trusted ID-based lookups, 13 justified platform-scope "", zero hardcoded "default" — the adoption path in item 1 was the single unjustified site); all 112 client request-construction sites across CLI/TUI/SDK/sandbox-runtime classified with no unjustified empty-workspace sends beyond the findings above; the full cross-object reference graph over all 14 record types tabled — the UUID-revalidated paths (service proxy, SSH forward) are fully delete/recreate-safe, and the DNS label math, hostname parse round-trip, and driver name-length backstops (docker 200, podman 255, k8s 63) all hold under the 19-char caps.

@russellb russellb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated workspace-scoping review of RFC 0011 Phase 1, focused on cross-workspace read/write confusion. Core control-plane read/write path scoped correctly; three actionable items below (one latent driver inconsistency + two hardening/upgrade notes).

labels.insert(LABEL_SANDBOX_ID.to_string(), sandbox.id.clone());
labels.insert(LABEL_SANDBOX_NAME.to_string(), sandbox.name.clone());
labels.insert(
LABEL_SANDBOX_WORKSPACE.to_string(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The openshell.ai/sandbox-workspace label is written here but is never used as a match criterion. find_managed_container_summary (~L1268) matches on namespace && id && name only, and require_sandbox_identifier still accepts name-only requests. This PR hardened the Podman and K8s drivers to require sandbox_id and look up exclusively by the globally-unique LABEL_SANDBOX_ID, but that fix was not mirrored here. All workspaces share one Docker namespace and name uniqueness is now only per-(workspace, name), so a name-only Get/Stop/Delete is workspace-ambiguous and .find() returns an arbitrary match — potentially operating on another workspace's same-named sandbox.

Latent today (the gateway always supplies sandbox_id), but the driver's gRPC contract still advertises name-only lookups as valid. Recommend mirroring the Podman/K8s hardening: require sandbox_id, or include LABEL_SANDBOX_WORKSPACE in the match predicate.

/// * `Err(Conflict)` - Resource version mismatch (for `MatchResourceVersion`)
/// * `Err(UniqueViolation)` - Object already exists (for `MustCreate`) or name conflict
#[allow(clippy::too_many_arguments)]
pub async fn put_if(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put_if is the production create/update path for every workspace-scoped resource (sandbox, provider, service, policy, inference route, ssh session), but unlike put_scoped_message and update_message_cas it does not enforce T::requires_workspace() against a non-empty workspace — and validate_object_metadata does not check workspace either. So the empty-workspace guard described in the RFC as living "in store write helpers" does not actually cover the primary write path.

Not reachable today because handlers resolve workspace to "default" before calling. Recommend adding the requires_workspace() check here (or in validate_object_metadata) so the invariant is enforced at the boundary rather than relying on every caller to resolve correctly.

///
/// Only `resource_version` is hydrated here; `workspace` is NOT backfilled from
/// the DB column because the workspace field is authoritative in the protobuf
/// payload at creation time. This is a breaking upgrade — pre-workspace records

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration 006 backfills the workspace column to "default" for pre-upgrade rows, but because the payload is intentionally not backfilled here, a migrated record decodes with metadata.workspace == "". On resume this causes (a) SandboxIndex to key as ("", name) and miss "default" lookups, and (b) owned records written from that sandbox (e.g. SSH sessions in grpc/sandbox.rs) to be persisted with workspace "".

This falls within the accepted breaking-upgrade behavior, but the practical impact for anyone who resumes rather than recreates sandboxes is index/owned-record divergence — not just cosmetic empty fields. Worth an explicit "recreate sandboxes on upgrade" note in the upgrade docs.

@NVIDIA NVIDIA deleted a comment from russellb Jul 16, 2026
@NVIDIA NVIDIA deleted a comment from russellb Jul 16, 2026
@NVIDIA NVIDIA deleted a comment from jhjaggars Jul 16, 2026
@NVIDIA NVIDIA deleted a comment from jhjaggars Jul 16, 2026
@NVIDIA NVIDIA deleted a comment from rhuss Jul 16, 2026
@NVIDIA NVIDIA deleted a comment from jhjaggars Jul 16, 2026
- VM driver: propagate workspace in sandbox_snapshot so watch events
  round-trip the correct workspace back to the server.
- Server: clean up service endpoints on sandbox delete, preventing
  orphaned endpoints from blocking workspace deletion.
- TUI: accept workspace parameter from CLI instead of hardcoding
  "default", so --workspace and OPENSHELL_WORKSPACE are respected.
- Python SDK: sync Sandbox context manager workspace from session
  after creation, so wait_ready/wait_deleted use the correct workspace
  when attaching via SandboxRef.
- CLI: fix workspace name help text from 63 to 19 char limit.
- RFC 0011: fix SSH alias format to match implementation.

Signed-off-by: Derek Carr <decarr@redhat.com>
@drew

drew commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Feedback from my agent review

[P1] Do not discard startup activity before workspace discovery

Locations:

  • crates/openshell-sandbox/src/lib.rs:364
  • crates/openshell-sandbox/src/lib.rs:409

The denial and activity aggregators drain a batch before invoking their flush callbacks. When the workspace is not yet known, these callbacks return successfully, so the drained events are permanently lost. This can drop policy proposals and activity telemetry during normal sandbox startup.

Buffer the batch until workspace discovery completes, wait on the workspace receiver before starting the aggregators, or return a retryable error so the aggregator retains the batch.

[P1] Preserve global profile scope throughout provider creation

Location: crates/openshell-cli/src/run.rs:4910

provider create --global-profile initially validates a custom profile using profile_workspace, but the --from-existing and --runtime-credentials paths subsequently fetch the profile using the provider's workspace. A valid global custom profile therefore fails unless a same-named workspace profile exists. If one does exist, discovery and credential validation can use that different profile while the persisted provider remains bound to the global profile.

Pass profile_workspace through every profile lookup involved in discovery and runtime-credential validation.

[P1] Prevent resource creation from racing workspace deletion

Location: crates/openshell-server/src/grpc/workspace.rs:223

DeleteWorkspace scans for blockers and deletes the workspace in separate operations. A concurrent resource creation can resolve the workspace and commit after the blocker scan, leaving a resource whose workspace no longer exists. Normal APIs will then refuse to resolve that workspace, making the resource effectively orphaned.

Use a persisted deleting state checked by resource creation, transactional exclusion, or another mechanism that closes the check/delete race.

[P2] Paginate sandbox-owned service cleanup

Location: crates/openshell-server/src/compute/mod.rs:1440

The new service-endpoint cleanup lists only the first 1,000 endpoints in the workspace and then deletes the sandbox. A sandbox whose endpoints fall after that page is deleted while its endpoints remain. Those orphaned records then block workspace deletion and can be presented as belonging to a later same-named sandbox.

Query endpoints by their persisted sandbox label, paginate until exhaustion, or store them under a sandbox-owned scope that supports delete_by_scope.

[P2] Keep Python workspace synchronization test-clean

Locations:

  • python/openshell/sandbox.py:813
  • python/openshell/sandbox_test.py:1555

The new unconditional self._session._workspace access breaks test_high_level_creation_forwards_name_and_labels: the high-level client double returns the public session shape used by this test but has no private _workspace field. The focused SDK run now fails with 74 passing tests and this one AttributeError.

Only resynchronize the workspace on the SandboxRef attachment path that needs it, or expose workspace through a stable session property and update the test double. The current head should not regress the existing Python unit suite.

[P2] Scope TUI profile joins by workspace

Locations:

  • crates/openshell-tui/src/lib.rs:1975
  • crates/openshell-tui/src/app.rs:2603

The all-workspaces provider view loads profiles only from current_workspace and applies them to every provider by provider.type. Provider-detail rendering then rejoins the fetched provider to cached entries using provider name alone. Same profile IDs or provider names in separate workspaces can therefore display another workspace's credentials, endpoints, discovery rules, and policy metadata.

Resolve profiles by (profile_workspace, profile_id) and identify providers by ID or (workspace, name).

[P2] Give all-workspace pagination a total ordering

Locations:

  • crates/openshell-server/src/persistence/postgres.rs:440
  • crates/openshell-server/src/persistence/sqlite.rs:476

Cross-workspace queries order results only by created_at_ms, name. Now that identical names may exist in separate workspaces, records created in the same millisecond can tie. Offset pagination over a non-total order can duplicate or omit resources, including during internal paginated scans.

Add workspace and id as deterministic tie-breakers to cross-workspace query ordering.

[P2] Persist workspace labels in the selector index

Location: crates/openshell-server/src/grpc/workspace.rs:118

CreateWorkspace places request labels in ObjectMeta but passes None for the persistence label column. ListWorkspaces.label_selector queries that persistence column, so it cannot find workspaces created with labels.

Serialize the already-validated workspace labels into the persistence label column before calling put_if.

[P2] Apply the new workspace-routing limit to generated sandbox names

Locations:

  • crates/openshell-server/src/grpc/validation.rs:138
  • crates/openshell-server/src/grpc/sandbox.rs:174

This PR lowers the explicit sandbox-name limit from 253 to 19 characters so workspace--sandbox--service fits in one DNS label. Sandbox-name validation still runs before petname generates a name for an unnamed request, so the server can persist a generated name that violates the new workspace-routing contract. Subsequent service operations reject that sandbox name even though the server generated it.

Validate the generated name and regenerate or fall back until it satisfies the new routable-name constraint.

[P2] Require managed-resource labels in driver ID lookups

Locations:

  • crates/openshell-driver-kubernetes/src/driver.rs:451
  • crates/openshell-driver-kubernetes/src/driver.rs:663
  • crates/openshell-driver-podman/src/driver.rs:601

Several get, existence, and deletion paths filter only by the OpenShell sandbox ID. They can select an unmanaged platform object carrying the same label, and deletion may act on that object.

Include the OpenShell managed-by label in every lookup, not only list/watch paths.

Lower-priority feedback

[P2] Enforce persistence workspace invariants

Location: crates/openshell-server/src/persistence/mod.rs:240

The new raw put_if interface accepts a workspace separately from an encoded typed payload without verifying that it matches ObjectMeta.workspace. A caller error can produce a record whose database identity and decoded resource identity disagree. Derive or verify workspace at the typed persistence boundary; do not duplicate the existing GitHub thread about requiring a non-empty workspace on raw writes.

[P3] Complete the global-profile CLI surface

provider profile export, import, update, lint, and delete expose --global, while provider list-profiles always lists the selected workspace. Add an explicit global listing option.

[P3] Qualify platform-profile dependency diagnostics

Global-profile validation and deletion diagnostics identify dependent sandboxes only by name and then deduplicate those names. For example, dependencies on both alpha/work and beta/work are reported only as work. Return workspace/name so the operator can locate every blocker.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants