review(workspace 2/3): gRPC handlers, compute, drivers#16
Open
rhuss wants to merge 1 commit into
Open
Conversation
Part 2/3 of workspace review split (PR 2243 mirror). gRPC: workspace-scoped sandbox/provider/service/policy handlers, auth enforcement points. Compute: workspace-aware SandboxIndex, lease scoping. Drivers: Docker/Podman/Kubernetes workspace-qualified container names and labels.
📝 WalkthroughWalkthroughWorkspace support is propagated through server persistence, sandbox/provider/policy APIs, compute drivers, and resource metadata. Docker, Podman, and Kubernetes lookup and naming now use workspace-aware identifiers, while gRPC exposes workspace management operations. ChangesWorkspace-scoped server lifecycle
Container and cluster drivers
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SandboxRPC
participant Store
participant ComputeRuntime
participant Driver
Client->>SandboxRPC: create sandbox with workspace
SandboxRPC->>Store: resolve workspace-scoped providers
SandboxRPC->>ComputeRuntime: create workspace-aware sandbox
ComputeRuntime->>Driver: provision sandbox with workspace identity
Driver-->>ComputeRuntime: return sandbox status and workspace
ComputeRuntime->>Store: persist sandbox lifecycle state by workspace
SandboxRPC-->>Client: return workspace-scoped sandbox
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/openshell-server/src/grpc/policy.rs (2)
2421-2429: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
GetSandboxLogsresolves but discardsworkspace— no cross-workspace scope check.
let _workspace = super::workspace::resolve_workspace(...)is computed and dropped; the handler never verifies the targetsandbox_id's workspace matches the caller's. Combined with#[rpc_auth(auth = "bearer", scope = "sandbox:read", role = "user")](no per-sandbox ownership check) ingrpc/mod.rs, any authenticated user withsandbox:readscope can read log content for a sandbox in a workspace they're not a member of. This is explicitly TODO'd as a Phase 2 item, but it's a real information-disclosure gap if this ships as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-server/src/grpc/policy.rs` around lines 2421 - 2429, Update handle_get_sandbox_logs to use the resolved workspace for authorization: load the requested sandbox by req.sandbox_id and verify it belongs to that workspace before fetching or returning logs, rejecting mismatches with the established permission/not-found Status. Remove the discarded _workspace binding and preserve the existing sandbox:read authentication requirements.
1381-1391: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPropagate
workspacethrough the provider-policy helpers
crates/openshell-server/src/grpc/policy.rs:1381-1549still has#[cfg(test)]wrappers that are called by production handlers, and both_with_cataloghelpers now referenceworkspacewithout taking it. That breaks non-test builds. Remove the test-only gating and threadworkspacethrough the_with_catalogsignatures and call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-server/src/grpc/policy.rs` around lines 1381 - 1391, Remove the #[cfg(test)] gating from the provider-policy helper wrappers used by production handlers. Thread workspace through compute_provider_env_revision_with_catalog and the other _with_catalog helper signatures, passing it from their wrapper and every call site so non-test builds compile and workspace-specific behavior is preserved.crates/openshell-server/src/grpc/provider.rs (1)
4080-4211: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMigrate these tests to the workspace-aware API.
state_with_authoritative_profiles_over_default_grants,configure_provider_refresh_uses_authoritative_profile_sources_for_expiry_update, anddelete_provider_refresh_uses_authoritative_profile_sources_for_expiry_updatestill use the old call shapes: theObjectMeta/Providerliterals omitworkspace/profile_workspace, both refresh requests omitworkspace,new_refresh_stateis still called with 3 args, andget_message_by_name::<Provider>is still called with one arg.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-server/src/grpc/provider.rs` around lines 4080 - 4211, Migrate state_with_authoritative_profiles_over_default_grants and both provider refresh tests to the workspace-aware API: populate workspace/profile_workspace fields in the ObjectMeta and Provider literals, include workspace in ConfigureProviderRefreshRequest and DeleteProviderRefreshRequest, pass the required workspace argument to new_refresh_state, and provide the workspace argument to each get_message_by_name::<Provider> call.
🧹 Nitpick comments (2)
crates/openshell-server/src/grpc/sandbox.rs (1)
3680-3685: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the workspace-aware delete handler in this isolation test.
Direct
Store::delete_by_namebypasseshandle_delete_sandboxandComputeRuntime::delete_sandbox, so the changed deletion contract is not tested. Delete through the RPC handler and verify the beta sandbox survives.As per PR objectives, sandbox lifecycle deletion must be workspace-scoped across gRPC, compute, and persistence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-server/src/grpc/sandbox.rs` around lines 3680 - 3685, Update the isolation test around the “default” deletion to invoke the workspace-aware gRPC delete path through handle_delete_sandbox, allowing it to exercise ComputeRuntime::delete_sandbox and persistence. Replace the direct Store::delete_by_name call, then retain assertions verifying the “beta” sandbox with the shared name survives.crates/openshell-server/src/grpc/mod.rs (1)
252-254: 🔒 Security & Privacy | 🔵 TrivialDeferred workspace-authorization gaps are tracked but still open.
These TODO(phase2) markers correctly document real gaps: exec/watch/ssh/tcp-forward RPCs have no workspace field to scope the sandbox lookup, and
all_workspacesonlist_sandboxes/list_services/list_providersis reachable by any authenticated "user" role rather than being restricted to a Platform Admin role. Until Phase 2 lands, any authenticated user can enumerate/act across all workspaces via these paths. Worth tracking as a follow-up issue so it isn't lost before the next stack PR.Also applies to: 271-272, 317-317, 329-329, 350-350, 375-376, 419-420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-server/src/grpc/mod.rs` around lines 252 - 254, Track the deferred workspace-authorization gaps from the TODO markers in the gRPC handlers, including data-plane RPCs and all_workspaces paths. Create a follow-up issue covering workspace scoping for sandbox actions and restricting all-workspaces enumeration to Platform Admin users, and reference that issue in each applicable TODO near the affected handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/openshell-driver-docker/src/lib.rs`:
- Around line 2925-2943: The overflow branches in the container-name
construction function can truncate away id_suffix, causing collisions. Update
the name.is_empty() and reserved >= MAX_CONTAINER_NAME_LEN paths to shorten only
the workspace/name portion while always preserving the complete id_suffix, and
add a regression test covering an oversized workspace with distinct sandbox IDs.
In `@crates/openshell-driver-kubernetes/src/driver.rs`:
- Around line 841-842: Update create_sandbox to call
validate_kube_resource_name_length with sandbox.workspace and sandbox.name
before kube_resource_name and DynamicObject::new. Propagate the validation error
with ?, ensuring oversized names return invalid_argument before contacting
Kubernetes.
- Around line 1155-1157: Update kube_resource_name to produce a collision-free
deterministic identifier for each workspace/name pair, using component escaping
or a stable hash suffix so embedded “--” values cannot alias distinct pairs.
Preserve its use as a valid Kubernetes object name and event index key, and
ensure the same input pair always returns the same result.
In `@crates/openshell-driver-podman/src/container.rs`:
- Around line 39-40: Update the Podman container discovery and filtering paths
using LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, and
LABEL_SANDBOX_WORKSPACE to also recognize the legacy openshell.sandbox-* label
keys. Continue writing only the new openshell.ai/sandbox-* labels, while
ensuring get, stop, delete, list, and watch operations remain compatible with
containers created using legacy labels.
In `@crates/openshell-driver-podman/src/driver.rs`:
- Around line 596-608: Constrain sandbox lookups to managed resources and reject
duplicate matches. In crates/openshell-driver-podman/src/driver.rs:596-608,
update find_container_name_by_id to include LABEL_MANAGED_FILTER and fail when
multiple containers match; apply the same filtering and duplicate rejection to
sandbox_exists and get_sandbox at
crates/openshell-driver-podman/src/driver.rs:677-710. Add the LABEL_MANAGED_BY
selector to get_sandbox, delete_sandbox, and sandbox_exists in
crates/openshell-driver-kubernetes/src/driver.rs:699-700, 912-913, and 994-995.
In `@crates/openshell-server/src/compute/mod.rs`:
- Line 780: Paginate every bounded store scan before processing or deleting
records: update crates/openshell-server/src/compute/mod.rs lines 780, 1080, and
1417-1422, plus crates/openshell-server/src/ssh_sessions.rs line 40, to
materialize all pages (or use stable keyset pagination) so resume,
reconciliation, workspace SSH cleanup, and expired/revoked session reaping cover
every record rather than only offset-zero results.
In `@crates/openshell-server/src/grpc/policy.rs`:
- Around line 2175-2215: Update the idempotent branch in the policy update
handler to return through update_config_response instead of constructing a bare
UpdateConfigResponse, preserving response_annotations and consistent response
behavior. Before the fallback put_policy_revision, revalidate the latest
policy/hash or use the helper’s guarded update path so a concurrent writer
cannot cause an unnecessary extra revision.
In `@crates/openshell-server/src/grpc/provider.rs`:
- Around line 1015-1027: Update dynamic_token_grant_bindings_for_provider to
return Result<Vec<DynamicTokenGrantBinding>, Status>, preserving the existing ?
propagation from get_provider_type_profile and wrapping both the empty and
successful binding results in Ok. Keep the existing call sites’ await? behavior
unchanged.
In `@crates/openshell-server/src/grpc/sandbox.rs`:
- Around line 156-157: Update validate_provider_environment_keys_unique and
validate_provider_environment_keys_unique_with_catalog to accept and forward
workspace into the workspace-scoped provider lookup, ensuring validation uses
records from the requested workspace. Add a regression test covering same-named
providers in different workspaces and verify each workspace validates against
its own records.
- Around line 138-140: Authorize access in every listed sandbox and service RPC
before reading or mutating the resolved workspace:
crates/openshell-server/src/grpc/sandbox.rs:138-140, 245-250, 269-318, 328-332,
341-342, 461-462, 554-560 and crates/openshell-server/src/grpc/service.rs:29-30,
136-137, 153-188, 205-206. After resolve_workspace, enforce caller membership
for the selected workspace, and require platform-admin authorization for each
all_workspaces branch; preserve existing RPC behavior only after these checks
succeed.
---
Outside diff comments:
In `@crates/openshell-server/src/grpc/policy.rs`:
- Around line 2421-2429: Update handle_get_sandbox_logs to use the resolved
workspace for authorization: load the requested sandbox by req.sandbox_id and
verify it belongs to that workspace before fetching or returning logs, rejecting
mismatches with the established permission/not-found Status. Remove the
discarded _workspace binding and preserve the existing sandbox:read
authentication requirements.
- Around line 1381-1391: Remove the #[cfg(test)] gating from the provider-policy
helper wrappers used by production handlers. Thread workspace through
compute_provider_env_revision_with_catalog and the other _with_catalog helper
signatures, passing it from their wrapper and every call site so non-test builds
compile and workspace-specific behavior is preserved.
In `@crates/openshell-server/src/grpc/provider.rs`:
- Around line 4080-4211: Migrate
state_with_authoritative_profiles_over_default_grants and both provider refresh
tests to the workspace-aware API: populate workspace/profile_workspace fields in
the ObjectMeta and Provider literals, include workspace in
ConfigureProviderRefreshRequest and DeleteProviderRefreshRequest, pass the
required workspace argument to new_refresh_state, and provide the workspace
argument to each get_message_by_name::<Provider> call.
---
Nitpick comments:
In `@crates/openshell-server/src/grpc/mod.rs`:
- Around line 252-254: Track the deferred workspace-authorization gaps from the
TODO markers in the gRPC handlers, including data-plane RPCs and all_workspaces
paths. Create a follow-up issue covering workspace scoping for sandbox actions
and restricting all-workspaces enumeration to Platform Admin users, and
reference that issue in each applicable TODO near the affected handlers.
In `@crates/openshell-server/src/grpc/sandbox.rs`:
- Around line 3680-3685: Update the isolation test around the “default” deletion
to invoke the workspace-aware gRPC delete path through handle_delete_sandbox,
allowing it to exercise ComputeRuntime::delete_sandbox and persistence. Replace
the direct Store::delete_by_name call, then retain assertions verifying the
“beta” sandbox with the shared name survives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d6d34a1-10cf-4666-b007-6ee6cde7a9e6
📒 Files selected for processing (22)
crates/openshell-driver-docker/src/lib.rscrates/openshell-driver-docker/src/tests.rscrates/openshell-driver-kubernetes/src/driver.rscrates/openshell-driver-kubernetes/src/grpc.rscrates/openshell-driver-podman/src/container.rscrates/openshell-driver-podman/src/driver.rscrates/openshell-driver-podman/src/grpc.rscrates/openshell-driver-podman/src/watcher.rscrates/openshell-server/src/auth/sandbox_methods.rscrates/openshell-server/src/compute/lease.rscrates/openshell-server/src/compute/mod.rscrates/openshell-server/src/grpc/auth_rpc.rscrates/openshell-server/src/grpc/mod.rscrates/openshell-server/src/grpc/policy.rscrates/openshell-server/src/grpc/provider.rscrates/openshell-server/src/grpc/sandbox.rscrates/openshell-server/src/grpc/service.rscrates/openshell-server/src/provider_refresh.rscrates/openshell-server/src/ssh_sessions.rscrates/openshell-server/src/supervisor_session.rscrates/openshell-server/tests/common/mod.rscrates/openshell-server/tests/supervisor_relay_integration.rs
Comment on lines
+2925
to
2943
| // Format: openshell-{workspace}--{name}-{id} | ||
| // The workspace and id are never truncated — they ensure uniqueness. | ||
| // Only the sandbox name portion is truncated when the total exceeds | ||
| // MAX_CONTAINER_NAME_LEN. | ||
|
|
||
| if name.is_empty() { | ||
| let mut base = format!("{CONTAINER_NAME_PREFIX}{id_suffix}"); | ||
| // The prefix is always < MAX_CONTAINER_NAME_LEN. Truncate the id | ||
| // suffix only if the sandbox id itself is pathologically long. | ||
| let mut base = format!("{CONTAINER_NAME_PREFIX}{workspace}---{id_suffix}"); | ||
| if base.len() > MAX_CONTAINER_NAME_LEN { | ||
| base.truncate(MAX_CONTAINER_NAME_LEN); | ||
| } | ||
| return base; | ||
| return trim_container_name_tail(base); | ||
| } | ||
|
|
||
| // Reserve space for the prefix and the `-<id_suffix>` tail so the id | ||
| // suffix — which is what makes the name unique between sandboxes that | ||
| // share a human-readable prefix — is never truncated away. | ||
| let reserved = CONTAINER_NAME_PREFIX.len() + 1 + id_suffix.len(); | ||
| // Reserve space for fixed parts: prefix + workspace + "--" + "-" + id | ||
| let reserved = CONTAINER_NAME_PREFIX.len() + workspace.len() + 2 + 1 + id_suffix.len(); | ||
| if reserved >= MAX_CONTAINER_NAME_LEN { | ||
| // Pathological sandbox id. Fall back to `<prefix><id>` and truncate. | ||
| let mut base = format!("{CONTAINER_NAME_PREFIX}{id_suffix}"); | ||
| let mut base = format!("{CONTAINER_NAME_PREFIX}{workspace}---{id_suffix}"); | ||
| base.truncate(MAX_CONTAINER_NAME_LEN); | ||
| return trim_container_name_tail(base); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section with line numbers
sed -n '2900,2975p' crates/openshell-driver-docker/src/lib.rs | cat -n
# Find the helper and related constants/usages
rg -n "CONTAINER_NAME_PREFIX|MAX_CONTAINER_NAME_LEN|trim_container_name_tail|id_suffix|container name" crates/openshell-driver-docker/src/lib.rs
# Show the full helper implementation if present nearby
ast-grep outline crates/openshell-driver-docker/src/lib.rs --view expanded | sed -n '1,220p'Repository: rhuss/OpenShell
Length of output: 15827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect nearby tests and references to container_name_for_sandbox
rg -n "container_name_for_sandbox|MAX_CONTAINER_NAME_LEN|trim_container_name_tail|oversized|truncate" crates/openshell-driver-docker/src/lib.rs crates/openshell-driver-docker -g '*test*' -g '*.rs'
# Read the test module area around the relevant function
start=$(rg -n "^#\[cfg\(test\)\]" crates/openshell-driver-docker/src/lib.rs | head -n 1 | cut -d: -f1 || true)
if [ -n "${start:-}" ]; then
sed -n "${start},$((start+260))p" crates/openshell-driver-docker/src/lib.rs | cat -n
fiRepository: rhuss/OpenShell
Length of output: 16316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the container-name tests around the relevant area
sed -n '1960,2045p' crates/openshell-driver-docker/src/tests.rs | cat -nRepository: rhuss/OpenShell
Length of output: 3528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for validation or limits on workspace/name lengths
rg -n "workspace.*len|name.*len|validate.*workspace|workspace.*validate|sandbox.*workspace|DriverSandboxTemplate|DriverSandbox" crates -g '*.rs'
# Inspect the sandbox validation area
sed -n '410,520p' crates/openshell-driver-docker/src/lib.rs | cat -nRepository: rhuss/OpenShell
Length of output: 50371
🏁 Script executed:
python3 - <<'PY'
MAX_CONTAINER_NAME_LEN = 200
PREFIX = "openshell-"
def sanitize(value: str) -> str:
out = []
for ch in value:
if ch.isascii() and (ch.isalnum() or ch in "_.-"):
out.append(ch.lower())
else:
out.append("-")
return "".join(out)
def trim_tail(value: str) -> str:
while value and value[-1] in "-._":
value = value[:-1]
return value
def container_name(workspace: str, name: str, sid: str) -> str:
id_suffix = sanitize(sid)
workspace = sanitize(workspace)
name = sanitize(name)
if not name:
base = f"{PREFIX}{workspace}---{id_suffix}"
if len(base) > MAX_CONTAINER_NAME_LEN:
base = base[:MAX_CONTAINER_NAME_LEN]
return trim_tail(base)
reserved = len(PREFIX) + len(workspace) + 2 + 1 + len(id_suffix)
if reserved >= MAX_CONTAINER_NAME_LEN:
base = f"{PREFIX}{workspace}---{id_suffix}"
base = base[:MAX_CONTAINER_NAME_LEN]
return trim_tail(base)
budget = MAX_CONTAINER_NAME_LEN - reserved
truncated_name = trim_tail(name[:budget]) if len(name) > budget else name
return f"{PREFIX}{workspace}--{truncated_name}-{id_suffix}"
# Find examples where the ID suffix is truncated away.
cases = [
("w" * 180, "n", "id-1234567890"),
("w" * 190, "n", "id-1234567890"),
("w" * 195, "n", "id-1234567890"),
]
for ws, nm, sid in cases:
cn = container_name(ws, nm, sid)
print("len(workspace)=", len(ws), "len(id)=", len(sid), "len(name)=", len(cn), "endswith id?", cn.endswith(sanitize(sid)))
print(cn)
print()
# Search for a collision by varying only the ID while keeping a long workspace.
ws = "w" * 190
nm = "n"
a = container_name(ws, nm, "id-aaaaaaaaaaaaaaaaaaaaaaaaaaaa")
b = container_name(ws, nm, "id-bbbbbbbbbbbbbbbbbbbbbbbbbbbb")
print("collision?", a == b)
print(a)
print(b)
PYRepository: rhuss/OpenShell
Length of output: 1375
Keep the sandbox ID when names overflow. In the name.is_empty() and reserved >= MAX_CONTAINER_NAME_LEN branches, base.truncate(MAX_CONTAINER_NAME_LEN) can cut off id_suffix entirely for long workspaces, so different sandboxes can end up with the same container name. Truncate the workspace/name portion instead and add a regression test for an oversized workspace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-docker/src/lib.rs` around lines 2925 - 2943, The
overflow branches in the container-name construction function can truncate away
id_suffix, causing collisions. Update the name.is_empty() and reserved >=
MAX_CONTAINER_NAME_LEN paths to shorten only the workspace/name portion while
always preserving the complete id_suffix, and add a regression test covering an
oversized workspace with distinct sandbox IDs.
Comment on lines
+841
to
+842
| let kube_name = kube_resource_name(&sandbox.workspace, name); | ||
| let mut obj = DynamicObject::new(&kube_name, &agent_sandbox_api.resource); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for validation helpers and create_sandbox call sites.
ast-grep outline crates/openshell-driver-kubernetes/src/driver.rs --view expanded || true
printf '\n--- search ---\n'
rg -n "validate_sandbox_create|validate_kube_resource_name_length|create_sandbox\(" crates/openshell-driver-kubernetes/src/driver.rs crates/openshell-driver-kubernetes/src -S
printf '\n--- file excerpt around create_sandbox ---\n'
sed -n '780,900p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n--- file excerpt around validation helpers ---\n'
rg -n "fn validate_sandbox_create|fn validate_kube_resource_name_length|kube_resource_name\(" crates/openshell-driver-kubernetes/src/driver.rs -n -SRepository: rhuss/OpenShell
Length of output: 24893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 9236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -n "validate_sandbox_create\|validate_kube_resource_name_length\|create_sandbox(" -n crates/openshell-driver-kubernetes/src/driver.rs
sed -n '820,880p' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 3428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,160p' crates/openshell-driver-kubernetes/src/grpc.rs
printf '\n--- callers of create_sandbox within repo ---\n'
rg -n "\.create_sandbox\(" crates/openshell-driver-kubernetes/src -SRepository: rhuss/OpenShell
Length of output: 5643
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how compute driver errors map to gRPC statuses.
rg -n "impl From<ComputeDriverError> for tonic::Status|enum ComputeDriverError|impl From<KubernetesDriverError> for openshell_core::ComputeDriverError" crates -S
printf '\n--- relevant core mapping ---\n'
rg -n "ComputeDriverError" crates/openshell-core -SRepository: rhuss/OpenShell
Length of output: 1280
Validate sandbox names in create_sandbox. CreateSandbox reaches this method directly, so oversized workspace/name pairs can still hit Kubernetes and surface as an internal error instead of invalid_argument. Add validate_kube_resource_name_length(&sandbox.workspace, &sandbox.name)? before building the object.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 841 - 842,
Update create_sandbox to call validate_kube_resource_name_length with
sandbox.workspace and sandbox.name before kube_resource_name and
DynamicObject::new. Propagate the validation error with ?, ensuring oversized
names return invalid_argument before contacting Kubernetes.
Comment on lines
+1155
to
+1157
| fn kube_resource_name(workspace: &str, name: &str) -> String { | ||
| format!("{workspace}--{name}") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the helper and its callers.
rg -n "fn kube_resource_name|kube_resource_name\\(" crates/openshell-driver-kubernetes/src/driver.rs crates/openshell-driver-kubernetes/src -S
# Show the relevant section around the helper.
sed -n '1120,1195p' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 5234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect nearby resource-name construction for collision implications.
rg -n "format!\\(\"\\{workspace\\}--\\{name\\}\"|--" crates/openshell-driver-kubernetes/src/driver.rs -SRepository: rhuss/OpenShell
Length of output: 2034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any Kubernetes API objects or caches keyed by this name.
rg -n "resource name|resource_name|kube_resource_name|ConfigMap|Secret|ServiceAccount|PersistentVolumeClaim|Job|Pod" crates/openshell-driver-kubernetes/src -SRepository: rhuss/OpenShell
Length of output: 12320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 7862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the surrounding context of the helper and its uses.
python3 - <<'PY'
from pathlib import Path
p = Path('crates/openshell-driver-kubernetes/src/driver.rs')
text = p.read_text()
for needle in ['fn kube_resource_name', 'kube_resource_name(']:
idx = text.find(needle)
print(f'NEEDLE={needle} IDX={idx}')
PYRepository: rhuss/OpenShell
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper and its immediate callers.
sed -n '1140,1185p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "kube_resource_name\\(" crates/openshell-driver-kubernetes/src/driver.rs -n -SRepository: rhuss/OpenShell
Length of output: 2187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect workspace/name validation and the tests around kube_resource_name.
sed -n '5708,5765p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "workspace|name.*validate|validate.*workspace|DNS-1123|kube_resource_name_length" crates/openshell-driver-kubernetes/src/driver.rs -SRepository: rhuss/OpenShell
Length of output: 10161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for other validation paths for sandbox workspace/name in the driver and shared core.
rg -n "workspace.*invalid|invalid.*workspace|sandbox\.workspace|sandbox\.name|name.*invalid|DNS-1123|validate_kube_resource_name_length|LABEL_SANDBOX_WORKSPACE|LABEL_SANDBOX_NAME" crates/openshell-driver-kubernetes/src crates/openshell-core/src -SRepository: rhuss/OpenShell
Length of output: 5192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the call site that uses the generated Kubernetes name.
sed -n '820,855p' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 2064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '5708,5765p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "workspace.*invalid|invalid.*workspace|sandbox\\.workspace|sandbox\\.name|name.*invalid|DNS-1123|validate_kube_resource_name_length|LABEL_SANDBOX_WORKSPACE|LABEL_SANDBOX_NAME" crates/openshell-driver-kubernetes/src crates/openshell-core/src -S
printf '\n---\n'
sed -n '820,855p' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 9382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the validation helpers and the create path around sandbox identifiers.
sed -n '300,380p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
sed -n '640,690p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "validate_.*workspace|validate_.*name|DNS-1123 label|DNS-1123 subdomain|sandbox\\.workspace|sandbox\\.name" crates/openshell-driver-kubernetes/src/driver.rs -n -SRepository: rhuss/OpenShell
Length of output: 7261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether sandbox.workspace / sandbox.name have any stricter validation in shared proto/core code.
rg -n "workspace|sandbox name|sandbox_name|sandbox\\.workspace|sandbox\\.name|DNS-1123|validate_.*sandbox" crates/openshell-core crates/openshell-driver-kubernetes -SRepository: rhuss/OpenShell
Length of output: 27543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the protobuf/struct definitions for DriverSandbox fields and any validators on them.
rg -n "message DriverSandbox|struct DriverSandbox|workspace =|name =" crates -g '!target' -SRepository: rhuss/OpenShell
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sandbox type definition and any docs on field constraints.
rg -n "DriverSandbox|workspace|name" crates/openshell-core/src/proto -SRepository: rhuss/OpenShell
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all direct uses of the generated Kubernetes name.
rg -n "kube_resource_name\\(|DynamicObject::new\\(&kube_name|metadata\\.name|name: Some\\(\".*--.*\"\\)" crates/openshell-driver-kubernetes/src/driver.rs -SRepository: rhuss/OpenShell
Length of output: 765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sandbox create/update validators in the gateway for workspace/name character rules.
sed -n '120,260p' crates/openshell-server/src/grpc/sandbox.rs
printf '\n---\n'
sed -n '820,920p' crates/openshell-server/src/grpc/validation.rs
printf '\n---\n'
rg -n "MAX_NAME_LEN|DNS-1123|workspace.*validation|name.*validation|sandbox_name|workspace" crates/openshell-server/src/grpc/validation.rs crates/openshell-server/src/grpc/sandbox.rs -SRepository: rhuss/OpenShell
Length of output: 21757
Make kube_resource_name collision-free. workspace--name is still ambiguous if either component can contain --, and this value is used as the Kubernetes object name and event index key. Escape the components or add a stable hash suffix so distinct sandbox pairs can’t alias each other.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 1155 - 1157,
Update kube_resource_name to produce a collision-free deterministic identifier
for each workspace/name pair, using component escaping or a stable hash suffix
so embedded “--” values cannot alias distinct pairs. Preserve its use as a valid
Kubernetes object name and event index key, and ensure the same input pair
always returns the same result.
Comment on lines
+39
to
+40
| pub use openshell_core::driver_utils::{ | ||
| LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve read compatibility with existing Podman labels.
All discovery paths now use the new openshell.ai/sandbox-* keys. Containers created by earlier versions with openshell.sandbox-* labels become invisible to get, stop, delete, list, and watch operations. Continue writing the new keys, but retain legacy read/filter fallbacks or provide an explicit migration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-podman/src/container.rs` around lines 39 - 40, Update
the Podman container discovery and filtering paths using LABEL_SANDBOX_ID,
LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, and LABEL_SANDBOX_WORKSPACE to also
recognize the legacy openshell.sandbox-* label keys. Continue writing only the
new openshell.ai/sandbox-* labels, while ensuring get, stop, delete, list, and
watch operations remain compatible with containers created using legacy labels.
Comment on lines
+596
to
+608
| /// Find the Podman container name for a sandbox by its ID using label lookup. | ||
| async fn find_container_name_by_id( | ||
| &self, | ||
| sandbox_id: &str, | ||
| ) -> Result<Option<String>, ComputeDriverError> { | ||
| let filter = format!("{LABEL_SANDBOX_ID}={sandbox_id}"); | ||
| let entries = self | ||
| .client | ||
| .list_containers(&filter) | ||
| .await | ||
| .map_err(ComputeDriverError::from)?; | ||
| Ok(entries.first().and_then(|e| e.names.first().cloned())) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding functions.
git ls-files 'crates/openshell-driver-podman/src/driver.rs' 'crates/openshell-driver-kubernetes/src/driver.rs'
echo '--- PODMAN outline ---'
ast-grep outline crates/openshell-driver-podman/src/driver.rs --view expanded | sed -n '1,260p'
echo '--- KUBERNETES outline ---'
ast-grep outline crates/openshell-driver-kubernetes/src/driver.rs --view expanded | sed -n '1,260p'
echo '--- PODMAN label/search snippets ---'
rg -n 'LABEL_SANDBOX_ID|managed|list_containers|inspect_container|sandbox_exists|get_sandbox|find_container_name_by_id' crates/openshell-driver-podman/src/driver.rs
echo '--- KUBERNETES label/search snippets ---'
rg -n 'LABEL_SANDBOX_ID|managed|ListParams::default\(\)\.labels|delete|exists|get_sandbox' crates/openshell-driver-kubernetes/src/driver.rsRepository: rhuss/OpenShell
Length of output: 23659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant line ranges around the cited sites.
echo '--- PODMAN 560-730 ---'
sed -n '560,730p' crates/openshell-driver-podman/src/driver.rs | cat -n
echo '--- KUBERNETES 670-1010 ---'
sed -n '670,1010p' crates/openshell-driver-kubernetes/src/driver.rs | cat -nRepository: rhuss/OpenShell
Length of output: 23945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how managed labels are defined and applied in both drivers.
echo '--- PODMAN container label definitions/usages ---'
rg -n 'LABEL_MANAGED_FILTER|LABEL_MANAGED_BY|LABEL_MANAGED_BY_VALUE|LABEL_SANDBOX_ID|labels' crates/openshell-driver-podman/src crates/openshell-driver-kubernetes/src
echo '--- PODMAN create path around labels ---'
sed -n '470,560p' crates/openshell-driver-podman/src/driver.rs | cat -n
echo '--- KUBERNETES sandbox_labels and managed-by related code ---'
sed -n '1160,1205p' crates/openshell-driver-kubernetes/src/driver.rs | cat -nRepository: rhuss/OpenShell
Length of output: 17796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the label definitions and the Podman create path to assess ownership scoping.
echo '--- PODMAN container module ---'
sed -n '1,220p' crates/openshell-driver-podman/src/container.rs | cat -n
echo '--- PODMAN create path ---'
sed -n '439,596p' crates/openshell-driver-podman/src/driver.rs | cat -n
echo '--- KUBERNETES sandbox labels ---'
sed -n '1170,1195p' crates/openshell-driver-kubernetes/src/driver.rs | cat -nRepository: rhuss/OpenShell
Length of output: 18422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether Kubernetes sandbox objects carry a managed-by label and whether Podman containers use managed labels on create.
rg -n 'LABEL_MANAGED_BY_VALUE|LABEL_MANAGED_BY|sandbox_labels\(|LABEL_MANAGED_FILTER|set_labels|labels.insert|managed' crates/openshell-driver-kubernetes/src/driver.rs crates/openshell-driver-podman/src/container.rs crates/openshell-driver-podman/src/driver.rs
echo '--- KUBERNETES sandbox_labels implementation ---'
sed -n '1170,1192p' crates/openshell-driver-kubernetes/src/driver.rs | cat -n
echo '--- PODMAN container label helpers ---'
sed -n '1,220p' crates/openshell-driver-podman/src/container.rs | cat -nRepository: rhuss/OpenShell
Length of output: 15110
Constrain sandbox-ID lookups to managed resources and fail on duplicates.
LABEL_SANDBOX_ID alone can still match the wrong object; these paths should scope to OpenShell-managed resources and avoid picking the first hit.
- Podman
find_container_name_by_id,sandbox_exists, andget_sandboxshould filter onLABEL_MANAGED_FILTERand reject multiple matches. - Kubernetes
get_sandbox,delete_sandbox, andsandbox_existsshould add theLABEL_MANAGED_BYselector.
📍 Affects 2 files
crates/openshell-driver-podman/src/driver.rs#L596-L608(this comment)crates/openshell-driver-podman/src/driver.rs#L677-L710crates/openshell-driver-kubernetes/src/driver.rs#L699-L700crates/openshell-driver-kubernetes/src/driver.rs#L912-L913crates/openshell-driver-kubernetes/src/driver.rs#L994-L995
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-driver-podman/src/driver.rs` around lines 596 - 608,
Constrain sandbox lookups to managed resources and reject duplicate matches. In
crates/openshell-driver-podman/src/driver.rs:596-608, update
find_container_name_by_id to include LABEL_MANAGED_FILTER and fail when multiple
containers match; apply the same filtering and duplicate rejection to
sandbox_exists and get_sandbox at
crates/openshell-driver-podman/src/driver.rs:677-710. Add the LABEL_MANAGED_BY
selector to get_sandbox, delete_sandbox, and sandbox_exists in
crates/openshell-driver-kubernetes/src/driver.rs:699-700, 912-913, and 994-995.
| let records = self | ||
| .store | ||
| .list(Sandbox::object_type(), 1000, 0) | ||
| .list_by_type(Sandbox::object_type(), 1000, 0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Paginate every bounded workspace/global sweep.
Each scan reads only offset zero. Records beyond the limit are never processed, and periodic sweeps repeatedly revisit the same first page.
crates/openshell-server/src/compute/mod.rs#L780-L780: resume all persisted sandboxes, not only the first 1,000.crates/openshell-server/src/compute/mod.rs#L1080-L1080: reconcile all store records, not only the first 500.crates/openshell-server/src/compute/mod.rs#L1417-L1422: scan all workspace SSH sessions before sandbox cleanup.crates/openshell-server/src/ssh_sessions.rs#L40-L40: reap expired/revoked sessions beyond the first 1,000.
Materialize all pages before performing deletions, or use stable keyset pagination.
📍 Affects 2 files
crates/openshell-server/src/compute/mod.rs#L780-L780(this comment)crates/openshell-server/src/compute/mod.rs#L1080-L1080crates/openshell-server/src/compute/mod.rs#L1417-L1422crates/openshell-server/src/ssh_sessions.rs#L40-L40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-server/src/compute/mod.rs` at line 780, Paginate every
bounded store scan before processing or deleting records: update
crates/openshell-server/src/compute/mod.rs lines 780, 1080, and 1417-1422, plus
crates/openshell-server/src/ssh_sessions.rs line 40, to materialize all pages
(or use stable keyset pagination) so resume, reconciliation, workspace SSH
cleanup, and expired/revoked session reaping cover every record rather than only
offset-zero results.
Comment on lines
+2175
to
+2215
| let latest = state | ||
| .store | ||
| .get_latest_policy(&sandbox_id) | ||
| .await | ||
| .map_err(|e| Status::internal(format!("fetch latest policy failed: {e}")))?; | ||
|
|
||
| let payload = new_policy.encode_to_vec(); | ||
| let hash = deterministic_policy_hash(&new_policy); | ||
|
|
||
| if let Some(ref current) = latest | ||
| && current.policy_hash == hash | ||
| { | ||
| return Ok(Response::new(UpdateConfigResponse { | ||
| version: u32::try_from(current.version).unwrap_or(0), | ||
| policy_hash: hash, | ||
| settings_revision: 0, | ||
| deleted: false, | ||
| })); | ||
| } | ||
|
|
||
| let next_version = latest.map_or(1, |r| r.version + 1); | ||
| let policy_id = uuid::Uuid::new_v4().to_string(); | ||
|
|
||
| state | ||
| .store | ||
| .put_policy_revision( | ||
| &policy_id, | ||
| &sandbox_id, | ||
| &workspace, | ||
| next_version, | ||
| &payload, | ||
| &hash, | ||
| ) | ||
| .await | ||
| .map_err(|e| Status::internal(format!("persist policy revision failed: {e}")))?; | ||
|
|
||
| let _ = state | ||
| .store | ||
| .supersede_older_policies(&sandbox_id, next_version) | ||
| .await; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/openshell-server/src/grpc/policy.rs"
# Show the surrounding function in manageable chunks
wc -l "$file"
sed -n '2050,2235p' "$file"
# Find response construction helpers and the annotations variable usage
rg -n "response_annotations|update_config_response|UpdateConfigResponse|annotations" "$file"Repository: rhuss/OpenShell
Length of output: 13209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/openshell-server/src/grpc/policy.rs"
# Locate the handler and response helper definitions precisely
rg -n "fn .*update_config|response_annotations|update_config_response" "$file"
# Print the specific region around the handler tail
start=$(rg -n "response_annotations" "$file" | head -n 1 | cut -d: -f1)
if [ -n "${start:-}" ]; then
s=$((start-40)); [ "$s" -lt 1 ] && s=1
e=$((start+120))
sed -n "${s},${e}p" "$file"
fiRepository: rhuss/OpenShell
Length of output: 8585
Use update_config_response on the idempotent path This branch returns a bare UpdateConfigResponse, so it drops response_annotations and diverges from the rest of the handler. Reuse the helper here; the fallback write below can also emit an extra revision if another writer advances the sandbox in between.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-server/src/grpc/policy.rs` around lines 2175 - 2215, Update
the idempotent branch in the policy update handler to return through
update_config_response instead of constructing a bare UpdateConfigResponse,
preserving response_annotations and consistent response behavior. Before the
fallback put_policy_revision, revalidate the latest policy/hash or use the
helper’s guarded update path so a concurrent writer cannot cause an unnecessary
extra revision.
Comment on lines
+1015
to
1027
| async fn dynamic_token_grant_bindings_for_provider( | ||
| store: &Store, | ||
| provider: &Provider, | ||
| ) -> Vec<DynamicTokenGrantBinding> { | ||
| let provider_name = provider.object_name().to_string(); | ||
| let profile_id = normalize_provider_type(&provider.r#type).unwrap_or(provider.r#type.as_str()); | ||
| let Some(profile) = get_provider_type_profile_with_catalog(catalog, profile_id) else { | ||
| return Vec::new(); | ||
| let Some(profile) = | ||
| get_provider_type_profile(store, &provider.profile_workspace, profile_id).await? | ||
| else { | ||
| return Ok(Vec::new()); | ||
| }; | ||
| dynamic_token_grant_bindings_for_profile(&provider_name, &profile.to_proto()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the function and referenced call sites/tests.
rg -n "dynamic_token_grant_bindings_for_provider|state_with_authoritative_profiles_over_default_grants|configure_provider_refresh_uses_authoritative_profile_sources_for_expiry_update|delete_provider_refresh_uses_authoritative_profile_sources_for_expiry_update|get_message_by_name::<Provider>|new_refresh_state\\(" crates/openshell-server/src/grpc/provider.rs
# Show the relevant implementation block.
sed -n '1000,1045p' crates/openshell-server/src/grpc/provider.rs
# Show the three test regions mentioned in the comment.
sed -n '4080,4215p' crates/openshell-server/src/grpc/provider.rsRepository: rhuss/OpenShell
Length of output: 9872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect surrounding type signatures and uses for the provider helper.
sed -n '960,1035p' crates/openshell-server/src/grpc/provider.rs
sed -n '1935,1965p' crates/openshell-server/src/grpc/provider.rs
# Check for the request/message field shapes used in the tests.
rg -n "struct (ObjectMeta|Provider|ConfigureProviderRefreshRequest|DeleteProviderRefreshRequest)|workspace:|profile_workspace:" crates/openshell-server/src/grpc/provider.rs crates -g '!target'Repository: rhuss/OpenShell
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Determine whether the provider helper's signature is used elsewhere in the file.
rg -n "dynamic_token_grant_bindings_for_provider\\(" crates/openshell-server/src/grpc/provider.rs
# Inspect request/message construction around the cited tests.
sed -n '4090,4210p' crates/openshell-server/src/grpc/provider.rsRepository: rhuss/OpenShell
Length of output: 5100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact signatures for the helpers/types mentioned in the tests.
rg -n "fn new_refresh_state|struct NewRefreshStateConfig|trait .*get_message_by_name|fn get_message_by_name|struct ConfigureProviderRefreshRequest|struct DeleteProviderRefreshRequest|workspace:" crates/openshell-server/src crates/openshell-core/src
# Show the definitions around new_refresh_state and the request structs.
sed -n '1,260p' crates/openshell-server/src/provider_refresh.rs
sed -n '1,220p' crates/openshell-server/src/grpc/provider.rsRepository: rhuss/OpenShell
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact signatures for the test-side helpers and request structs.
rg -n "fn new_refresh_state|struct NewRefreshStateConfig|get_message_by_name<|struct ConfigureProviderRefreshRequest|struct DeleteProviderRefreshRequest" crates/openshell-server/src crates/openshell-core/src
# Read the implementation areas around the relevant signatures.
sed -n '1,260p' crates/openshell-server/src/provider_refresh.rs
sed -n '1,180p' crates/openshell-server/src/grpc/provider.rsRepository: rhuss/OpenShell
Length of output: 14897
Return Result from dynamic_token_grant_bindings_for_provider.
The helper uses ? on get_provider_type_profile(...).await and returns Ok(Vec::new()), so its signature must be Result<Vec<DynamicTokenGrantBinding>, Status>. Both call sites already use .await?, so the current form won’t compile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-server/src/grpc/provider.rs` around lines 1015 - 1027,
Update dynamic_token_grant_bindings_for_provider to return
Result<Vec<DynamicTokenGrantBinding>, Status>, preserving the existing ?
propagation from get_provider_type_profile and wrapping both the empty and
successful binding results in Ok. Keep the existing call sites’ await? behavior
unchanged.
Comment on lines
+138
to
+140
| let workspace = | ||
| super::workspace::resolve_workspace(state.store.as_ref(), &request.workspace).await?; | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline crates/openshell-server/src/grpc/workspace.rs \
--match resolve_workspace --view expanded
rg -n -C5 'async fn (create_sandbox|get_sandbox|list_sandboxes|expose_service|get_service|list_services|delete_service)' \
crates/openshell-server/src/grpc/mod.rsRepository: rhuss/OpenShell
Length of output: 4992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate workspace-related grpc files
fd -a 'workspace.rs' crates/openshell-server/src || true
fd -a 'sandbox.rs' crates/openshell-server/src/grpc || true
fd -a 'service.rs' crates/openshell-server/src/grpc || true
# Map the relevant handlers and workspace resolver
ast-grep outline crates/openshell-server/src/grpc/sandbox.rs --view expanded || true
printf '\n--- service.rs ---\n'
ast-grep outline crates/openshell-server/src/grpc/service.rs --view expanded || true
# Find workspace resolution helper usage/definition
rg -n -C3 'resolve_workspace|workspace.*authorize|platform admin|Platform Admin|all_workspaces' crates/openshell-server/src/grpcRepository: rhuss/OpenShell
Length of output: 48969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the workspace resolver and any membership/authorization helpers it uses
rg -n -C4 'async fn resolve_workspace|fn resolve_workspace|resolve_workspace\(' crates/openshell-server/src
printf '\n--- workspace-related auth helpers ---\n'
rg -n -C3 'workspace.*member|member.*workspace|authorize.*workspace|platform admin|PlatformAdmin|is_platform_admin|admin role|role = "admin"|role = "platform"' crates/openshell-server/srcRepository: rhuss/OpenShell
Length of output: 41889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual resolve_workspace definition and any membership checks around it.
rg -n -C6 'resolve_workspace' crates/openshell-server/src
printf '\n--- workspace member checks ---\n'
rg -n -C4 'workspace member|workspace_members|is_workspace|member.*workspace|principal.*workspace|authorized.*workspace|authorize.*membership' crates/openshell-server/src/grpc crates/openshell-server/srcRepository: rhuss/OpenShell
Length of output: 45343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual workspace resolver definition and workspace membership helpers.
rg -n -C5 'pub\(super\)? async fn resolve_workspace|fn resolve_workspace|resolve_workspace\(' crates openshell-core
printf '\n--- workspace membership / authorization helpers ---\n'
rg -n -C4 'workspace member|workspace_members|principal.*workspace|authorize.*membership|is_workspace_member|resolve_.*for_principal|for_principal' crates/openshell-server/src crates/openshell-core/srcRepository: rhuss/OpenShell
Length of output: 33216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the workspace module and any resolver definition.
fd -a 'workspace*.rs' crates/openshell-server/src || true
fd -a 'workspace*' crates/openshell-server/src || true
# Search for the workspace resolver and any membership enforcement around it.
rg -n -C6 'resolve_workspace|workspace member|workspace_members|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member' crates/openshell-server/srcRepository: rhuss/OpenShell
Length of output: 40318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the workspace module and any functions that gate access by principal/membership.
git ls-files 'crates/openshell-server/src/**' | rg '/workspace(\.rs|/|$)'
printf '\n--- module declarations ---\n'
rg -n -C3 'mod workspace|pub mod workspace|workspace::' crates/openshell-server/src/grpc/mod.rs crates/openshell-server/src/grpc/*.rs crates/openshell-server/src/*.rs
printf '\n--- resolver / membership checks ---\n'
rg -n -C5 'resolve_workspace|workspace.*member|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member|for_principal' crates/openshell-server/srcRepository: rhuss/OpenShell
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the workspace module / resolver anywhere in openshell-server.
git ls-files 'crates/openshell-server/src/**' | rg 'workspace'
printf '\n--- possible resolver definitions ---\n'
rg -n -C5 'resolve_workspace|handle_(add|remove|list)_workspace_members|workspace member|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member' crates/openshell-server/srcRepository: rhuss/OpenShell
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the resolver and any membership/authorization logic anywhere in the repo.
printf '%s\n' '--- files named workspace ---'
fd -a 'workspace.rs' . || true
fd -a 'workspace' . || true
printf '\n%s\n' '--- resolve_workspace usages/definitions ---'
rg -n -C6 'resolve_workspace' .
printf '\n%s\n' '--- workspace access checks ---'
rg -n -C4 'workspace member|workspace_members|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member|for_principal|membership' .Repository: rhuss/OpenShell
Length of output: 48405
Authorize workspace access before using the requested scope.
resolve_workspace only resolves the named workspace; these handlers still need an explicit caller-membership check before reading or mutating workspace data. The all_workspaces branches also need a platform-admin gate.
- sandbox/service RPCs: enforce workspace membership on the selected workspace.
all_workspaces: restrict to platform admins.
📍 Affects 2 files
crates/openshell-server/src/grpc/sandbox.rs#L138-L140(this comment)crates/openshell-server/src/grpc/sandbox.rs#L245-L250crates/openshell-server/src/grpc/sandbox.rs#L269-L318crates/openshell-server/src/grpc/sandbox.rs#L328-L332crates/openshell-server/src/grpc/sandbox.rs#L341-L342crates/openshell-server/src/grpc/sandbox.rs#L461-L462crates/openshell-server/src/grpc/sandbox.rs#L554-L560crates/openshell-server/src/grpc/service.rs#L29-L30crates/openshell-server/src/grpc/service.rs#L136-L137crates/openshell-server/src/grpc/service.rs#L153-L188crates/openshell-server/src/grpc/service.rs#L205-L206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-server/src/grpc/sandbox.rs` around lines 138 - 140,
Authorize access in every listed sandbox and service RPC before reading or
mutating the resolved workspace:
crates/openshell-server/src/grpc/sandbox.rs:138-140, 245-250, 269-318, 328-332,
341-342, 461-462, 554-560 and crates/openshell-server/src/grpc/service.rs:29-30,
136-137, 153-188, 205-206. After resolve_workspace, enforce caller membership
for the selected workspace, and require platform-admin authorization for each
all_workspaces branch; preserve existing RPC behavior only after these checks
succeed.
Comment on lines
+156
to
+157
| validate_provider_environment_keys_unique(state.store.as_ref(), &workspace, &spec.providers) | ||
| .await?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline crates/openshell-server/src/grpc/provider.rs \
--match 'validate_provider_environment_keys_unique|validate_provider_environment_keys_unique_with_catalog' \
--view expanded
rg -n -C6 'validate_provider_environment_keys_unique' \
crates/openshell-server/src/grpc/provider.rsRepository: rhuss/OpenShell
Length of output: 4238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '884,980p' crates/openshell-server/src/grpc/provider.rs
printf '\n---\n'
sed -n '140,170p' crates/openshell-server/src/grpc/sandbox.rs
printf '\n---\n'
rg -n -C2 'validate_provider_environment_keys_unique_with_catalog|validate_provider_environment_keys_unique_at|validate_provider_environment_keys_unique\(' crates/openshell-server/src/grpc/provider.rs crates/openshell-server/src/grpc/sandbox.rsRepository: rhuss/OpenShell
Length of output: 9098
Thread workspace into validate_provider_environment_keys_unique_with_catalog.
validate_provider_environment_keys_unique receives workspace, but the inner helper drops it before the workspace-scoped lookup. Same-named providers in different workspaces can be validated against the wrong records, so pass workspace through and add a cross-workspace regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/openshell-server/src/grpc/sandbox.rs` around lines 156 - 157, Update
validate_provider_environment_keys_unique and
validate_provider_environment_keys_unique_with_catalog to accept and forward
workspace into the workspace-scoped provider lookup, ensuring validation uses
records from the requested workspace. Add a regression test covering same-named
providers in different workspaces and verify each workspace validates against
its own records.
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
1 participant
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.
Scope: gRPC handlers (sandbox, provider, service, policy, auth), compute/SandboxIndex, Docker/Podman/Kubernetes drivers (22 files).
Split for bot review analysis. Not intended to be merged.
Summary by CodeRabbit