mirror of
https://github.com/block/buzz.git
synced 2026-11-14 05:12:27 +00:00
fix(acp): classify channel_full as transient in transcript reducer
handlePermissionDecisionResult treated every non-sent/already_decided status as an authoritative delivery failure, incrementing deliveryFailed and triggering the PermissionDecisionButtons useEffect to call setPending(null). With channel_full now keeping the retransmit orchestrator alive, this caused two concurrent delivery loops for the same nonce: the original retransmit loop still subscribed, plus a second loop from the newly re-enabled button click. Add channel_full as an early-return alongside sent/already_decided in the reducer. Both channel_full producers (mixed owner-Full/sibling-Sent and pure-Full) are transient queue-saturation conditions; the orchestrator handles resend automatically and the card must stay disabled until the retry settles or the deadline expires. Update LifecycleActivity.tsx deliveryFailed prop doc and the re-enable comment to name the three authoritative statuses (no_active_turn, channel_closed, no_channel) and explain that channel_full does not increment the token. Tests: - Fix buildTranscript_control_result_second_failure_increments_delivery_failed to use no_channel for the second failure (channel_closed would still work, but channel_full was the prior value and is now transient) - Add buildTranscript_control_result_channel_full_does_not_mark_delivery_failed: transcript 79→80; mutation (removing the guard) → test red at expected undefined got 1 - Add test_channel_full_reducer_to_component_buttons_stay_disabled in LifecycleActivity.render.test.mjs: drives full pipeline acp_read → buildTranscript → click → channel_full through reducer → assert both buttons stay disabled + no second delivery starts; companion case (no_active_turn) asserts buttons DO re-enable; mutation → red at disabled assertion Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
@@ -517,6 +517,249 @@ test("test_f3_cross_layer_four_options_acp_read_to_lifecycle_activity_two_button
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
// Cross-layer reducer+mounted regression: channel_full leaves buttons disabled
|
||||||
|
//
|
||||||
|
// Proves that the full pipeline — acp_read → buildTranscript reducer →
|
||||||
|
// LifecycleActivity component — correctly leaves both buttons DISABLED after
|
||||||
|
// a `channel_full` control_result, matching the "transient, retransmit
|
||||||
|
// orchestrator keeps going" contract.
|
||||||
|
//
|
||||||
|
// The companion case proves authoritative failures (`no_active_turn`) DO
|
||||||
|
// re-enable buttons — so the effect path is also covered.
|
||||||
|
//
|
||||||
|
// Mutation proof: restoring `channel_full` to increment `deliveryFailed` in
|
||||||
|
// `handlePermissionDecisionResult` → the card acquires `deliveryFailed: 1` →
|
||||||
|
// the component re-renders with `deliveryFailed={1}` → the useEffect fires →
|
||||||
|
// `setPending(null)` re-enables both buttons → the disabled assertion fails.
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
test("test_channel_full_reducer_to_component_buttons_stay_disabled", async () => {
|
||||||
|
const { createElement, act } = await import("react");
|
||||||
|
const { render, fireEvent } = await import("@testing-library/react");
|
||||||
|
|
||||||
|
const FAKE_NOW_SECS = Math.floor(FAKE_NOW_MS / 1000);
|
||||||
|
const FUTURE_EXPIRY = FAKE_NOW_SECS + 9_999_999;
|
||||||
|
const nonce = "nonce-cross-layer-cf";
|
||||||
|
|
||||||
|
// Base acp_read event.
|
||||||
|
const acpReadEvent = {
|
||||||
|
seq: 1,
|
||||||
|
timestamp: "2026-09-01T10:00:00.000Z",
|
||||||
|
kind: "acp_read",
|
||||||
|
agentIndex: 0,
|
||||||
|
channelId: "ch-cross-cf",
|
||||||
|
sessionId: "sess-cross-cf",
|
||||||
|
turnId: "turn-cross-cf",
|
||||||
|
payload: {
|
||||||
|
jsonrpc: "2.0",
|
||||||
|
id: "req-cross-cf",
|
||||||
|
method: "session/request_permission",
|
||||||
|
params: {
|
||||||
|
title: "Tool requires approval",
|
||||||
|
toolCallId: "tc-cross-cf",
|
||||||
|
options: [
|
||||||
|
{
|
||||||
|
optionId: "opt-allow-once",
|
||||||
|
kind: "allow_once",
|
||||||
|
name: "Allow once",
|
||||||
|
},
|
||||||
|
{ optionId: "opt-reject-once", kind: "reject_once", name: "Deny" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
authorization: {
|
||||||
|
requestNonce: nonce,
|
||||||
|
actionable: true,
|
||||||
|
expiresAt: FUTURE_EXPIRY,
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
// `channel_full` control_result — transient; must NOT set deliveryFailed.
|
||||||
|
const channelFullResult = {
|
||||||
|
seq: 2,
|
||||||
|
timestamp: "2026-09-01T10:00:01.000Z",
|
||||||
|
kind: "control_result",
|
||||||
|
agentIndex: 0,
|
||||||
|
channelId: "ch-cross-cf",
|
||||||
|
sessionId: "sess-cross-cf",
|
||||||
|
turnId: "turn-cross-cf",
|
||||||
|
payload: {
|
||||||
|
type: "permission_decision",
|
||||||
|
status: "channel_full",
|
||||||
|
requestNonce: nonce,
|
||||||
|
optionId: "opt-allow-once",
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
// `no_active_turn` control_result — authoritative failure; MUST set deliveryFailed.
|
||||||
|
const authoritativeFailure = {
|
||||||
|
seq: 2,
|
||||||
|
timestamp: "2026-09-01T10:00:01.000Z",
|
||||||
|
kind: "control_result",
|
||||||
|
agentIndex: 0,
|
||||||
|
channelId: "ch-cross-cf",
|
||||||
|
sessionId: "sess-cross-cf",
|
||||||
|
turnId: "turn-cross-cf",
|
||||||
|
payload: {
|
||||||
|
type: "permission_decision",
|
||||||
|
status: "no_active_turn",
|
||||||
|
requestNonce: nonce,
|
||||||
|
optionId: "opt-allow-once",
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
// Build both card states through the real transcript reducer.
|
||||||
|
const cardAfterChannelFull = buildTranscript([
|
||||||
|
acpReadEvent,
|
||||||
|
channelFullResult,
|
||||||
|
]).find((i) => i.renderClass === "permission");
|
||||||
|
const cardAfterAuthoritativeFailure = buildTranscript([
|
||||||
|
acpReadEvent,
|
||||||
|
authoritativeFailure,
|
||||||
|
]).find((i) => i.renderClass === "permission");
|
||||||
|
assert.ok(cardAfterChannelFull, "card must exist after channel_full");
|
||||||
|
assert.ok(
|
||||||
|
cardAfterAuthoritativeFailure,
|
||||||
|
"card must exist after authoritative failure",
|
||||||
|
);
|
||||||
|
|
||||||
|
// Reducer-level gate: channel_full must NOT set deliveryFailed.
|
||||||
|
assert.equal(
|
||||||
|
cardAfterChannelFull.deliveryFailed,
|
||||||
|
undefined,
|
||||||
|
"channel_full must not set deliveryFailed in the reducer (mutation: restoring increment → 1 here → test fails)",
|
||||||
|
);
|
||||||
|
// Reducer-level gate: no_active_turn MUST set deliveryFailed.
|
||||||
|
assert.equal(
|
||||||
|
cardAfterAuthoritativeFailure.deliveryFailed,
|
||||||
|
1,
|
||||||
|
"no_active_turn must set deliveryFailed in the reducer",
|
||||||
|
);
|
||||||
|
|
||||||
|
// ── Component: channel_full → buttons stay disabled ───────────────────────
|
||||||
|
// Start with the initial card (no deliveryFailed), click Allow to set pending.
|
||||||
|
const initialCard = buildTranscript([acpReadEvent]).find(
|
||||||
|
(i) => i.renderClass === "permission",
|
||||||
|
);
|
||||||
|
|
||||||
|
// Track delivery calls to ensure no second delivery is started.
|
||||||
|
const deliveryCalls = [];
|
||||||
|
// The first delivery is intentionally stalled — never resolves.
|
||||||
|
function stalledDelivery({ optionId }) {
|
||||||
|
deliveryCalls.push(optionId);
|
||||||
|
return new Promise(() => {});
|
||||||
|
}
|
||||||
|
|
||||||
|
let container, rerender;
|
||||||
|
await act(async () => {
|
||||||
|
({ container, rerender } = render(
|
||||||
|
createElement(LifecycleActivity, {
|
||||||
|
...BASE_PROPS,
|
||||||
|
item: initialCard,
|
||||||
|
_deliveryFn: stalledDelivery,
|
||||||
|
}),
|
||||||
|
));
|
||||||
|
});
|
||||||
|
|
||||||
|
// Click Allow — sets pending, disables both buttons.
|
||||||
|
const allowBtn = container.querySelector(
|
||||||
|
'[data-testid="permission-decision-opt-allow-once"]',
|
||||||
|
);
|
||||||
|
assert.ok(allowBtn, "allow_once button must be present before click");
|
||||||
|
await act(async () => {
|
||||||
|
fireEvent.click(allowBtn);
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
assert.equal(deliveryCalls.length, 1, "first delivery must fire on click");
|
||||||
|
|
||||||
|
// Now rerender with the post-channel_full card (deliveryFailed undefined).
|
||||||
|
// The useEffect must NOT fire (deliveryFailed didn't change), so pending stays
|
||||||
|
// set and both buttons remain disabled.
|
||||||
|
await act(async () => {
|
||||||
|
rerender(
|
||||||
|
createElement(LifecycleActivity, {
|
||||||
|
...BASE_PROPS,
|
||||||
|
item: cardAfterChannelFull,
|
||||||
|
_deliveryFn: stalledDelivery,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
const allowBtnAfterCF = container.querySelector(
|
||||||
|
'[data-testid="permission-decision-opt-allow-once"]',
|
||||||
|
);
|
||||||
|
const denyBtnAfterCF = container.querySelector(
|
||||||
|
'[data-testid="permission-decision-opt-reject-once"]',
|
||||||
|
);
|
||||||
|
assert.ok(allowBtnAfterCF, "allow button must still be in DOM");
|
||||||
|
assert.ok(denyBtnAfterCF, "deny button must still be in DOM");
|
||||||
|
assert.ok(
|
||||||
|
allowBtnAfterCF.disabled,
|
||||||
|
"allow button must remain DISABLED after channel_full (mutation: increment deliveryFailed → setPending(null) fires → button enabled → this fails)",
|
||||||
|
);
|
||||||
|
assert.ok(
|
||||||
|
denyBtnAfterCF.disabled,
|
||||||
|
"deny button must remain DISABLED after channel_full — both buttons stay disabled during automatic retry",
|
||||||
|
);
|
||||||
|
assert.equal(
|
||||||
|
deliveryCalls.length,
|
||||||
|
1,
|
||||||
|
"no second delivery must start after channel_full — retransmit orchestrator handles resend, not a second click",
|
||||||
|
);
|
||||||
|
|
||||||
|
// ── Companion: authoritative failure re-enables buttons ───────────────────
|
||||||
|
// Render a fresh card, click, then rerender with deliveryFailed: 1.
|
||||||
|
const deliveryCalls2 = [];
|
||||||
|
function stalledDelivery2({ optionId }) {
|
||||||
|
deliveryCalls2.push(optionId);
|
||||||
|
return new Promise(() => {});
|
||||||
|
}
|
||||||
|
|
||||||
|
let container2, rerender2;
|
||||||
|
await act(async () => {
|
||||||
|
({ container: container2, rerender: rerender2 } = render(
|
||||||
|
createElement(LifecycleActivity, {
|
||||||
|
...BASE_PROPS,
|
||||||
|
item: initialCard,
|
||||||
|
_deliveryFn: stalledDelivery2,
|
||||||
|
}),
|
||||||
|
));
|
||||||
|
});
|
||||||
|
|
||||||
|
const allowBtn2 = container2.querySelector(
|
||||||
|
'[data-testid="permission-decision-opt-allow-once"]',
|
||||||
|
);
|
||||||
|
assert.ok(allowBtn2, "allow button must be present for companion case");
|
||||||
|
await act(async () => {
|
||||||
|
fireEvent.click(allowBtn2);
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Rerender with authoritative failure card (deliveryFailed: 1).
|
||||||
|
// useEffect sees deliveryFailed change 0→1 → setPending(null) → buttons enabled.
|
||||||
|
await act(async () => {
|
||||||
|
rerender2(
|
||||||
|
createElement(LifecycleActivity, {
|
||||||
|
...BASE_PROPS,
|
||||||
|
item: cardAfterAuthoritativeFailure,
|
||||||
|
_deliveryFn: stalledDelivery2,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
await Promise.resolve();
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
const allowBtnAfterFail = container2.querySelector(
|
||||||
|
'[data-testid="permission-decision-opt-allow-once"]',
|
||||||
|
);
|
||||||
|
assert.ok(
|
||||||
|
!allowBtnAfterFail.disabled,
|
||||||
|
"allow button must be RE-ENABLED after no_active_turn — user can retry",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
// F3 interactive delivery-seam: acp_read → buildTranscript → LifecycleActivity
|
// F3 interactive delivery-seam: acp_read → buildTranscript → LifecycleActivity
|
||||||
// click buttons → assert _deliveryFn called with ruled allow_once/reject_once IDs
|
// click buttons → assert _deliveryFn called with ruled allow_once/reject_once IDs
|
||||||
|
|||||||
@@ -90,8 +90,11 @@ function PermissionDecisionButtons({
|
|||||||
requestNonce: string;
|
requestNonce: string;
|
||||||
/**
|
/**
|
||||||
* Monotonically increasing failure token from the reducer — incremented on
|
* Monotonically increasing failure token from the reducer — incremented on
|
||||||
* every non-`sent` `control_result`. Keying the effect on this number (not a
|
* every authoritative delivery failure (`no_active_turn`, `channel_closed`,
|
||||||
* boolean) ensures a second failure after a retry also re-enables buttons.
|
* `no_channel`). The transient `channel_full` status does NOT increment this
|
||||||
|
* token; the retransmit orchestrator handles that status automatically.
|
||||||
|
* Keying the effect on this number (not a boolean) ensures a second failure
|
||||||
|
* after a retry also re-enables buttons.
|
||||||
*/
|
*/
|
||||||
deliveryFailed?: number;
|
deliveryFailed?: number;
|
||||||
/**
|
/**
|
||||||
@@ -108,9 +111,11 @@ function PermissionDecisionButtons({
|
|||||||
const deliveryFn = _deliveryFn ?? startPermissionDecisionDelivery;
|
const deliveryFn = _deliveryFn ?? startPermissionDecisionDelivery;
|
||||||
const [pending, setPending] = React.useState<string | null>(null);
|
const [pending, setPending] = React.useState<string | null>(null);
|
||||||
|
|
||||||
// Re-enable buttons when the reducer signals delivery failure (non-`sent`
|
// Re-enable buttons when the reducer signals an authoritative delivery
|
||||||
// control_result status). The relay send succeeded but the harness couldn't
|
// failure (`no_active_turn`, `channel_closed`, `no_channel`). The transient
|
||||||
// route the click — the user should be able to retry.
|
// `channel_full` status does NOT increment this token — the retransmit
|
||||||
|
// orchestrator stays subscribed and keeps resending automatically, so buttons
|
||||||
|
// must remain disabled until the retry settles or the deadline expires.
|
||||||
React.useEffect(() => {
|
React.useEffect(() => {
|
||||||
if (deliveryFailed) {
|
if (deliveryFailed) {
|
||||||
setPending(null);
|
setPending(null);
|
||||||
|
|||||||
@@ -2403,7 +2403,7 @@ test("buildTranscript_control_result_second_failure_increments_delivery_failed",
|
|||||||
turnId: "turn-1",
|
turnId: "turn-1",
|
||||||
payload: {
|
payload: {
|
||||||
type: "permission_decision",
|
type: "permission_decision",
|
||||||
status: "channel_closed",
|
status: "no_channel",
|
||||||
requestNonce: nonce,
|
requestNonce: nonce,
|
||||||
optionId: "allow_once",
|
optionId: "allow_once",
|
||||||
},
|
},
|
||||||
@@ -2499,6 +2499,53 @@ test("buildTranscript_control_result_already_decided_does_not_mark_delivery_fail
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("buildTranscript_control_result_channel_full_does_not_mark_delivery_failed", () => {
|
||||||
|
// `channel_full` is a transient queue-saturation status — the retransmit
|
||||||
|
// orchestrator stays subscribed and keeps resending automatically. The card
|
||||||
|
// must remain DISABLED (deliveryFailed must NOT be incremented) so a second
|
||||||
|
// click cannot start a racing delivery loop while the first is still alive.
|
||||||
|
//
|
||||||
|
// Mutation proof: restoring `channel_full` to increment `deliveryFailed` in
|
||||||
|
// `handlePermissionDecisionResult` → `deliveryFailed` becomes 1 → this
|
||||||
|
// assertion fails at `expected undefined, got 1`.
|
||||||
|
const nonce = "nonce-channel-full";
|
||||||
|
const events = [
|
||||||
|
makePermissionRequestWithAuth(1, "req-cf", nonce),
|
||||||
|
{
|
||||||
|
seq: 2,
|
||||||
|
timestamp: "2026-07-01T10:00:01.000Z",
|
||||||
|
kind: "control_result",
|
||||||
|
agentIndex: 0,
|
||||||
|
channelId: "ch-1",
|
||||||
|
sessionId: "session-1",
|
||||||
|
turnId: "turn-1",
|
||||||
|
payload: {
|
||||||
|
type: "permission_decision",
|
||||||
|
status: "channel_full",
|
||||||
|
requestNonce: nonce,
|
||||||
|
optionId: "allow_once",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
];
|
||||||
|
const transcript = buildTranscript(events);
|
||||||
|
|
||||||
|
const card = transcript.find(
|
||||||
|
(i) => i.renderClass === "permission" && i.requestNonce === nonce,
|
||||||
|
);
|
||||||
|
assert.ok(card, "permission card must exist");
|
||||||
|
assert.equal(
|
||||||
|
card.deliveryFailed,
|
||||||
|
undefined,
|
||||||
|
"deliveryFailed must not be set on channel_full — it is transient; buttons must stay disabled while the retransmit loop is active",
|
||||||
|
);
|
||||||
|
// Card must remain actionable (the request is still live).
|
||||||
|
assert.equal(
|
||||||
|
card.actionable,
|
||||||
|
true,
|
||||||
|
"card must remain actionable after channel_full — the retransmit loop is still in progress",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
// ─── permission index cleanup + FOREIGN-nonce tests (Pass 4) ─────────────────
|
// ─── permission index cleanup + FOREIGN-nonce tests (Pass 4) ─────────────────
|
||||||
|
|
||||||
import { buildTranscriptState } from "./agentSessionTranscript.ts";
|
import { buildTranscriptState } from "./agentSessionTranscript.ts";
|
||||||
|
|||||||
@@ -481,17 +481,25 @@ export function handlePermissionWrite(
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Handle a `control_result` frame for a `permission_decision` delivery.
|
* Handle a `control_result` frame for a `permission_decision` delivery.
|
||||||
* A non-success status means the click did not reach the harness — marks the
|
|
||||||
* card with an incremented `deliveryFailed` counter so buttons re-enable for
|
|
||||||
* retry.
|
|
||||||
*
|
*
|
||||||
* `sent` and `already_decided` are both success: `sent` means the harness
|
* Three statuses are treated as authoritative failures — the harness cannot
|
||||||
* forwarded the decision to the live read loop; `already_decided` means a
|
* route the decision at all, so the card's `deliveryFailed` token is
|
||||||
* retransmit matched a nonce the harness had previously forwarded (delivery
|
* incremented and the buttons re-enable for a manual retry:
|
||||||
* suppressed — the deciding task has since ended). Neither may fail the card —
|
* `no_active_turn`, `channel_closed`, `no_channel`
|
||||||
* an `already_decided` that incremented `deliveryFailed` would flip a
|
*
|
||||||
* correctly-resolved card back to a clickable/failed state, the exact P1 the
|
* Two statuses are success and must not fail the card:
|
||||||
* retransmit loop exists to avoid.
|
* `sent` — the harness forwarded the decision to the live read loop.
|
||||||
|
* `already_decided` — a retransmit matched a nonce the harness had already
|
||||||
|
* applied (suppressed); incrementing `deliveryFailed` here would flip a
|
||||||
|
* correctly-resolved card back to clickable, the exact P1 this exists to
|
||||||
|
* prevent.
|
||||||
|
*
|
||||||
|
* `channel_full` is a transient queue-saturation condition (one or more
|
||||||
|
* eligible queues could not accept the frame). The retransmit orchestrator
|
||||||
|
* (`retransmitPermissionDecision.ts`) stays subscribed and keeps resending
|
||||||
|
* until the harness accepts or the deadline expires. The card must remain
|
||||||
|
* DISABLED while the automatic retry is in progress — do NOT increment
|
||||||
|
* `deliveryFailed` here.
|
||||||
*/
|
*/
|
||||||
export function handlePermissionDecisionResult(
|
export function handlePermissionDecisionResult(
|
||||||
d: PermissionDraftSlice,
|
d: PermissionDraftSlice,
|
||||||
@@ -500,8 +508,12 @@ export function handlePermissionDecisionResult(
|
|||||||
const frameType = asString(payload.type);
|
const frameType = asString(payload.type);
|
||||||
if (frameType !== "permission_decision") return;
|
if (frameType !== "permission_decision") return;
|
||||||
const deliveryStatus = asString(payload.status);
|
const deliveryStatus = asString(payload.status);
|
||||||
|
// Success statuses — no card update needed.
|
||||||
if (deliveryStatus === "sent" || deliveryStatus === "already_decided") return;
|
if (deliveryStatus === "sent" || deliveryStatus === "already_decided") return;
|
||||||
// Delivery failed — find the card by nonce and mark it retryable.
|
// Transient queue-saturation — retransmit orchestrator keeps retrying;
|
||||||
|
// do not re-enable the buttons mid-retry.
|
||||||
|
if (deliveryStatus === "channel_full") return;
|
||||||
|
// Authoritative failure — find the card by nonce and mark it retryable.
|
||||||
const nonce = asString(payload.requestNonce);
|
const nonce = asString(payload.requestNonce);
|
||||||
if (!nonce) return;
|
if (!nonce) return;
|
||||||
const itemId = d.pendingPermissionsByNonce.get(nonce);
|
const itemId = d.pendingPermissionsByNonce.get(nonce);
|
||||||
|
|||||||
Reference in New Issue
Block a user