fix(policy): validate the applicable advisor candidate (#2850)

* fix(policy): bind reviews to applicable candidates

Build and validate the exact effective-policy candidate before approval, bind review to live policy/provider/credential inputs, and preserve inspected endpoint contracts during mechanistic expansion.

Closes #2821

Signed-off-by: John Myers <johntmyers@users.noreply.github.com>

* fix(policy): canonicalize advisor review inputs

Serialize nested protobuf maps in stable key order for proposal review tokens and effective-policy hashes. Narrow reused multi-port endpoint contracts to the denied port so advisor proposals cannot widen binary access. Add regressions for both cases.

Signed-off-by: John Myers <johntmyers@users.noreply.github.com>

* test(e2e): keep advisor sandbox running

Create the issue 2821 regression sandbox detached with a durable canonical main process so policy denial, approval, and hot-reload checks run before lifecycle exit.

Signed-off-by: John Myers <johntmyers@users.noreply.github.com>

* fix(policy): apply reviewed draft batches atomically

Signed-off-by: John Myers <johntmyers@users.noreply.github.com>

---------

Signed-off-by: John Myers <johntmyers@users.noreply.github.com>
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>

Author: johntmyers

.agents/skills/openshell-cli/SKILL.md

@@ -493,7 +493,7 @@ Avoid `--yes` during interactive work. A global policy locks policy control for
 
 ### Review agent-authored rule proposals
 
-Sandboxes created with `--approval-mode manual` place every proposal in the review inbox. `auto` approves only proposals with an empty prover delta; findings still require review.
+Sandboxes created with `--approval-mode manual` place every proposal in the review inbox. `auto` approves only valid effective-policy candidates with an empty prover delta; findings still require review. The CLI binds approval to the candidate's current review token. If live policy, provider, or credential inputs change, approval leaves the chunk pending with a refreshed candidate and requires a fresh review.
 
 ```bash
 openshell rule get dev --status pending
@@ -502,7 +502,7 @@ openshell rule reject dev --chunk-id <chunk-id> --reason "too broad"
 openshell rule history dev
 ```
 
-Review the proposed scope and prover findings before approval. Treat `rule approve-all --include-security-flagged` as a high-risk bulk action.
+Review the proposed scope, candidate hash, prover findings, and application errors before approval. Treat `rule approve-all --include-security-flagged` as a high-risk bulk action.
 
 ---
 

.agents/skills/openshell-cli/cli-reference.md

@@ -471,7 +471,7 @@ Review agent-authored network rule proposals. This command group is intentionall
 - `openshell rule clear [name]`
 - `openshell rule history [name]`
 
-Sandbox names default to the last-used sandbox. Bulk approval of security-flagged proposals requires explicit `--include-security-flagged`.
+Sandbox names default to the last-used sandbox. The CLI fetches and submits each proposal's current review token; a changed live candidate remains pending until it is reviewed again. Bulk approval of security-flagged proposals requires explicit `--include-security-flagged`.
 
 ---
 

architecture/security-policy.md

@@ -216,24 +216,29 @@ through the proposal loop instead of treating the denial as terminal.
 
 1. **Submit.** Both proposers POST through the same `SubmitPolicyAnalysis`
    path. Each chunk is persisted with its `analysis_mode` for audit provenance.
-2. **Validate.** The gateway runs the prover (`openshell-prover`) on every
-   chunk regardless of mode. The prover builds a Z3 model from the merged
-   policy plus the sandbox's attached-provider credential set, then computes
-   the delta of findings between the current baseline and the merged policy.
+2. **Build and validate the candidate.** The gateway first canonicalizes a
+   mechanistic proposal against the live effective policy. If an endpoint is
+   already governed by an inspected or provider-owned contract, the candidate
+   preserves that contract and adds only the proposed sandbox binary. Provider
+   rules are immutable inputs; the sandbox contribution is stored as an
+   overlay. The gateway then performs the same merge, policy validation,
+   provider composition, credential preflight, and prover evaluation that the
+   candidate would encounter when applied. Each chunk stores the resulting
+   effective candidate, its hashes, any application error, and a review token
+   derived from the candidate and its non-secret live inputs.
 3. **Auto-approval gate (proposer-agnostic, opt-in).** Auto-approval fires
    only when *all three* conditions hold: (a) `proposal_approval_mode`
    resolves to `"auto"` — gateway scope wins, sandbox scope is the
    per-sandbox override, default is `"manual"`; (b) the prover delta is empty
    (`prover: no new findings`); and (c) the security notes recomputed from
    the chunk's current proposed rule are empty (see
    [Security-notes gate](#security-notes-gate)). Before merging, the gateway
-   reloads the stored chunk and reruns both checks on its current rule. This is
-   important after edits and mechanistic deduplication: the stored rule, not a
-   duplicate incoming payload or stale persisted analysis, controls the
-   decision. The recalculated prover verdict is decision-local rather than
-   persisted, so `validation_result` reads can still show the submit-time
-   verdict after an edit or deduplication. Decode, prover, or merge failures
-   leave the chunk pending. The audit event uses `CONFIG:APPROVED` and carries
+   reloads the stored chunk and recomputes its candidate from live policy,
+   provider, and credential inputs. If the review token is unchanged, the
+   gateway reuses the persisted prover result. If it changed, the gateway
+   persists the refreshed candidate and requires a fresh review instead of
+   applying it. Decode, prover, merge, provider-composition, or credential
+   failures leave the chunk pending with an application error. The audit event uses `CONFIG:APPROVED` and carries
    `auto=true`, `source=<mode>`, `prover_delta=empty`, and
    `resolved_from=<gateway|sandbox>` as unmapped fields, with message text
    `"auto-approved: no new prover findings"` — never `safe`. The opt-in gate
@@ -259,6 +264,10 @@ through the proposal loop instead of treating the denial as terminal.
    policy.
 6. **Escalation.** Anything else lands in `pending` for human review.
 
+After any successful policy write, pending chunks already covered by the new
+live effective policy are rejected as redundant. This keeps the review inbox
+aligned with what the sandbox currently enforces.
+
 ### Security-notes gate
 
 Separately from the prover, each chunk carries advisory `security_notes`.

crates/openshell-cli/src/run.rs

@@ -6878,10 +6878,25 @@ pub async fn sandbox_draft_get(
         if !chunk.validation_result.is_empty() {
             println!(
                 "  {} {}",
-                "Validation:".dimmed(),
+                "Prover:".dimmed(),
                 chunk.validation_result.cyan()
             );
         }
+        if !chunk.application_error.is_empty() {
+            println!(
+                "  {} {}",
+                "Application:".dimmed(),
+                chunk.application_error.red()
+            );
+        }
+        if !chunk.candidate_effective_policy_hash.is_empty() {
+            println!(
+                "  {} {}",
+                "Candidate:".dimmed(),
+                &chunk.candidate_effective_policy_hash
+                    [..12.min(chunk.candidate_effective_policy_hash.len())]
+            );
+        }
 
         if let Some(ref rule) = chunk.proposed_rule {
             println!("  {} {}", "Endpoints:".dimmed(), format_endpoints(rule));
@@ -6915,12 +6930,27 @@ pub async fn sandbox_draft_approve(
     tls: &TlsOptions,
 ) -> Result<()> {
     let mut client = grpc_client(server, tls).await?;
+    let review_token = client
+        .get_draft_policy(GetDraftPolicyRequest {
+            name: name.to_string(),
+            status_filter: String::new(),
+            workspace: workspace.to_string(),
+        })
+        .await
+        .into_diagnostic()?
+        .into_inner()
+        .chunks
+        .into_iter()
+        .find(|chunk| chunk.id == chunk_id)
+        .ok_or_else(|| miette::miette!("draft chunk '{chunk_id}' not found"))?
+        .review_token;
 
     let response = client
         .approve_draft_chunk(ApproveDraftChunkRequest {
             name: name.to_string(),
             chunk_id: chunk_id.to_string(),
             workspace: workspace.to_string(),
+            review_token,
         })
         .await
         .into_diagnostic()?;
@@ -6971,12 +7001,29 @@ pub async fn sandbox_draft_approve_all(
     tls: &TlsOptions,
 ) -> Result<()> {
     let mut client = grpc_client(server, tls).await?;
+    let approvals = client
+        .get_draft_policy(GetDraftPolicyRequest {
+            name: name.to_string(),
+            status_filter: "pending".to_string(),
+            workspace: workspace.to_string(),
+        })
+        .await
+        .into_diagnostic()?
+        .into_inner()
+        .chunks
+        .into_iter()
+        .map(|chunk| openshell_core::proto::DraftChunkApproval {
+            chunk_id: chunk.id,
+            review_token: chunk.review_token,
+        })
+        .collect();
 
     let response = client
         .approve_all_draft_chunks(ApproveAllDraftChunksRequest {
             name: name.to_string(),
             include_security_flagged,
             workspace: workspace.to_string(),
+            approvals,
         })
         .await
         .into_diagnostic()?;

crates/openshell-policy/src/lib.rs

@@ -41,8 +41,8 @@ pub use l7_validate::{
     validate_l7_endpoint_semantics,
 };
 pub use merge::{
-    PolicyMergeError, PolicyMergeOp, PolicyMergeResult, PolicyMergeWarning, generated_rule_name,
-    merge_policy, policy_covers_rule,
+    PolicyMergeError, PolicyMergeOp, PolicyMergeResult, PolicyMergeWarning,
+    canonicalize_advisor_add_rule, generated_rule_name, merge_policy, policy_covers_rule,
 };
 pub use middleware::middleware_host_matches;
 pub use middleware::validate_json as validate_network_middleware_json;
@@ -1206,6 +1206,12 @@ pub enum PolicyViolation {
     },
     /// `credential_signing` and `request_body_credential_rewrite` are both set.
     CredentialSigningWithBodyRewrite { policy_name: String, host: String },
+    /// An endpoint contains a deterministic L7 semantic error.
+    InvalidL7Endpoint {
+        policy_name: String,
+        endpoint_index: usize,
+        reason: String,
+    },
     /// A middleware configuration is structurally invalid.
     InvalidMiddlewareConfig { name: String, reason: String },
     /// Too many middleware configurations are attached to one policy.
@@ -1340,6 +1346,14 @@ impl fmt::Display for PolicyViolation {
                      and request_body_credential_rewrite set; these options are mutually exclusive"
                 )
             }
+            Self::InvalidL7Endpoint {
+                policy_name,
+                endpoint_index,
+                reason,
+            } => write!(
+                f,
+                "network policy '{policy_name}': endpoint {endpoint_index} has invalid L7 configuration: {reason}"
+            ),
             Self::InvalidMiddlewareConfig { name, reason } => {
                 write!(f, "middleware config '{name}' is invalid: {reason}")
             }
@@ -1475,7 +1489,7 @@ pub fn validate_sandbox_policy(
         } else {
             rule.name.clone()
         };
-        for ep in &rule.endpoints {
+        for (endpoint_index, ep) in rule.endpoints.iter().enumerate() {
             let explicit_tcp = l7_validate::is_explicit_tcp_protocol(&ep.protocol);
             if ep.host.trim().is_empty() && explicit_tcp {
                 violations.push(PolicyViolation::MissingTcpEndpointHost {
@@ -1558,6 +1572,127 @@ pub fn validate_sandbox_policy(
                     host: ep.host.clone(),
                 });
             }
+
+            let rules_would_deny_all = !ep.rules.is_empty()
+                && ep.rules.iter().all(|rule| {
+                    rule.allow.as_ref().is_none_or(|allow| {
+                        allow.method.is_empty()
+                            && allow.path.is_empty()
+                            && allow.command.is_empty()
+                            && allow.operation_type.is_empty()
+                            && allow.operation_name.is_empty()
+                            && allow.fields.is_empty()
+                            && allow.params.is_empty()
+                    })
+                });
+            let fields = L7EndpointFields {
+                protocol: &ep.protocol,
+                access: &ep.access,
+                has_rules: !ep.rules.is_empty(),
+                has_deny_rules: !ep.deny_rules.is_empty(),
+                rules_would_deny_all,
+                allow_all_known_mcp_methods: ep
+                    .mcp
+                    .as_ref()
+                    .and_then(|mcp| mcp.allow_all_known_mcp_methods)
+                    .unwrap_or(false),
+            };
+            let mut l7_errors = validate_l7_endpoint_semantics(&fields);
+            let mut explicit_tcp_fields = Vec::new();
+            if !ep.enforcement.is_empty() {
+                explicit_tcp_fields.push("enforcement");
+            }
+            if !ep.path.is_empty() {
+                explicit_tcp_fields.push("path");
+            }
+            if ep.allow_encoded_slash {
+                explicit_tcp_fields.push("allow_encoded_slash");
+            }
+            if ep.websocket_credential_rewrite {
+                explicit_tcp_fields.push("websocket_credential_rewrite");
+            }
+            if ep.request_body_credential_rewrite {
+                explicit_tcp_fields.push("request_body_credential_rewrite");
+            }
+            if !ep.persisted_queries.is_empty() {
+                explicit_tcp_fields.push("persisted_queries");
+            }
+            if !ep.graphql_persisted_queries.is_empty() {
+                explicit_tcp_fields.push("graphql_persisted_queries");
+            }
+            if ep.graphql_max_body_bytes > 0 {
+                explicit_tcp_fields.push("graphql_max_body_bytes");
+            }
+            if ep.json_rpc_max_body_bytes > 0 {
+                explicit_tcp_fields.push("json_rpc_max_body_bytes");
+            }
+            if ep.mcp.is_some() {
+                explicit_tcp_fields.push("mcp");
+            }
+            l7_errors.extend(validate_explicit_tcp_additional_fields(
+                &ep.protocol,
+                &explicit_tcp_fields,
+            ));
+            if !ep.path.is_empty() && !ep.path.starts_with('/') && ep.path != "**" {
+                l7_errors.push("path must start with '/' or be '**'".to_string());
+            }
+            if !ep.persisted_queries.is_empty()
+                && !matches!(ep.persisted_queries.as_str(), "deny" | "allow_registered")
+            {
+                l7_errors.push(format!(
+                    "persisted_queries must be 'deny' or 'allow_registered', got '{}'",
+                    ep.persisted_queries
+                ));
+            }
+            if ep.protocol == "sql" && ep.enforcement == "enforce" {
+                l7_errors.push(
+                    "SQL enforcement requires full SQL parsing; use enforcement: audit".to_string(),
+                );
+            }
+            if ep.mcp.is_some() && ep.protocol != "mcp" {
+                l7_errors.push("mcp options are only valid for protocol mcp".to_string());
+            }
+            if ep.protocol == "graphql" {
+                for (rule_index, rule) in ep.rules.iter().enumerate() {
+                    let operation_type = rule
+                        .allow
+                        .as_ref()
+                        .map(|allow| allow.operation_type.as_str())
+                        .unwrap_or_default();
+                    if !matches!(operation_type, "query" | "mutation" | "subscription") {
+                        l7_errors.push(format!(
+                            "rules[{rule_index}].allow.operation_type must be query, mutation, or subscription"
+                        ));
+                    }
+                }
+                for (rule_index, rule) in ep.deny_rules.iter().enumerate() {
+                    if !matches!(
+                        rule.operation_type.as_str(),
+                        "query" | "mutation" | "subscription"
+                    ) {
+                        l7_errors.push(format!(
+                            "deny_rules[{rule_index}].operation_type must be query, mutation, or subscription"
+                        ));
+                    }
+                }
+                for (key, operation) in &ep.graphql_persisted_queries {
+                    if !matches!(
+                        operation.operation_type.as_str(),
+                        "query" | "mutation" | "subscription"
+                    ) {
+                        l7_errors.push(format!(
+                            "graphql_persisted_queries[{key}].operation_type must be query, mutation, or subscription"
+                        ));
+                    }
+                }
+            }
+            violations.extend(l7_errors.into_iter().map(|reason| {
+                PolicyViolation::InvalidL7Endpoint {
+                    policy_name: name.clone(),
+                    endpoint_index,
+                    reason,
+                }
+            }));
         }
     }