Skip to content

fix(sandbox): acknowledge initial policy revision; expose SDK labels/selectors#2170

Merged
johntmyers merged 10 commits into
NVIDIA:mainfrom
KyleZheng1284:fix/2159-initial-policy-ack-and-sdk-labels/KyleZheng1284
Jul 9, 2026
Merged

fix(sandbox): acknowledge initial policy revision; expose SDK labels/selectors#2170
johntmyers merged 10 commits into
NVIDIA:mainfrom
KyleZheng1284:fix/2159-initial-policy-ack-and-sdk-labels/KyleZheng1284

Conversation

@KyleZheng1284

@KyleZheng1284 KyleZheng1284 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Running into two lifecycle/API gaps that prevent consumers from
reliably managing policy-bound sandboxes.

First, a sandbox can reach Ready with an effective policy while the exact
initial sandbox-scoped policy revision remains Pending and
current_policy_version remains 0. This occurs when startup enriches the
policy before constructing the OPA engine: the policy is enforced successfully,
but the initial revision is never acknowledged through
ReportPolicyStatus. This PR reports that revision as LOADED after successful
construction, or FAILED with the original error when construction fails.

Second, the gateway already supports request-level sandbox labels and
selector-based listing, but the public Python SDK does not expose them. This PR
adds creation-time name and labels, selector-based list and list_ids,
and immutable gateway labels on SandboxRef.

These changes let clients verify that the gateway’s declared policy state
matches the policy OpenShell actually enforces and reliably discover sandboxes
by workload metadata. They do not weaken attestation or introduce a new policy
state model; they make the existing state authoritative and accessible through
the Python SDK.

Related Issue

Closes #2159

Changes

fix(sandbox): acknowledge initial policy revision (+ retry-bound follow-up)

  • After OpaEngine::from_proto() succeeds, acknowledge the exact sandbox-scoped
    revision the supervisor loaded (ReportPolicyStatus(LOADED)), and seed the poll
    loop from that revision so it is not re-reported.
  • Report FAILED with the original construction error if engine construction or
    policy conversion fails.
  • Only sandbox-sourced revisions (PolicySource::Sandbox, version > 0) whose
    canonical content matches the loaded policy are acknowledged; global and
    local-file policies are untouched.
  • Delivery uses the existing bounded gRPC retry, is non-fatal on transient
    failure, and a pending initial acknowledgement is delivered before any newer
    revision so policy history is never reordered. The pending retry is itself
    bounded so a permanently-undeliverable ack cannot stall later hot-reloads.
    Existing hot-reload behavior is unchanged.

feat(python): expose sandbox labels and selectors (+ hashability follow-up)

  • Additive name/labels on SandboxClient.create, create_session, and the
    high-level Sandbox; label_selector on list/list_ids.
  • SandboxRef carries gateway labels as an immutable mapping (default empty, so
    SandboxRef(id, name, status) still works); labels are excluded from
    equality/hash to preserve the original identity and keep the frozen dataclass
    hashable.
  • Attaching the high-level Sandbox to an existing sandbox rejects name/labels.
    Caller label mappings are copied. Template labels remain a separate concept.

No protobuf, database, or configuration changes.

Testing

Validated locally on macOS 26 / Apple Silicon with Docker Desktop. Branch
Checks pass on Linux amd64 and arm64. Linux hard_requirement acceptance has
not been validated.

  • Python unit tests (mise run test:python, 72 passed) — label/selector
    forwarding, immutable-labels, attach-rejection, SandboxRef-hashability.
  • Rust unit tests (cargo test -p openshell-sandbox, 72 passed) for the
    initial-ack candidate / structural-match helpers.
  • cargo clippy -p openshell-sandbox --all-targets — 0 warnings; cargo fmt
    and ruff check/ruff format --check clean.
  • Branch Checks pass on Linux amd64 and arm64.
  • Docker E2E regression (branch-built gateway + openshell/supervisor:dev):
    initial_sparse_policy_is_acknowledged_as_loaded — sparse policy enriches to
    revision 2 with no revision left Pending. (See validation comment below.)

Known limitations — remaining before merge

  • Full mise run e2e:rust suite not gated here — the fix(sandbox): acknowledge initial policy load and expose SDK labels #2159 regression above
    passes in isolation; the full Docker suite has broader environment needs.
  • Initial-revision acknowledgement uses a re-fetch + normalized structural
    match
    (provider rule names stripped, version zeroed) rather than binding to
    the exact canonical revision object. Wants review for provider-composed policies.
  • Linux hard_requirement acceptance not run.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (architecture/sandbox.md, docs/sandboxes/manage-sandboxes.mdx)

Ready for maintainer review. Unit, lint, formatting, and Branch Checks pass on
Linux amd64 and arm64. The targeted Docker regression and AI-Q integration pass
on macOS. Remaining review areas are provider-composed policy matching and Linux
hard_requirement acceptance.

@copy-pr-bot

copy-pr-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@KyleZheng1284 KyleZheng1284 force-pushed the fix/2159-initial-policy-ack-and-sdk-labels/KyleZheng1284 branch from b33c83f to 1819886 Compare July 8, 2026 03:44
@copy-pr-bot

copy-pr-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@KyleZheng1284 KyleZheng1284 force-pushed the fix/2159-initial-policy-ack-and-sdk-labels/KyleZheng1284 branch from cb3efa5 to eacda4d Compare July 8, 2026 03:58
@KyleZheng1284

Copy link
Copy Markdown
Contributor Author

Local validation (macOS 26 / Apple Silicon, Docker Desktop)

Docker E2E regression — branch-built gateway + supervisor. The harness
compiled this branch into the running components (openshell-server gateway,
openshell-cli, and openshell-sandbox baked into openshell/supervisor:dev),
started a live Docker gateway, and ran the new regression against it:

running 1 test
test initial_sparse_policy_is_acknowledged_as_loaded ... ok
test result: ok. 1 passed; 0 failed; 2 filtered out

The test creates a sandbox from the network-only
examples/policy-advisor/sandbox-policy.yaml, which the supervisor enriches
with baseline filesystem paths at startup (creating revision 2, superseding
revision 1), and asserts the effective policy reaches revision 2 with no
revision left Pending.

Unit / lint (macOS):

  • cargo test -p openshell-sandbox — 72 passed, 0 failed
  • cargo clippy -p openshell-sandbox --all-targets — 0 warnings
  • cargo fmt — clean
  • mise run test:python — 71 passed; ruff check / ruff format --check clean

Downstream integration (AI-Q). Concurrent per-job sandboxes each received a
unique sandbox id + name (shared=False), and every one attested
policy_version=1 with zero attestation errors — the initial revision was
acknowledged on the no-enrichment path. Together with the E2E above
(enrichment → rev 2), both acknowledgement branches are exercised.

Not covered here: Linux hard_requirement acceptance, and self-contained
Z3 packaging (bundled-z3) for release artifacts.

@KyleZheng1284 KyleZheng1284 marked this pull request as ready for review July 8, 2026 04:27
@KyleZheng1284

Copy link
Copy Markdown
Contributor Author

Root cause: ReportPolicyStatus(LOADED) only fires in the poll loop's hot-reload branch, but the loop seeds its hash from the first GetSandboxConfig — so an enriched initial revision already matches the seed, policy_changed is never true, and the load is never reported.

Fix: Report the exact loaded revision once after OpaEngine::from_proto() succeeds (or FAILED with the original error) and seed the poll loop from it so it isn't re-reported. Reuses the existing bounded retry, stays non-fatal, leaves hot-reload untouched, and makes no proto/DB/config changes.

The supervisor loaded and enforced a sandbox-scoped policy but never told
the gateway which revision it loaded. The policy poll loop seeded itself
with the initial revision's hash on its first poll, so `policy_changed`
was never true for that revision and `ReportPolicyStatus(LOADED)` — which
only ran in the hot-reload branch — was never called. The revision stayed
`Pending` and `current_policy_version` stayed 0 even though the sandbox was
`Ready` and the policy was effective. This was most visible with sparse
policies that get baseline-enriched into a new revision during startup.

After the OPA engine is constructed, report the exact sandbox revision the
supervisor loaded as LOADED, and seed the poll loop from that revision so
it is not re-reported. Report FAILED with the original construction error
if engine construction or conversion fails. Only sandbox-sourced revisions
(version > 0) whose canonical content matches the loaded policy are
acknowledged; global and local-file policies are untouched. Delivery uses
the shared bounded retry, is non-fatal on transient failure, and a pending
initial acknowledgement is delivered before any newer revision so policy
history is never reordered.

Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
The gateway protobuf and CLI already support request-level sandbox labels
(`CreateSandboxRequest.name`/`labels`) and selector-based listing
(`ListSandboxesRequest.label_selector`), but the public Python SDK dropped
them, so Python-created sandboxes could not be found via
`openshell sandbox list --selector ...`.

Add optional, source-compatible `name`/`labels` to `SandboxClient.create`,
`create_session`, and the high-level `Sandbox`, and `label_selector` to
`list`/`list_ids`. `SandboxRef` now carries the gateway labels as an
immutable mapping (default empty, so `SandboxRef(id, name, status)` still
works). Caller-provided label mappings are copied. Attaching the high-level
`Sandbox` to an existing sandbox rejects `name`/`labels` since creation
metadata cannot change on attach. Template labels remain a separate concept.
No protobuf changes are required.

Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Excluding the new immutable `labels` field from SandboxRef equality/hash
(`compare=False`) preserves the original (id, name, status) identity and keeps
the frozen dataclass hashable — a MappingProxyType field would otherwise make
`hash(SandboxRef(...))` raise. Also defensively copy caller-provided labels in
the high-level `Sandbox` so later caller mutation cannot change what is sent.

Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
The poll loop retried a pending initial acknowledgement before processing any
newer revision, but retried unconditionally forever. A permanently
undeliverable ack (e.g. the revision was superseded before it could be
reported) would then stall all later policy hot-reloads and provider-env
refreshes. Cap the retries; after the bound, give up and resume normal polling
so the loop cannot livelock on a stuck acknowledgement.

Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Regression for NVIDIA#2159: create a sandbox with the network-only policy-advisor
fixture, which the supervisor enriches with baseline filesystem paths during
startup (creating revision 2, superseding revision 1). Assert the effective
policy reaches revision 2 and no revision remains Pending once the supervisor
acknowledges the load. Adds SandboxGuard::create_keep_with_args to create a
kept sandbox with an initial --policy.

Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
@KyleZheng1284 KyleZheng1284 force-pushed the fix/2159-initial-policy-ack-and-sdk-labels/KyleZheng1284 branch from eacda4d to e7c3790 Compare July 8, 2026 04:36
@KyleZheng1284

Copy link
Copy Markdown
Contributor Author

recheck

@johntmyers

Copy link
Copy Markdown
Collaborator

@KyleZheng1284 please address the failing tests and DCO statement. you need to post the actual DCO text.

@KyleZheng1284

Copy link
Copy Markdown
Contributor Author

I have read the DCO document and I hereby sign the DCO.

Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Principal-engineer code review found three actionable issues. Each inline comment includes a concrete sequence showing how the problem would surface in practice. Tests and CI were not run as part of this code-only review.

Comment thread crates/openshell-sandbox/src/lib.rs Outdated
Comment thread crates/openshell-sandbox/src/lib.rs
Comment thread python/openshell/sandbox.py Outdated
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
@KyleZheng1284 KyleZheng1284 requested a review from johntmyers July 8, 2026 21:11
KyleZheng1284 and others added 2 commits July 8, 2026 16:00
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Jul 8, 2026
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Label test:e2e applied for f9448b7. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers johntmyers merged commit ff9af8e into NVIDIA:main Jul 9, 2026
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(sandbox): acknowledge initial policy load and expose SDK labels

2 participants