fix(sandbox): acknowledge unchanged policy revisions

[NaveCohenMonday]

Summary

Decouple sandbox policy-revision acknowledgement from OPA policy loading. A
newer sandbox revision whose effective policy hash is already loaded can now be
acknowledged without reloading identical policy content, allowing desired and
current revisions to converge.

Related Issue

Fixes #2518

Changes

• Track the acknowledged sandbox policy version separately from the loaded
  policy hash.
• Acknowledge only newer, sandbox-scoped revisions with the same non-empty
  effective policy hash.
• Wait for required policy-runtime or middleware reconciliation to succeed
  before acknowledging the revision.
• Exclude global policy, local overrides, equal or older versions, empty
  hashes, and different policy content from the shortcut.
• Emit acknowledgement success telemetry only after the gateway accepts the
  loaded-status report.
• Document the same-hash revision acknowledgement invariant.

Testing

• CARGO_INCREMENTAL=0 mise run pre-commit passes
• Unit tests added/updated
• E2E tests added/updated (maintainer requested test:e2e after review)

Validation:

CARGO_INCREMENTAL=0 mise exec -- cargo test -p openshell-sandbox --lib

test result: ok. 99 passed; 0 failed

CARGO_INCREMENTAL=0 mise exec -- cargo test -p openshell-sandbox \
  same_hash_ --lib

test result: ok. 3 passed; 0 failed

The scripted poll-loop tests cover v1/H → v2/H exactly-once acknowledgement
without an OPA reload, failed middleware reconciliation with no premature
status followed by retry and one acknowledgement, and the local/global,
changed/empty-hash, equal, and older-version scopes.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated

[copy-pr-bot]

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]

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

[NaveCohenMonday]

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

[drew]

gator-agent

PR Review Status

Validation: This concentrated sandbox-policy fix directly addresses confirmed issue #2518 and has no duplicate candidate.
Head SHA: 182ef51768dfa66becb6847e0349170621cbddab

Review findings:

• Warning — missing stateful regression coverage: the added predicate tests do not exercise the poll loop, so they cannot catch duplicate or premature acknowledgements. Please add a scripted-poll test proving v1/H → v2/H emits exactly one LOADED(2) without reloading OPA, a failed required middleware reconciliation emits nothing and retries, and a later success emits exactly one acknowledgement. Include the relevant negative scopes (local override, global source, changed/empty hash, and equal/older version).
• See the inline warning about emitting success telemetry before gateway acknowledgement succeeds.

Docs: The architecture invariant is documented. No Fern docs/navigation update is required because this fixes existing --wait semantics without changing a command or user workflow.

Tests: No local tests were run. The author reports pre-commit and two focused predicate tests passing; required CI is still waiting for the copy-PR mirror.

Next state: gator:in-review pending an author update. A test:e2e pipeline run will be required after review feedback is resolved because this changes sandbox policy-enforcement reconciliation.

[NaveCohenMonday]

Addressed in c20b6b8c.

The poll loop now accepts a scripted gateway and middleware connector in tests,
while production continues to use the cached gRPC client and normal registry
connector. The new stateful coverage proves:

• v1/H → v2/H emits exactly one LOADED(2) and leaves the OPA generation
  unchanged;
• a failed required middleware reconciliation emits no LOADED(2);
• the identical next poll retries reconciliation, and a later success emits
  exactly one LOADED(2);
• local override, global source, empty hash, equal version, and older version
  do not use the shortcut;
• a changed hash follows the normal OPA reload and status path.

I also carried the acknowledgement event kind and policy hash in
PolicyStatusUpdate. The OCSF success event now emits from
run_policy_status_reporter only after report_policy_status returns
successfully.

Validation:

CARGO_INCREMENTAL=0 mise exec -- cargo test -p openshell-sandbox --lib
test result: ok. 99 passed; 0 failed

CARGO_INCREMENTAL=0 mise run pre-commit
Finished successfully

I additionally mutation-checked the two critical guards: allowing equal
versions caused the exactly-once test to fail, and allowing acknowledgement
before runtime reconciliation caused the failed-middleware test to fail.

[drew]

gator-agent

Re-check After Author Update

Thanks @NaveCohenMonday. I reviewed current head c20b6b8c365dd5821594673d5fe06bf4cbe2ae93 after your update explaining the new scripted poll-loop coverage and post-RPC acknowledgement telemetry.

Validation: This concentrated sandbox-policy fix directly addresses confirmed issue #2518.
Head SHA: c20b6b8c365dd5821594673d5fe06bf4cbe2ae93

What I checked: the full current-head diff, the same-hash candidate and reconciliation gates, status-reporter delivery path, stateful positive/negative poll tests, and the architecture invariant.

Disposition: the two prior gator findings are resolved. The poll-loop tests now cover same-hash exactly-once acknowledgement, middleware failure/retry, and the requested negative scopes; success OCSF telemetry now follows a successful gateway status RPC. The independent review found no blocking correctness or security issue. It left one non-blocking determinism warning and one optional telemetry-test suggestion inline.

Docs: architecture/sandbox.md accurately documents the invariant. No Fern docs/navigation update is required because this fixes existing semantics without adding a direct UX workflow.

Tests: Code-only review; no local tests were run. A test:e2e pipeline run is required because this changes sandbox policy-enforcement reconciliation.

Next state: gator:watch-pipeline.

[github-actions]

Label test:e2e applied, but pull-request/2557 does not exist yet. A maintainer needs to comment /ok to test c20b6b8c365dd5821594673d5fe06bf4cbe2ae93 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

[drew]

/ok to test c20b6b8

[NaveCohenMonday]

Rebased onto current main (736e431d) to resolve the conflict with the proxy-egress/runtime reconciliation changes from #2373.

The resolution preserves upstream's typed PolicyValidation vs MiddlewareRegistry failure handling and routes the existing injected test connector through that helper. It also handles the new overlap with fail-closed recovery: recovery takes precedence over the ordinary same-hash path, so restoring the last-known-good hash emits only one LOADED acknowledgement.

Validation on new head 4e6ed1ba77d3b45c62414a51328804e8cf917819:

cargo test -p openshell-sandbox
107 library tests, 5 binary tests, and 1 integration test passed

mise run pre-commit
passed

I also mutation-checked both conflict-sensitive guards: changing the newer-version comparison from > to >= fails the strict-scope test, and disabling recovery precedence fails its new assertion.

The force-push invalidated the previous copy-PR approval, so the new head will need /ok to test 4e6ed1ba77d3b45c62414a51328804e8cf917819 from a maintainer.

[johntmyers]

/ok to test 4e6ed1b

[johntmyers]

gator-agent

Re-check After Author Update

Thanks @NaveCohenMonday. I reviewed current head 4e6ed1ba77d3b45c62414a51328804e8cf917819 after your August 2 update explaining the rebase across the proxy-egress/runtime reconciliation changes and the fail-closed recovery overlap.

Head SHA: 4e6ed1ba77d3b45c62414a51328804e8cf917819
Base SHA: 736e431d454c7de8a71e0fcdd3221ad6f9a552cb
Merge base SHA: 736e431d454c7de8a71e0fcdd3221ad6f9a552cb
Patch ID: 76105582c3c712f7f4d52b37605c7b6515c2d860
Gator payload: 3
Review mode: follow_up
Previous reviewed SHA: c20b6b8c365dd5821594673d5fe06bf4cbe2ae93

What I checked: the durable Gator feedback ledger, the author-only range-diff, the runtime/middleware conflict resolution, and recovery precedence for a restored last-known-good hash. Drew's prior findings and their resolutions remain preserved; I did not reopen or duplicate them.

Disposition: resolved. The rebase preserves the prior acknowledgement and post-RPC telemetry fixes, recovery takes precedence over the ordinary same-hash acknowledgement path, and the independent follow-up review found no new blocking findings.

Blocking findings:

• No blocking findings remain.

Carried findings:

• None. The prior Gator findings remain resolved.

Docs: architecture/sandbox.md remains sufficient; this fixes existing behavior without introducing a new direct UX workflow.

Tests: test:e2e remains required because this changes sandbox policy-enforcement reconciliation. I posted /ok to test 4e6ed1ba77d3b45c62414a51328804e8cf917819 for the rebased head so the copy-PR pipeline can proceed.

Next state: gator:watch-pipeline.

[johntmyers]

@NaveCohenMonday you're hitting some flakey tests which have been turned off as of today. i'd rebase to see if we can get the pipeline green so we can prep to merge