mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): enforce deploy-receipt invariant for applied permission policy
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 <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
@@ -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<crate::managed_agents::permission_policy::PermissionPolicy> =
|
||||
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;
|
||||
|
||||
@@ -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<PermissionPolicy, String> {
|
||||
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"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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("<select"),
|
||||
"local agent must render the editable policy select",
|
||||
);
|
||||
assert.ok(
|
||||
!html.includes("Applied policy:"),
|
||||
"local agent must never show a drift row",
|
||||
);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Provider selected but not yet deployed (backendAgentId null): no drift row
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
test("test_provider_not_yet_deployed_renders_no_drift_row", () => {
|
||||
const html = render(deployedAgent({ backendAgentId: null }));
|
||||
|
||||
assert.ok(
|
||||
!html.includes("Applied policy:"),
|
||||
"an undeployed provider agent has no confirmed receipt — no drift row",
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user