fix(policy): refresh pending proposals when the sandbox policy changes - #3923
Conversation
Approving, removing, or undoing a rule, or updating the sandbox policy, changes the inputs every other pending proposal was evaluated against. Only proposals the new policy covered were reconciled; the rest kept their old prover result and review token. The review surface (GetDraftPolicy) therefore showed a stale evaluation, and the first approval of the next proposal refreshed it and failed with FAILED_PRECONDITION, so approving proposals one after another always failed once. Re-evaluate the remaining pending proposals at each policy change, reusing the cached prover result unless the proposal's inputs changed. Approval still rejects a review token that does not match the stored evaluation, so a reviewer holding a pre-refresh evaluation must still refetch it. When a refresh does happen at approval time (inputs changed between fetch and approve), the CLI now explains that the rule was re-evaluated and how to review it, instead of printing the raw gRPC status. Closes NVIDIA#3884 Signed-off-by: fede-kamel <fkamelhar@gmail.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This is a valid, focused fix for #3884, but the new refresh path can lose a concurrent proposal edit and can turn an agent-controlled pending queue into unbounded synchronous work during policy mutations. The user-facing refresh guidance also needs the corresponding Fern documentation update described by the linked issue.
Action required: @fede-kamel, please address the two inline Warnings and update the relevant docs/ workflow pages (including navigation only if needed).
Blocking findings:
GATOR-64662972-01: stale refresh persistence can overwrite a newer proposal edit.GATOR-64662972-02: an unbounded pending queue can amplify policy-mutation latency and the global mutation-lock hold time.GATOR-64662972-03: the direct CLI workflow change is missing its Fern documentation update.
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Project-valid concentrated policy correctness and CLI UX fix linked to reproducible issue #3884; no duplicate work found.
- Docs: Missing for the direct
rule approve/rule approve-allUX change; update the relevant Fern pages underdocs/. - Checks: DCO and vouch gates pass; required branch, Helm, Trivy, and E2E workflows are not yet all dispatched for this head.
- E2E:
test:e2ewill be required because the PR changes policy enforcement and gateway behavior, but dispatch waits until blocking review feedback is resolved. - Head SHA:
6466297265f699b1817f70a5d552c09acbff109e - Base SHA:
252882f37f2daa63ef4078b3bbb89adbcf43b409 - Merge base SHA:
0ea0d3102089ebaa4e891a12389056f545b48415 - Patch ID:
c372705fa20f61d8c7182fb423feafab0b776380 - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
the
gator:in-review
Store refreshed evaluations with a compare-and-swap: the store re-reads the proposal, refuses when its rule name, proposed rule, or review token changed since the evaluation read it, copies only the evaluation fields onto the stored record, and updates only if the payload is still the one it read. A refresh can no longer revert a concurrent edit or observation, and the edit path uses the same guard against a concurrent refresh. Bound each refresh to the 32 newest pending proposals; the rest keep the approval-time recheck, which still refuses a stale review token. Operator decisions (approve, approve-all, remove, undo) refresh before responding. UpdateConfig, which holds the gateway-wide sandbox sync guard, and agent-driven auto-approval refresh in a background task instead, one per sandbox with later changes coalesced into a single rerun. Refs NVIDIA#3884 Signed-off-by: fede-kamel <fkamelhar@gmail.com>
Explain that approving, removing, or undoing a rule rechecks the other pending proposals so they can be approved one after another, when the recheck is deferred or bounded, and what rule approve reports when a proposal changed after it was listed. Show rule approve-all in Run Your First Agent with its security-flag behavior. Refs NVIDIA#3884 Signed-off-by: fede-kamel <fkamelhar@gmail.com>
|
Addressed all three gator findings:
|
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @fede-kamel. I checked the compare-and-swap persistence, bounded/background refresh paths, regression coverage, and both Fern updates on the latest head. The stale-write and unbounded synchronous-work findings are resolved, and I resolved their Gator threads. One carried documentation/correctness obligation remains: the guide now says other policy updates refresh pending proposals in the background, but the full-policy UpdateConfig path used by openshell policy set returns after the matching atomic write before reaching the new background-refresh call. That can still leave rule get showing the stale candidate and make the next approval fail with the refresh precondition.
Action required: @fede-kamel, either schedule the background refresh before the full-policy matching-write return and add a regression test for that path, or narrow the documentation so it accurately preserves the approval-time recheck guidance for full policy replacements.
Blocking findings:
GATOR-64662972-03: the published proposal-review workflow is materially false for full sandbox policy replacement.
Carried findings:
GATOR-64662972-03: still open; no replacement finding or duplicate thread was created.
Gator metadata
- Validation: Project-valid concentrated policy correctness and CLI UX fix linked to #3884.
- Docs: Updated, but the background-refresh guarantee is inaccurate for full-policy
UpdateConfig. - Checks: Current-head branch and Helm gates remain pending; required E2E dispatch waits until blocking review feedback is resolved.
- E2E:
test:e2eremains required because this changes policy enforcement and gateway behavior. - Head SHA:
2373b2b9e1ed6c02070cd944a3022f08db507950 - Base SHA:
252882f37f2daa63ef4078b3bbb89adbcf43b409 - Merge base SHA:
0ea0d3102089ebaa4e891a12389056f545b48415 - Patch ID:
3ca02bc7184f1790f4a2ca81461d100567fa6698 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
6466297265f699b1817f70a5d552c09acbff109e - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
A full policy UpdateConfig (openshell policy set) re-reads the latest revision after its atomic write, finds the revision it just committed, and returns before reaching the pending-proposal refresh at the end of the handler. Pending proposals kept their stale evaluation, so rule get showed the old candidate and the next approval failed with the refresh precondition. Schedule the background refresh right after the commit. Refs NVIDIA#3884 Signed-off-by: fede-kamel <fkamelhar@gmail.com>
|
Thanks, confirmed: after the full-policy atomic write, the handler re-reads the revision it just committed and returns through the matching-revision branch, so the refresh at the end of the handler was unreachable for GATOR-64662972-03 fixed in aea952b by scheduling the background refresh immediately after the commit, before that return. The dedup return earlier in the handler (policy unchanged) correctly schedules nothing. The later call stays for the fallthrough case, and the per-sandbox coalescing folds any duplicate into one rerun, so the docs' "other policy updates recheck pending proposals in the background" is now accurate. Regression test
|
|
Probed live on Windows 11 + WSL 2 with Docker Desktop 4.71, using the reproduction from #3884. A sandbox opens two blocked connections (
On 0.1.2, the identical retry succeeds without anyone looking at the refreshed proposal. With this PR, the applied policy hash equals the candidate This run predates aea952b, the |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @fede-kamel. I checked your fix for the full-policy replacement path against the prior carried obligation: the background refresh is now scheduled immediately after the atomic policy commit and before the matching-revision return, and the new regression test exercises handle_update_config and observes the pending proposal token change. GATOR-64662972-03 is resolved, so no blocking findings remain.
Blocking findings:
- No blocking findings remain.
Carried findings:
GATOR-64662972-03: resolved by the current-head scheduling fix and regression coverage.
Gator metadata
- Validation: Project-valid concentrated policy correctness and CLI UX fix linked to #3884.
- Docs: Fern guidance is now accurate for full-policy replacements and the sequential approval workflow.
- Checks: Current-head DCO and Trivy gates are green; branch, Helm, and required E2E dispatch remain to be confirmed.
- E2E:
test:e2eis required because the PR changes policy enforcement and gateway behavior; dispatch follows this review. - Head SHA:
aea952bc77d8abc6409cddccf5000b8a3886c637 - Base SHA:
252882f37f2daa63ef4078b3bbb89adbcf43b409 - Merge base SHA:
0ea0d3102089ebaa4e891a12389056f545b48415 - Patch ID:
b116e533cb08da488b231a2f38d515e889292dc9 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
2373b2b9e1ed6c02070cd944a3022f08db507950 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
|
Label |
|
/ok to test aea952b |
johntmyers
added
gator:watch-pipeline
|
Went through the E2E failures across the three attempts of run 36726439249. Each attempt failed a different job, each time while starting a sandbox, and each failed job passed on another attempt with the same head (aea952b):
On attempt 3 both Kubernetes jobs and the Podman job passed. Why I don't think this PR causes them:
Could someone rerun the failed conformance job? |
johntmyers
added
test:e2e
|
Label |
johntmyers
added
gator:approval-needed
|
Follow-up to the CI analysis above: I also ran the full conformance suite against this PR's head outside CI, and attempt 4 has since passed here too. Setup: a fresh Ubuntu 24.04 VM (12 vCPU, 24 GB, Docker 29.1.3), a release build of aea952b (
30 of 30 scenario runs passed, including |
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: Gator review feedback was resolved, maintainer approval was present, and the required branch, Helm, Trivy, and E2E checks passed before merge. I removed the active |
johntmyers
removed
the
gator:approval-needed
2 participants
Summary
Approving pending policy proposals one after another always failed once with
FAILED_PRECONDITION: proposal inputs changed; evaluation refreshed. After a policy change the gateway only reconciled proposals the new policy covered; the rest kept their old prover result and review token, sorule getshowed a stale evaluation and the next approval refreshed it and failed. This PR re-evaluates the remaining pending proposals at every policy change, and makes the CLI explain the refresh when it still happens.Related Issue
Closes #3884
Changes
openshell-server(grpc/policy.rs): newrefresh_pending_chunk_evaluationsre-evaluates every pending proposal against live inputs and persists the result. It reuses the cached prover result and reruns the prover only when the proposal's review token changed.reconcile_pending_chunks_after_policy_change), removal of an approved rule, undo, and bothUpdateConfigpolicy writes.openshell-cli:rule approveandrule approve-allturn the refresh precondition into an actionable error with aopenshell rule get <sandbox> --status pendinghint instead of the raw gRPC status.Testing
mise run pre-commitpassesapproval_refreshes_other_pending_proposals_for_sequential_review: approve A, then B's refreshed token (asrule getreturns it) approves first time, and B's pre-refresh token is rejected with "refetch and review again".draft_receipts_replay_after_chunk_state_and_review_tokens_changecovers the undo path (it failed until undo refreshed too).draft_approval_error_explains_refreshed_evaluationfor the CLI message.cargo test -p openshell-server --features bundled-z3: 1861 passed.cargo test -p openshell-cli: all pass exceptssh::tests::launch_editor_returns_friendly_error_when_binary_missing, which fails identically on unmodifiedmainin this environment.Checklist