fix(policy): validate the applicable advisor candidate

.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,
+ }
+ }));
  }
  }