mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(acp): structural round — one nonce-keyed permission record, finish_permission() helper
Implements Thufir's minimal shape across three passes of residual defects: Harness (acp.rs): - Remove PermissionEntryState::Resolved — entries are removed from the map on every terminal transition (applied/timed_out/cancelled). The absence of a nonce is the replay guard; no tombstones means capacity counts only live (Pending|Writing) requests, fixing the 9th-request-in-one-turn bug. - Add finish_permission() terminal helper owning Pending→Writing→Resolved for all terminals. Exactly one write+flush, exactly one nonce-correlated authorized acp_write with the terminal reason. Any write failure poisons the process and emits a permission_terminal observer-only event so Desktop can retire the card (the uncertain path). - Cancel path: write failure also poisons and emits permission_terminal. cancel-during-write emits permission_terminal for the in-flight entry. - Re-arm idle deadline when live pending count reaches zero so a slow human decision grants a fresh idle window instead of insta-cancelling the turn. - Capacity check now counts only live (Pending|Writing) entries. - Clippy: fix assert_eq!(x, true) → assert!(x), while-let-loop, doc overindented list items in acp.rs and config.rs. - Fmt: cargo fmt applied. Desktop (agentSessionTranscript.ts): - acp_write authorized frames correlate exclusively by authorization.requestNonce (primary); JSON-RPC id correlation is a legacy fallback for non-ask paths. - Terminal copy derives from authorization.reason (applied/timed_out/cancelled/ uncertain) via describePermissionTerminalReason — timeout now renders 'Timed out' not 'Denied (reject_once)'. - set actionable: false on all retirement paths. - permission_terminal observer event handler retires the card via nonce. - turn_completed and turn_error backstop: retireAllLivePermissionCards() retires any still-live cards so missing telemetry and archive replay cannot reconstruct live controls. - Biome format applied. lib.rs: - fit_observer_event_to_budget: early return without mutation when event.authorization.is_some() — authorized frames are never leaf-trimmed or stubbed (NIP-AO §3 byte-for-byte requirement). - Enqueue suppresses over-cap authorized frames entirely (defense in depth). - Test: test_authorized_frame_payload_is_never_trimmed. Tests: - ask_production_path_emits_request_captures_nonce_and_delivers_decision: real script emits session/request_permission, harness captures nonce from in-process observer, routes decision through channel, asserts end_turn. - cancel_writes_exactly_one_response_per_pending_id_no_replay: registers entries via production path, captures nonces before cancel, verifies each emitted cancel nonce matches a registered entry nonce, verifies no replay. - ask_permission_idle_is_suspended_while_pending_entry_exists: paused-time, asserts entry present at 299s. - ask_permission_deadline_fires_at_300_seconds: paused-time, asserts entry removed at exactly 300s. - ask_permission_idle_rearmed_after_last_entry_resolves: paused-time, proves idle deadline re-armed after decision applied. - ask_nine_sequential_requests_all_succeed_after_capacity_recovery: nine sequential requests each decided before the next is queued; asserts 9 distinct authorized acp_write observer nonces. NIP-AO.md: - session_resolved = session establishment (not terminal). - Added turn_completed and turn_error rows as terminal lifecycle events. - uncertain path: permission_terminal observer event replaces wrong session_resolved reference. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
+561
-362
File diff suppressed because it is too large
Load Diff
@@ -115,10 +115,11 @@ impl std::fmt::Display for RespondTo {
|
||||
/// `configId: "mode"` (e.g. `claude-agent-acp`).
|
||||
///
|
||||
/// - `default` — agent's built-in behaviour (permission requests per tool call).
|
||||
/// - `auto` — fully autonomous execution; model-gated (requires `supportsAutoMode`);
|
||||
/// - `auto` — fully autonomous execution; model-gated classifier (requires `supportsAutoMode`);
|
||||
/// the adapter degrades gracefully to `default` when the active model does not
|
||||
/// support it. The adapter self-approves all tool calls internally — no
|
||||
/// `session/request_permission` ever crosses ACP under this mode.
|
||||
/// support it. The adapter auto-approves most tool calls internally, but residual
|
||||
/// `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.
|
||||
/// - `dontAsk` — never prompt; reject anything that would require permission.
|
||||
/// - `plan` — planning-only mode (no tool execution).
|
||||
@@ -187,11 +188,11 @@ impl std::fmt::Display for PermissionMode {
|
||||
/// per-agent or fleet-wide value; headless defaults to `reject`.
|
||||
///
|
||||
/// - `allow` — auto-select the unique `allow_once` option; fail closed if
|
||||
/// zero or multiple `allow_once` candidates, malformed options,
|
||||
/// or any validation error.
|
||||
/// zero or multiple `allow_once` candidates, malformed options,
|
||||
/// or any validation error.
|
||||
/// - `ask` — surface the request as an actionable card for the owner;
|
||||
/// fail closed on timeout (300 s) or if the observer / owner is
|
||||
/// unavailable.
|
||||
/// fail closed on timeout (300 s) or if the observer / owner is
|
||||
/// unavailable.
|
||||
/// - `reject` — deny every request (today's behaviour, headless default).
|
||||
#[derive(Debug, Clone, Copy, PartialEq, clap::ValueEnum)]
|
||||
pub enum PermissionPolicy {
|
||||
@@ -617,9 +618,9 @@ pub struct CliArgs {
|
||||
///
|
||||
/// - `reject` (headless default) — deny all permission requests.
|
||||
/// - `ask` — surface as an actionable card; auto-deny on timeout (300 s)
|
||||
/// or when the observer / owner is unavailable.
|
||||
/// or when the observer / owner is unavailable.
|
||||
/// - `allow` — auto-approve via the unique `allow_once` option;
|
||||
/// fail closed if zero or multiple `allow_once` candidates.
|
||||
/// fail closed if zero or multiple `allow_once` candidates.
|
||||
///
|
||||
/// Desktop injects the resolved per-agent or fleet-wide value.
|
||||
/// Headless installations should leave this unset (defaults to `reject`).
|
||||
|
||||
@@ -453,7 +453,19 @@ impl ObserverPublishQueue {
|
||||
// Pre-trim at enqueue so (a) byte accounting reflects what will ship
|
||||
// and (b) one oversized leaf cannot force every frame it touches into
|
||||
// whole-envelope elision downstream.
|
||||
//
|
||||
// Authorization frames must not be leaf-trimmed (NIP-AO §3 requires
|
||||
// byte-for-byte reproduction). `fit_observer_event_to_budget` returns
|
||||
// without mutating them; if they are still over-cap after that guard,
|
||||
// suppress entirely rather than enqueue an over-budget frame.
|
||||
fit_observer_event_to_budget(&mut event);
|
||||
if event.authorization.is_some() && serialized_len(&event) > OBSERVER_MAX_PLAINTEXT_LEN {
|
||||
tracing::warn!(
|
||||
kind = %event.kind,
|
||||
"suppressing authorized observer frame at enqueue: over-cap after fit"
|
||||
);
|
||||
return;
|
||||
}
|
||||
let bytes = serialized_len(&event);
|
||||
self.pending_bytes += bytes;
|
||||
self.events.push_back((bytes, source_events, event));
|
||||
@@ -895,6 +907,19 @@ fn fit_observer_event_to_budget(event: &mut observer::ObserverEvent) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Authorization frames carry byte-for-byte raw ACP that must not be
|
||||
// rewritten — NIP-AO §3 requires the payload to be reproduced exactly as
|
||||
// received. If the annotated event is still over-cap after the early-return
|
||||
// above, suppress it entirely rather than mutate the ACP bytes.
|
||||
if event.authorization.is_some() {
|
||||
tracing::warn!(
|
||||
kind = %event.kind,
|
||||
"dropping authorized observer frame: annotated size exceeds cap \
|
||||
and payload must not be trimmed"
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
// Raw size of the payload we are about to trim, captured before mutation so
|
||||
// the stub's `originalBytes` reports source bytes discarded, not serialized
|
||||
// overflow — consistent with the per-leaf marker's raw byte count.
|
||||
@@ -8009,4 +8034,41 @@ mod observer_payload_trim_tests {
|
||||
assert!(leaf.ends_with('…'));
|
||||
assert!(leaf.contains("[elided"));
|
||||
}
|
||||
|
||||
/// Authorized observer frames must never be leaf-trimmed or stubbed.
|
||||
/// `fit_observer_event_to_budget` must leave the payload untouched when
|
||||
/// `authorization` is present, even if the serialized frame is over-cap.
|
||||
#[test]
|
||||
fn test_authorized_frame_payload_is_never_trimmed() {
|
||||
// Build an over-cap authorized frame (big payload, authorization present).
|
||||
let big = "x".repeat(OBSERVER_MAX_PLAINTEXT_LEN + 1000);
|
||||
let mut event = event_with_payload(
|
||||
"acp_read",
|
||||
serde_json::json!({ "method": "session/request_permission", "body": big }),
|
||||
);
|
||||
event.authorization = Some(crate::observer::AuthorizationEnvelope {
|
||||
request_nonce: "test-nonce".to_string(),
|
||||
actionable: true,
|
||||
reason: None,
|
||||
});
|
||||
|
||||
let payload_before = event.payload.clone();
|
||||
assert!(
|
||||
serialized(&event).len() > OBSERVER_MAX_PLAINTEXT_LEN,
|
||||
"precondition: authorized frame is over-cap"
|
||||
);
|
||||
|
||||
fit_observer_event_to_budget(&mut event);
|
||||
|
||||
// Payload must be byte-for-byte identical — no leaf trim, no stub.
|
||||
assert_eq!(
|
||||
event.payload, payload_before,
|
||||
"authorized frame payload must not be mutated by fit_observer_event_to_budget"
|
||||
);
|
||||
// Authorization envelope must still be present and intact.
|
||||
assert!(
|
||||
event.authorization.is_some(),
|
||||
"authorization envelope must survive fit_observer_event_to_budget"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,8 +50,9 @@ export type TranscriptState = {
|
||||
/**
|
||||
* Maps `requestNonce` → `itemId` for actionable permission cards.
|
||||
* Populated alongside `pendingPermissions` when the `authorization` envelope
|
||||
* is present on the `acp_read` frame. Used by the `permission_decision`
|
||||
* `control_result` handler to retire the card on any terminal outcome.
|
||||
* is present on the `acp_read` frame. Used by the nonce-correlated `acp_write`
|
||||
* terminal handler and the `permission_terminal` event handler to retire the
|
||||
* card on any terminal outcome (applied, timed_out, cancelled, uncertain).
|
||||
*/
|
||||
pendingPermissionsByNonce: Map<string, string>;
|
||||
continuationSeq: number;
|
||||
@@ -274,9 +275,84 @@ function describePermissionOutcome(
|
||||
return outcome;
|
||||
}
|
||||
|
||||
/**
|
||||
* Derive human-readable outcome copy from the `authorization.reason` field
|
||||
* that accompanies terminal `acp_write` events. This is preferred over
|
||||
* deriving copy from the ACP `result.outcome` field directly because the
|
||||
* `reason` values are harness-level semantics (applied / timed_out /
|
||||
* cancelled) whereas `result.outcome` is adapter-level (selected / reject_once
|
||||
* etc.) and does not distinguish timeout from explicit denial.
|
||||
*
|
||||
* Falls back to `describePermissionOutcome` when `reason` is absent (legacy
|
||||
* paths that predate the authorization envelope).
|
||||
*/
|
||||
function describePermissionTerminalReason(
|
||||
reason: string | undefined,
|
||||
outcomeKind: string | null | undefined,
|
||||
optionId: string | null,
|
||||
options:
|
||||
| Array<{ optionId: string; kind: string; label?: string }>
|
||||
| undefined,
|
||||
): string {
|
||||
if (reason === "applied") {
|
||||
// Build optionNames map from the card's options array.
|
||||
const optionNames = new Map(
|
||||
(options ?? []).map((o) => [o.optionId, o.kind]),
|
||||
);
|
||||
return describePermissionOutcome(
|
||||
outcomeKind ?? "selected",
|
||||
optionId,
|
||||
optionNames,
|
||||
);
|
||||
}
|
||||
if (reason === "timed_out") return "Timed out";
|
||||
if (reason === "cancelled") return "Cancelled";
|
||||
if (reason === "uncertain") {
|
||||
return "Approval outcome unknown; agent process stopped before it could continue.";
|
||||
}
|
||||
// No reason: fall back to ACP outcome-level copy.
|
||||
const optionNames = new Map((options ?? []).map((o) => [o.optionId, o.kind]));
|
||||
return describePermissionOutcome(outcomeKind ?? "", optionId, optionNames);
|
||||
}
|
||||
|
||||
/**
|
||||
* Retire all live (actionable) permission cards for a given channel.
|
||||
* Called on terminal turn/process events (`turn_error`, `agent_panic`,
|
||||
* `turn_completed`) as a backstop so cards do not remain clickable after
|
||||
* the turn that owned them has ended.
|
||||
*/
|
||||
function retireAllLivePermissionCards(d: TranscriptDraft, channelId: string) {
|
||||
const prefix = `permission:${channelId}:`;
|
||||
let retired = false;
|
||||
for (const [id, item] of d.itemsById) {
|
||||
if (
|
||||
id.startsWith(prefix) &&
|
||||
item.type === "lifecycle" &&
|
||||
item.renderClass === "permission" &&
|
||||
item.actionable
|
||||
) {
|
||||
if (!retired) {
|
||||
// Copy on first mutation.
|
||||
d.items = [...d.items];
|
||||
d.itemsById = new Map(d.itemsById);
|
||||
retired = true;
|
||||
d.changed = true;
|
||||
}
|
||||
const updated = { ...item, actionable: false };
|
||||
d.itemsById.set(id, updated);
|
||||
const idx = d.items.findIndex((i) => i.id === id);
|
||||
if (idx !== -1) d.items[idx] = updated;
|
||||
// Clean up nonce index if present.
|
||||
if (item.requestNonce) {
|
||||
d.pendingPermissionsByNonce = new Map(d.pendingPermissionsByNonce);
|
||||
d.pendingPermissionsByNonce.delete(item.requestNonce);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Stable map key for a JSON-RPC id, which may be a string or a finite number
|
||||
* per the spec. Using JSON.stringify avoids collisions between the number 1 and
|
||||
* the string "1". Returns null for null, undefined, or non-id values (objects,
|
||||
* booleans) so callers can gate on presence without a separate type check.
|
||||
*/
|
||||
@@ -810,6 +886,39 @@ export function processTranscriptEvent(
|
||||
ctx,
|
||||
event.kind,
|
||||
);
|
||||
// Backstop: retire any still-live permission cards for this channel so
|
||||
// missing telemetry and archive replay never reconstruct live controls
|
||||
// after a terminal turn/process state.
|
||||
retireAllLivePermissionCards(d, ch);
|
||||
} else if (event.kind === "turn_completed") {
|
||||
// Backstop: retire any still-live permission cards for this channel.
|
||||
// Applied/timed-out/cancelled cards should already be retired via their
|
||||
// nonce-correlated acp_write frames, but uncertain (process-poison) cards
|
||||
// may only receive a turn_completed — this ensures they are not left
|
||||
// actionable in live state or archive replay.
|
||||
retireAllLivePermissionCards(d, ch);
|
||||
} else if (event.kind === "permission_terminal") {
|
||||
// Observer-only terminal event for uncertain outcomes (process poison,
|
||||
// cancel-during-write). No ACP wire response was confirmed; the harness
|
||||
// emits this so Desktop can retire the card without a JSON-RPC response.
|
||||
// Carry the nonce from the authorization envelope.
|
||||
const auth = event.authorization;
|
||||
const nonce = auth?.requestNonce;
|
||||
if (nonce) {
|
||||
const itemId = d.pendingPermissionsByNonce.get(nonce);
|
||||
if (itemId) {
|
||||
const existing = d.itemsById.get(itemId);
|
||||
if (existing?.type === "lifecycle") {
|
||||
replaceItem(d, itemId, {
|
||||
...existing,
|
||||
outcome: "Uncertain (process restarting)",
|
||||
actionable: false,
|
||||
});
|
||||
}
|
||||
d.pendingPermissionsByNonce = new Map(d.pendingPermissionsByNonce);
|
||||
d.pendingPermissionsByNonce.delete(nonce);
|
||||
}
|
||||
}
|
||||
} else if (event.kind === "acp_read" || event.kind === "acp_write") {
|
||||
const payload = asRecord(event.payload);
|
||||
const method = asString(payload.method);
|
||||
@@ -866,24 +975,70 @@ export function processTranscriptEvent(
|
||||
}
|
||||
} else if (event.kind === "acp_write" && !method) {
|
||||
// Permission response: {"id": <same as request>, "result": {"outcome": {...}}}
|
||||
//
|
||||
// Primary correlation: by `authorization.requestNonce` — a nonce-keyed
|
||||
// lookup is immune to JSON-RPC id reuse across channels/sessions.
|
||||
// Legacy fallback: by JSON-RPC id, scoped to channel `ch` so at least
|
||||
// cross-channel collisions are avoided.
|
||||
const auth = event.authorization;
|
||||
const nonce = auth?.requestNonce;
|
||||
const responseId = jsonRpcId(payload.id);
|
||||
const result = asRecord(asRecord(payload.result).outcome);
|
||||
const outcomeKind = asString(result.outcome);
|
||||
const pending = responseId ? d.pendingPermissions.get(responseId) : null;
|
||||
if (pending && outcomeKind && responseId) {
|
||||
|
||||
// Derive terminal label from authorization.reason when present; this
|
||||
// gives "Timed out" for timed_out rather than rendering the ACP
|
||||
// outcome kind directly (which says "reject_once", not "Timed out").
|
||||
const terminalReason = auth?.reason;
|
||||
|
||||
// Resolve the permission card: nonce-keyed wins; fall back to id-keyed.
|
||||
const itemIdByNonce = nonce
|
||||
? d.pendingPermissionsByNonce.get(nonce)
|
||||
: null;
|
||||
const pendingById = responseId
|
||||
? d.pendingPermissions.get(responseId)
|
||||
: null;
|
||||
|
||||
if (itemIdByNonce) {
|
||||
// Nonce-correlated path: resolve the card and derive copy from reason.
|
||||
const existing = d.itemsById.get(itemIdByNonce);
|
||||
if (existing?.type === "lifecycle") {
|
||||
const outcomeText = describePermissionTerminalReason(
|
||||
terminalReason,
|
||||
outcomeKind,
|
||||
asString(result.optionId) ?? null,
|
||||
existing.options,
|
||||
);
|
||||
replaceItem(d, itemIdByNonce, {
|
||||
...existing,
|
||||
outcome: outcomeText,
|
||||
actionable: false,
|
||||
});
|
||||
}
|
||||
// Clean up both indexes.
|
||||
if (nonce) {
|
||||
d.pendingPermissionsByNonce = new Map(d.pendingPermissionsByNonce);
|
||||
d.pendingPermissionsByNonce.delete(nonce);
|
||||
}
|
||||
if (responseId) {
|
||||
d.pendingPermissions = new Map(d.pendingPermissions);
|
||||
d.pendingPermissions.delete(responseId);
|
||||
}
|
||||
} else if (pendingById && outcomeKind && responseId) {
|
||||
// Legacy id-correlation fallback (non-ask paths with no nonce).
|
||||
const optionId = asString(result.optionId) ?? null;
|
||||
const outcomeText = describePermissionOutcome(
|
||||
outcomeKind,
|
||||
optionId,
|
||||
pending.optionNames,
|
||||
pendingById.optionNames,
|
||||
);
|
||||
const existing = d.itemsById.get(pending.itemId);
|
||||
const existing = d.itemsById.get(pendingById.itemId);
|
||||
if (existing?.type === "lifecycle") {
|
||||
replaceItem(d, pending.itemId, {
|
||||
replaceItem(d, pendingById.itemId, {
|
||||
...existing,
|
||||
outcome: outcomeText,
|
||||
actionable: false,
|
||||
});
|
||||
// Remove from pending map — the outcome is now recorded.
|
||||
d.pendingPermissions = new Map(d.pendingPermissions);
|
||||
d.pendingPermissions.delete(responseId);
|
||||
}
|
||||
|
||||
+9
-5
@@ -120,7 +120,9 @@ below). It is omitted on all other frame kinds.
|
||||
| `acp_read` | Inbound ACP protocol frame (model → harness) |
|
||||
| `acp_write` | Outbound ACP protocol frame (harness → model) |
|
||||
| `turn_started` | A new agent turn has begun |
|
||||
| `session_resolved` | Session completed or terminated |
|
||||
| `session_resolved` | Session ready — emitted once when the agent session is established (before the first prompt) |
|
||||
| `turn_completed` | Terminal lifecycle — emitted when a turn ends (success, cancel, or timeout) |
|
||||
| `turn_error` | Terminal lifecycle — emitted when a turn ends with an error or process death |
|
||||
| `control_result` | Acknowledgement telemetry emitted after processing a control frame |
|
||||
|
||||
Permission `acp_read` frames (carrying `session/request_permission` calls) always
|
||||
@@ -165,10 +167,12 @@ call, the `ObserverEvent` carries an `authorization` field:
|
||||
| `"cancelled"` | The turn was cancelled while the request was pending; request failed closed (denial). |
|
||||
|
||||
The `uncertain` terminal (cancel arriving while the write is in flight) does NOT
|
||||
produce an `acp_write` observer event — the process is irrecoverably poisoned and
|
||||
will be respawned by the pool. Desktop clients MUST NOT expect an `acp_write` for
|
||||
every `acp_read` they receive; a missing `acp_write` after a `session_resolved`
|
||||
frame with a poisoned outcome indicates the `uncertain` path.
|
||||
produce an `acp_write` observer event — instead the harness emits a
|
||||
`permission_terminal` observer event with `authorization.reason = "uncertain"` so
|
||||
Desktop clients can retire the card without an ACP wire response. The process is
|
||||
irrecoverably poisoned and will be respawned by the pool. Desktop clients MUST NOT
|
||||
expect an `acp_write` for every `acp_read` they receive; the corresponding
|
||||
`turn_error` and `turn_completed` events are the reliable terminal lifecycle signals.
|
||||
|
||||
**Nonce binding.** The nonce is bound to the agent, channel, session, turn, request
|
||||
ID, and exact option snapshot at generation time. It MUST NOT be reused across
|
||||
|
||||
Reference in New Issue
Block a user