diff --git a/docs/spec/company-brain/approvals.md b/docs/spec/company-brain/approvals.md index 52f7be2fc..c89d4308b 100644 --- a/docs/spec/company-brain/approvals.md +++ b/docs/spec/company-brain/approvals.md @@ -228,6 +228,68 @@ card can appear live. It is deliberately thin — an id, a dotted kind, a thread because the effect's payload is redacted in exactly one place and must not acquire a second. A reader re-reads the approvals feed for the rest. +### One turn is asked about once (issue #842) + +A research turn that reaches `espn.com`, `bbc.com` and `theguardian.com` parks +three approvals, and asking three times is the same fact told badly: it is one +piece of work, and every interruption costs a re-dispatch cycle that can +dead-end. So the parks a single turn raised are **surfaced as one request**. + +The grouping key is not new. Issue #469 already journals the parking cycle, so +that a turn blocked on four decisions is continued exactly once when the last +one lands. `ApprovalSummary.batch` projects that same key, which is what makes +the two agree by construction: the batch an operator is asked about in one card +is precisely the batch the runtime holds a single continuation for. It is opaque +— an equality key, never an ordering, a count, or anything to show an operator. + +**The grant model does not change at all.** There is no batch entity on the +host, no batch resolve on the wire, and nothing new in how a grant is minted, +stored or revoked. Each approval keeps its own id, its own verdict and — on +approve — its own host-scoped grant, so approving three fetches still leaves +three independently revocable rows under `Standing permissions`, one per host, +each with its own expiry. Batching the *asking* is not batching the *granting*, +and widening a grant to save a click would be exactly the leak `grants.md` +exists to prevent. + +Two renderings over that one state, divided by what each surface is **for**: + +- **Chat is the fast path: all-or-nothing.** One card per turn, listing the + hosts it covers, with a single Approve/Decline and the ordinary scope + control. Approve grants every call in the batch; Decline grants none. The + operator is mid-conversation and wants one decision, not a form. It answers + every item it is still asking about, because the turn stays blocked until each + parked call has a verdict — a decision that left one open would hold the turn + while looking as though it had resolved the card. +- **The Approvals page is the granular path: itemised.** One row per gated + call, approved or declined on its own, matching how `Standing permissions` + lists one revocable row per grant. It is where an operator goes for precision, + or to clean up after the fact. A row says how many others came from the same + turn, so someone arriving from the toast can tell one batch from an unrelated + queue. + +Granular control in *both* places would be redundant, and would double the state +that has to stay in step between two surfaces — so it lives in one. + +**A decision that does not land is named, not swallowed.** One click fans out to +one resolve per item, so a failure on the third leaves two effects authorised +and one not. A toast is the wrong home for that — it does not say *which*, and +it is gone by the time the operator looks back at the card — so the row that +failed says so itself, the card counts the failures honestly (never "nothing was +recorded" about a click that authorised two of three), and the buttons stay live, +because a retry is the way out. A retry re-resolves only what is still pending. + +The two must not drift, and do not, because neither owns any state: both render +the same feed, and both react to the `approval_resolved` frame. Deciding a row +on the page settles that item on the chat card without a reload, and the card +reports a partial state (`1 of 3 decided`) rather than going on claiming three +things are pending. + +An approval with **no** batch — a workflow node, a scheduler tick, a park +journaled before #469 — is never grouped, not even with another one like it. +Absent means "the host did not say which turn this came from", and folding two +unknowns together would invent a batch out of a shared silence. Each is shown +alone, exactly as before this existed. + ## Delegation levels (standing rules) Prosumers adjust the fence in plain language, which compiles to policy: diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index d7cc868db..fdfc47275 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -365,6 +365,27 @@ export interface ApprovalSummary { * alone, exactly as every approval did before this shipped. */ thread?: string | null; + /** + * Which turn's gated calls this one belongs to (#842) — an opaque key shared + * by every approval a single agent turn parked. + * + * **A display grouping, never a decision.** One research turn that reaches + * three sites parks three approvals, and each stays its own record with its + * own id, its own approve/decline and — on approve — its own host-scoped + * grant (#739). The conversation consolidates them into one card so it + * interrupts once instead of three times; the Approvals page deliberately + * keeps one row per approval, matching how `Standing permissions` lists one + * revocable row per grant. Resolving is per id on both surfaces. + * + * Never compare it for anything but equality, and never show it: it is a + * runtime identifier, which the glossary rule keeps off an operator's screen. + * + * Absent for an approval no turn raised (a workflow node, a scheduler tick) + * and against a host that predates the field. Both are grouped alone, which + * is exactly the pre-#842 rendering — so an old host still produces a card + * that can be decided. + */ + batch?: string | null; } /** diff --git a/frontend/src/components/app-shell.tsx b/frontend/src/components/app-shell.tsx index c5364ba23..9e64b6a40 100644 --- a/frontend/src/components/app-shell.tsx +++ b/frontend/src/components/app-shell.tsx @@ -351,6 +351,21 @@ export function AppShell({ () => new Map(), ); const [decidedApprovals, setDecidedApprovals] = useState>({}); + /** + * Decisions that did **not** land, per approval id (#842 review). + * + * A third map, and it earns its keep because of consolidation. Deciding three + * cards separately, a failure belongs to the one card just clicked and the + * toast is beside it. Deciding one card that covers three, a failure on the + * third leaves two effects authorised and one not — and an item that simply + * drops back to its pending look reads as "still working", not "this one did + * not take". The operator clicked once and got two thirds of what they asked + * for, with nothing on screen saying which third. + * + * Cleared when that item is decided again, so a retry starts from a clean + * state rather than showing the previous attempt's error under a live one. + */ + const [failedApprovals, setFailedApprovals] = useState>({}); const pending = feed.status.pending_approvals; @@ -442,6 +457,7 @@ export function AppShell({ // must not survive the switch as a ghost in the new company's channels. setDecidedApprovals({}); setDecidingApprovals(new Map()); + setFailedApprovals({}); const hydrate = (threadId: string) => { client @@ -893,6 +909,16 @@ export function AppShell({ }); }, []); + /** Drops a recorded failure — a retry is starting, or the item is gone. */ + const clearFailure = useCallback((id: string) => { + setFailedApprovals((prev) => { + if (!(id in prev)) return prev; + const next = { ...prev }; + delete next[id]; + return next; + }); + }, []); + /** * Decide an approval from inside the conversation it was raised in (#379). * @@ -922,6 +948,9 @@ export function AppShell({ ) => { if (decidingApprovals.has(approval.id)) return; markDeciding(approval.id, verdict); + // A retry starts clean: the previous attempt's error must not sit under a + // live one, or the operator cannot tell which attempt it belongs to. + clearFailure(approval.id); try { await client.resolveApproval(approval.id, verdict, undefined, company, { detach: true, @@ -943,6 +972,12 @@ export function AppShell({ const msg = err instanceof ApiError ? err.message : "something went wrong"; toast.error(`Couldn't record your decision — ${msg}`); noteInChannel(approval.thread, `Couldn't record your decision — ${msg}`); + // On the card as well as in a toast, and keyed to the item that failed. + // A toast is the wrong and only home for this once one click covers + // several calls: it says a decision failed without saying *which*, and it + // is gone by the time the operator looks back at the card. The row that + // did not take has to say so itself. + setFailedApprovals((prev) => ({ ...prev, [approval.id]: msg })); } finally { markDeciding(approval.id, null); void feed.refresh(); @@ -1011,6 +1046,14 @@ export function AppShell({ prev[event.approvalId] ? prev : { ...prev, [event.approvalId]: { verdict, approval } }, ); } + // A failed attempt here is superseded the moment the approval resolves + // anywhere (#842 review). The retry path clears its own failure, but a + // decision made on the Approvals page or in another tab arrives only as + // this frame — and a settled approval that still carried "not recorded" + // would be the card contradicting the queue, which is the drift the + // batching work exists to remove. Cleared unconditionally on the id, + // whether or not this console ever held a summary for it. + clearFailure(event.approvalId); } void feed.refresh(); }, @@ -1117,6 +1160,7 @@ export function AppShell({ } decidingApprovals={decidingApprovals} decidedApprovals={decidedApprovals} + failedApprovals={failedApprovals} /> )} {view === "conversation" && ( diff --git a/frontend/src/views/ApprovalsView.tsx b/frontend/src/views/ApprovalsView.tsx index b43c99f5d..4e6caa59f 100644 --- a/frontend/src/views/ApprovalsView.tsx +++ b/frontend/src/views/ApprovalsView.tsx @@ -97,6 +97,29 @@ export function ApprovalsView({ client, company, feed, onResolved, onGoToConvers const { approvals, now } = feed; const askerNames = useAskerNames(client, company, approvals); const { grants, granterNames, refreshGrants } = useStandingGrants(client, company); + /** + * How many rows each turn's batch still has waiting (#842). + * + * The page stays **itemised** — one row per gated call, each independently + * approvable, exactly as `Standing permissions` below lists one revocable row + * per grant. This is the one thing it borrows from the conversation's + * consolidated card: a row says how many others were asked for alongside it, + * so an operator who arrives here from the toast can tell "this is one of + * three from one turn" from "these are three unrelated requests" — which is + * the difference between deciding the batch and deciding a queue. + * + * Counted over what is still pending rather than over the whole batch, so the + * number shrinks as rows are decided instead of promising a fourth row that + * has already been signed off. + */ + const batchTotals = useMemo(() => { + const counts = new Map(); + for (const a of approvals) { + if (!a.batch) continue; + counts.set(a.batch, (counts.get(a.batch) ?? 0) + 1); + } + return counts; + }, [approvals]); const markInFlight = (id: string, verdict: Verdict | null) => setInFlight((prev) => { @@ -207,6 +230,7 @@ export function ApprovalsView({ client, company, feed, onResolved, onGoToConvers now={now} askerNames={askerNames} deciding={inFlight.get(a.id) ?? null} + batchTotal={batchTotals.get(a.batch ?? "") ?? 1} onDecide={(verdict, scope) => void decide(a, verdict, scope)} /> ))} @@ -477,6 +501,7 @@ function ApprovalCard({ now, askerNames, deciding, + batchTotal, onDecide, }: { approval: ApprovalSummary; @@ -484,6 +509,11 @@ function ApprovalCard({ askerNames: Map; /** The verdict this card is waiting on, or `null` when it is idle (#373). */ deciding: Verdict | null; + /** + * How many rows this turn's batch still has waiting, including this one + * (#842). `1` — the default for an approval with no batch — says nothing. + */ + batchTotal: number; onDecide: (verdict: Verdict, scope: GrantScope) => void; }) { // Per-card, like the in-flight verdict and for the same reason: two cards can @@ -559,7 +589,17 @@ function ApprovalCard({ approve is not done when the button stops spinning, it is handed to the agent. A decline IS terminal, so it only has to record. */ status={ - deciding ? (deciding === "approve" ? "Waiting for the agent…" : "Recording…") : undefined + deciding + ? deciding === "approve" + ? "Waiting for the agent…" + : "Recording…" + : batchTotal > 1 + ? // Deliberately a count and not a link: the row is decided + // here, on its own, and pointing at the others would imply a + // batch decision this page does not offer. The conversation's + // card is where one Approve covers all of them (#842). + `1 of ${batchTotal} from the same turn` + : undefined } /> diff --git a/frontend/src/views/ChatView.tsx b/frontend/src/views/ChatView.tsx index 438272bee..1a4b3d19e 100644 --- a/frontend/src/views/ChatView.tsx +++ b/frontend/src/views/ChatView.tsx @@ -127,6 +127,13 @@ interface Props { /** The verdict each card is waiting on, and the ones already witnessed. */ decidingApprovals?: ReadonlyMap; decidedApprovals?: Record; + /** + * Decisions that did not land, per approval id (#842) — the message to show + * on that item. Owned by the shell, like the two maps above, because a + * failure has to outlive this view unmounting: the operator's next move after + * one is often to open the Approvals page and come back. + */ + failedApprovals?: Record; } /** @@ -161,6 +168,7 @@ export function ChatView({ onDecideApproval, decidingApprovals, decidedApprovals, + failedApprovals, }: Props) { // Which (connection, company) this subtree's browser-local state belongs to. const scope = useLocalScope(); @@ -827,6 +835,7 @@ export function ChatView({ now={now} askerNames={askerNames} decidingApprovals={decidingApprovals} + failedApprovals={failedApprovals} onDecideApproval={onDecideApproval} /> {consoleOnlyMember && ( diff --git a/frontend/src/views/chat/ApprovalRow.tsx b/frontend/src/views/chat/ApprovalRow.tsx index b840d5d15..6fbd42368 100644 --- a/frontend/src/views/chat/ApprovalRow.tsx +++ b/frontend/src/views/chat/ApprovalRow.tsx @@ -1,4 +1,5 @@ -// A parked approval, raised inside the conversation that produced it (#379). +// The gated calls one turn parked, raised inside the conversation that produced +// them (#379, consolidated by #842). // // The same content the Approvals page shows — headline, payload, asker, waiting // time, and the grant-scope choice (#431), all from `@/components/approval-card` @@ -6,15 +7,40 @@ // panel bolted beside it. Sharing the scope control rather than restating it is // what keeps the two surfaces from offering different things for one approval. // -// The one thing it does differently is how it resolves: **detached** (#391). -// The default resolve answers with the follow-up turn's replies, and rendering -// those here would put the continuation into the channel once from the POST -// body and again from its SSE echo. Detach has exactly one delivery path, so -// the duplicate-bubble race #391 deliberately left open outside chat POSTs -// cannot exist here. +// ## One card, however many calls (#842) +// +// A research turn that reaches three sites parks three approvals, and asking +// three times is the same fact told badly: it is one piece of work, and each +// interruption costs a re-dispatch cycle (#561) that can dead-end. So a batch +// renders as **one card with one Approve**, listing the hosts it covers. +// +// **All-or-nothing, deliberately.** Approve grants every call in the batch; +// Decline grants none. The operator is mid-conversation and wants one decision, +// not a form. Granularity is not missing — it lives on the Approvals page, +// which itemises the same parks one row at a time and is where an operator goes +// when they want precision or are cleaning up after the fact. Offering +// per-item control in both places would be redundant, and would double the +// state that has to stay in step between two surfaces. +// +// **What is not batched is the deciding.** One Approve resolves each item on +// its own id, so each approved call still mints its own host-scoped grant +// (#739) — three fetches produce three independently revocable standing +// permissions, exactly as they do today. Nothing about how grants are minted, +// stored or revoked changes here; only the asking is consolidated. There is no +// batch decision on the wire and no batch record on the host. +// +// A single-item batch renders exactly as this card did before #842: no list, +// no counts. The consolidation has to earn its extra furniture. +// +// The one thing it does differently from the page is how it resolves: +// **detached** (#391). The default resolve answers with the follow-up turn's +// replies, and rendering those here would put the continuation into the channel +// once from the POST body and again from its SSE echo. Detach has exactly one +// delivery path, so the duplicate-bubble race #391 deliberately left open +// outside chat POSTs cannot exist here. -import { Check, Loader2, X } from "lucide-react"; -import { useState } from "react"; +import { Check, Loader2, ShieldCheck, TriangleAlert, X } from "lucide-react"; +import { useMemo, useState } from "react"; import type { ApprovalSummary, GrantScope, Verdict } from "@/api/types"; import { @@ -22,8 +48,10 @@ import { ApprovalMeta, ApprovalPayload, ApprovalScopeControl, + approvalIcon, } from "@/components/approval-card"; import { Button } from "@/components/ui/button"; +import { approvalAction, payloadLines } from "@/lib/language"; import { cn } from "@/lib/utils"; /** What the card says once a verdict has been witnessed. */ @@ -36,81 +64,189 @@ function settledLabel(verdict: Verdict): string { : "Declined — recorded, and nothing will run"; } +/** + * What the batch says while some of it is still undecided (#842). + * + * The sentence the two surfaces would otherwise drift apart on. An operator can + * approve one row on the Approvals page while this card is on screen, and a + * card that went on claiming three things were pending would be showing them a + * queue that no longer exists. `decided` arrives from the shell's witnessed + * map, which is fed by the `approval_resolved` stream frame, so this settles + * without a reload wherever the decision was actually made. + */ +function partialLabel(settled: number, total: number): string { + return `${settled} of ${total} decided — ${total - settled} still waiting on you`; +} + +/** + * What the card says when a decision did not land (#842 review). + * + * The failure consolidation makes worse, said out loud. Deciding three cards + * separately, a failure belongs to the one card just clicked. Deciding one card + * covering three, a failure on the third leaves two effects authorised and one + * not — and a toast is both the wrong home for that (it does not say *which*) + * and a temporary one. So the count is stated on the card, the failed rows name + * themselves, and the buttons stay live because a retry is the way out. + * + * Never "nothing was recorded" unless that is true: on a batch, saying so about + * a click that authorised two of three would be a fresh lie in place of the + * silence it replaces. + */ +function failureLabel(failedCount: number, total: number): string { + if (total === 1) return "Not recorded — try again"; + return failedCount === total + ? `None of the ${total} were recorded — try again` + : `${failedCount} of ${total} weren't recorded — try again`; +} + +/** + * One item's line in a batch: what this particular call will do. + * + * The **first payload line**, which is the tool's leading argument — the URL for + * a fetch, the command for a shell call — because `PAYLOAD_KEY_ORDER` already + * promotes the argument that is the thing being consented to. Falls back to the + * action's own words when the host sent no payload (an old host) or withheld it + * (#618), so a row never renders blank and never invents a value it does not + * have. + */ +function itemLabel(a: ApprovalSummary): string { + if (a.contents_hidden) return `${approvalAction(a)} — details hidden by your role`; + return payloadLines(a)[0]?.value ?? approvalAction(a); +} + export function ApprovalRow({ - approval, + approvals, now, askerNames, deciding, decided, + failed, onDecide, }: { - approval: ApprovalSummary; + /** The gated calls this turn parked. Never empty — see `TimelineItem`. */ + approvals: ApprovalSummary[]; now: number; askerNames: Map; - /** The verdict this card is waiting on, or `null` when idle. */ - deciding: Verdict | null; - /** A verdict already witnessed — from this console or from the page. */ - decided: Verdict | null; - onDecide: (verdict: Verdict, scope: GrantScope) => void; + /** The verdict an item is waiting on, keyed by approval id; empty when idle. */ + deciding: ReadonlyMap; + /** Verdicts already witnessed — from this console or from the page. */ + decided: Record; + /** + * Decisions that did not land, keyed by approval id — the message to show. + * + * Separate from {@link decided} because a failed decision is neither: the + * item is not settled, and it is not simply still pending either. One click + * covering three calls can leave two authorised and one not, and an item that + * dropped back to its pending look would read as "still working" rather than + * "this one did not take" — the operator would believe they got all three. + */ + failed: Record; + onDecide: (approval: ApprovalSummary, verdict: Verdict, scope: GrantScope) => void; }) { - // Per-card, exactly as on the page: two approvals can be parked in one - // channel and each carries its own decision. Defaults to `once`, so a card - // decided without touching the control behaves as it did before #431 — the - // scope is opt-in here too. + // Per-card, exactly as on the page: two batches can be parked in one channel + // and each carries its own decision. Defaults to `once`, so a card decided + // without touching the control behaves as it did before #431 — the scope is + // opt-in here too. const [scope, setScope] = useState({ kind: "once" }); + const lead = approvals[0]; + const pending = useMemo(() => approvals.filter((a) => !decided[a.id]), [approvals, decided]); + const settledCount = approvals.length - pending.length; + const failedCount = pending.filter((a) => failed[a.id]).length; + const busy = deciding.size > 0; + // Everything decided: the card has nothing left to ask and steps back. + const done = pending.length === 0; + /** Whether any item in this card is waiting on `verdict` right now. */ + const awaiting = (verdict: Verdict) => [...deciding.values()].includes(verdict); + + /** + * The decision, applied to every item the card is still asking about. + * + * **Every** item, and that is what makes the card honest: the turn is blocked + * until each call it parked has an answer (#469), so a decision that left one + * undecided would hold the turn open while looking like it had resolved the + * card. One click answers all of them, and the runtime continues the turn + * once, when the last of them lands. + * + * `pending` and not `approvals`: an item already decided on the Approvals + * page has been dropped by the host, and resolving it again would be a second + * decision on an approval that no longer exists. + * + * A decline carries no scope — there is nothing to grant, and the host + * refuses the pairing anyway. + */ + const decideAll = (verdict: Verdict) => { + for (const a of pending) { + onDecide(a, verdict, verdict === "approve" ? scope : { kind: "once" }); + } + }; + + const actions = done ? undefined : ( + <> + + + + ); + return (
1 ? "Approval request for several actions" : "Approval request"} + data-approval-id={lead.id} + data-approval-count={approvals.length} className={cn( "rounded-xl border bg-card px-4 py-3 shadow-sm", // A settled card steps back rather than disappearing: the operator // has to be able to see their own decision land. - decided && "opacity-70", + done && "opacity-70", )} >
- - - - - ) - } - /> + {approvals.length > 1 ? ( + + ) : ( + + )} - + {approvals.length > 1 ? ( + // What the one decision covers, spelled out. Read-only: the card is + // all-or-nothing, so a control here would offer a choice the + // buttons below do not honour. An item settled elsewhere still says + // so, which is how the card stops claiming three things are pending + // when one has been decided on the page. +
    + {approvals.map((a) => ( + + ))} +
+ ) : ( + + )} {/* * The same control the page renders, from the same module — it @@ -118,29 +254,47 @@ export function ApprovalRow({ * granted broadly shows nothing here for exactly the reason it shows * nothing there. A settled card drops it: there is no decision left * to scope. + * + * Rendered against the first still-undecided item, and it means the + * same thing for every item it covers: each approved call mints its + * own grant, scoped to its own arguments (#739). One choice, one + * grant per item — never one grant spanning them. */} - {!decided && ( + {!done && ( )} 1 + ? `All ${approvals.length} decided` + : // A single-item card says what it always said. `pending` is + // empty here, so the lead's verdict is present — the `??` + // only satisfies the type, it is not a reachable state. + settledLabel(decided[lead.id] ?? "deny") + : busy + ? awaiting("approve") ? "Waiting for the agent…" : "Recording…" - : undefined + : // A failure outranks the partial count, because it is the + // one thing here the operator has to act on. It also has to + // reach a single-item card, which renders no item list to + // carry the per-row form. + failedCount > 0 + ? failureLabel(failedCount, approvals.length) + : settledCount > 0 + ? partialLabel(settledCount, approvals.length) + : undefined } />
@@ -148,3 +302,141 @@ export function ApprovalRow({
); } + +/** + * The batch's headline: what the turn is asking for, and how many of them. + * + * Named by the shared action when every call is the same tool — the reported + * case, three fetches from one research turn — and by a neutral count when they + * are not, because "Fetch web pages" over a batch that also sends mail would be + * a card that understates what approving it does. + */ +function BatchHeadline({ + approvals, + askerNames, + actions, +}: { + approvals: ApprovalSummary[]; + askerNames: Map; + actions?: React.ReactNode; +}) { + const lead = approvals[0]; + const sameKind = approvals.every((a) => a.kind === lead.kind); + // A mixed batch has no one glyph that is true of it, so it wears the neutral + // one rather than the first item's — an envelope over a card that also + // deploys a website would be the icon quietly making a claim. + const Icon = sameKind ? approvalIcon(lead.kind) : ShieldCheck; + const asker = lead.agent ? (askerNames.get(lead.agent) ?? lead.agent) : null; + const title = sameKind ? approvalAction(lead) : `${approvals.length} actions need your sign-off`; + + return ( +
+
+ +
+
+

{title}

+

+ {asker ? `${asker} needs` : "This turn needs"} {approvals.length} sign-offs before it + can carry on +

+
+ {actions &&
{actions}
} +
+ ); +} + +/** + * One call inside a batch: what it will do, and whether it is still waiting. + * + * Read-only. The card decides all-or-nothing, so a control on a line would + * offer a choice the buttons do not honour — and the granular path already + * exists on the Approvals page, which lists these same parks one row at a time. + * + * An item already decided — here, or on the page while this card was open — + * states its verdict instead. That is the half of #842 that keeps the two + * surfaces honest: a card cannot keep listing as pending something that has + * already been answered somewhere else. + */ +function BatchItem({ + approval: a, + verdict, + deciding, + failure, +}: { + approval: ApprovalSummary; + /** A verdict already witnessed for this item, or `null` while it is pending. */ + verdict: Verdict | null; + /** The verdict this item is waiting on, or `null` when idle. */ + deciding: Verdict | null; + /** Why this item's decision did not land, or `null` when none has failed. */ + failure: string | null; +}) { + const label = itemLabel(a); + + // A failed decision outranks the pending look, and says which item and why. + // Silence here is the failure mode worth designing against: the operator + // clicked once for three calls, two were authorised, and a third that merely + // looks unstarted reads as "still working". Stated plainly, with the way back + // — the card's own buttons are still live, so a retry is one press. + // + // **A settled verdict outranks it in turn**, which is why `verdict` is tested + // first. A failure describes one *attempt*; a verdict describes the approval. + // An item that failed here and was then resolved on the Approvals page or in + // another tab has both, and showing "not recorded" over an approval the host + // has already acted on would be the card contradicting the queue — the exact + // drift this work exists to remove. The shell also clears the failure when + // that frame arrives; this ordering is what makes the render correct + // regardless of which state reaches it first. + if (failure && !deciding && !verdict) { + return ( +
  • + + {label} + Not recorded — {failure} +
    • + ); + } + + return ( + // Addressable per item, because the card is no longer one approval. The + // group keeps `data-approval-id` for the first — an existing selector still + // finds the card the request was raised in — and this is how a caller + // reaches any of the others. +
  • + {verdict === "approve" ? ( + + ) : verdict === "deny" ? ( + + ) : ( + + · + + )} + + {label} + + {deciding ? ( + + ) : ( + verdict && ( + + {verdict === "approve" ? "Approved" : "Declined"} + + ) + )} +
    • + ); +} diff --git a/frontend/src/views/chat/MessageTimeline.tsx b/frontend/src/views/chat/MessageTimeline.tsx index a2a314456..7ebd888eb 100644 --- a/frontend/src/views/chat/MessageTimeline.tsx +++ b/frontend/src/views/chat/MessageTimeline.tsx @@ -36,6 +36,8 @@ interface Props { askerNames?: Map; /** The verdict each inline card is currently waiting on. */ decidingApprovals?: ReadonlyMap; + /** Decisions that did not land, per approval id (#842) — see `ApprovalRow`. */ + failedApprovals?: Record; onDecideApproval?: (approval: ApprovalSummary, verdict: Verdict, scope: GrantScope) => void; } @@ -78,6 +80,7 @@ export function MessageTimeline({ now, askerNames, decidingApprovals, + failedApprovals, onDecideApproval, }: Props) { const scroller = useRef(null); @@ -151,12 +154,18 @@ export function MessageTimeline({ ) : ( onDecideApproval?.(item.approval, verdict, scope)} + failed={failedApprovals ?? EMPTY_FAILURES} + onDecide={(approval, verdict, scope) => + onDecideApproval?.(approval, verdict, scope) + } /> ), )} @@ -173,6 +182,34 @@ export function MessageTimeline({ /** Stable identity so a missing `askerNames` cannot churn the card's props. */ const EMPTY_NAMES: Map = new Map(); +/** Stable identity, for the same reason as {@link EMPTY_NAMES}. */ +const EMPTY_DECIDING: ReadonlyMap = new Map(); + +/** Ditto — no failure has been recorded on any card. */ +const EMPTY_FAILURES: Record = {}; + +/** + * The in-flight verdicts belonging to one card's items (#842). + * + * The shell keeps one map for the whole console, and a batch card must not + * spin — or disable its buttons — because a different turn's approval is being + * decided. Returns the shared empty map when nothing of this card's is in + * flight, so the common case allocates nothing and the props stay identical + * between renders. + */ +function decidingIn( + approvals: ApprovalSummary[], + deciding: ReadonlyMap | undefined, +): ReadonlyMap { + if (!deciding?.size) return EMPTY_DECIDING; + const mine = new Map(); + for (const a of approvals) { + const verdict = deciding.get(a.id); + if (verdict) mine.set(a.id, verdict); + } + return mine.size ? mine : EMPTY_DECIDING; +} + function DayDivider({ label }: { label: string }) { return (
    ; }; /** @@ -408,6 +427,20 @@ export type TimelineItem = * * A `decided` card is kept even once the feed has dropped it, so the operator * sees their own decision land rather than the card disappearing. + * + * ## One card per turn (#842) + * + * Approvals sharing a `batch` — the host's key for the turn that parked them — + * collapse into a single item. The conversation is interrupted once for one + * piece of work, which is the whole of the issue; the grouping is presentation + * only, and every approval inside the item is still decided on its own id. + * + * Approvals with **no** batch are never grouped, not even with each other. An + * absent key means "the host did not say which turn this came from" — a + * workflow node, a scheduler tick, an older host — and folding those together + * would invent a batch out of two facts that are only alike in being unknown, + * which is how an operator ends up approving something they were never shown. + * Each gets its own card, exactly as before this existed. */ export function buildTimelineItems( entries: TimelineEntry[], @@ -420,15 +453,42 @@ export function buildTimelineItems( at: entry.message.at, entry, })); + + // Insertion-ordered, so a batch lands where its **first** approval did rather + // than wherever the last one happened to arrive. The caller hands us the + // pending feed followed by the settled ones, so an item decided on the + // Approvals page rejoins the card it was raised in instead of opening a + // second one below it. + const batches = new Map(); for (const approval of approvals) { + // The id is the fallback key, which is what makes "ungrouped" the safe + // default: an id is unique, so a batchless approval can only ever group + // with itself. + const key = approval.batch ?? `solo:${approval.id}`; + const bucket = batches.get(key); + if (bucket) bucket.push(approval); + else batches.set(key, [approval]); + } + + for (const [key, batch] of batches) { + batch.sort((a, b) => a.at_millis - b.at_millis || (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); + const verdicts: Record = {}; + for (const approval of batch) { + const verdict = decided[approval.id]?.verdict; + if (verdict) verdicts[approval.id] = verdict; + } items.push({ kind: "approval", - key: `approval:${approval.id}`, - at: approval.at_millis, - approval, - decided: decided[approval.id]?.verdict ?? null, + key: `approval:${key}`, + // The turn asked once, at the moment its first call was gated. Placing + // the card at the earliest of the batch is what keeps it beside the + // message that provoked it. + at: batch[0].at_millis, + approvals: batch, + decided: verdicts, }); } + // Stable within a millisecond: a card raised by the very turn whose reply // shares its timestamp should sit after that reply, not shuffle between // renders. `sort` is stable in every engine this ships to, so equal `at` diff --git a/frontend/test/unit/approval-batch-card.test.ts b/frontend/test/unit/approval-batch-card.test.ts new file mode 100644 index 000000000..bd1f15b38 --- /dev/null +++ b/frontend/test/unit/approval-batch-card.test.ts @@ -0,0 +1,293 @@ +// @vitest-environment jsdom + +import { act, createElement } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import type { ApprovalSummary, GrantScope, Verdict } from "@/api/types"; +import { ApprovalRow } from "@/views/chat/ApprovalRow"; + +/** + * The consolidated card's decisions (issue #842). + * + * This suite is normally for pure functions — see `vitest.config.ts` — and the + * exception is earned the same way `provider-detail-render` earns it: the thing + * under test *is* what reaches the operator's hand. The issue's whole claim is + * that one click can answer three gated calls **without** widening what any of + * them buys, and three of those claims are only true at the click: + * + * 1. one Approve resolves every item, each on its own id — so each approved + * call still mints its own host-scoped grant (#739) rather than one grant + * spanning the batch, and three fetches still produce three independently + * revocable standing permissions; + * 2. the card is **all-or-nothing** — it answers every item it is still asking + * about, because the turn stays blocked until each parked call has a + * verdict (#469), so a decision that left one open would hold the turn + * while looking as though it had resolved the card; + * 3. an item decided elsewhere — the Approvals page, another tab — is not + * re-resolved, and the card stops listing it as pending. + * + * A pure test of the grouping cannot reach any of them: it can see the card is + * built, not what pressing it sends. + */ + +const T0 = new Date("2026-03-02T10:00:00Z").getTime(); + +function approval(id: string, url: string): ApprovalSummary { + return { + id, + kind: "web_fetch", + amount_usd: null, + at_millis: T0, + agent: "seo", + thread: "desk-marketing", + batch: "turn-1", + broadly_grantable: true, + payload: { url }, + }; +} + +const ESPN = approval("a1", "https://espn.com/nba"); +const BBC = approval("a2", "https://bbc.com/sport"); +const GUARDIAN = approval("a3", "https://theguardian.com/uk"); + +interface Decision { + id: string; + verdict: Verdict; + scope: GrantScope; +} + +let container: HTMLDivElement; +let root: Root; +let decisions: Decision[]; + +async function render( + approvals: ApprovalSummary[], + decided: Record = {}, + failed: Record = {}, + deciding: ReadonlyMap = new Map(), +) { + await act(async () => { + root.render( + createElement(ApprovalRow, { + approvals, + now: T0 + 60_000, + askerNames: new Map([["seo", "SEO Specialist"]]), + deciding, + decided, + failed, + onDecide: (approval: ApprovalSummary, verdict: Verdict, scope: GrantScope) => + decisions.push({ id: approval.id, verdict, scope }), + }), + ); + }); +} + +/** Every item line on the card, in render order. */ +function items(): HTMLElement[] { + return [...container.querySelectorAll("[data-approval-item]")]; +} + +function button(label: string): HTMLButtonElement { + const match = [...container.querySelectorAll("button")].find((b) => + (b.textContent ?? "").includes(label), + ); + if (!match) throw new Error(`no "${label}" button on the card: ${container.textContent}`); + return match as HTMLButtonElement; +} + +async function click(el: HTMLElement) { + await act(async () => { + el.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); +} + +/** + * Toggle a checkbox or radio the way a person does — by clicking it. + * + * Deliberately **not** by assigning `.checked` first: React tracks an input's + * last-rendered value to decide whether a click changed anything, so setting it + * by hand makes the click look like a no-op and `onChange` never fires. The + * click's own activation behaviour flips the box, which is both what a browser + * does and what React is watching for. + */ +async function toggle(input: HTMLInputElement) { + await act(async () => { + input.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); +} + +beforeEach(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + decisions = []; +}); + +afterEach(() => { + act(() => root.unmount()); + container.remove(); +}); + +describe("the consolidated approval card", () => { + it("asks once for a turn's three gated calls, naming each of them", async () => { + await render([ESPN, BBC, GUARDIAN]); + + const text = container.textContent ?? ""; + expect(text).toContain("SEO Specialist"); + expect(text).toContain("3 sign-offs"); + for (const url of [ + "https://espn.com/nba", + "https://bbc.com/sport", + "https://theguardian.com/uk", + ]) { + expect(text).toContain(url); + } + // One decision to make, not three — and no per-item control beside it. + // Granularity is the Approvals page's job; offering it here too would be a + // second copy of the same state to keep in step. + expect(container.querySelectorAll("button")).toHaveLength(2); + expect(items()).toHaveLength(3); + expect(container.querySelectorAll('input[type="checkbox"]')).toHaveLength(0); + }); + + it("resolves every item on its own id, so each mints its own grant", async () => { + await render([ESPN, BBC, GUARDIAN]); + await click(button("Approve")); + + // Three resolves, not one batch resolve. The host has no batch to decide — + // the park is the unit of truth, and a grant is minted per approval from + // that approval's own arguments. + expect(decisions).toEqual([ + { id: "a1", verdict: "approve", scope: { kind: "once" } }, + { id: "a2", verdict: "approve", scope: { kind: "once" } }, + { id: "a3", verdict: "approve", scope: { kind: "once" } }, + ]); + }); + + it("carries the chosen scope to every item, so each gets its own standing grant", async () => { + await render([ESPN, BBC]); + // The broader option. One choice on the card, one standing permission per + // item — each scoped to that item's own host when the host mints it (#739), + // which is why approving three fetches leaves three independently revocable + // rows under Standing permissions rather than one that spans them. + const forAPeriod = [...container.querySelectorAll('input[type="radio"]')][1]; + await toggle(forAPeriod); + await click(button("Approve")); + + expect(decisions).toEqual([ + { id: "a1", verdict: "approve", scope: { kind: "tool", expiresInMillis: 60 * 60 * 1000 } }, + { id: "a2", verdict: "approve", scope: { kind: "tool", expiresInMillis: 60 * 60 * 1000 } }, + ]); + }); + + it("declines the whole batch with one Decline, granting nothing", async () => { + await render([ESPN, BBC]); + // Even with the broader scope selected: a decline has nothing to grant, so + // it must not carry a duration the operator picked for a yes. + const forAPeriod = [...container.querySelectorAll('input[type="radio"]')][1]; + await toggle(forAPeriod); + await click(button("Decline")); + + expect(decisions).toEqual([ + { id: "a1", verdict: "deny", scope: { kind: "once" } }, + { id: "a2", verdict: "deny", scope: { kind: "once" } }, + ]); + }); + + it("stops listing an item decided on the Approvals page, and says how many are left", async () => { + // The drift case: both surfaces open, one row approved over there. The card + // must not go on claiming three things are pending. + await render([ESPN, BBC, GUARDIAN], { a1: "approve" }); + + const text = container.textContent ?? ""; + expect(text).toContain("Approved"); + expect(text).toContain("1 of 3 decided"); + // Still listed — the operator has to see their own decision land — but + // shown as settled rather than as something still being asked about. + expect(items()).toHaveLength(3); + expect(items()[0].textContent).toContain("Approved"); + + await click(button("Approve")); + // And an approve here covers only what is still open. Re-resolving a1 would + // be a second decision on an approval the host has already dropped. + expect(decisions.map((d) => d.id)).toEqual(["a2", "a3"]); + }); + + it("names the item whose decision did not land, and does not call it pending", async () => { + // The failure consolidation makes worse. One click, three resolves, and the + // third fails: two effects are authorised and one is not. An item that + // simply dropped back to its pending look would read as "still working", + // and the operator's honest conclusion would be that they got all three. + await render([ESPN, BBC, GUARDIAN], { a1: "approve", a2: "approve" }, { a3: "host is away" }); + + const text = container.textContent ?? ""; + expect(text).toContain("Not recorded"); + expect(text).toContain("host is away"); + // Which one, on the row itself — a toast says a decision failed without + // saying which, and is gone by the time the operator looks back. + const failedRow = container.querySelector('[data-approval-failed="true"]'); + expect(failedRow?.getAttribute("data-approval-item")).toBe("a3"); + expect(failedRow?.textContent).toContain("https://theguardian.com/uk"); + }); + + it("counts the failures honestly rather than claiming nothing was recorded", async () => { + await render([ESPN, BBC, GUARDIAN], { a1: "approve", a2: "approve" }, { a3: "host is away" }); + + // Two of the three DID take. "Nothing was recorded" would be a fresh lie in + // place of the silence this replaces. + const text = container.textContent ?? ""; + expect(text).toContain("1 of 3 weren't recorded"); + expect(text).not.toContain("None of the 3"); + }); + + it("shows the settled verdict, not a stale failure, once the item resolves elsewhere", async () => { + // Failed here, then resolved on the Approvals page or in another tab: the + // item carries both a failure and a verdict. A failure describes one + // *attempt*; the verdict describes the approval, and the host has already + // acted on it. Saying "not recorded" over that would be the card + // contradicting the queue — the drift this whole change exists to remove. + await render([ESPN, BBC], { a2: "approve" }, { a2: "host is away" }); + + const settled = container.querySelector('[data-approval-item="a2"]'); + expect(settled?.textContent).toContain("Approved"); + expect(settled?.textContent).not.toContain("Not recorded"); + expect(container.querySelector('[data-approval-failed="true"]')).toBeNull(); + // And the card counts only what is still open: a2 is decided, so it is not + // one of the failures still waiting on anybody. + expect(container.textContent ?? "").not.toContain("weren't recorded"); + }); + + it("leaves the buttons live after a failure, because a retry is the way out", async () => { + await render([ESPN, BBC, GUARDIAN], { a1: "approve", a2: "approve" }, { a3: "host is away" }); + + expect(button("Approve").disabled).toBe(false); + await click(button("Approve")); + // Only the item that never landed is retried — the two that did are settled + // and re-resolving them would be a second decision on approvals the host + // has already dropped. + expect(decisions.map((d) => d.id)).toEqual(["a3"]); + }); + + it("settles rather than vanishing once every item is decided", async () => { + await render([ESPN, BBC], { a1: "approve", a2: "deny" }); + + const text = container.textContent ?? ""; + expect(text).toContain("All 2 decided"); + expect(text).toContain("Declined"); + // Nothing left to decide, so nothing left to press. + expect(container.querySelectorAll("button")).toHaveLength(0); + }); + + it("renders a single-call turn exactly as it did before batching", async () => { + await render([ESPN]); + + // No item list, no counts — the consolidation earns its furniture only when + // there is something to consolidate. + expect(items()).toHaveLength(0); + expect(container.textContent ?? "").not.toContain("sign-offs"); + await click(button("Approve")); + expect(decisions).toEqual([{ id: "a1", verdict: "approve", scope: { kind: "once" } }]); + }); +}); diff --git a/frontend/test/unit/approval-batch-isolation.test.ts b/frontend/test/unit/approval-batch-isolation.test.ts new file mode 100644 index 000000000..a9d903020 --- /dev/null +++ b/frontend/test/unit/approval-batch-isolation.test.ts @@ -0,0 +1,140 @@ +// @vitest-environment jsdom + +import { act, createElement } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import type { ApprovalSummary, Verdict } from "@/api/types"; +import { MessageTimeline } from "@/views/chat/MessageTimeline"; +import { buildTimelineItems, type Channel } from "@/views/chat/model"; + +/** + * One card is not disabled by another card's decision (#842, over #373). + * + * The shell keeps **one** in-flight map for the whole console, keyed by + * approval id, because two decisions genuinely can be in flight at once — the + * host serialises them behind its per-company lock and the console must not + * pretend otherwise. `MessageTimeline` narrows that map to each card's own + * items before handing it down; the card then reads `deciding.size > 0` as "I + * am busy". + * + * Without the narrowing that read is wrong in the one direction that matters. + * Any approval being decided anywhere — another channel, another turn, the + * Approvals page in the same tab — would make every batch card in the + * transcript report itself busy and grey out its buttons. That is issue #373's + * bug exactly, one surface up: a single in-flight slot freezing rows it has + * nothing to do with. + * + * **This suite exists because a test of the narrowing helper alone would not + * catch it.** The helper is pure and easy to test in isolation, and it would go + * on passing if someone deleted the call and passed the shell-wide map straight + * through — which is the actual regression. So the assertion has to go through + * the component that does the wiring. + */ + +const T0 = new Date("2026-03-02T10:00:00Z").getTime(); + +const CHANNEL: Channel = { + id: "marketing", + name: "marketing", + voice: "Marketing", + kind: "channel", + purpose: "", +}; + +function approval(id: string, batch: string, url: string): ApprovalSummary { + return { + id, + kind: "web_fetch", + amount_usd: null, + at_millis: T0, + agent: "seo", + thread: CHANNEL.id, + batch, + payload: { url }, + }; +} + +/** One turn's batch of two, and a second, unrelated turn's single call. */ +const ESPN = approval("a1", "turn-1", "https://espn.com/nba"); +const BBC = approval("a2", "turn-1", "https://bbc.com/sport"); +const OTHER_TURN = approval("b1", "turn-2", "https://crates.io/crates/serde"); + +let container: HTMLDivElement; +let root: Root; + +async function render(deciding: ReadonlyMap) { + const items = buildTimelineItems([], [ESPN, BBC, OTHER_TURN]); + await act(async () => { + root.render( + createElement(MessageTimeline, { + channel: CHANNEL, + items, + openThreadId: null, + typing: false, + onOpenThread: () => {}, + onReact: () => {}, + now: T0 + 60_000, + askerNames: new Map([["seo", "SEO Specialist"]]), + decidingApprovals: deciding, + onDecideApproval: () => {}, + }), + ); + }); +} + +/** The card whose first item is `approvalId`, as the DOM exposes it. */ +function card(approvalId: string): HTMLElement { + const el = container.querySelector(`[data-approval-id="${approvalId}"]`); + if (!el) throw new Error(`no card for ${approvalId}: ${container.textContent}`); + return el; +} + +function buttons(el: HTMLElement): HTMLButtonElement[] { + return [...el.querySelectorAll("button")]; +} + +beforeEach(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(() => { + act(() => root.unmount()); + container.remove(); +}); + +describe("in-flight decisions are scoped to the card they belong to", () => { + it("leaves one turn's card live while a different turn's decision is in flight", async () => { + // `b1` belongs to turn-2. Turn-1's card has nothing to do with it and must + // stay decidable — the operator can answer two conversations' requests + // without the first one freezing the second. + await render(new Map([["b1", "approve"]])); + + const turnOne = buttons(card("a1")); + expect(turnOne).toHaveLength(2); + expect(turnOne.every((b) => b.disabled)).toBe(false); + // Nor is it spinning: a card with no decision of its own in flight is idle, + // and showing it as working would be the same lie in a quieter register. + expect(card("a1").querySelectorAll(".animate-spin")).toHaveLength(0); + }); + + it("does disable the card whose own item is being decided", async () => { + // The other direction, so the test above cannot pass by the narrowing + // simply dropping everything on the floor. + await render(new Map([["a1", "approve"]])); + + expect(buttons(card("a1")).every((b) => b.disabled)).toBe(true); + expect(buttons(card("b1")).every((b) => b.disabled)).toBe(false); + }); + + it("disables a card when any one of its own items is in flight", async () => { + // One click fans out to one resolve per item, so mid-flight only some of a + // card's ids are in the map. The card is still busy. + await render(new Map([["a2", "approve"]])); + + expect(buttons(card("a1")).every((b) => b.disabled)).toBe(true); + }); +}); diff --git a/frontend/test/unit/approval-batch.test.ts b/frontend/test/unit/approval-batch.test.ts new file mode 100644 index 000000000..a0a394a0e --- /dev/null +++ b/frontend/test/unit/approval-batch.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it } from "vitest"; + +import type { ApprovalSummary, Verdict } from "@/api/types"; +import { buildTimelineItems, type TimelineItem } from "@/views/chat/model"; + +/** + * One turn asks once (issue #842). + * + * A research turn that reached espn.com, bbc.com and theguardian.com parked + * three approvals and interrupted the conversation three times — three + * sign-offs, and three re-dispatch cycles (#561) for one piece of work. The + * host already knew they belonged together: #469 records the parking turn so it + * can continue that turn exactly once. `batch` carries that key out to the + * console, and this is where it is turned into one card. + * + * What must **not** happen is the reason this suite is worth its length. The + * grouping is presentation over records that stay individually decidable, and + * two mistakes would each be invisible in a screenshot: + * + * * folding approvals the host did *not* say belonged together — an operator + * approving something they were never shown; + * * a card that keeps claiming three things are pending after one was decided + * on the Approvals page, which is the two surfaces drifting. + */ + +const T0 = new Date("2026-03-02T10:00:00Z").getTime(); + +function approval(over: Partial & Pick): ApprovalSummary { + return { + kind: "web_fetch", + amount_usd: null, + at_millis: T0, + agent: "seo", + thread: "desk-marketing", + ...over, + }; +} + +/** Every approval card in the timeline, in the order it renders. */ +function cards(items: TimelineItem[]) { + return items.filter((i): i is Extract => i.kind === "approval"); +} + +function decidedMap(entries: [ApprovalSummary, Verdict][]) { + return Object.fromEntries(entries.map(([approval, verdict]) => [approval.id, { verdict, approval }])); +} + +describe("buildTimelineItems — batching a turn's gated calls (#842)", () => { + it("raises one card carrying every call the same turn parked", () => { + const espn = approval({ id: "a1", batch: "turn-1", at_millis: T0 }); + const bbc = approval({ id: "a2", batch: "turn-1", at_millis: T0 + 900 }); + + const items = cards(buildTimelineItems([], [espn, bbc])); + + expect(items).toHaveLength(1); + expect(items[0].approvals.map((a) => a.id)).toEqual(["a1", "a2"]); + // At the moment the turn first asked, so the card sits beside the message + // that provoked it rather than beside whichever call was gated last. + expect(items[0].at).toBe(T0); + }); + + it("keeps two turns apart, however close together they parked", () => { + // The failure this guards is not hypothetical: grouping by "same agent, + // same thread, close together" — the shape a console would reach for + // without a host key — folds exactly this pair, and the operator approves a + // second turn's work with one click meant for the first. + const first = approval({ id: "a1", batch: "turn-1" }); + const second = approval({ id: "a2", batch: "turn-2", at_millis: T0 + 1 }); + + const items = cards(buildTimelineItems([], [first, second])); + + expect(items).toHaveLength(2); + expect(items.map((i) => i.approvals.map((a) => a.id))).toEqual([["a1"], ["a2"]]); + }); + + it("never groups approvals the host gave no batch for", () => { + // A workflow node, a scheduler tick, an older host: `batch` is absent, and + // absent means "unknown", not "the same one". Two unknowns are not a batch. + const workflow = approval({ id: "a1", batch: null, thread: null }); + const tick = approval({ id: "a2", thread: null }); + + const items = cards(buildTimelineItems([], [workflow, tick])); + + expect(items).toHaveLength(2); + expect(items.every((i) => i.approvals.length === 1)).toBe(true); + }); + + it("reports a batch's verdicts per item, so a partly-decided card can say so", () => { + const espn = approval({ id: "a1", batch: "turn-1" }); + const bbc = approval({ id: "a2", batch: "turn-1", at_millis: T0 + 1 }); + const guardian = approval({ id: "a3", batch: "turn-1", at_millis: T0 + 2 }); + + // What the shell holds after one row was approved on the Approvals page: + // the host has dropped it from the feed, and the witnessed map is the only + // thing that still knows it existed. + const items = cards( + buildTimelineItems([], [bbc, guardian, espn], decidedMap([[espn, "approve"]])), + ); + + expect(items).toHaveLength(1); + // The settled item rejoins the card it was raised in rather than opening a + // second one below it — and the batch is still ordered by when each call + // was gated, not by the order the feed happened to hand them over. + expect(items[0].approvals.map((a) => a.id)).toEqual(["a1", "a2", "a3"]); + expect(items[0].decided).toEqual({ a1: "approve" }); + }); + + it("carries a mixed batch's verdicts without collapsing them into one", () => { + const espn = approval({ id: "a1", batch: "turn-1" }); + const bbc = approval({ id: "a2", batch: "turn-1", at_millis: T0 + 1 }); + + const items = cards( + buildTimelineItems([], [espn, bbc], decidedMap([[espn, "approve"], [bbc, "deny"]])), + ); + + expect(items[0].decided).toEqual({ a1: "approve", a2: "deny" }); + }); +}); diff --git a/src/company/runtime.rs b/src/company/runtime.rs index 8ae673c5d..a7fa485df 100644 --- a/src/company/runtime.rs +++ b/src/company/runtime.rs @@ -1787,6 +1787,12 @@ impl CompanyRuntime { // happens at the edge, in `server::approval_visibility`, so // per-role logic stays out of the domain layer. contents_hidden: false, + // Issue #842: which turn asked for it, so the conversation can + // ask about a turn's gated calls once. Projected, never + // derived — grouping by "same agent, same thread, close + // together" would guess at a fact the journal already records, + // and would guess wrong exactly when two turns overlap. + batch: p.batch, }) .collect() } diff --git a/src/harness/policy.rs b/src/harness/policy.rs index 124998b68..289fd2474 100644 --- a/src/harness/policy.rs +++ b/src/harness/policy.rs @@ -4432,6 +4432,98 @@ mod tests { } } + /// **Issue #842's security property, stated as the mixed batch it is + /// about.** + /// + /// A research turn asks about three sites in one card. The operator ticks + /// two and leaves the third; the two approvals mint a host-scoped grant + /// each and the untouched one mints nothing. What must come out the other + /// side is exactly this: the two granted hosts run unattended, and the + /// third parks again as if it had never been on the card — because it was + /// never approved. + /// + /// The reason this is worth its own test beside the single-grant one above + /// is that batching creates a shape that could not previously exist: two + /// live grants for one agent and one tool, differing only in scope. A fix + /// that batched the *grant* instead of the *asking* — one permission + /// covering "the sites in that request" — would pass every assertion about + /// espn and bbc and fail the third one silently, which is precisely the + /// leak #739 exists to prevent. The declined host is therefore the + /// load-bearing assertion here, not an afterthought. + /// + /// Both parking tiers, for the reason the test above runs both: an outward + /// fetch parks under `auto` too, so the grant is genuinely what decides. + #[tokio::test] + async fn approving_two_items_of_a_batch_grants_two_hosts_and_leaves_the_third_parking() { + for tier in ["supervised", "auto"] { + let queue = ApprovalRequestQueue::default(); + let grants = queue.grants(); + let p = policy(tier, &[], None) + .with_requests(queue) + .with_agent("seo"); + + // What approving two of the three items mints: one host-scoped + // grant per approved item, exactly as a lone approval would. + grants.grant_standing(scoped_standing( + "seo", + crate::policy::consequence::WEB_FETCH, + "https://espn.com", + far_future(), + )); + grants.grant_standing(crate::runtime::grants::StandingGrant { + id: crate::runtime::grants::GrantId::new("g2"), + ..scoped_standing( + "seo", + crate::policy::consequence::WEB_FETCH, + "https://bbc.com", + far_future(), + ) + }); + + for url in [ + "https://espn.com/nba/scores", + "https://bbc.com/sport/football", + ] { + assert!( + matches!( + p.check(&request("web_fetch", serde_json::json!({ "url": url }))) + .await, + ToolPolicyDecision::Allow + ), + "an approved item's own host-scoped grant must admit it — `{tier}`: {url}" + ); + } + + // The item the operator left unticked. Same agent, same tool, same + // turn, two live grants — and none of them says yes to this host. + assert!( + matches!( + p.check(&request( + "web_fetch", + serde_json::json!({ "url": "https://theguardian.com/uk" }) + )) + .await, + ToolPolicyDecision::RequireApproval { .. } + ), + "approving two sites is not consent to the third — `{tier}`" + ); + + // And a host nobody ever asked about is no more reachable for + // having been adjacent to two that were. + assert!( + matches!( + p.check(&request( + "web_fetch", + serde_json::json!({ "url": "https://crates.io/crates/serde" }) + )) + .await, + ToolPolicyDecision::RequireApproval { .. } + ), + "a host that was never on the card parks — `{tier}`" + ); + } + } + /// Without a grant an outward fetch still parks in both parking tiers — the /// status quo #673 must not have widened. /// diff --git a/src/runtime/cycle.rs b/src/runtime/cycle.rs index c6357e806..0ff5441dc 100644 --- a/src/runtime/cycle.rs +++ b/src/runtime/cycle.rs @@ -5078,6 +5078,123 @@ mod test { ); } + /// Issue #842: **every gated call one turn parks carries that turn's key**, + /// and a different turn's parks carry a different one. + /// + /// This is the whole of the batching mechanism, and it is deliberately not + /// a new one. #469 already records the parking cycle so a turn blocked on + /// four decisions is continued once rather than four times; the operator + /// was simply never shown that grouping, so a research turn that reached + /// three sites interrupted the conversation three times to ask about one + /// piece of work. Projecting the key it already had is what lets the + /// conversation ask once. + /// + /// The second host is the half that matters. A key every park shares would + /// consolidate correctly and also fold two unrelated turns into one card — + /// an operator approving a batch they never saw raised. Grouping is only + /// safe because the key separates turns, so both directions are asserted. + /// + /// What is *not* changed here, and is asserted to make the point: the parks + /// stay two records with two ids. Chat groups them for display; each is + /// still decided on its own and still mints its own host-scoped grant + /// (#739). + #[tokio::test] + async fn every_approval_one_turn_parks_carries_that_turns_batch_key() { + let home_dir = tmp_home(); + let home = home_dir.path().to_path_buf(); + let sender = Arc::new(RecordingMailSender::new()); + let rt = RuntimeBuilder::new(home.clone(), manifest("supervised")) + .with_mail(CompanyMail { + sender: sender.clone(), + smtp: test_smtp("ceo@acme.test"), + }) + .build() + .await + .unwrap(); + + // One turn, two gated calls — the shape the issue reports, where a + // research turn reaches several outside hosts before it yields. + let turn = CycleHostImpl::new( + rt.id().clone(), + "cyc-research".into(), + &rt, + None, + ApprovalConversation { + thread: Some("desk-marketing".to_string()), + parent: None, + }, + ); + turn.park_effect(harness_effect( + "seo", + "web_fetch", + serde_json::json!({ "url": "https://espn.com/nba" }), + )) + .await + .unwrap(); + turn.park_effect(harness_effect( + "seo", + "web_fetch", + serde_json::json!({ "url": "https://bbc.com/sport" }), + )) + .await + .unwrap(); + + // A later, unrelated turn in the same conversation. + let other = CycleHostImpl::new( + rt.id().clone(), + "cyc-later".into(), + &rt, + None, + ApprovalConversation { + thread: Some("desk-marketing".to_string()), + parent: None, + }, + ); + other + .park_effect(harness_effect( + "seo", + "web_fetch", + serde_json::json!({ "url": "https://theguardian.com/uk" }), + )) + .await + .unwrap(); + + let pending = rt.pending_approvals(); + assert_eq!(pending.len(), 3, "one record per gated call, still"); + let batches: Vec> = pending.iter().map(|p| p.batch.clone()).collect(); + assert!( + batches.iter().all(Option::is_some), + "a park raised by a turn must name it: {batches:?}" + ); + + let by_url = |url: &str| { + pending + .iter() + .find(|p| p.payload.as_ref().is_some_and(|v| v["url"] == url)) + .unwrap_or_else(|| panic!("no parked approval for {url}")) + }; + let espn = by_url("https://espn.com/nba"); + let bbc = by_url("https://bbc.com/sport"); + let guardian = by_url("https://theguardian.com/uk"); + + assert_eq!( + espn.batch, bbc.batch, + "two calls one turn parked belong to one batch, so the operator is asked once" + ); + assert_ne!( + espn.batch, guardian.batch, + "a different turn is a different question — consolidating across turns would ask \ + an operator to approve work they never saw raised" + ); + // Still three decisions underneath. The batch is presentation; the park + // is the unit of truth, and each keeps its own id to be resolved by. + assert_eq!( + std::collections::HashSet::from([&espn.id, &bbc.id, &guardian.id]).len(), + 3, + "grouping must not merge the records it groups" + ); + } + /// The correlation key itself (#333): which card a cycle is working, read /// off its own trigger events. #[test] diff --git a/src/runtime/journal.rs b/src/runtime/journal.rs index 549ccc56a..d40b40a1e 100644 --- a/src/runtime/journal.rs +++ b/src/runtime/journal.rs @@ -390,6 +390,25 @@ pub struct PendingApproval { /// whose triggers were ambiguous). Both are the same fact downstream: no /// channel owns this approval, so it is shown on the Approvals page only. pub thread: Option, + /// The turn that parked it (issue #469), carried out to the read side by + /// issue #842 so the console can ask about a turn's gated calls **once**. + /// + /// Not a new fact and deliberately not a new record: `ApprovalParked` + /// already journals the parking cycle, because #469 needed to know which + /// approvals one turn is blocked on in order to continue it exactly once. + /// #842 is the same grouping seen from the operator's side — a turn that + /// reached three sites parked three calls, and being asked three times is + /// the same fact told badly. Projecting the key it already had is the whole + /// of the mechanism; each park stays its own record, its own decision and + /// its own host-scoped grant. + /// + /// `None` for a pre-#469 journal line and for every park raised outside a + /// cycle (a workflow node, a scheduler tick): `park_and_journal` in + /// `workflows::delivery` passes no turn key, because a run holds no + /// continuation for one to belong to. Both read downstream as "belongs to + /// no batch", which renders exactly as it did before this field existed: + /// one card, decided on its own. + pub batch: Option, } /// What an approval *was*, retained for the whole life of the journal — after @@ -1690,6 +1709,13 @@ impl RuntimeJournal { at_millis: parked.at_millis, task: parked.task.clone(), thread: parked.thread.clone(), + // Issue #842: the turn key the entry already carries for #469's + // continuation counter, read out rather than recomputed. The + // two must name the same set — the batch the operator is asked + // about in one card is precisely the batch the runtime holds a + // single continuation for — and reading one field is how that + // stays true without a rule anyone has to remember. + batch: parked.cycle.clone(), }) .collect(); out.sort_by(|a, b| { diff --git a/src/runtime/types.rs b/src/runtime/types.rs index e696d6626..0f1593fc0 100644 --- a/src/runtime/types.rs +++ b/src/runtime/types.rs @@ -215,6 +215,36 @@ pub struct ApprovalSummary { /// before, and a console that reads no such field is unaffected. #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub contents_hidden: bool, + /// Which turn's gated calls this one belongs to (issue #842) — an opaque + /// grouping key shared by every approval a single agent turn parked. + /// + /// **Presentation, not a new unit of truth.** One research turn that + /// reaches three sites parks three approvals, and each stays exactly what + /// it was: its own record, its own decision, its own host-scoped grant on + /// approve (issue #739). This field only says that they were asked for + /// together, so the conversation can ask once — "three sites" with the + /// hosts listed — instead of interrupting three times. The Approvals page + /// deliberately keeps rendering one row per approval, matching how + /// `Standing permissions` lists one revocable row per grant. + /// + /// The value is the parking turn key issue #469 already journals, so the + /// batch a card consolidates is by construction the same batch the runtime + /// continues exactly once. Nothing else may be inferred from it: it is an + /// opaque id, not an ordering, a count, or an address. + /// + /// `None` — and omitted from the wire — for an approval raised outside a + /// cycle (a workflow node, a scheduler tick) and for every park journaled + /// before #469. A console groups those alone, which is the pre-#842 + /// rendering, so an old host and a new one both produce a card an operator + /// can decide. + /// + /// Survives role redaction on purpose: + /// [`approval_visibility`](crate::server::approval_visibility) withholds + /// *contents*, and which requests arrived together is not contents. A + /// Member sees a batch of three withheld cards rather than three unrelated + /// ones, which is less information than an admin gets and still the truth. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub batch: Option, } #[cfg(test)] diff --git a/src/server/approval_visibility.rs b/src/server/approval_visibility.rs index 94dd9bde5..79b97091f 100644 --- a/src/server/approval_visibility.rs +++ b/src/server/approval_visibility.rs @@ -160,6 +160,7 @@ mod tests { thread: None, broadly_grantable: false, contents_hidden: false, + batch: Some("turn-1".to_string()), } } @@ -189,6 +190,16 @@ mod tests { assert_eq!(out[0].kind, "email.send"); assert_eq!(out[0].agent.as_deref(), Some("ops")); assert_eq!(out[0].at_millis, 1_000); + // Issue #842: including which requests arrived together. Which turn + // asked is not *contents* — withholding it would split one batch into + // unrelated single cards for a member, so the two roles would see the + // conversation interrupted a different number of times for the same + // turn. Less detail than an admin gets; the same shape of request. + assert_eq!( + out[0].batch.as_deref(), + Some("turn-1"), + "role redaction withholds contents, not the grouping" + ); } /// Issue #618's stated trap: a platform bearer carries no `UserRole`, and