From 0bc5683e52482185e538b4f77cc1a6952691ea8a Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 10 Aug 2026 15:51:45 -0400 Subject: [PATCH] fix(desktop): enforce deploy-receipt invariant for applied permission policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The applied permission policy was extracted from the deploy payload via .ok(), so a missing or unparseable launch.policy_env value stamped a silent None on a successful deploy — suppressing the drift row and defeating the field. The prior regression test manually seeded applied_permission_policy and would have passed even with the production stamp deleted. Extract the applied policy through extract_applied_permission_policy before the provider is invoked; a broken payload invariant now fails the deploy instead of stamping None. record_deploy_success/record_deploy_failure make the receipt transitions explicit: success stamps the exact sent value, redeploy updates it, failed redeploy retains the last confirmed value. Discriminating receipt and transition tests replace the seeded regression, plus a TSX render test for the drift row and an AGENTS.md contract entry. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/commands/agents.rs | 19 +-- .../src-tauri/src/commands/agents_deploy.rs | 138 ++++++++++++++++++ .../src/managed_agents/permission_policy.rs | 24 +-- desktop/src/features/agents/AGENTS.md | 31 ++++ ...AgentPermissionPolicyField.render.test.mjs | 116 +++++++++++++++ 5 files changed, 300 insertions(+), 28 deletions(-) create mode 100644 desktop/src/features/agents/ui/AgentPermissionPolicyField.render.test.mjs diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index d58e5f035..0dc6aa020 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -475,10 +475,11 @@ async fn deploy_to_provider( .map_or_else(|| resolve_provider_binary(provider_id), Ok)?; let config_clone = config.clone(); - let applied_policy: Option = - agent_json["launch"]["policy_env"]["BUZZ_ACP_PERMISSION_POLICY"] - .as_str() - .and_then(|s| serde_json::from_value(serde_json::Value::String(s.to_string())).ok()); + // Enforce the deploy-receipt invariant BEFORE invoking the provider: the + // applied policy is the byte-identical value build_deploy_payload wrote, and + // a missing/unparseable one is a broken JSON-boundary invariant that must fail + // the deploy rather than silently stamp None and suppress the drift row. + let applied_policy = extract_applied_permission_policy(&agent_json)?; let deploy_result = tokio::task::spawn_blocking(move || provider_deploy(&bin_path, &agent_json, &config_clone)) .await @@ -496,15 +497,10 @@ async fn deploy_to_provider( match deploy_result { Ok(backend_agent_id) => { - rec.backend_agent_id = Some(backend_agent_id); - rec.last_started_at = Some(now_iso()); - rec.updated_at = now_iso(); - rec.last_error = None; - rec.applied_permission_policy = applied_policy; + record_deploy_success(rec, backend_agent_id, applied_policy); } Err(ref e) => { - rec.last_error = Some(e.clone()); - rec.updated_at = now_iso(); + record_deploy_failure(rec, e); save_managed_agents(app, &records)?; return Err(e.clone()); } @@ -1363,6 +1359,7 @@ use deploy::build_deploy_payload; use deploy::{deploy_payload_json, DeployProjections}; #[cfg(test)] use deploy::{ensure_remote_provider_supported, resolve_deploy_model_provider}; +use deploy::{extract_applied_permission_policy, record_deploy_failure, record_deploy_success}; #[path = "agents_profile.rs"] mod profile; diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index d1a495db9..39d14e7ca 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -240,6 +240,50 @@ pub(super) fn deploy_payload_json( }) } +use crate::managed_agents::permission_policy::PermissionPolicy; + +/// Extract the applied permission policy from a deploy payload — the byte-identical +/// value `build_deploy_payload` wrote into `launch.policy_env`, not a recompute. +/// +/// The key is unconditionally present in every payload `build_deploy_payload` +/// produces (it falls back to `desktop_default`), so a missing or unparseable +/// value is a broken invariant on the JSON boundary, not a legacy shape. Callers +/// must fail the deploy rather than stamping a silent `None` that would suppress +/// the drift row and defeat the field's purpose. +pub(super) fn extract_applied_permission_policy( + agent_json: &serde_json::Value, +) -> Result { + let raw = agent_json["launch"]["policy_env"]["BUZZ_ACP_PERMISSION_POLICY"] + .as_str() + .ok_or("deploy payload is missing launch.policy_env.BUZZ_ACP_PERMISSION_POLICY")?; + serde_json::from_value(serde_json::Value::String(raw.to_string())) + .map_err(|_| format!("deploy payload has unrecognized permission policy {raw:?}")) +} + +/// Record the outcome of a successful provider deploy. Stamps the confirmed +/// receipt: `applied_permission_policy` is the exact value that was sent, so a +/// later global-default flip is detectable as drift against the live worker. +pub(super) fn record_deploy_success( + record: &mut ManagedAgentRecord, + backend_agent_id: String, + applied_policy: PermissionPolicy, +) { + record.backend_agent_id = Some(backend_agent_id); + record.last_started_at = Some(crate::util::now_iso()); + record.updated_at = crate::util::now_iso(); + record.last_error = None; + record.applied_permission_policy = Some(applied_policy); +} + +/// Record a failed provider deploy. The previous `applied_permission_policy` is +/// intentionally retained: it is the last confirmed deployment receipt, the old +/// worker may still be running that policy, and `last_error` records the failed +/// new attempt. Clearing it would destroy known truth. +pub(super) fn record_deploy_failure(record: &mut ManagedAgentRecord, error: &str) { + record.last_error = Some(error.to_string()); + record.updated_at = crate::util::now_iso(); +} + #[cfg(test)] mod tests { use super::*; @@ -577,4 +621,98 @@ mod tests { "global allow policy must be injected when record has no per-agent policy" ); } + + // ── Deploy-receipt invariant + applied-policy state transitions ────────── + + /// A payload built by `build_launch_block` always carries the policy key, and + /// `extract_applied_permission_policy` reads back the byte-identical value — + /// not a recompute. This is the receipt the deploy path stamps. + #[test] + fn extract_applied_policy_reads_the_exact_sent_value() { + let record = record(); + let descriptor = EffectiveHarnessDescriptor { + command: "goose".into(), + args: vec![], + env: BTreeMap::new(), + }; + let launch = build_launch_block( + &record, + &descriptor, + &[], + None, + None, + "owner-hex", + Some(PermissionPolicy::Allow), + ); + let payload = serde_json::json!({ "launch": launch }); + assert_eq!( + extract_applied_permission_policy(&payload), + Ok(PermissionPolicy::Allow), + "extract must return the exact policy the payload carries" + ); + } + + /// A payload missing the policy key is a broken invariant: extraction errors + /// so the deploy fails before the provider is invoked, never stamping None. + #[test] + fn extract_applied_policy_missing_key_errors() { + let payload = serde_json::json!({ "launch": { "policy_env": {} } }); + assert!( + extract_applied_permission_policy(&payload).is_err(), + "missing policy key must error, not silently stamp None" + ); + } + + /// A payload whose policy value is not a recognized enum variant errors — + /// the deploy fails before the provider is invoked. + #[test] + fn extract_applied_policy_unparseable_value_errors() { + let payload = serde_json::json!({ + "launch": { "policy_env": { "BUZZ_ACP_PERMISSION_POLICY": "bogus" } } + }); + assert!( + extract_applied_permission_policy(&payload).is_err(), + "unrecognized policy value must error, not silently stamp None" + ); + } + + /// Successful deploy stamps the exact sent value as the confirmed receipt and + /// clears any prior error. + #[test] + fn record_deploy_success_stamps_exact_sent_value() { + let mut rec = record(); + rec.last_error = Some("stale error".into()); + record_deploy_success(&mut rec, "backend-1".into(), PermissionPolicy::Allow); + assert_eq!(rec.backend_agent_id.as_deref(), Some("backend-1")); + assert_eq!(rec.applied_permission_policy, Some(PermissionPolicy::Allow)); + assert_eq!(rec.last_error, None); + } + + /// Successful redeploy overwrites the applied receipt with the new sent value. + #[test] + fn record_deploy_success_redeploy_updates_applied_value() { + let mut rec = record(); + record_deploy_success(&mut rec, "backend-1".into(), PermissionPolicy::Allow); + record_deploy_success(&mut rec, "backend-1".into(), PermissionPolicy::Reject); + assert_eq!( + rec.applied_permission_policy, + Some(PermissionPolicy::Reject), + "redeploy must update the applied receipt to the new sent value" + ); + } + + /// Failed redeploy retains the last confirmed applied policy — the old worker + /// may still be running it — while recording the new error. + #[test] + fn record_deploy_failure_retains_last_confirmed_applied_value() { + let mut rec = record(); + record_deploy_success(&mut rec, "backend-1".into(), PermissionPolicy::Allow); + record_deploy_failure(&mut rec, "provider unreachable"); + assert_eq!( + rec.applied_permission_policy, + Some(PermissionPolicy::Allow), + "failed redeploy must retain the last confirmed applied policy" + ); + assert_eq!(rec.last_error.as_deref(), Some("provider unreachable")); + } } diff --git a/desktop/src-tauri/src/managed_agents/permission_policy.rs b/desktop/src-tauri/src/managed_agents/permission_policy.rs index 16e888786..ab1a29b07 100644 --- a/desktop/src-tauri/src/managed_agents/permission_policy.rs +++ b/desktop/src-tauri/src/managed_agents/permission_policy.rs @@ -180,37 +180,27 @@ mod tests { assert_eq!(source, PermissionPolicySource::Agent); } - /// Wes's regression: deploy under Allow, then flip global default to Reject. - /// The summary's *desired* policy changes (global Reject wins) but the - /// *applied* policy on the record must stay Allow — the worker is still - /// running the policy it was launched with. The UI detects drift by - /// comparing these two values and prompts a redeploy. + /// Desired-vs-applied drift at the resolver level (Wes's regression, resolver + /// half): after a post-deploy global flip to Reject, the recomputed *desired* + /// policy is Reject while the persisted *applied* receipt stays Allow, so the + /// two diverge and the UI can flag drift. The production stamp/receipt half — + /// that `applied` is written from the byte-identical sent value and survives a + /// failed redeploy — is pinned by the discriminating transition tests in + /// `commands/agents_deploy.rs`. #[test] fn test_applied_policy_survives_global_flip_deploy_allow_global_flips_to_reject() { let mut record = empty_record(); - // Simulate: agent was deployed with no per-agent override, global=Allow - // at deploy time → applied_permission_policy stamped as Allow. record.permission_policy = None; record.applied_permission_policy = Some(PermissionPolicy::Allow); - // Global is now flipped to Reject (post-deploy mutation). let global_after_flip = GlobalAgentConfig { permission_policy: Some(PermissionPolicy::Reject), ..Default::default() }; - // Desired policy reflects the new global. let (desired, source) = resolve_effective_permission_policy(&record, &global_after_flip); assert_eq!(desired, PermissionPolicy::Reject); assert_eq!(source, PermissionPolicySource::GlobalDefault); - - // Applied policy is unchanged — still what the worker was launched with. - assert_eq!( - record.applied_permission_policy, - Some(PermissionPolicy::Allow) - ); - - // Drift is detectable: applied ≠ desired. assert_ne!(record.applied_permission_policy, Some(desired)); } } diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index b578326eb..2f2e3faf9 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -171,6 +171,27 @@ with a TypeScript lookup table or an id comparison in a component. `getAgentAccessOwnerOnly()` is true, every managed agent's access control is locked to owner-only, including provider-backed agents. A provider backend does not prove remote execution and must never create a policy carve-out. +12. **A remote deploy's permission policy has two truths: desired and applied.** + The *desired* policy is recomputed on every summary from the mutable agent + record + global config (`resolve_effective_permission_policy`). The *applied* + policy is the byte-identical value that was sent to the provider at deploy + time, persisted on the record as `applied_permission_policy` and re-exposed on + the summary — never recomputed. Flipping the global default after a deploy + changes desired but not applied, so the remote worker keeps running the policy + it was launched with until a redeploy. **Stamp the applied value at the single + deploy choke point** (`deploy_to_provider`): `extract_applied_permission_policy` + reads it from `launch.policy_env.BUZZ_ACP_PERMISSION_POLICY` and **must run + before the provider is invoked** — a missing or unparseable value is a broken + payload invariant that fails the deploy, never a silent `None` (a silent `None` + would suppress the drift row and defeat the field). **A failed redeploy retains + the last confirmed applied value** (`record_deploy_failure` leaves it untouched) + — the old worker may still be running it, and `last_error` records the new + attempt; clearing it would destroy known truth. **Legacy fail-quiet:** a record + with `applied_permission_policy` absent (pre-feature, or provider-selected but + never deployed) shows no drift row. `AgentPermissionPolicyField` renders the + amber "applied X · desired Y — redeploy required" row only when the agent is + remotely deployed (`backend.type === "provider"` **and** `backendAgentId !== + null`) **and** applied is non-null **and** applied differs from desired. ## The tests that enforce this @@ -198,6 +219,16 @@ with a TypeScript lookup table or an id comparison in a component. - Rust: `runtime_metadata_env_vars` tests pin spawn-time key application. - Rust: persona sharing/retention tests pin relay+owner scoping, durable enqueue errors, relay rejection/unavailability, and accepted publication. +- Rust: `commands/agents_deploy.rs` tests pin the applied-policy deploy receipt — + `extract_applied_permission_policy` reads the exact sent value and errors on a + missing/unparseable one; `record_deploy_success` stamps it and a redeploy + updates it; `record_deploy_failure` retains the last confirmed value. +- Rust: `managed_agents/permission_policy.rs` pins the resolver-level drift — a + post-deploy global flip diverges desired from the persisted applied receipt. +- `ui/AgentPermissionPolicyField.render.test.mjs` — the drift row: shown for + remote+drift with both values and "redeploy required"; hidden when applied + equals desired, when applied is absent, for local agents, and for a + provider-selected-but-undeployed agent. ## Keep this file true diff --git a/desktop/src/features/agents/ui/AgentPermissionPolicyField.render.test.mjs b/desktop/src/features/agents/ui/AgentPermissionPolicyField.render.test.mjs new file mode 100644 index 000000000..acda28fef --- /dev/null +++ b/desktop/src/features/agents/ui/AgentPermissionPolicyField.render.test.mjs @@ -0,0 +1,116 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import React from "react"; +import { renderToStaticMarkup } from "react-dom/server"; + +import { AgentPermissionPolicyField } from "./AgentPermissionPolicyField.tsx"; + +// --------------------------------------------------------------------------- +// Shared fixtures +// --------------------------------------------------------------------------- + +// A remotely deployed agent: backend is a provider and a backendAgentId exists. +// The drift row only appears for this shape, so most cases build on it. +function deployedAgent(overrides) { + return { + backend: { type: "provider", id: "openclaw", config: {} }, + backendAgentId: "backend-1", + permissionPolicy: "reject", + permissionPolicySource: "global_default", + appliedPermissionPolicy: "allow", + ...overrides, + }; +} + +function render(agent) { + return renderToStaticMarkup( + React.createElement(AgentPermissionPolicyField, { agent, disabled: false }), + ); +} + +// --------------------------------------------------------------------------- +// Remote + drift: applied ≠ desired ⇒ amber drift row with both values +// --------------------------------------------------------------------------- + +test("test_remote_drift_shows_applied_desired_and_redeploy_required", () => { + const html = render(deployedAgent()); + + assert.ok( + html.includes("Applied policy:"), + "drift row must label the applied policy", + ); + assert.ok(html.includes("allow"), "drift row must show the applied value"); + assert.ok(html.includes("reject"), "drift row must show the desired value"); + assert.ok( + html.includes("redeploy required"), + "drift row must prompt a redeploy", + ); +}); + +// --------------------------------------------------------------------------- +// Remote, applied == desired: no drift row +// --------------------------------------------------------------------------- + +test("test_remote_no_drift_when_applied_equals_desired_renders_no_drift_row", () => { + const html = render( + deployedAgent({ + permissionPolicy: "allow", + appliedPermissionPolicy: "allow", + }), + ); + + assert.ok( + !html.includes("Applied policy:"), + "no drift row when applied equals desired", + ); +}); + +// --------------------------------------------------------------------------- +// Remote, applied absent (pre-feature / never-redeployed): no drift row +// --------------------------------------------------------------------------- + +test("test_remote_absent_applied_renders_no_drift_row", () => { + const html = render(deployedAgent({ appliedPermissionPolicy: null })); + + assert.ok( + !html.includes("Applied policy:"), + "absent applied policy must fail quiet — no drift row", + ); +}); + +// --------------------------------------------------------------------------- +// Local agent: editable select, never a drift row even if applied differs +// --------------------------------------------------------------------------- + +test("test_local_agent_renders_editable_select_and_no_drift_row", () => { + const html = render({ + backend: { type: "local" }, + backendAgentId: null, + permissionPolicy: "reject", + permissionPolicySource: "global_default", + appliedPermissionPolicy: "allow", + }); + + assert.ok( + html.includes(" { + const html = render(deployedAgent({ backendAgentId: null })); + + assert.ok( + !html.includes("Applied policy:"), + "an undeployed provider agent has no confirmed receipt — no drift row", + ); +});