fix(cli): respect file upload destination name on git path - #1
Open
rh-dnagornuks wants to merge 1 commit into
Open
fix(cli): respect file upload destination name on git path#1rh-dnagornuks wants to merge 1 commit into
rh-dnagornuks wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe upload path now preserves an explicit destination basename for eligible single-file uploads from git repositories. The E2E test covers uploading ChangesGit upload destination handling
Estimated code review effort: 3 (Moderate) | ~15–30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-cli/src/ssh.rs`:
- Around line 961-975: Update the destination handling around split_sandbox_path
so parent "/" also uses the SinglePath upload with target_name, ensuring
root-level destinations such as "/renamed.txt" preserve the requested name.
Remove or adjust the parent != "/" guard while retaining existing behavior for
other parents, and add a regression case covering a root-level renamed
destination.
🪄 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 Plus
Run ID: bd25172b-32c6-4abe-b71c-19fc1f1a2a26
📒 Files selected for processing (2)
crates/openshell-cli/src/ssh.rse2e/rust/tests/sync.rs
rh-dnagornuks
force-pushed
the
sandbox-upload-fix
branch
from
July 17, 2026 11:16
f6b5e33 to
c143a7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/openshell-server/src/compute/mod.rs (1)
509-543: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not update the sandbox index before persistence succeeds.
A create using an existing name but a different ID updates the in-memory mapping at Line 510, then
put_ifreturnsAlreadyExistswithout restoring it. The valid stored sandbox is subsequently mapped to the rejected ID. Move the index update into the successful driver-create branch.Proposed fix
- self.sandbox_index.update_from_sandbox(&sandbox); let mut sandbox = sandbox; ... Ok(_) => { + self.sandbox_index.update_from_sandbox(&sandbox); self.sandbox_watch_bus.notify(sandbox.object_id());🤖 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` around lines 509 - 543, Move the self.sandbox_index.update_from_sandbox(&sandbox) call out of the pre-persistence path and into the successful sandbox creation branch after put_if and driver creation complete. Ensure any AlreadyExists or other persistence failure leaves the in-memory index unchanged, while successful creation still updates it exactly once.crates/openshell-sandbox/src/lib.rs (1)
2601-2605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the tracker aligned with the registry actually installed in local-file mode.
File-mode startup installs only built-ins, but
TrackOnlyrecords all gateway services as current. WithSynchronizedstatus, the following polls see no service-set difference and never perform the reconciliation promised at Lines 2650-2661.Proposed fix
InitialPollDisposition::TrackOnly => { apply_ocsf_json_setting(&ctx.ocsf_enabled, &result.settings); current_config_revision = result.config_revision; current_policy_hash = result.policy_hash.clone(); - current_middleware_services = result.supervisor_middleware_services; current_settings = result.settings;The empty tracker then correctly triggers external-service reconciliation on the next poll.
🤖 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-sandbox/src/lib.rs` around lines 2601 - 2605, Update the InitialPollDisposition::TrackOnly branch so local-file mode initializes current_middleware_services as an empty tracker, reflecting that only built-ins were installed instead of copying result.supervisor_middleware_services. Preserve the existing gateway-service assignment for non-local-file startup and leave the revision, policy hash, and OCSF updates unchanged.
🟡 Minor comments (6)
docs/sandboxes/providers-v2.mdx-325-325 (1)
325-325: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
aws_session_tokenas optional source material.Temporary source credentials are supported when the session token accompanies the access/secret pair, but this table lists only the long-lived pair.
🤖 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 `@docs/sandboxes/providers-v2.mdx` at line 325, Add aws_session_token as an optional credential field in the aws_sts_assume_role documentation, alongside aws_access_key_id and aws_secret_access_key, while preserving the existing role and session configuration details.crates/openshell-providers/src/profiles.rs-419-465 (1)
419-465: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject or normalize surrounding whitespace consistently.
Validation uses trimmed output IDs and credential names, but
co_minted_credential_namesandresolved_additional_output_keysconsume the raw strings. A profile such asoutput: " session_token "can pass linting yet fail runtime resolution and empty-credential detection.Also applies to: 1460-1496
🤖 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-providers/src/profiles.rs` around lines 419 - 465, The output and credential-name lookups in co_minted_credential_names and resolved_additional_output_keys must use the same trimmed values as profile validation. Normalize surrounding whitespace before collecting minted credential names, matching target credentials, and producing semantic output IDs; preserve the existing empty-map and invalid-target behavior.docs/observability/logging.mdx-133-138 (1)
133-138: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the sanitized operator-middleware finding representation.
prototype-content-guardappears operator-run, but the example logs a content-specific title and type. This conflicts withdocs/extensibility/supervisor-middleware.mdxline 167, which requires a platform label, registration name, and aggregate count without service-provided finding text or metadata. Update the example to the actual sanitized fields.🤖 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 `@docs/observability/logging.mdx` around lines 133 - 138, Update the OCSF FINDING:CREATE example in the supervisor middleware logging section to use the sanitized operator-middleware representation: a platform label, the registered middleware name, and the aggregate count only. Remove the content-specific title, finding type, and other service-provided metadata while leaving the separate HTTP event unchanged.examples/aws-s3-sts.md-258-263 (1)
258-263: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDelete the
s3-testprovider during cleanup.The guide creates and configures
s3-test, but cleanup deletes onlypypi. This leaves the provider, refresh metadata, and potentially current STS outputs in the gateway after the IAM role is removed.Proposed fix
# Delete the sandboxes and the pypi provider openshell sandbox delete s3-smoke openshell sandbox delete s3-curl +openshell provider delete s3-test openshell provider delete pypi🤖 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 `@examples/aws-s3-sts.md` around lines 258 - 263, Update the cleanup command block in the guide to delete the s3-test provider in addition to the existing sandbox and pypi deletions, matching the provider created earlier in the example.examples/aws-s3-sts.md-130-130 (1)
130-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the CLI’s actual gateway endpoint variable.
This guide invokes
openshell, but exportsOPENSHELL_BASE_URL. The CLI’s global endpoint option is bound toOPENSHELL_GATEWAY_ENDPOINT, so the following commands will not reliably targethttp://localhost:18080. (raw.githubusercontent.com)Proposed fix
-export OPENSHELL_BASE_URL=http://localhost:18080 +export OPENSHELL_GATEWAY_ENDPOINT=http://localhost:18080🤖 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 `@examples/aws-s3-sts.md` at line 130, Update the environment variable export in the AWS S3 STS guide to use OPENSHELL_GATEWAY_ENDPOINT with the localhost gateway URL, matching the endpoint variable consumed by the openshell CLI.Source: MCP tools
rfc/0009-supervisor-middleware/README.md-135-137 (1)
135-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the
tls: skiprule with the implemented fail-open behavior.This says any middleware-selector overlap with a
tls: skipendpoint is rejected. The validator rejects overlap only when the matching chain includes a required (fail_closed, including default) stage; an all-fail_openchain is permitted and emits a bypass finding. (raw.githubusercontent.com)Proposed wording
-Policy validation rejects any middleware selector whose possible hosts overlap an endpoint configured with `tls: skip`, +Policy validation rejects any middleware selector with a required (`fail_closed`) stage whose possible hosts overlap an endpoint configured with `tls: skip`,🤖 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 `@rfc/0009-supervisor-middleware/README.md` around lines 135 - 137, Update the `tls: skip` validation description in the README to reflect the implemented chain-level behavior: reject selector overlap only when the matching middleware chain contains a required `fail_closed` stage, including the default, while permitting an all-`fail_open` chain that relays the request and emits a bypass `DetectionFinding`.Source: MCP tools
🤖 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-ocsf/src/format/shorthand.rs`:
- Around line 121-135: Escape denial reasons before inserting them into
shorthand bracketed contexts. Update both reason paths, including reason_text
and the additional path around reason_tag, to apply escape_context_field after
truncation while preserving the existing empty-value and fallback behavior.
In `@crates/openshell-policy/src/lib.rs`:
- Around line 1375-1380: Update host_wildcard_shape_invalid and its callers to
reject wildcard hosts with at most two labels, matching the supervisor network
runtime behavior; this must reject both foo.* and foo*. Add regression coverage
for these two invalid host forms while preserving validation for permitted
multi-label wildcards.
In `@crates/openshell-prover/src/credentials.rs`:
- Around line 189-221: Replace the local credential-overlap matching in the
relevant matcher flow, including labels_match and its duplicate coverage
helpers, with openshell_core::host_pattern::host_patterns_overlap so ?, [...]
and other runtime-supported patterns are handled consistently. Remove the
redundant helpers and add regression coverage for tenant-?.example.com and
tenant-[ab].example.com overlap cases.
In `@crates/openshell-server/src/grpc/provider.rs`:
- Around line 2166-2201: Move the sts_endpoint_url, source access-key pair, and
session-token validation behind the AwsStsAssumeRole strategy check in the
surrounding provider request handling. Preserve all existing rejection rules for
AwsStsAssumeRole, while allowing non-STS strategies to use these material keys
without validation errors.
- Line 2322: Update the provider update logic surrounding additional_output_keys
so a supplied expires_at_ms is applied to every co-minted credential, including
existing secret and session values, rather than only credential_key. Ensure all
credentials sharing the STS expiry become non-injectable when that expiry is
reached.
In `@crates/openshell-server/src/middleware.rs`:
- Around line 8-19: Update validate_policy to preserve typed internal and
transport errors returned by validate_policy_configs, mapping only genuine
policy rejection errors to Status::invalid_argument. Keep the existing
validation call and ensure remote timeouts or service failures remain classified
for appropriate retry behavior.
In `@crates/openshell-server/src/persistence/sqlite.rs`:
- Around line 175-184: Update ComputeRuntime::set_supervisor_session_state to
treat PersistenceError::Conflict with current_resource_version: None as an
absent target, preserving deletion-race handling. Apply the consistent CAS
contract at crates/openshell-server/src/persistence/sqlite.rs:175-184 and
crates/openshell-server/src/persistence/postgres.rs:167-176, where absent rows
remain represented as Conflict(None); update any matching consumers rather than
relying on the former database “not found” error.
In `@crates/openshell-server/src/provider_refresh.rs`:
- Around line 412-456: The generation claim in the refresh flow does not protect
the subsequent provider credential mutation from deletion, reconfiguration, or
competing rotations. Update the paths around persist_refresh_state_if_current
and apply_minted_credential, including the other refresh implementations noted
by the review, to hold a transaction or refresh lease/lock across the claim and
provider write so stale or out-of-order rotations cannot install credentials.
Add a test that pauses after the claim and verifies a concurrent deletion or
reconfiguration prevents the provider mutation.
- Around line 749-785: Update the expiry calculations in the AssumeRole flow to
use the capped lifetime represented by max_lifetime, converting it to a safe i64
duration before multiplying by 1000 or adding to now_ms. Apply this capped
duration both to the expiration fallback and max_expires calculations, while
preserving the STS request lifetime behavior.
In `@crates/openshell-supervisor-network/src/l7/relay.rs`:
- Around line 2091-2130: Move the ActionId::Allowed audit event from its current
pre-middleware location to after middleware processing, credential injection,
and successful relay completion. Ensure every fail-closed return or middleware
denial occurs before the allowed event is emitted, preserving allowed records
only for requests actually forwarded successfully.
In `@crates/openshell-supervisor-network/src/proxy.rs`:
- Around line 1249-1262: In
crates/openshell-supervisor-network/src/proxy.rs:1249-1262, move the tls: skip
middleware gate using middleware_uninspectable_gate before establishing the
upstream connection and sending the 200 response, preserving the existing denial
response there. In crates/openshell-supervisor-network/src/proxy.rs:1504-1551,
for unsupported tunneled protocols discovered after CONNECT succeeds, emit the
existing telemetry and close the connection without writing a 403 HTTP response.
- Around line 4609-4677: The upstream connection setup currently occurs before
middleware evaluation and final policy validation. In the forward request flow
surrounding the middleware pipeline, defer the upstream connect step until after
credential processing, middleware application, and the final generation check,
then use that connection for authorized forwarding while preserving fail-closed
denial behavior.
- Around line 438-444: Update middleware_uninspectable_gate to accept the
connection’s captured policy generation, retain the generation returned by
query_middleware_chain_with_generation, and compare them before constructing the
gate. On any mismatch, fail closed instead of returning a gate derived from the
newly loaded chain; preserve the existing gate construction for matching
generations.
- Around line 3615-3620: Update the HTTPS branch in the proxy request handling
flow to make the Content-Length header match the actual 26-byte “Use CONNECT for
HTTPS URLs” response body, while preserving the existing 400 response and
guidance.
- Around line 4780-4800: Update the final OCSF success event construction around
HttpActivityBuilder so query parameters are removed from the forwarded target
before use. Ensure both OcsfUrl::new and the message passed to message use only
the path component without the query, while preserving the existing host, port,
and other event fields.
In `@e2e/rust/tests/vm_gateway_resume.rs`:
- Around line 39-43: Update the shell script constructed in the test’s format!
call to fail immediately when sync fails, by enabling errexit before writing and
flushing the resume marker. Preserve the existing readiness marker and wait-loop
behavior only after a successful sync.
In `@rfc/0009-supervisor-middleware/appendices/deployment-options.md`:
- Around line 29-33: Revise the containerized OpenShell sandbox description to
avoid claiming sandbox isolation prevents data leakage. In the paragraph
beginning “This is the most direct answer,” state that sandboxing reduces direct
exfiltration paths while acknowledging that compromised middleware could still
leak data through permitted outputs such as RPC results, transformed bodies, or
findings, consistent with the RFC’s trust-boundary caveat.
---
Outside diff comments:
In `@crates/openshell-sandbox/src/lib.rs`:
- Around line 2601-2605: Update the InitialPollDisposition::TrackOnly branch so
local-file mode initializes current_middleware_services as an empty tracker,
reflecting that only built-ins were installed instead of copying
result.supervisor_middleware_services. Preserve the existing gateway-service
assignment for non-local-file startup and leave the revision, policy hash, and
OCSF updates unchanged.
In `@crates/openshell-server/src/compute/mod.rs`:
- Around line 509-543: Move the self.sandbox_index.update_from_sandbox(&sandbox)
call out of the pre-persistence path and into the successful sandbox creation
branch after put_if and driver creation complete. Ensure any AlreadyExists or
other persistence failure leaves the in-memory index unchanged, while successful
creation still updates it exactly once.
---
Minor comments:
In `@crates/openshell-providers/src/profiles.rs`:
- Around line 419-465: The output and credential-name lookups in
co_minted_credential_names and resolved_additional_output_keys must use the same
trimmed values as profile validation. Normalize surrounding whitespace before
collecting minted credential names, matching target credentials, and producing
semantic output IDs; preserve the existing empty-map and invalid-target
behavior.
In `@docs/observability/logging.mdx`:
- Around line 133-138: Update the OCSF FINDING:CREATE example in the supervisor
middleware logging section to use the sanitized operator-middleware
representation: a platform label, the registered middleware name, and the
aggregate count only. Remove the content-specific title, finding type, and other
service-provided metadata while leaving the separate HTTP event unchanged.
In `@docs/sandboxes/providers-v2.mdx`:
- Line 325: Add aws_session_token as an optional credential field in the
aws_sts_assume_role documentation, alongside aws_access_key_id and
aws_secret_access_key, while preserving the existing role and session
configuration details.
In `@examples/aws-s3-sts.md`:
- Around line 258-263: Update the cleanup command block in the guide to delete
the s3-test provider in addition to the existing sandbox and pypi deletions,
matching the provider created earlier in the example.
- Line 130: Update the environment variable export in the AWS S3 STS guide to
use OPENSHELL_GATEWAY_ENDPOINT with the localhost gateway URL, matching the
endpoint variable consumed by the openshell CLI.
In `@rfc/0009-supervisor-middleware/README.md`:
- Around line 135-137: Update the `tls: skip` validation description in the
README to reflect the implemented chain-level behavior: reject selector overlap
only when the matching middleware chain contains a required `fail_closed` stage,
including the default, while permitting an all-`fail_open` chain that relays the
request and emits a bypass `DetectionFinding`.
🪄 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 Plus
Run ID: f0c63cd2-b91f-4d7b-a65f-4a3731abc2c3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (110)
.github/workflows/branch-docs.yml.github/workflows/docker-build.yml.github/workflows/e2e-test.yml.github/workflows/publish-docs-website.yml.github/workflows/release-dev.yml.github/workflows/release-tag.yml.github/workflows/release-vm-kernel.yml.github/workflows/sync-docs.ymlAGENTS.mdCI.mdCONTRIBUTING.mdCargo.tomlTESTING.mdarchitecture/build.mdarchitecture/gateway.mdarchitecture/sandbox.mdarchitecture/security-policy.mdcrates/openshell-cli/Cargo.tomlcrates/openshell-cli/src/main.rscrates/openshell-cli/src/run.rscrates/openshell-cli/src/ssh.rscrates/openshell-core/Cargo.tomlcrates/openshell-core/README.mdcrates/openshell-core/src/config.rscrates/openshell-core/src/grpc_client.rscrates/openshell-core/src/host_pattern.rscrates/openshell-core/src/lib.rscrates/openshell-core/src/middleware.rscrates/openshell-core/src/proto/mod.rscrates/openshell-core/src/proto_struct.rscrates/openshell-driver-docker/README.mdcrates/openshell-driver-docker/src/lib.rscrates/openshell-driver-docker/src/tests.rscrates/openshell-driver-kubernetes/src/driver.rscrates/openshell-driver-podman/src/container.rscrates/openshell-driver-podman/src/driver.rscrates/openshell-driver-vm/scripts/openshell-vm-sandbox-init.shcrates/openshell-ocsf/src/builders/finding.rscrates/openshell-ocsf/src/builders/http.rscrates/openshell-ocsf/src/format/shorthand.rscrates/openshell-policy/Cargo.tomlcrates/openshell-policy/src/lib.rscrates/openshell-policy/src/middleware.rscrates/openshell-prover/src/credentials.rscrates/openshell-providers/src/lib.rscrates/openshell-providers/src/profiles.rscrates/openshell-sandbox/Cargo.tomlcrates/openshell-sandbox/src/lib.rscrates/openshell-sandbox/src/sidecar_control.rscrates/openshell-sdk/tests/client_mock.rscrates/openshell-server/Cargo.tomlcrates/openshell-server/src/cli.rscrates/openshell-server/src/compute/driver_config.rscrates/openshell-server/src/compute/mod.rscrates/openshell-server/src/config_file.rscrates/openshell-server/src/grpc/policy.rscrates/openshell-server/src/grpc/provider.rscrates/openshell-server/src/grpc/sandbox.rscrates/openshell-server/src/grpc/validation.rscrates/openshell-server/src/lib.rscrates/openshell-server/src/middleware.rscrates/openshell-server/src/persistence/postgres.rscrates/openshell-server/src/persistence/sqlite.rscrates/openshell-server/src/provider_refresh.rscrates/openshell-supervisor-middleware-builtins/Cargo.tomlcrates/openshell-supervisor-middleware-builtins/src/lib.rscrates/openshell-supervisor-middleware-builtins/src/regex.rscrates/openshell-supervisor-middleware/Cargo.tomlcrates/openshell-supervisor-middleware/src/headers.rscrates/openshell-supervisor-middleware/src/lib.rscrates/openshell-supervisor-middleware/src/remote.rscrates/openshell-supervisor-network/Cargo.tomlcrates/openshell-supervisor-network/data/sandbox-policy.regocrates/openshell-supervisor-network/src/l7/middleware.rscrates/openshell-supervisor-network/src/l7/mod.rscrates/openshell-supervisor-network/src/l7/relay.rscrates/openshell-supervisor-network/src/l7/rest.rscrates/openshell-supervisor-network/src/l7/token_grant_injection.rscrates/openshell-supervisor-network/src/opa.rscrates/openshell-supervisor-network/src/proxy.rscrates/openshell-tui/src/app.rsdeploy/docker/Dockerfile.cli-macosdeploy/docker/Dockerfile.python-wheelsdeploy/docker/Dockerfile.python-wheels-macosdocs/extensibility/supervisor-middleware.mdxdocs/index.ymldocs/observability/logging.mdxdocs/providers/aws-sigv4.mdxdocs/reference/gateway-config.mdxdocs/reference/policy-schema.mdxdocs/reference/sandbox-compute-drivers.mdxdocs/sandboxes/manage-providers.mdxdocs/sandboxes/manage-sandboxes.mdxdocs/sandboxes/policies.mdxdocs/sandboxes/providers-v2.mdxe2e/rust/e2e-vm.she2e/rust/tests/sync.rse2e/rust/tests/vm_gateway_resume.rsexamples/aws-s3-sts.mdproto/openshell.protoproto/sandbox.protoproto/supervisor_middleware.protoproviders/aws-s3.yamlproviders/aws.yamlrfc/0009-supervisor-middleware/README.mdrfc/0009-supervisor-middleware/appendices/deployment-options.mdrfc/0009-supervisor-middleware/appendices/protocol-extensions.mdscripts/bin/openshelltasks/ci.tomltasks/python.toml
💤 Files with no reviewable changes (2)
- crates/openshell-cli/Cargo.toml
- scripts/bin/openshell
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/rust/tests/sync.rs
- crates/openshell-cli/src/ssh.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/openshell-server/src/compute/mod.rs (1)
509-543: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not update the sandbox index before persistence succeeds.
A create using an existing name but a different ID updates the in-memory mapping at Line 510, then
put_ifreturnsAlreadyExistswithout restoring it. The valid stored sandbox is subsequently mapped to the rejected ID. Move the index update into the successful driver-create branch.Proposed fix
- self.sandbox_index.update_from_sandbox(&sandbox); let mut sandbox = sandbox; ... Ok(_) => { + self.sandbox_index.update_from_sandbox(&sandbox); self.sandbox_watch_bus.notify(sandbox.object_id());🤖 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` around lines 509 - 543, Move the self.sandbox_index.update_from_sandbox(&sandbox) call out of the pre-persistence path and into the successful sandbox creation branch after put_if and driver creation complete. Ensure any AlreadyExists or other persistence failure leaves the in-memory index unchanged, while successful creation still updates it exactly once.crates/openshell-sandbox/src/lib.rs (1)
2601-2605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the tracker aligned with the registry actually installed in local-file mode.
File-mode startup installs only built-ins, but
TrackOnlyrecords all gateway services as current. WithSynchronizedstatus, the following polls see no service-set difference and never perform the reconciliation promised at Lines 2650-2661.Proposed fix
InitialPollDisposition::TrackOnly => { apply_ocsf_json_setting(&ctx.ocsf_enabled, &result.settings); current_config_revision = result.config_revision; current_policy_hash = result.policy_hash.clone(); - current_middleware_services = result.supervisor_middleware_services; current_settings = result.settings;The empty tracker then correctly triggers external-service reconciliation on the next poll.
🤖 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-sandbox/src/lib.rs` around lines 2601 - 2605, Update the InitialPollDisposition::TrackOnly branch so local-file mode initializes current_middleware_services as an empty tracker, reflecting that only built-ins were installed instead of copying result.supervisor_middleware_services. Preserve the existing gateway-service assignment for non-local-file startup and leave the revision, policy hash, and OCSF updates unchanged.
🟡 Minor comments (6)
docs/sandboxes/providers-v2.mdx-325-325 (1)
325-325: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
aws_session_tokenas optional source material.Temporary source credentials are supported when the session token accompanies the access/secret pair, but this table lists only the long-lived pair.
🤖 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 `@docs/sandboxes/providers-v2.mdx` at line 325, Add aws_session_token as an optional credential field in the aws_sts_assume_role documentation, alongside aws_access_key_id and aws_secret_access_key, while preserving the existing role and session configuration details.crates/openshell-providers/src/profiles.rs-419-465 (1)
419-465: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject or normalize surrounding whitespace consistently.
Validation uses trimmed output IDs and credential names, but
co_minted_credential_namesandresolved_additional_output_keysconsume the raw strings. A profile such asoutput: " session_token "can pass linting yet fail runtime resolution and empty-credential detection.Also applies to: 1460-1496
🤖 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-providers/src/profiles.rs` around lines 419 - 465, The output and credential-name lookups in co_minted_credential_names and resolved_additional_output_keys must use the same trimmed values as profile validation. Normalize surrounding whitespace before collecting minted credential names, matching target credentials, and producing semantic output IDs; preserve the existing empty-map and invalid-target behavior.docs/observability/logging.mdx-133-138 (1)
133-138: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the sanitized operator-middleware finding representation.
prototype-content-guardappears operator-run, but the example logs a content-specific title and type. This conflicts withdocs/extensibility/supervisor-middleware.mdxline 167, which requires a platform label, registration name, and aggregate count without service-provided finding text or metadata. Update the example to the actual sanitized fields.🤖 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 `@docs/observability/logging.mdx` around lines 133 - 138, Update the OCSF FINDING:CREATE example in the supervisor middleware logging section to use the sanitized operator-middleware representation: a platform label, the registered middleware name, and the aggregate count only. Remove the content-specific title, finding type, and other service-provided metadata while leaving the separate HTTP event unchanged.examples/aws-s3-sts.md-258-263 (1)
258-263: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDelete the
s3-testprovider during cleanup.The guide creates and configures
s3-test, but cleanup deletes onlypypi. This leaves the provider, refresh metadata, and potentially current STS outputs in the gateway after the IAM role is removed.Proposed fix
# Delete the sandboxes and the pypi provider openshell sandbox delete s3-smoke openshell sandbox delete s3-curl +openshell provider delete s3-test openshell provider delete pypi🤖 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 `@examples/aws-s3-sts.md` around lines 258 - 263, Update the cleanup command block in the guide to delete the s3-test provider in addition to the existing sandbox and pypi deletions, matching the provider created earlier in the example.examples/aws-s3-sts.md-130-130 (1)
130-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the CLI’s actual gateway endpoint variable.
This guide invokes
openshell, but exportsOPENSHELL_BASE_URL. The CLI’s global endpoint option is bound toOPENSHELL_GATEWAY_ENDPOINT, so the following commands will not reliably targethttp://localhost:18080. (raw.githubusercontent.com)Proposed fix
-export OPENSHELL_BASE_URL=http://localhost:18080 +export OPENSHELL_GATEWAY_ENDPOINT=http://localhost:18080🤖 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 `@examples/aws-s3-sts.md` at line 130, Update the environment variable export in the AWS S3 STS guide to use OPENSHELL_GATEWAY_ENDPOINT with the localhost gateway URL, matching the endpoint variable consumed by the openshell CLI.Source: MCP tools
rfc/0009-supervisor-middleware/README.md-135-137 (1)
135-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the
tls: skiprule with the implemented fail-open behavior.This says any middleware-selector overlap with a
tls: skipendpoint is rejected. The validator rejects overlap only when the matching chain includes a required (fail_closed, including default) stage; an all-fail_openchain is permitted and emits a bypass finding. (raw.githubusercontent.com)Proposed wording
-Policy validation rejects any middleware selector whose possible hosts overlap an endpoint configured with `tls: skip`, +Policy validation rejects any middleware selector with a required (`fail_closed`) stage whose possible hosts overlap an endpoint configured with `tls: skip`,🤖 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 `@rfc/0009-supervisor-middleware/README.md` around lines 135 - 137, Update the `tls: skip` validation description in the README to reflect the implemented chain-level behavior: reject selector overlap only when the matching middleware chain contains a required `fail_closed` stage, including the default, while permitting an all-`fail_open` chain that relays the request and emits a bypass `DetectionFinding`.Source: MCP tools
🤖 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-ocsf/src/format/shorthand.rs`:
- Around line 121-135: Escape denial reasons before inserting them into
shorthand bracketed contexts. Update both reason paths, including reason_text
and the additional path around reason_tag, to apply escape_context_field after
truncation while preserving the existing empty-value and fallback behavior.
In `@crates/openshell-policy/src/lib.rs`:
- Around line 1375-1380: Update host_wildcard_shape_invalid and its callers to
reject wildcard hosts with at most two labels, matching the supervisor network
runtime behavior; this must reject both foo.* and foo*. Add regression coverage
for these two invalid host forms while preserving validation for permitted
multi-label wildcards.
In `@crates/openshell-prover/src/credentials.rs`:
- Around line 189-221: Replace the local credential-overlap matching in the
relevant matcher flow, including labels_match and its duplicate coverage
helpers, with openshell_core::host_pattern::host_patterns_overlap so ?, [...]
and other runtime-supported patterns are handled consistently. Remove the
redundant helpers and add regression coverage for tenant-?.example.com and
tenant-[ab].example.com overlap cases.
In `@crates/openshell-server/src/grpc/provider.rs`:
- Around line 2166-2201: Move the sts_endpoint_url, source access-key pair, and
session-token validation behind the AwsStsAssumeRole strategy check in the
surrounding provider request handling. Preserve all existing rejection rules for
AwsStsAssumeRole, while allowing non-STS strategies to use these material keys
without validation errors.
- Line 2322: Update the provider update logic surrounding additional_output_keys
so a supplied expires_at_ms is applied to every co-minted credential, including
existing secret and session values, rather than only credential_key. Ensure all
credentials sharing the STS expiry become non-injectable when that expiry is
reached.
In `@crates/openshell-server/src/middleware.rs`:
- Around line 8-19: Update validate_policy to preserve typed internal and
transport errors returned by validate_policy_configs, mapping only genuine
policy rejection errors to Status::invalid_argument. Keep the existing
validation call and ensure remote timeouts or service failures remain classified
for appropriate retry behavior.
In `@crates/openshell-server/src/persistence/sqlite.rs`:
- Around line 175-184: Update ComputeRuntime::set_supervisor_session_state to
treat PersistenceError::Conflict with current_resource_version: None as an
absent target, preserving deletion-race handling. Apply the consistent CAS
contract at crates/openshell-server/src/persistence/sqlite.rs:175-184 and
crates/openshell-server/src/persistence/postgres.rs:167-176, where absent rows
remain represented as Conflict(None); update any matching consumers rather than
relying on the former database “not found” error.
In `@crates/openshell-server/src/provider_refresh.rs`:
- Around line 412-456: The generation claim in the refresh flow does not protect
the subsequent provider credential mutation from deletion, reconfiguration, or
competing rotations. Update the paths around persist_refresh_state_if_current
and apply_minted_credential, including the other refresh implementations noted
by the review, to hold a transaction or refresh lease/lock across the claim and
provider write so stale or out-of-order rotations cannot install credentials.
Add a test that pauses after the claim and verifies a concurrent deletion or
reconfiguration prevents the provider mutation.
- Around line 749-785: Update the expiry calculations in the AssumeRole flow to
use the capped lifetime represented by max_lifetime, converting it to a safe i64
duration before multiplying by 1000 or adding to now_ms. Apply this capped
duration both to the expiration fallback and max_expires calculations, while
preserving the STS request lifetime behavior.
In `@crates/openshell-supervisor-network/src/l7/relay.rs`:
- Around line 2091-2130: Move the ActionId::Allowed audit event from its current
pre-middleware location to after middleware processing, credential injection,
and successful relay completion. Ensure every fail-closed return or middleware
denial occurs before the allowed event is emitted, preserving allowed records
only for requests actually forwarded successfully.
In `@crates/openshell-supervisor-network/src/proxy.rs`:
- Around line 1249-1262: In
crates/openshell-supervisor-network/src/proxy.rs:1249-1262, move the tls: skip
middleware gate using middleware_uninspectable_gate before establishing the
upstream connection and sending the 200 response, preserving the existing denial
response there. In crates/openshell-supervisor-network/src/proxy.rs:1504-1551,
for unsupported tunneled protocols discovered after CONNECT succeeds, emit the
existing telemetry and close the connection without writing a 403 HTTP response.
- Around line 4609-4677: The upstream connection setup currently occurs before
middleware evaluation and final policy validation. In the forward request flow
surrounding the middleware pipeline, defer the upstream connect step until after
credential processing, middleware application, and the final generation check,
then use that connection for authorized forwarding while preserving fail-closed
denial behavior.
- Around line 438-444: Update middleware_uninspectable_gate to accept the
connection’s captured policy generation, retain the generation returned by
query_middleware_chain_with_generation, and compare them before constructing the
gate. On any mismatch, fail closed instead of returning a gate derived from the
newly loaded chain; preserve the existing gate construction for matching
generations.
- Around line 3615-3620: Update the HTTPS branch in the proxy request handling
flow to make the Content-Length header match the actual 26-byte “Use CONNECT for
HTTPS URLs” response body, while preserving the existing 400 response and
guidance.
- Around line 4780-4800: Update the final OCSF success event construction around
HttpActivityBuilder so query parameters are removed from the forwarded target
before use. Ensure both OcsfUrl::new and the message passed to message use only
the path component without the query, while preserving the existing host, port,
and other event fields.
In `@e2e/rust/tests/vm_gateway_resume.rs`:
- Around line 39-43: Update the shell script constructed in the test’s format!
call to fail immediately when sync fails, by enabling errexit before writing and
flushing the resume marker. Preserve the existing readiness marker and wait-loop
behavior only after a successful sync.
In `@rfc/0009-supervisor-middleware/appendices/deployment-options.md`:
- Around line 29-33: Revise the containerized OpenShell sandbox description to
avoid claiming sandbox isolation prevents data leakage. In the paragraph
beginning “This is the most direct answer,” state that sandboxing reduces direct
exfiltration paths while acknowledging that compromised middleware could still
leak data through permitted outputs such as RPC results, transformed bodies, or
findings, consistent with the RFC’s trust-boundary caveat.
---
Outside diff comments:
In `@crates/openshell-sandbox/src/lib.rs`:
- Around line 2601-2605: Update the InitialPollDisposition::TrackOnly branch so
local-file mode initializes current_middleware_services as an empty tracker,
reflecting that only built-ins were installed instead of copying
result.supervisor_middleware_services. Preserve the existing gateway-service
assignment for non-local-file startup and leave the revision, policy hash, and
OCSF updates unchanged.
In `@crates/openshell-server/src/compute/mod.rs`:
- Around line 509-543: Move the self.sandbox_index.update_from_sandbox(&sandbox)
call out of the pre-persistence path and into the successful sandbox creation
branch after put_if and driver creation complete. Ensure any AlreadyExists or
other persistence failure leaves the in-memory index unchanged, while successful
creation still updates it exactly once.
---
Minor comments:
In `@crates/openshell-providers/src/profiles.rs`:
- Around line 419-465: The output and credential-name lookups in
co_minted_credential_names and resolved_additional_output_keys must use the same
trimmed values as profile validation. Normalize surrounding whitespace before
collecting minted credential names, matching target credentials, and producing
semantic output IDs; preserve the existing empty-map and invalid-target
behavior.
In `@docs/observability/logging.mdx`:
- Around line 133-138: Update the OCSF FINDING:CREATE example in the supervisor
middleware logging section to use the sanitized operator-middleware
representation: a platform label, the registered middleware name, and the
aggregate count only. Remove the content-specific title, finding type, and other
service-provided metadata while leaving the separate HTTP event unchanged.
In `@docs/sandboxes/providers-v2.mdx`:
- Line 325: Add aws_session_token as an optional credential field in the
aws_sts_assume_role documentation, alongside aws_access_key_id and
aws_secret_access_key, while preserving the existing role and session
configuration details.
In `@examples/aws-s3-sts.md`:
- Around line 258-263: Update the cleanup command block in the guide to delete
the s3-test provider in addition to the existing sandbox and pypi deletions,
matching the provider created earlier in the example.
- Line 130: Update the environment variable export in the AWS S3 STS guide to
use OPENSHELL_GATEWAY_ENDPOINT with the localhost gateway URL, matching the
endpoint variable consumed by the openshell CLI.
In `@rfc/0009-supervisor-middleware/README.md`:
- Around line 135-137: Update the `tls: skip` validation description in the
README to reflect the implemented chain-level behavior: reject selector overlap
only when the matching middleware chain contains a required `fail_closed` stage,
including the default, while permitting an all-`fail_open` chain that relays the
request and emits a bypass `DetectionFinding`.
🪄 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 Plus
Run ID: f0c63cd2-b91f-4d7b-a65f-4a3731abc2c3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (110)
.github/workflows/branch-docs.yml.github/workflows/docker-build.yml.github/workflows/e2e-test.yml.github/workflows/publish-docs-website.yml.github/workflows/release-dev.yml.github/workflows/release-tag.yml.github/workflows/release-vm-kernel.yml.github/workflows/sync-docs.ymlAGENTS.mdCI.mdCONTRIBUTING.mdCargo.tomlTESTING.mdarchitecture/build.mdarchitecture/gateway.mdarchitecture/sandbox.mdarchitecture/security-policy.mdcrates/openshell-cli/Cargo.tomlcrates/openshell-cli/src/main.rscrates/openshell-cli/src/run.rscrates/openshell-cli/src/ssh.rscrates/openshell-core/Cargo.tomlcrates/openshell-core/README.mdcrates/openshell-core/src/config.rscrates/openshell-core/src/grpc_client.rscrates/openshell-core/src/host_pattern.rscrates/openshell-core/src/lib.rscrates/openshell-core/src/middleware.rscrates/openshell-core/src/proto/mod.rscrates/openshell-core/src/proto_struct.rscrates/openshell-driver-docker/README.mdcrates/openshell-driver-docker/src/lib.rscrates/openshell-driver-docker/src/tests.rscrates/openshell-driver-kubernetes/src/driver.rscrates/openshell-driver-podman/src/container.rscrates/openshell-driver-podman/src/driver.rscrates/openshell-driver-vm/scripts/openshell-vm-sandbox-init.shcrates/openshell-ocsf/src/builders/finding.rscrates/openshell-ocsf/src/builders/http.rscrates/openshell-ocsf/src/format/shorthand.rscrates/openshell-policy/Cargo.tomlcrates/openshell-policy/src/lib.rscrates/openshell-policy/src/middleware.rscrates/openshell-prover/src/credentials.rscrates/openshell-providers/src/lib.rscrates/openshell-providers/src/profiles.rscrates/openshell-sandbox/Cargo.tomlcrates/openshell-sandbox/src/lib.rscrates/openshell-sandbox/src/sidecar_control.rscrates/openshell-sdk/tests/client_mock.rscrates/openshell-server/Cargo.tomlcrates/openshell-server/src/cli.rscrates/openshell-server/src/compute/driver_config.rscrates/openshell-server/src/compute/mod.rscrates/openshell-server/src/config_file.rscrates/openshell-server/src/grpc/policy.rscrates/openshell-server/src/grpc/provider.rscrates/openshell-server/src/grpc/sandbox.rscrates/openshell-server/src/grpc/validation.rscrates/openshell-server/src/lib.rscrates/openshell-server/src/middleware.rscrates/openshell-server/src/persistence/postgres.rscrates/openshell-server/src/persistence/sqlite.rscrates/openshell-server/src/provider_refresh.rscrates/openshell-supervisor-middleware-builtins/Cargo.tomlcrates/openshell-supervisor-middleware-builtins/src/lib.rscrates/openshell-supervisor-middleware-builtins/src/regex.rscrates/openshell-supervisor-middleware/Cargo.tomlcrates/openshell-supervisor-middleware/src/headers.rscrates/openshell-supervisor-middleware/src/lib.rscrates/openshell-supervisor-middleware/src/remote.rscrates/openshell-supervisor-network/Cargo.tomlcrates/openshell-supervisor-network/data/sandbox-policy.regocrates/openshell-supervisor-network/src/l7/middleware.rscrates/openshell-supervisor-network/src/l7/mod.rscrates/openshell-supervisor-network/src/l7/relay.rscrates/openshell-supervisor-network/src/l7/rest.rscrates/openshell-supervisor-network/src/l7/token_grant_injection.rscrates/openshell-supervisor-network/src/opa.rscrates/openshell-supervisor-network/src/proxy.rscrates/openshell-tui/src/app.rsdeploy/docker/Dockerfile.cli-macosdeploy/docker/Dockerfile.python-wheelsdeploy/docker/Dockerfile.python-wheels-macosdocs/extensibility/supervisor-middleware.mdxdocs/index.ymldocs/observability/logging.mdxdocs/providers/aws-sigv4.mdxdocs/reference/gateway-config.mdxdocs/reference/policy-schema.mdxdocs/reference/sandbox-compute-drivers.mdxdocs/sandboxes/manage-providers.mdxdocs/sandboxes/manage-sandboxes.mdxdocs/sandboxes/policies.mdxdocs/sandboxes/providers-v2.mdxe2e/rust/e2e-vm.she2e/rust/tests/sync.rse2e/rust/tests/vm_gateway_resume.rsexamples/aws-s3-sts.mdproto/openshell.protoproto/sandbox.protoproto/supervisor_middleware.protoproviders/aws-s3.yamlproviders/aws.yamlrfc/0009-supervisor-middleware/README.mdrfc/0009-supervisor-middleware/appendices/deployment-options.mdrfc/0009-supervisor-middleware/appendices/protocol-extensions.mdscripts/bin/openshelltasks/ci.tomltasks/python.toml
💤 Files with no reviewable changes (2)
- crates/openshell-cli/Cargo.toml
- scripts/bin/openshell
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/rust/tests/sync.rs
- crates/openshell-cli/src/ssh.rs
🛑 Comments failed to post (17)
crates/openshell-ocsf/src/format/shorthand.rs (1)
121-135: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape denial reasons before inserting them into shorthand contexts.
reason_textonly normalizes CR/LF. A reason containing], tabs, or control characters can terminate or forge the bracketed context. Applyescape_context_fieldafter truncation in both reason paths.Proposed fix
fn reason_tag(base: &BaseEventData) -> String { reason_text(base.status_detail.as_deref().or(base.message.as_deref())) - .map_or_else(String::new, |text| format!(" [reason:{text}]")) + .map_or_else(String::new, |text| { + format!(" [reason:{}]", escape_context_field(&text)) + }) } - fields.push(format!("reason:{reason}")); + fields.push(format!("reason:{}", escape_context_field(&reason)));Also applies to: 158-165
🤖 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-ocsf/src/format/shorthand.rs` around lines 121 - 135, Escape denial reasons before inserting them into shorthand bracketed contexts. Update both reason paths, including reason_text and the additional path around reason_tag, to apply escape_context_field after truncation while preserving the existing empty-value and fallback behavior.crates/openshell-policy/src/lib.rs (1)
1375-1380: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject TLD and single-label wildcards consistently with the runtime.
host_wildcard_shape_invalidacceptsfoo.*andfoo*, whilecrates/openshell-supervisor-network/src/l7/mod.rsrejects every wildcard host with at most two labels. These policies can therefore pass canonical validation but fail or behave differently at the proxy layer.Proposed validation and regression coverage
- if ep.host.contains('*') && (ep.host.starts_with("*.") || ep.host.starts_with("**.")) { + if ep.host.contains('*') && !matches!(ep.host.as_str(), "*" | "**") { let label_count = ep.host.split('.').count(); if label_count <= 2 { violations.push(PolicyViolation::TldWildcard {Add rejection cases for
foo.*andfoo*.Also applies to: 1417-1434
🤖 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-policy/src/lib.rs` around lines 1375 - 1380, Update host_wildcard_shape_invalid and its callers to reject wildcard hosts with at most two labels, matching the supervisor network runtime behavior; this must reject both foo.* and foo*. Add regression coverage for these two invalid host forms while preserving validation for permitted multi-label wildcards.crates/openshell-prover/src/credentials.rs (1)
189-221: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use the shared host-pattern matcher for credential overlap.
labels_matchrecognizes only*, while the runtime matcher also supports?and[...]. Such credentials can match at runtime but be omitted from prover analysis. Delegate toopenshell_core::host_pattern::host_patterns_overlapand add non-asterisk glob regressions.Proposed fix
fn host_patterns_overlap(left: &str, right: &str) -> bool { let left = normalize_host(left); let right = normalize_host(right); - if left.is_empty() || right.is_empty() { - return false; - } - left == right || host_pattern_covers(&left, &right) || host_pattern_covers(&right, &left) + openshell_core::host_pattern::host_patterns_overlap(&left, &right) + .unwrap_or(false) }Then remove the duplicate coverage helpers and test patterns such as
tenant-?.example.comandtenant-[ab].example.com.Also applies to: 437-466
🤖 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-prover/src/credentials.rs` around lines 189 - 221, Replace the local credential-overlap matching in the relevant matcher flow, including labels_match and its duplicate coverage helpers, with openshell_core::host_pattern::host_patterns_overlap so ?, [...] and other runtime-supported patterns are handled consistently. Remove the redundant helpers and add regression coverage for tenant-?.example.com and tenant-[ab].example.com overlap cases.crates/openshell-server/src/grpc/provider.rs (2)
2166-2201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope AWS source-credential validation to
AwsStsAssumeRole.These checks currently reject
sts_endpoint_urlor partialaws_*material for every strategy, breaking custom non-STS profiles that legitimately use those material names.Proposed fix
- if request.material.contains_key("sts_endpoint_url") { - ... - } - let has_source_access_key = ...; - ... + if strategy == ProviderCredentialRefreshStrategy::AwsStsAssumeRole { + if request.material.contains_key("sts_endpoint_url") { + return Err(Status::invalid_argument( + "sts_endpoint_url material is not permitted", + )); + } + // Validate the explicit AWS source credential set here. + ... + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if strategy == ProviderCredentialRefreshStrategy::AwsStsAssumeRole { // The STS endpoint is always resolved from the region in production. Reject // any caller-supplied override so an AWS-signed AssumeRole request cannot be // redirected at an arbitrary service (CWE-918). Tests inject a mock endpoint // through test-only code paths, not this boundary. if request.material.contains_key("sts_endpoint_url") { return Err(Status::invalid_argument( "sts_endpoint_url material is not permitted", )); } // Explicit AWS source credentials are all-or-nothing. Reject a partial pair // early so a lone key can't later fall through to the gateway's ambient // identity at mint time (CWE-20). let has_source_access_key = request .material .get("aws_access_key_id") .is_some_and(|value| !value.trim().is_empty()); let has_source_secret_key = request .material .get("aws_secret_access_key") .is_some_and(|value| !value.trim().is_empty()); if has_source_access_key != has_source_secret_key { return Err(Status::invalid_argument( "aws_access_key_id and aws_secret_access_key must both be set or both omitted", )); } // An optional session token supports temporary source credentials (SSO or a // prior AssumeRole). It only makes sense alongside the source key pair. let has_source_session_token = request .material .get("aws_session_token") .is_some_and(|value| !value.trim().is_empty()); if has_source_session_token && !has_source_access_key { return Err(Status::invalid_argument( "aws_session_token requires aws_access_key_id and aws_secret_access_key", )); } }🤖 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 2166 - 2201, Move the sts_endpoint_url, source access-key pair, and session-token validation behind the AwsStsAssumeRole strategy check in the surrounding provider request handling. Preserve all existing rejection rules for AwsStsAssumeRole, while allowing non-STS strategies to use these material keys without validation errors.
2322-2322: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply the configured expiry to every co-minted credential.
When
expires_at_msis supplied, the subsequent provider update sets it only forcredential_key. Existing secret/session values therefore remain injectable after their shared STS credentials expire until a successful mint replaces them.Proposed fix
- credential_expires_at_ms: std::collections::HashMap::from([( - credential_key.to_string(), - expires_at_ms, - )]), + credential_expires_at_ms: std::iter::once(credential_key.to_string()) + .chain(state_record.additional_output_keys.values().cloned()) + .map(|key| (key, expires_at_ms)) + .collect(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.credential_expires_at_ms: std::iter::once(credential_key.to_string()) .chain(state_record.additional_output_keys.values().cloned()) .map(|key| (key, expires_at_ms)) .collect(),🤖 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` at line 2322, Update the provider update logic surrounding additional_output_keys so a supplied expires_at_ms is applied to every co-minted credential, including existing secret and session values, rather than only credential_key. Ensure all credentials sharing the STS expiry become non-injectable when that expiry is reached.crates/openshell-server/src/middleware.rs (1)
8-19: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve middleware transport failures instead of mapping everything to
InvalidArgument.
validate_policy_configscan await remote middleware validation, so timeouts or service failures may escape alongside genuine invalid-policy errors. ReturningInvalidArgumentfor every failure misclassifies outages as client mistakes and prevents correct retry behavior. Preserve typed internal/transport errors and map only policy rejection toInvalidArgument.🤖 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/middleware.rs` around lines 8 - 19, Update validate_policy to preserve typed internal and transport errors returned by validate_policy_configs, mapping only genuine policy rejection errors to Status::invalid_argument. Keep the existing validation call and ensure remote timeouts or service failures remain classified for appropriate retry behavior.crates/openshell-server/src/persistence/sqlite.rs (1)
175-184: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve not-found handling when changing the CAS error contract.
Both backends now represent an absent CAS target as
Conflict(None), butComputeRuntime::set_supervisor_session_statestill ignores deletion races only when it receives the former"not found"database error.
crates/openshell-server/src/persistence/sqlite.rs#L175-L184: return an explicit not-found error or update all consumers to handleConflict(None)as absence.crates/openshell-server/src/persistence/postgres.rs#L167-L176: apply the same contract and consumer fix consistently.📍 Affects 2 files
crates/openshell-server/src/persistence/sqlite.rs#L175-L184(this comment)crates/openshell-server/src/persistence/postgres.rs#L167-L176🤖 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/persistence/sqlite.rs` around lines 175 - 184, Update ComputeRuntime::set_supervisor_session_state to treat PersistenceError::Conflict with current_resource_version: None as an absent target, preserving deletion-race handling. Apply the consistent CAS contract at crates/openshell-server/src/persistence/sqlite.rs:175-184 and crates/openshell-server/src/persistence/postgres.rs:167-176, where absent rows remain represented as Conflict(None); update any matching consumers rather than relying on the former database “not found” error.crates/openshell-server/src/provider_refresh.rs (2)
412-456: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The generation claim does not cover the provider write.
After Line 421 succeeds, a delete or reconfiguration can supersede the refresh before Line 436 writes the provider. The stale rotation still installs credentials; two rotations can also claim consecutive versions and apply in reverse order. Use a transaction or refresh lease/lock spanning the state claim and provider mutation, and add a test paused after the claim.
Also applies to: 491-547, 1994-2222
🤖 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/provider_refresh.rs` around lines 412 - 456, The generation claim in the refresh flow does not protect the subsequent provider credential mutation from deletion, reconfiguration, or competing rotations. Update the paths around persist_refresh_state_if_current and apply_minted_credential, including the other refresh implementations noted by the review, to hold a transaction or refresh lease/lock across the claim and provider write so stale or out-of-order rotations cannot install credentials. Add a test that pauses after the claim and verifies a concurrent deletion or reconfiguration prevents the provider mutation.
749-785: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the capped request lifetime for overflow-safe expiry calculations.
The STS request caps the lifetime to
i32, but fallback and maximum expiry calculations use the original unrestrictedi64. A large profile/material value can overflowmax_lifetime_i64 * 1000ornow_ms + ..., producing a panic or corrupted expiry.Proposed fix
let max_lifetime = i32::try_from(max_lifetime_i64.min(i64::from(i32::MAX))).unwrap_or(i32::MAX); + let max_expires = now_ms.saturating_add( + i64::from(max_lifetime).saturating_mul(1000), + ); ... let expires_at_ms = creds .expiration() .to_millis() - .unwrap_or_else(|_| now_ms + max_lifetime_i64 * 1000); - let max_expires = now_ms + max_lifetime_i64 * 1000; + .unwrap_or(max_expires);🤖 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/provider_refresh.rs` around lines 749 - 785, Update the expiry calculations in the AssumeRole flow to use the capped lifetime represented by max_lifetime, converting it to a safe i64 duration before multiplying by 1000 or adding to now_ms. Apply this capped duration both to the expiration fallback and max_expires calculations, while preserving the STS request lifetime behavior.crates/openshell-supervisor-network/src/l7/relay.rs (1)
2091-2130: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not record the request as allowed before middleware can reject it.
Lines 2069-2088 emit an
ActionId::Allowedevent, but this new fail-closed path can return without forwarding anything. Move that success event after middleware, credential injection, and relay completion to avoid contradictory audit records.🤖 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-supervisor-network/src/l7/relay.rs` around lines 2091 - 2130, Move the ActionId::Allowed audit event from its current pre-middleware location to after middleware processing, credential injection, and successful relay completion. Ensure every fail-closed return or middleware denial occurs before the allowed event is emitted, preserving allowed records only for requests actually forwarded successfully.crates/openshell-supervisor-network/src/proxy.rs (5)
438-444: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the middleware gate on the captured policy generation.
The queried generation is discarded. A reload can therefore remove a fail-closed chain between the L4 decision and this gate, allowing an uninspectable raw tunnel under mixed policy generations. Compare the returned generation with the connection’s captured generation and fail closed on mismatch.
🤖 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-supervisor-network/src/proxy.rs` around lines 438 - 444, Update middleware_uninspectable_gate to accept the connection’s captured policy generation, retain the generation returned by query_middleware_chain_with_generation, and compare them before constructing the gate. On any mismatch, fail closed instead of returning a gate derived from the newly loaded chain; preserve the existing gate construction for matching generations.
1249-1262: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Never write HTTP denial responses after
200 Connection Established. Once CONNECT succeeds, subsequent bytes belong to the tunneled protocol.
crates/openshell-supervisor-network/src/proxy.rs#L1249-L1262: move thetls: skipmiddleware gate before upstream connection and the200.crates/openshell-supervisor-network/src/proxy.rs#L1504-L1551: for unsupported tunneled protocols discovered after200, emit telemetry and close without writing a403.📍 Affects 1 file
crates/openshell-supervisor-network/src/proxy.rs#L1249-L1262(this comment)crates/openshell-supervisor-network/src/proxy.rs#L1504-L1551🤖 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-supervisor-network/src/proxy.rs` around lines 1249 - 1262, In crates/openshell-supervisor-network/src/proxy.rs:1249-1262, move the tls: skip middleware gate using middleware_uninspectable_gate before establishing the upstream connection and sending the 200 response, preserving the existing denial response there. In crates/openshell-supervisor-network/src/proxy.rs:1504-1551, for unsupported tunneled protocols discovered after CONNECT succeeds, emit the existing telemetry and close the connection without writing a 403 HTTP response.
3615-3620: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the HTTPS guidance response framing.
The body is 26 bytes, but
Content-Lengthdeclares 27, causing clients to treat the response as truncated.Proposed fix
- b"HTTP/1.1 400 Bad Request\r\nContent-Length: 27\r\n\r\nUse CONNECT for HTTPS URLs", + b"HTTP/1.1 400 Bad Request\r\nContent-Length: 26\r\nConnection: close\r\n\r\nUse CONNECT for HTTPS URLs",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if scheme == "https" { respond( client, b"HTTP/1.1 400 Bad Request\r\nContent-Length: 26\r\nConnection: close\r\n\r\nUse CONNECT for HTTPS URLs", ) .await?;🤖 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-supervisor-network/src/proxy.rs` around lines 3615 - 3620, Update the HTTPS branch in the proxy request handling flow to make the Content-Length header match the actual 26-byte “Use CONNECT for HTTPS URLs” response body, while preserving the existing 400 response and guidance.
4609-4677: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Run middleware before opening the upstream connection.
The upstream TCP connection is created at Lines 4572-4607 before middleware buffering, remote evaluation, or fail-closed denial. Denied or slow client requests can therefore consume upstream connections without forwarding authorized traffic. Move the connect step after middleware, credential processing, and the final generation check.
🤖 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-supervisor-network/src/proxy.rs` around lines 4609 - 4677, The upstream connection setup currently occurs before middleware evaluation and final policy validation. In the forward request flow surrounding the middleware pipeline, defer the upstream connect step until after credential processing, middleware application, and the final generation check, then use that connection for authorized forwarding while preserving fail-closed denial behavior.
4780-4800: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove query parameters from the final OCSF success event.
For non-L7 forward requests,
pathstill includes the absolute target’s query. BothOcsfUrland the message can therefore persist tokens or credentials.Proposed fix
+ let log_path = path.split_once('?').map_or(path.as_str(), |(path, _)| path); let event = HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) ... - OcsfUrl::new("http", &host_lc, &path, port), + OcsfUrl::new("http", &host_lc, log_path, port), ... - .message(format!("FORWARD allowed {method} {host_lc}:{port}{path}")) + .message(format!("FORWARD allowed {method} {host_lc}:{port}{log_path}"))As per coding guidelines, “Never log secrets, credentials, or query parameters in OCSF messages.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{ let log_path = path.split_once('?').map_or(path.as_str(), |(path, _)| path); let event = HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) .activity(ActivityId::Other) .action(ActionId::Allowed) .disposition(DispositionId::Allowed) .severity(SeverityId::Informational) .status(StatusId::Success) .http_request(HttpRequest::new( method, OcsfUrl::new("http", &host_lc, log_path, port), )) .dst_endpoint(Endpoint::from_domain(&host_lc, port)) .src_endpoint(Endpoint::from_ip(peer_addr.ip(), peer_addr.port())) .actor_process( Process::from_bypass(&binary_str, &pid_str, &ancestors_str) .with_cmd_line(&cmdline_str), ) .firewall_rule(policy_str, "opa") .message(format!("FORWARD allowed {method} {host_lc}:{port}{log_path}")) .build(); ocsf_emit!(event);🤖 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-supervisor-network/src/proxy.rs` around lines 4780 - 4800, Update the final OCSF success event construction around HttpActivityBuilder so query parameters are removed from the forwarded target before use. Ensure both OcsfUrl::new and the message passed to message use only the path component without the query, while preserving the existing host, port, and other event fields.Source: Coding guidelines
e2e/rust/tests/vm_gateway_resume.rs (1)
39-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed if
syncfails.Because the commands use
;, a missing or failedsyncstill emitsREADY_MARKER, allowing the test to pass without verifying durable state. Addset -eor explicitly exit on failure.Proposed fix
- "echo before-restart > {RESUME_FILE}; sync; echo {READY_MARKER}; while true; do sleep 1; done" + "set -e; echo before-restart > {RESUME_FILE}; sync; echo {READY_MARKER}; while true; do sleep 1; done"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."set -e; echo before-restart > {RESUME_FILE}; sync; echo {READY_MARKER}; while true; do sleep 1; done"🤖 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 `@e2e/rust/tests/vm_gateway_resume.rs` around lines 39 - 43, Update the shell script constructed in the test’s format! call to fail immediately when sync fails, by enabling errexit before writing and flushing the resume marker. Preserve the existing readiness marker and wait-loop behavior only after a successful sync.rfc/0009-supervisor-middleware/appendices/deployment-options.md (1)
29-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not claim sandbox isolation makes data leakage impossible.
A compromised middleware with no direct network egress could still encode observed data in its RPC result, transformed body, findings, or another permitted output path. Rephrase this as reducing direct exfiltration paths, consistent with the main RFC’s trust-boundary caveat.
Proposed wording
-A PII redactor with no network egress cannot leak what it sees, even if the image is compromised. +A PII redactor with no network egress has fewer direct exfiltration paths, but sandboxing does not by itself guarantee that a compromised middleware cannot leak inspected data through allowed outputs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Package the middleware as a container image and run it inside an OpenShell sandbox, then route egress content to it. The middleware would inherit sandbox isolation: policy-enforced egress, filesystem and syscall constraints, and no open internet access unless explicitly granted. This is the most direct answer to the trust concern. Instead of trusting the middleware not to exfiltrate the content it inspects, the operator constrains it the same way any other sandbox is constrained. A PII redactor with no network egress has fewer direct exfiltration paths, but sandboxing does not by itself guarantee that a compromised middleware cannot leak inspected data through allowed outputs. This option depends on sandbox-to-sandbox communication ([`#1049`](https://github.com/NVIDIA/OpenShell/issues/1049)), which is not available yet. When it lands, this becomes the most attractive shape for untrusted or third-party middleware.🤖 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 `@rfc/0009-supervisor-middleware/appendices/deployment-options.md` around lines 29 - 33, Revise the containerized OpenShell sandbox description to avoid claiming sandbox isolation prevents data leakage. In the paragraph beginning “This is the most direct answer,” state that sandboxing reduces direct exfiltration paths while acknowledging that compromised middleware could still leak data through permitted outputs such as RPC results, transformed bodies, or findings, consistent with the RFC’s trust-boundary caveat.
Use the logic for single-file upload from `sandbox_sync_up` to upload file with the correct destination name Update the E2E test for single-file upload from git repo to correctly assert the behavior Signed-off-by: Daniels Nagornuks <dnagornu@redhat.com>
rh-dnagornuks
force-pushed
the
sandbox-upload-fix
branch
from
July 21, 2026 08:40
c143a7b to
490726c
Compare
rh-dnagornuks
pushed a commit
that referenced
this pull request
Aug 13, 2026
…VIDIA#2271) * feat(sdk/go): add Go SDK foundation, types, and sandbox client (A) Add the Go SDK module with the full API contract and a working sandbox client as the first vertical slice. All other resource clients are present as stubs returning Unimplemented errors, to be replaced with real implementations in subsequent PRs. Contents: - Module setup (go.mod, Makefile, mise.toml) - All domain types (types/ package) - Full ClientInterface with all sub-client accessors - Shared infrastructure (errors, auth, gRPC connection, logging) - Sandbox client with converter and tests (fully functional) - Stub clients for remaining resources (exec, file, health, provider, profile, config, refresh, policy, service, ssh, tcp) Part of the Go SDK decomposition plan (NVIDIA#2270). Implements NVIDIA#2044. * fix(sdk/go): address review feedback on PR NVIDIA#2271 - Make scheme parsing drive transport selection: http:// uses plaintext gRPC, https:// or no scheme uses TLS. Add regression tests. - Add Resources and DriverConfig fields to SandboxTemplate and update both converter directions (SandboxFromProto/SandboxSpecToProto). - Regenerate proto bindings from current canonical proto sources to eliminate drift (SigV4/MCP fields, params matchers, reserved fields). - Run gofmt/goimports on all handwritten Go files. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(sdk/go): address principal engineer review findings - Remove dead boolCount function that would fail golangci-lint (#1) - Emit EventAdded for the first watch event instead of EventModified, matching k8s watch semantics (NVIDIA#7) - Add mutex locking to all mock server methods that access the shared sandboxes map, fixing latent race conditions (NVIDIA#12) - Skip HealthCheck integration test that calls an unimplemented stub (NVIDIA#13) - Scope doc.go examples: mark sections for sub-clients not yet available in this PR with "available in a future release" (NVIDIA#4) - Document Config.Timeout/RetryPolicy/Logger and WatchOptions fields as reserved for future use (NVIDIA#2, NVIDIA#6) Signed-off-by: Roland Huß <rhuss@redhat.com> * refactor(sdk/go): migrate mise config to centralized task include Move Go SDK mise configuration from standalone sdk/go/mise.toml into the project's centralized pattern: - Add Go tools (go, golangci-lint, protoc-gen-go, protoc-gen-go-grpc) to root mise.toml [tools] section - Create tasks/go.toml with all SDK tasks using go: namespace prefix and dir=sdk/go for working directory - Update sdk/go/Makefile to reference namespaced task names - Update proto:sync default path for monorepo layout Addresses review feedback from drew on PR NVIDIA#2271 regarding mise convention alignment. Signed-off-by: Roland Huß <rhuss@redhat.com> * refactor(sdk/go): remove UPSTREAM_VERSION standalone repo artifact Remove sdk/go/proto/UPSTREAM_VERSION file and its exclusion from proto:check. This was a leftover from the standalone repo prototype. In a monorepo, proto drift is detectable via git diff between sdk/go/proto/ and proto/ directly. Signed-off-by: Roland Huß <rhuss@redhat.com> * refactor(sdk/go): switch proto generation from protoc to buf Replace raw protoc invocations with buf for Go SDK proto code generation, aligning with the TS SDK approach (PR NVIDIA#2122). - Add repo-level buf.yaml declaring proto/ as the buf module with lint and breaking change detection config - Add sdk/go/buf.gen.yaml configuring buf to generate Go code directly from root proto/ (no more vendored .proto copies) - Delete vendored .proto source files from sdk/go/proto/ - Rewrite go:proto:gen and go:proto:check mise tasks to use buf - Remove go:proto:sync and go:proto:clean tasks (no longer needed) - Add proto target to sdk/go/Makefile - Add buf 1.72.0 to root mise.toml tool dependencies - Include options.proto in generation (was stripped from vendored copies) - Regenerate all .pb.go files via the new buf pipeline Signed-off-by: Roland Huß <rhuss@redhat.com> * test(sdk/go): add proto-converter field coverage detection Use protobuf reflection to enumerate all fields on key proto messages (SandboxSpec, SandboxTemplate, SandboxStatus, SandboxCondition, SandboxPolicy) and compare against explicit handled/skipped sets in the converter tests. Unhandled fields produce warnings (t.Log), not failures, so proto contributors are not forced to fix SDK converters in the same PR. Stale entries in the handled set (removed proto fields) do fail, since they indicate the converter references something that no longer exists. A follow-up CI workflow will create GitHub issues when converter drift lands on main. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(sdk/go): bump Go to 1.26 and fix errcheck lint violations The upstream go.mod now has `toolchain go1.26.4`, which requires Go 1.26 to build golangci-lint. Bump the mise.toml Go version from 1.25 to 1.26 and wrap deferred Close() calls in test helpers to satisfy errcheck. Assisted-By: 🤖 Claude Code * feat(sdk/go): add ObjectMeta fields (annotations, workspace, deletion_timestamp) Add three new proto ObjectMeta fields to Sandbox and Provider domain types: Annotations (map), Workspace (string), and DeletionTimestamp (*time.Time). Update converters in both directions, deep-copy maps at the proto/SDK boundary, and add TimeFromMillisPtr/MillisFromTimePtr helper functions. Assisted-By: 🤖 Claude Code * chore(sdk/go): regenerate proto bindings after rebase Pick up workspace fields from upstream PR NVIDIA#2445 (Wire authorization into workspace model). All request messages now include workspace parameter in the generated Go bindings. Assisted-By: 🤖 Claude Code * feat(sdk/go): add workspace scoping to all RPC interfaces Add workspace parameter to every sandbox-scoped RPC method across all interfaces (Sandbox, Exec, File, Service, SSH, TCP, Config, Policy, Provider, Profile, Refresh). The workspace string is passed as the second parameter after ctx, following the convention workspace then resource-name. Key changes: - SandboxInterface: all 10 methods gain workspace parameter - sandbox_client.go: passes Workspace field in every proto request - ListOptions: add AllWorkspaces field for cross-workspace queries - All stub interfaces updated to match new signatures - All sandbox client tests updated with "default" workspace Assisted-By: 🤖 Claude Code * chore(sdk/go): remove coverage.out from tracking Assisted-By: 🤖 Claude Code * fix(sdk/go): address review feedback from mrunalp - Add RefreshStrategyAWSStsAssumeRole to match proto enum value 6, fulfilling the "all domain types upfront" contract - Wrap context.DeadlineExceeded and context.Canceled in StatusError so IsDeadlineExceeded() and IsCancelled() helpers work correctly - Return error from mapToStruct/SandboxSpecToProto instead of silently discarding structpb.NewStruct failures on invalid template maps Signed-off-by: Roland Huss <rhuss@redhat.com> * fix(sdk/go): address remaining review items - Wire go:ci into root ci task so SDK is tested in repository CI - Fix gofmt formatting on converter files - Add goimports to mise.toml tools - Add coverage.out to .gitignore - Add Go SDK section to AGENTS.md and CONTRIBUTING.md - Add regression tests for context-error wrapping (IsDeadlineExceeded, IsCancelled) and invalid template map rejection - Remove panic from SandboxToProto, return error instead Signed-off-by: Roland Huss <rhuss@redhat.com> * fix(sdk/go): pin goimports version and update lockfile Pin goimports to 0.48.0 instead of "latest" and regenerate mise.lock to include the new entry. Signed-off-by: Roland Huss <rhuss@redhat.com> * fix(sdk/go): TLS.Insecure means skip-verify, not plaintext Align TLS.Insecure semantics with the Rust SDK: Insecure: true now uses TLS with InsecureSkipVerify (skip cert verification) instead of switching to plaintext. Only the http:// scheme triggers plaintext. This fixes token auth against dev/k3d gateways: StaticToken and RefreshableToken require transport security, which real TLS (even with InsecureSkipVerify) satisfies, but plaintext does not. For http:// + token auth (dev gateways without TLS), wrap the auth provider to override RequireTransportSecurity, matching the Rust SDK's behavior where http:// accepts any auth mode. Transport decision table (matches Rust SDK crates/openshell-sdk): http:// + any TLS config -> plaintext (TLS config ignored) https:// + Insecure: true -> TLS, skip cert verify https:// + Insecure: false -> TLS, full verification no scheme -> same as https:// Signed-off-by: Roland Huss <rhuss@redhat.com> * feat(sdk/go): add missing policy proto fields Add 6 previously silently dropped fields to the network policy types and converters, preventing security-relevant data loss on round-trip: NetworkEndpoint fields 19-23: - CredentialSigning: SigV4 re-signing mode - SigningService: AWS service name for SigV4 - SigningRegion: AWS region override for SigV4 - JsonRpcMaxBodyBytes: JSON-RPC body inspection limit - Mcp: MCP-specific policy options (new McpOptions type) L7Allow and L7DenyRule field 9: - Params: MCP params matcher map for tools/call filtering New type McpOptions with StrictToolNames and AllowAllKnownMcpMethods optional booleans matching the proto definitions. Signed-off-by: Roland Huss <rhuss@redhat.com> * fix(sdk/go): enforce coverage test and extend to policy messages Change coverage_test.go from t.Logf (silent) to t.Errorf so that unhandled proto fields fail the test immediately. Add coverage tests for NetworkEndpoint (23 fields), L7Allow (8 fields), L7DenyRule (8 fields), and McpOptions (2 fields). Any new proto field that is not in the handled set or explicitly skipped now breaks the build, closing the silent-drift gap. Signed-off-by: Roland Huss <rhuss@redhat.com> * ci(sdk/go): add Go SDK job to branch-checks workflow Add a Go SDK job to branch-checks.yml that runs mise run go:ci (lint, build, test, proto-check, docs-check) on every PR. This ensures the SDK is tested in CI, not just locally. Signed-off-by: Roland Huss <rhuss@redhat.com> * fix(sdk/go): address should-fix review items NVIDIA#6 Fix broken godoc examples: add workspace parameter to all method calls in doc.go that were broken after workspace scoping. NVIDIA#7 Add Err field to Event[T]: Watch error events now carry the underlying error instead of discarding it. NVIDIA#8 Separate Unauthenticated from PermissionDenied: add ErrorUnauthenticated code and IsUnauthenticated() helper. gRPC Unauthenticated (401) now maps to its own code instead of collapsing into PermissionDenied (403). NVIDIA#9 Add Unwrap to StatusError: replace dead Details field with Cause error field. StatusError.Unwrap() returns Cause, enabling errors.Is/As unwrapping. FromGRPCError and contextError both populate Cause. Signed-off-by: Roland Huss <rhuss@redhat.com> * ci(sdk/go): add go:format:check to CI pipeline Add gofmt format verification to go:ci. Catches unformatted Go files before they reach the PR. Fix formatting on coverage_test.go. Signed-off-by: Roland Huss <rhuss@redhat.com> * chore(sdk/go): remove Makefile in favor of mise tasks All build, lint, test, and proto-gen tasks are already defined in tasks/go.toml and invoked via mise. The Makefile was a leftover that duplicated this and raised questions in review. Signed-off-by: Roland Huß <rhuss@redhat.com> * feat(sdk/go): sync proto bindings and add credential handle support Regenerate Go proto bindings after rebase to pick up new CredentialHandle message and Provider.credential_handles and profile_workspace fields from upstream. Add domain types, converter support, and proto field coverage tests for Provider and CredentialHandle. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(sdk/go): reject plaintext auth leak and fix watch error handling Reject http:// addresses when the auth provider requires transport security instead of silently stripping the requirement. Remove the insecureAuthWrapper that overrode RequireTransportSecurity. Fix watch stream error handling: use blocking send for terminal errors so they are never silently dropped when the channel is full, and wrap mid-stream errors with converter.FromGRPCError so SDK error helpers like IsUnavailable work on watch Event.Err. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(sdk/go): address review findings from multi-agent code review - WaitReady now detects SandboxDeleting phase and returns immediately instead of polling indefinitely - Watch goroutine defers streamCancel() to prevent context leaks - Fix StopOnTerminal=false test to keep stream open (was wrong-reason pass due to stream ending, not StopOnTerminal logic) - Add EventDeleted test covering the Deleting phase branch - Add provider converter unit tests for CredentialHandle round-trip, nil handling, and empty maps Signed-off-by: Roland Huß <rhuss@redhat.com> --------- Signed-off-by: Roland Huß <rhuss@redhat.com> Signed-off-by: Roland Huss <rhuss@redhat.com>
|
This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. |
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.
Summary
sandbox uploadcreates a directory instead of a file at the destination path when the source file lives inside a git repository and the destination basename differs from the source basename. This extends the cp-style destination fix from PR NVIDIA#694 to cover the git-filtered upload path (sandbox_sync_up_files), which was missed in the original fix.Related Issue
Fixes: NVIDIA/OpenShell#1740
Changes
crates/openshell-cli/src/ssh.rs: Whensandbox_sync_up_filesreceives a single file and a destination that looks like a file path (doesn't end with/), apply the samesplit_sandbox_pathlogic assandbox_sync_up— split the destination into parent directory + target basename and useUploadSource::SinglePathwith the renamed tar entry. Multi-file and directory uploads are unchanged.e2e/rust/tests/sync.rs: Updatedupload_single_file_from_git_repo_only_uploads_that_fileto assert correct cp-style rename semantics instead of the previous broken directory-creation behavior. The test now uploads to a differently-named destination and verifies the file lands with the target basename.Testing
cargo check --package openshell-cli --features bundled-z3passesmise run pre-commitpasses (fails due to pre-existingannotationsfield breakage inopenshell-sdkonmain, but passes otherwise if the field is included)sandbox uploadcreates a directory instead of a file when source is inside a git repository NVIDIA/OpenShell#1740 confirms both git and non-git paths now produce a regular file at the destinationChecklist
Summary by CodeRabbit