mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(acp): restore correct merge resolution for buzz-acp post-main-revert
The74c9200admerge let main's revert-of-#4609 semantics leak into branch-owned buzz-acp code. Specifically: - config.rs: BypassPermissions variant reintroduced with no guard in PermissionConfig::resolve; test_default_config_uses_bypass_permissions asserted effective_mode == DontAsk - pool.rs: ResolvedPermissionConfig replaced with bare PermissionMode, relay_event_publisher/permission_decision_tx stripped, sentinel wiring removed from run_prompt_task, doc comments spliced mid-sentence - acp.rs: test helpers options()/outcome() deleted while call sites remained (5 E0425 compile errors); find_allow_once_returns_none_when_absent body corrupted with undefined `response` variable - lib.rs: import regression Fix: restore all four crates/buzz-acp files to the branch tip (d808c4bd) which correctly supersedes both #4609 and its revert #5323. No semantics from the revert survive on this branch. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
+45
-38
@@ -235,7 +235,7 @@ pub struct AcpClient {
|
||||
/// Under `ask` the full map is `pending_permissions` below.
|
||||
pending_permission_id: Option<serde_json::Value>,
|
||||
/// Whether we have already sent a response to the pending permission request.
|
||||
/// Guards against double-response if a timeout fires after the allow_once
|
||||
/// Guards against double-response if a timeout fires after the rejection
|
||||
/// response was written but before `pending_permission_id` was cleared.
|
||||
permission_responded: bool,
|
||||
/// Pending `session/request_permission` entries under the `ask` policy.
|
||||
@@ -1724,7 +1724,8 @@ impl AcpClient {
|
||||
///
|
||||
/// While waiting, handles:
|
||||
/// - `session/update` notifications → logged via tracing
|
||||
/// - `session/request_permission` requests → auto-approved with `allow_once`
|
||||
/// - `session/request_permission` requests → rejected unless an owner has
|
||||
/// already selected a non-interactive permission mode at session setup
|
||||
/// - Any other messages → debug-logged and ignored; if they carry an `id`
|
||||
/// (i.e. they are requests, not notifications), a JSON-RPC -32601 error is sent.
|
||||
///
|
||||
@@ -3761,42 +3762,53 @@ mod tests {
|
||||
assert_eq!(StopReason::from_str("Refusal"), Some(StopReason::Refusal));
|
||||
}
|
||||
|
||||
fn options(json: &str) -> Vec<serde_json::Value> {
|
||||
serde_json::from_str(json).expect("option list")
|
||||
}
|
||||
|
||||
fn outcome(response: &serde_json::Value) -> Option<&str> {
|
||||
response["result"]["outcome"]["outcome"].as_str()
|
||||
}
|
||||
|
||||
/// The offered `allow_once` and `allow_always` options must be ignored:
|
||||
/// there is no human to click them, so choosing either would make every
|
||||
/// admitted prompt an implicit approval. `optionId`s are deliberately
|
||||
/// non-obvious to prove they are matched by `kind`, never hardcoded.
|
||||
#[test]
|
||||
fn find_allow_once_by_kind_not_by_option_id() {
|
||||
// optionId values are intentionally non-obvious to prove we don't hardcode them.
|
||||
let options: Vec<serde_json::Value> = serde_json::from_str(
|
||||
fn permission_requests_select_reject_once_not_allow_once() {
|
||||
let options = options(
|
||||
r#"[
|
||||
{"optionId": "opt-reject-42", "name": "Reject", "kind": "reject_once"},
|
||||
{"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"},
|
||||
{"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"}
|
||||
]"#,
|
||||
)
|
||||
.unwrap();
|
||||
);
|
||||
|
||||
let allow_once = options
|
||||
.iter()
|
||||
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
|
||||
let response =
|
||||
permission_denial_response(&serde_json::json!(7), &options).expect("denial response");
|
||||
|
||||
assert!(allow_once.is_some(), "should find allow_once option");
|
||||
let opt = allow_once.unwrap();
|
||||
// Found by kind, not by hardcoded optionId
|
||||
assert_eq!(opt["kind"].as_str(), Some("allow_once"));
|
||||
assert_eq!(opt["optionId"].as_str(), Some("opt-allow-99"));
|
||||
assert_eq!(outcome(&response), Some("selected"));
|
||||
assert_eq!(
|
||||
response["result"]["outcome"]["optionId"].as_str(),
|
||||
Some("opt-reject-42"),
|
||||
"must select reject_once even when allow options are offered"
|
||||
);
|
||||
}
|
||||
|
||||
/// Fail-closed backstop: an adapter that offers no `reject_once` must still
|
||||
/// be denied, via the protocol's cancelled outcome rather than an error or
|
||||
/// an approval.
|
||||
#[test]
|
||||
fn find_allow_once_returns_none_when_absent() {
|
||||
let options: Vec<serde_json::Value> = serde_json::from_str(
|
||||
fn permission_request_without_reject_once_is_cancelled() {
|
||||
let options = options(
|
||||
r#"[
|
||||
{"optionId": "reject-1", "name": "Reject", "kind": "reject_once"},
|
||||
{"optionId": "reject-always", "name": "Always reject", "kind": "reject_always"}
|
||||
{"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"},
|
||||
{"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"}
|
||||
]"#,
|
||||
)
|
||||
.unwrap();
|
||||
);
|
||||
|
||||
let allow_once = options
|
||||
.iter()
|
||||
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
|
||||
let response = permission_denial_response(&serde_json::json!("req-1"), &options)
|
||||
.expect("cancelled response");
|
||||
|
||||
assert_eq!(outcome(&response), Some("cancelled"));
|
||||
assert_eq!(
|
||||
@@ -3833,22 +3845,17 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_reject_once_fallback_when_no_allow_once() {
|
||||
let options: Vec<serde_json::Value> = serde_json::from_str(
|
||||
r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#,
|
||||
)
|
||||
.unwrap();
|
||||
fn find_reject_once_by_kind() {
|
||||
let options =
|
||||
options(r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#);
|
||||
|
||||
let allow_once = options
|
||||
.iter()
|
||||
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
|
||||
assert!(allow_once.is_none());
|
||||
let response =
|
||||
permission_denial_response(&serde_json::json!(1), &options).expect("denial response");
|
||||
|
||||
let reject_once = options
|
||||
.iter()
|
||||
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once"));
|
||||
assert!(reject_once.is_some());
|
||||
assert_eq!(reject_once.unwrap()["optionId"].as_str(), Some("rej-x"));
|
||||
assert_eq!(
|
||||
response["result"]["outcome"]["optionId"].as_str(),
|
||||
Some("rej-x")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -121,7 +121,6 @@ impl std::fmt::Display for RespondTo {
|
||||
/// `session/request_permission` escalations may still cross ACP when the model
|
||||
/// chooses manual approval for a specific call.
|
||||
/// - `acceptEdits` — auto-approve file edits, still ask for other tools.
|
||||
/// - `bypassPermissions` — skip the permission flow entirely.
|
||||
/// - `dontAsk` — never prompt; reject anything that would require permission.
|
||||
/// - `plan` — planning-only mode (no tool execution).
|
||||
#[derive(Debug, Clone, Copy, PartialEq, clap::ValueEnum)]
|
||||
@@ -148,9 +147,6 @@ pub enum PermissionMode {
|
||||
/// Auto-approve file edits, still ask for other tools.
|
||||
#[value(alias = "acceptEdits")]
|
||||
AcceptEdits,
|
||||
/// Skip the permission flow entirely.
|
||||
#[value(alias = "bypassPermissions")]
|
||||
BypassPermissions,
|
||||
/// Never prompt; reject anything that would require permission.
|
||||
#[value(alias = "dontAsk")]
|
||||
DontAsk,
|
||||
@@ -167,7 +163,6 @@ impl PermissionMode {
|
||||
Self::Default => "default",
|
||||
Self::Auto => "auto",
|
||||
Self::AcceptEdits => "acceptEdits",
|
||||
Self::BypassPermissions => "bypassPermissions",
|
||||
Self::DontAsk => "dontAsk",
|
||||
Self::Plan => "plan",
|
||||
}
|
||||
@@ -629,7 +624,6 @@ pub struct CliArgs {
|
||||
///
|
||||
/// Desktop injects the resolved per-agent or fleet-wide value.
|
||||
/// Headless installations should leave this unset (defaults to `reject`).
|
||||
|
||||
#[arg(
|
||||
long,
|
||||
env = "BUZZ_ACP_PERMISSION_POLICY",
|
||||
@@ -1684,7 +1678,6 @@ mod tests {
|
||||
Some(PermissionMode::DontAsk),
|
||||
)
|
||||
.expect("test config"),
|
||||
|
||||
respond_to: RespondTo::Anyone,
|
||||
respond_to_allowlist: HashSet::new(),
|
||||
allowed_respond_to: Vec::new(),
|
||||
@@ -2485,10 +2478,6 @@ channels = "ALL"
|
||||
fn test_permission_mode_wire_strings() {
|
||||
assert_eq!(PermissionMode::Default.as_wire_str(), "default");
|
||||
assert_eq!(PermissionMode::AcceptEdits.as_wire_str(), "acceptEdits");
|
||||
assert_eq!(
|
||||
PermissionMode::BypassPermissions.as_wire_str(),
|
||||
"bypassPermissions"
|
||||
);
|
||||
assert_eq!(PermissionMode::DontAsk.as_wire_str(), "dontAsk");
|
||||
assert_eq!(PermissionMode::Plan.as_wire_str(), "plan");
|
||||
}
|
||||
@@ -2496,7 +2485,6 @@ channels = "ALL"
|
||||
#[test]
|
||||
fn test_permission_mode_is_default() {
|
||||
assert!(PermissionMode::Default.is_default());
|
||||
assert!(!PermissionMode::BypassPermissions.is_default());
|
||||
assert!(!PermissionMode::AcceptEdits.is_default());
|
||||
assert!(!PermissionMode::DontAsk.is_default());
|
||||
assert!(!PermissionMode::Plan.is_default());
|
||||
@@ -2504,10 +2492,7 @@ channels = "ALL"
|
||||
|
||||
#[test]
|
||||
fn test_permission_mode_display() {
|
||||
assert_eq!(
|
||||
format!("{}", PermissionMode::BypassPermissions),
|
||||
"bypassPermissions"
|
||||
);
|
||||
assert_eq!(format!("{}", PermissionMode::DontAsk), "dontAsk");
|
||||
assert_eq!(format!("{}", PermissionMode::Default), "default");
|
||||
}
|
||||
|
||||
@@ -2519,10 +2504,9 @@ channels = "ALL"
|
||||
Some(PermissionMode::DontAsk),
|
||||
)
|
||||
.expect("test config");
|
||||
|
||||
let s = config.summary();
|
||||
assert!(
|
||||
s.contains("permission_mode=bypassPermissions"),
|
||||
s.contains("permission_mode=dontAsk"),
|
||||
"summary should include permission_mode, got: {s}"
|
||||
);
|
||||
}
|
||||
@@ -2540,7 +2524,7 @@ channels = "ALL"
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_default_config_uses_bypass_permissions() {
|
||||
fn test_default_config_rejects_interactive_permissions() {
|
||||
let config = test_config(SubscribeMode::Mentions);
|
||||
assert_eq!(
|
||||
config.permission_config.effective_mode,
|
||||
@@ -2556,7 +2540,6 @@ channels = "ALL"
|
||||
let cases = [
|
||||
("default", PermissionMode::Default),
|
||||
("accept-edits", PermissionMode::AcceptEdits),
|
||||
("bypass-permissions", PermissionMode::BypassPermissions),
|
||||
("dont-ask", PermissionMode::DontAsk),
|
||||
("plan", PermissionMode::Plan),
|
||||
];
|
||||
@@ -2571,14 +2554,12 @@ channels = "ALL"
|
||||
|
||||
#[test]
|
||||
fn test_permission_mode_value_enum_camel_case_aliases() {
|
||||
// Operators may set env vars using the camelCase wire-format strings
|
||||
// (e.g. BUZZ_ACP_PERMISSION_MODE=bypassPermissions). The #[value(alias)]
|
||||
// attributes ensure these parse correctly.
|
||||
// Operators may set env vars using the camelCase wire-format strings.
|
||||
// The #[value(alias)] attributes ensure these parse correctly.
|
||||
use clap::ValueEnum;
|
||||
let cases = [
|
||||
("default", PermissionMode::Default),
|
||||
("acceptEdits", PermissionMode::AcceptEdits),
|
||||
("bypassPermissions", PermissionMode::BypassPermissions),
|
||||
("dontAsk", PermissionMode::DontAsk),
|
||||
("plan", PermissionMode::Plan),
|
||||
];
|
||||
@@ -2591,6 +2572,18 @@ channels = "ALL"
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_permission_mode_rejects_unattended_bypass() {
|
||||
use clap::ValueEnum;
|
||||
|
||||
for input in ["bypass-permissions", "bypassPermissions"] {
|
||||
assert!(
|
||||
PermissionMode::from_str(input, true).is_err(),
|
||||
"{input:?} must not disable the ACP permission boundary"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Helper: resolve idle_timeout_secs using the same precedence logic as Config::from_args.
|
||||
/// Precedence: explicit --idle-timeout > --turn-timeout (deprecated) > `DEFAULT_IDLE_TIMEOUT_SECS`.
|
||||
fn resolve_idle_timeout(idle: Option<u64>, turn: Option<u64>) -> u64 {
|
||||
|
||||
@@ -6369,7 +6369,6 @@ mod build_mcp_servers_tests {
|
||||
None,
|
||||
)
|
||||
.expect("test config"),
|
||||
|
||||
respond_to: config::RespondTo::Anyone,
|
||||
respond_to_allowlist: std::collections::HashSet::new(),
|
||||
allowed_respond_to: vec![],
|
||||
@@ -6596,7 +6595,6 @@ mod error_outcome_emission_tests {
|
||||
None,
|
||||
)
|
||||
.expect("test config"),
|
||||
|
||||
respond_to: config::RespondTo::Anyone,
|
||||
respond_to_allowlist: HashSet::new(),
|
||||
allowed_respond_to: vec![],
|
||||
|
||||
@@ -1148,11 +1148,7 @@ async fn apply_model_switch(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Set the session permission mode via `session/set_config_option`.
|
||||
///
|
||||
/// Non-fatal for most errors: logs and proceeds. The agent falls back
|
||||
/// to its default permission mode (`"default"`), which still works via
|
||||
/// Check if the agent's `session/new` response advertises a given mode ID
|
||||
/// Check whether the agent's `session/new` response advertises a given mode ID
|
||||
/// in `result.modes.availableModes[].id`. Returns `false` if the modes
|
||||
/// field is absent or the mode isn't listed.
|
||||
fn agent_supports_mode(session_new_result: &serde_json::Value, mode_wire: &str) -> bool {
|
||||
@@ -1168,7 +1164,11 @@ fn agent_supports_mode(session_new_result: &serde_json::Value, mode_wire: &str)
|
||||
.unwrap_or(false)
|
||||
}
|
||||
|
||||
/// per-tool auto-approval in `handle_permission_request`.
|
||||
/// Set the session permission mode via `session/set_config_option`.
|
||||
///
|
||||
/// Non-fatal for most errors: logs and proceeds. The agent falls back to its
|
||||
/// default mode, and any interactive permission request is rejected by
|
||||
/// `handle_permission_request`.
|
||||
///
|
||||
/// **Fatal exception:** if the agent process exits (e.g., goose crashes on
|
||||
/// unrecognized methods), returns `Err(AgentExited)` so the caller can respawn.
|
||||
@@ -1208,7 +1208,7 @@ async fn apply_permission_mode(
|
||||
Ok(Err(e)) => {
|
||||
tracing::warn!(
|
||||
target: "pool::permission",
|
||||
"failed to set permission mode {wire:?}: {e} — falling back to per-tool auto-approval"
|
||||
"failed to set permission mode {wire:?}: {e} — falling back to per-tool rejection"
|
||||
);
|
||||
}
|
||||
Err(_) => {
|
||||
|
||||
Reference in New Issue
Block a user