diff --git a/docs/ui/advisor-consult-log.md b/docs/ui/advisor-consult-log.md new file mode 100644 index 00000000..c5665af8 --- /dev/null +++ b/docs/ui/advisor-consult-log.md @@ -0,0 +1,103 @@ +# Advisor Consult Log + +Status: **implemented (session-only)** + +Supersedes [`advisor-interaction-map.md`](./advisor-interaction-map.md), which +was drawn for the preflight-era Advisor and plans a graph around prompt +injection. That injection step does not exist in the on-demand Advisor, so that +document is historical and must not be read as live guidance. + +## The problem + +The on-demand Advisor lets the primary model consult a separate read-only model +mid-turn via the `stave_consult_advisor` Local MCP tool. Before this surface, +none of it was reviewable: + +- `advisorExchangeByTask` holds **one** snapshot per task. A second consult in + the same turn overwrote the first; only a `settledConsults` counter survived. +- Provider events are rAF-batched, and rAF is paused while the window is hidden + or occluded, so one flush routinely carries several *complete* consults. Any + "diff the map after the flush" archive would have kept only the last one. +- The floating exchange card auto-hides after 6s settled / 20s attention. + +So the question text and the advice existed for a few seconds and were then +unrecoverable. + +## What the surface does and does not claim + +It shows, per consult: what was asked, what came back, the lifecycle, the +isolation and effort the runtime actually applied, what the consult cost, and +which tool calls of the same turn started after it settled. + +It does **not** infer impact. Advice returns as an MCP tool result and the +primary is free to ignore it; there is no injection step and no `applied` phase. +Two lines of copy carry that limit and are asserted verbatim in +`tests/advisor-consult-log-render.test.tsx`: + +- *"Tool calls in this turn that started after the consult settled, in order. + Sequence only — Stave cannot tell whether the advice caused them."* +- *"Reported by the runtime for the advisor call only. Stave reports usage per + message, not per turn, so this is not a share of the turn's total."* + +The second exists because `ChatMessage.usage` is per message and carries no +`turnId`, and `buildUsageMetric` sums over the *paged* message list — so any +"% of the turn" denominator the renderer could build would silently drift. + +Effectiveness is therefore a **user-set verdict** (Helpful / Not helpful / +Ignored), aggregated per advisor model rather than per task so it survives ring +eviction. The aggregate line has no denominator for the same reason. + +## Shape + +| Piece | Where | +| --- | --- | +| Pure state, ring, verdict tally | `src/lib/providers/advisor-consult-log.ts` | +| Archive hook inside the fold loop | `applyAdvisorActivityEvents`, `src/lib/providers/advisor-activity.ts` | +| Store slices and four actions | `app-store.types.ts`, `app-store-provider-interaction-actions.ts` | +| Presentation projection | `src/components/session/advisor-consult-log.utils.ts` | +| Dialog + store-connected host | `src/components/session/AdvisorConsultLogDialog.tsx` | + +No new provider event, IPC channel, schema, or SQLite table: `advisor_activity` +already carries every field. + +`foldAdvisorEvent` returns its input snapshot **by reference** to mean "nothing +changed", which is what lets the archive run once per folded *step* instead of +once per flush. That is the whole fix for the batching loss above. + +The host is mounted in `ChatArea`, not inside either trigger, because both +triggers are short-lived: the exchange card clears on its linger timer and the +turn activity shelf is keyed `${taskId}:${activeTurnId}`. A dialog owned by +either would vanish mid-read. Store-held open state is also what lets the shelf +trigger exist without touching `ChatInput.tsx`. + +The shelf's advisor row uses `detailSurface: "advisor-consult-log"` rather than +borrowing `toolUseId`: that field asserts "the transcript can reveal this call", +and a consult has nothing to reveal. `data-turn-activity-revealable` stays +tool-only; the advisor row carries `data-turn-activity-opens` instead. + +## Bounds + +- `ADVISOR_CONSULT_LOG_LIMIT = 24` per task. Must stay **≥ + `MAX_ADVISOR_CONSULT_LIMIT` (20)**, or a turn that spends its whole consult + budget evicts its own earliest consults — the exact failure being fixed. +- `ADVISOR_CONSULT_LOG_TASK_LIMIT = 8`, matching + `RETAINED_TURN_ACTIVITY_LIMIT`. +- Worst case ≈ 4 MB, in memory, shed with the task. + +## Known limits + +- **"What ran after" is a lossy sample.** Work items exist only for the live + turn and the last finished turn per task, and are capped + (`PROVIDER_TURN_WORK_ITEM_LIMIT = 12`, + `PROVIDER_TURN_GENERAL_TOOL_LIMIT = 3`). Older consults render the empty + state, which says so rather than implying nothing ran. +- **Verdicts outlive their entries** by design; the "this session" wording + carries that. +- **`consultIndex` is not unique** — a recoverable provider retry can repeat + it. Entries key on `exchangeId` (falling back to `startedAt`), so no code may + assume index uniqueness. +- **A reload erases the log.** A durable SQLite-backed log is the follow-up; + `selectAdvisorConsultLog` plus a hydrate action is the only swap needed, with + no component changes. +- `ADVISOR_STAGE_LIMIT = 12` still truncates the lifecycle inside an archived + entry. diff --git a/docs/ui/advisor-interaction-map.md b/docs/ui/advisor-interaction-map.md index 2c28d9e8..1290c6f0 100644 --- a/docs/ui/advisor-interaction-map.md +++ b/docs/ui/advisor-interaction-map.md @@ -1,14 +1,19 @@ # Advisor Interaction Map -Status: **proposed / not implemented — partially superseded** +Status: **historical — superseded, do not implement** > ⚠️ This plan was drawn for the preflight-era Advisor (one blocking call -> before the turn, advice injected into the primary prompt). The Advisor has -> since become **on-demand**: the primary consults it mid-turn via the -> `stave_consult_advisor` Local MCP tool, advice returns as the tool result, -> and the `applied`/`primary_started` phases no longer exist. The injection -> and handoff panels below would need to be redesigned per-consult before this -> is implemented. +> before the turn, advice injected into the primary prompt). The Advisor is now +> **on-demand**: the primary consults it mid-turn via the +> `stave_consult_advisor` Local MCP tool, advice returns as the tool result, and +> the `applied` / `primary_started` phases and the injection step do not exist. +> The graph below is therefore drawn around a relationship the runtime cannot +> report. +> +> The shipped surface is [`advisor-consult-log.md`](./advisor-consult-log.md), +> which shows sequence and cost and asks the user for the effectiveness call +> rather than inferring causality. This file is kept only as a record of the +> preflight-era design. This document is the durable visual and implementation plan for Advisor UX prototype 3. The existing Handoff Monitor remains the ambient surface, and its diff --git a/src/components/session/AdvisorCheckIcon.tsx b/src/components/session/AdvisorCheckIcon.tsx new file mode 100644 index 00000000..1f1e4d77 --- /dev/null +++ b/src/components/session/AdvisorCheckIcon.tsx @@ -0,0 +1,24 @@ +import { Check, CircleDashed, Minus, X } from "lucide-react"; + +import type { AdvisorCheck } from "@/components/session/advisor-exchange.utils"; + +/** + * Status mark for one `buildAdvisorChecks` row. + * + * Shared by the floating exchange card and the consult log dialog so the same + * check never renders as a tick in one surface and a dash in the other. + */ +export function AdvisorCheckIcon(props: { status: AdvisorCheck["status"] }) { + if (props.status === "pass") { + return ; + } + if (props.status === "fail") { + return ; + } + if (props.status === "pending") { + return ( + + ); + } + return ; +} diff --git a/src/components/session/AdvisorConsultLogDialog.tsx b/src/components/session/AdvisorConsultLogDialog.tsx new file mode 100644 index 00000000..5f9c8ae0 --- /dev/null +++ b/src/components/session/AdvisorConsultLogDialog.tsx @@ -0,0 +1,601 @@ +import { useMemo } from "react"; +import { useShallow } from "zustand/react/shallow"; + +import { ChoiceButtons } from "@/components/layout/settings-dialog.shared"; +import { AdvisorCheckIcon } from "@/components/session/AdvisorCheckIcon"; +import { + ADVISOR_VERDICT_OPTIONS, + describeAdvisorConsultLogStatus, + describeAdvisorVerdict, + describeAdvisorVerdictTally, + formatAdvisorSpend, + resolveAdvisorConsultWorkItems, + resolveAdvisorConsultLogStatus, + resolveAdvisorPostConsultWorkItems, + summarizeAdvisorTurnSpend, + type AdvisorConsultLogStatus, +} from "@/components/session/advisor-consult-log.utils"; +import { + buildAdvisorChecks, + describeAdvisorEffort, + describeAdvisorIsolation, + describeAdvisorParticipant, + describeAdvisorPhase, + formatAdvisorDuration, +} from "@/components/session/advisor-exchange.utils"; +import { useScopedTaskId } from "@/components/session/task-scope-context"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog"; +import type { AdvisorExchangeSnapshot } from "@/lib/providers/advisor-activity"; +import { + advisorVerdictKey, + selectAdvisorConsultLog, + type AdvisorConsultLogEntry, + type AdvisorConsultVerdict, + type AdvisorVerdictTallyByModel, +} from "@/lib/providers/advisor-consult-log"; +import type { ProviderTurnWorkItem } from "@/lib/providers/turn-status"; +import { cn } from "@/lib/utils"; +import { useAppStore } from "@/store/app.store"; + +/** + * Copy that the tests assert verbatim, because each line is the only thing + * stopping the surface from being read as causal evidence it cannot provide. + */ +const ADVISOR_SPEND_FOOTNOTE = + "Reported by the runtime for the advisor call only. Stave reports usage per message, not per turn, so this is not a share of the turn's total."; +const ADVISOR_POST_CONSULT_SUBLINE = + "Tool calls in this turn that started after the consult settled, in order. Sequence only — Stave cannot tell whether the advice caused them."; +const ADVISOR_POST_CONSULT_EMPTY = + "No tool calls from this turn are still in memory, so Stave cannot say what ran after."; +const ADVISOR_VERDICT_SUBLINE = + "Your own judgement, recorded per consult. Stave does not infer this."; +const ADVISOR_QUESTION_EMPTY = "The runtime did not report the question."; +const ADVISOR_UNRESOLVED_DETAIL = + "Its turn ended before the runtime reported an outcome, so this consult has no result and no cost to show."; +const ADVISOR_LOG_TITLE = "Advisor consults"; +const ADVISOR_LOG_DESCRIPTION = + "Every consult this session, with what was asked, what came back, and what it cost."; +const HEADER_CLASS = "border-b border-border/60 px-4 py-3"; + +const STATUS_CHIP_CLASS: Record = { + armed: "border-border/60 bg-muted/40 text-muted-foreground", + pending: "border-info/40 bg-info/10 text-info", + completed: "border-success/40 bg-success/10 text-success", + failed: "border-warning/40 bg-warning/10 text-warning", + timeout: "border-warning/40 bg-warning/10 text-warning", + aborted: "border-warning/40 bg-warning/10 text-warning", + skipped: "border-warning/40 bg-warning/10 text-warning", + unresolved: "border-border/60 bg-muted/40 text-muted-foreground", +}; + +const VERDICT_DOT_CLASS: Record = { + helpful: "bg-success", + not_helpful: "bg-warning", + ignored: "bg-muted-foreground/60", +}; + +function SectionLabel(props: { children: React.ReactNode }) { + return ( +

+ {props.children} +

+ ); +} + +function ProseBlock(props: { children: React.ReactNode; muted?: boolean }) { + return ( +

+ {props.children} +

+ ); +} + +function consultLabel(snapshot: AdvisorExchangeSnapshot) { + if (snapshot.consultIndex === undefined) { + return "Consult"; + } + return snapshot.consultLimit === undefined + ? `Consult ${snapshot.consultIndex}` + : `Consult ${snapshot.consultIndex}/${snapshot.consultLimit}`; +} + +function ConsultRow(props: { + entry: AdvisorConsultLogEntry; + selected: boolean; + status: AdvisorConsultLogStatus; + isCurrentTurn: boolean; + onSelect: () => void; +}) { + const { snapshot } = props.entry; + return ( + + ); +} + +function ConsultDetail(props: { + entry: AdvisorConsultLogEntry; + entries: readonly AdvisorConsultLogEntry[]; + status: AdvisorConsultLogStatus; + workItems: readonly ProviderTurnWorkItem[]; + tallyByModel: AdvisorVerdictTallyByModel; + onSetVerdict: (verdict: AdvisorConsultVerdict) => void; +}) { + const { snapshot } = props.entry; + const checks = useMemo(() => buildAdvisorChecks(snapshot), [snapshot]); + const settled = snapshot.outcome !== "pending" && snapshot.outcome !== "armed"; + const postConsult = useMemo( + () => + resolveAdvisorPostConsultWorkItems({ + entry: props.entry, + workItems: props.workItems, + }), + [props.entry, props.workItems], + ); + const turnSpend = useMemo( + () => + summarizeAdvisorTurnSpend({ + entries: props.entries, + turnId: snapshot.turnId, + }), + [props.entries, snapshot.turnId], + ); + const tallyKey = advisorVerdictKey({ + providerId: snapshot.advisorProviderId, + model: snapshot.advisorModel, + }); + const tally = tallyKey ? props.tallyByModel[tallyKey] : undefined; + + return ( +
+

+ {describeAdvisorConsultLogStatus(props.status)} +

+ {props.status === "unresolved" ? ( + // The checks below read off `outcome`, which is still `pending`, so + // without this they would say the advisor is being waited on — for a + // turn that ended long ago. +

+ {ADVISOR_UNRESOLVED_DETAIL} +

+ ) : null} + +
+ Did the advisor system work? +
    + {checks.map((check) => ( +
  • + +
    +

    + {check.label} +

    +

    + {check.detail} +

    +
    +
  • + ))} +
+
+ +
+ Question asked + {snapshot.question ? ( + {snapshot.question} + ) : ( + {ADVISOR_QUESTION_EMPTY} + )} +
+ + {snapshot.advice ? ( +
+ Advice returned + {snapshot.advice} +
+ ) : null} + +
+ Lifecycle +
    + {snapshot.stages.map((stage, index) => ( +
  1. + + +{formatAdvisorDuration(stage.at - snapshot.startedAt)} + + + {describeAdvisorPhase(stage.phase)} + {stage.detail ? ` — ${stage.detail}` : ""} + +
  2. + ))} +
+
+ +
+ Setup +
+
+
+ Isolation +
+
+ {describeAdvisorIsolation(snapshot.isolation)} +
+
+
+
+ Effort +
+
+ {describeAdvisorEffort(snapshot.advisorEffort)} +
+
+
+
+ Deadline +
+
+ {snapshot.timeoutMs === undefined + ? "Not reported" + : formatAdvisorDuration(snapshot.timeoutMs)} +
+
+
+
+ Duration +
+
+ {snapshot.durationMs === undefined + ? "Not reported" + : formatAdvisorDuration(snapshot.durationMs)} +
+
+
+
+ +
+ Advisor spend +
+
+
+ This consult +
+
+ {formatAdvisorSpend({ + inputTokens: snapshot.inputTokens ?? 0, + outputTokens: snapshot.outputTokens ?? 0, + totalCostUsd: snapshot.totalCostUsd ?? null, + })} +
+
+
+
+ This turn's consults +
+
+ {formatAdvisorSpend(turnSpend)} +
+
+
+

+ {ADVISOR_SPEND_FOOTNOTE} +

+
+ + {settled ? ( +
+ What ran after this consult +

+ {ADVISOR_POST_CONSULT_SUBLINE} +

+ {postConsult.length === 0 ? ( + {ADVISOR_POST_CONSULT_EMPTY} + ) : ( +
    + {postConsult.map((item) => ( +
  1. + + + + {formatAdvisorDuration( + item.startedAt - + (snapshot.outcomeAt ?? snapshot.startedAt), + )} + + + {item.title} + +
  2. + ))} +
+ )} +
+ ) : null} + + {settled ? ( +
+ Your call +

+ {ADVISOR_VERDICT_SUBLINE} +

+
+ {/* The unrated state is a value outside the option set rather than + a fourth "Not rated" button: the control is set-only, so an + option the user can never legitimately choose would be dead. */} + + aria-label="Was this consult helpful?" + value={props.entry.verdict ?? ""} + onChange={(value) => { + if (value) { + props.onSetVerdict(value); + } + }} + options={ADVISOR_VERDICT_OPTIONS} + /> +
+

+ {describeAdvisorVerdictTally(tally)} +

+
+ ) : null} +
+ ); +} + +/** + * The session's consult log. Pure and prop-driven so the terminal states are + * testable without a browser, and so the Lens harness can render it from real + * snapshots. + */ +export function AdvisorConsultLogDialog(props: { + open: boolean; + onOpenChange: (open: boolean) => void; + entries: readonly AdvisorConsultLogEntry[]; + selectedKey: string | null; + onSelectEntry: (entryKey: string) => void; + activeTurnId: string | null; + workItems: readonly ProviderTurnWorkItem[]; + tallyByModel: AdvisorVerdictTallyByModel; + onSetVerdict: (args: { + entryKey: string; + verdict: AdvisorConsultVerdict; + }) => void; +}) { + const selected = + props.entries.find((entry) => entry.key === props.selectedKey) ?? + props.entries[0] ?? + null; + + const body = ( + <> + {props.entries.length === 0 ? ( +

+ No consults have been recorded for this task yet. +

+ ) : ( +
+
+ {props.entries.map((entry) => ( + props.onSelectEntry(entry.key)} + /> + ))} +
+ {selected ? ( + + props.onSetVerdict({ entryKey: selected.key, verdict }) + } + /> + ) : null} +
+ )} + + ); + + // Static-render escape hatch, mirroring `WorkspaceSettingsDialog`: the dialog + // primitive needs a portal target and its own context, neither of which + // exists under `renderToStaticMarkup`, so the terminal states would be + // untestable without a browser. + if (props.open && (typeof document === "undefined" || !document.body)) { + return ( +
+
+

{ADVISOR_LOG_TITLE}

+

+ {ADVISOR_LOG_DESCRIPTION} +

+
+ {body} +
+ ); + } + + return ( + + + + {ADVISOR_LOG_TITLE} + {ADVISOR_LOG_DESCRIPTION} + + {body} + + + ); +} + +/** + * Store-connected host. + * + * Mounted once per chat area and rendered `null` unless the open view names + * *this* task, which keeps exactly one dialog open across split panes. It lives + * here rather than inside either trigger because both triggers are short-lived + * — the floating card clears on its linger timer and the turn activity shelf is + * keyed per turn — so a dialog owned by either would vanish mid-read. + */ +export function AdvisorConsultLogHost() { + const taskId = useScopedTaskId(); + const [view, logByTask, tallyByModel, activeTurnId, activity, retained] = + useAppStore( + useShallow((state) => [ + state.advisorConsultLogView, + state.advisorConsultLogByTask, + state.advisorVerdictTallyByModel, + state.activeTurnIdsByTask[taskId] ?? null, + state.providerTurnActivityByTask[taskId] ?? null, + state.retainedTurnActivityByTask[taskId] ?? null, + ]), + ); + const selectEntry = useAppStore((state) => state.selectAdvisorConsultLogEntry); + const closeLog = useAppStore((state) => state.closeAdvisorConsultLog); + const setVerdict = useAppStore((state) => state.setAdvisorConsultVerdict); + + const entries = selectAdvisorConsultLog(logByTask, taskId); + const selectedEntry = + entries.find((entry) => entry.key === view?.entryKey) ?? + entries[0] ?? + null; + // Derived outside the selector on purpose: a selector that flattened the work + // items would return a fresh array on every unrelated store write. The helper + // also refuses to lend a newer turn's work items to an older consult. + const workItems = useMemo(() => { + if (view?.taskId !== taskId) { + return []; + } + return resolveAdvisorConsultWorkItems({ + entry: selectedEntry, + activity, + retained, + }); + }, [activity, retained, selectedEntry, taskId, view?.taskId]); + + if (view?.taskId !== taskId) { + return null; + } + + return ( + { + if (!open) { + closeLog(); + } + }} + entries={entries} + selectedKey={view.entryKey} + onSelectEntry={(entryKey) => selectEntry({ entryKey })} + activeTurnId={activeTurnId} + workItems={workItems} + tallyByModel={tallyByModel} + onSetVerdict={({ entryKey, verdict }) => + setVerdict({ taskId, entryKey, verdict }) + } + /> + ); +} diff --git a/src/components/session/AdvisorExchangeMonitor.tsx b/src/components/session/AdvisorExchangeMonitor.tsx index 12af3cf4..c68c3af3 100644 --- a/src/components/session/AdvisorExchangeMonitor.tsx +++ b/src/components/session/AdvisorExchangeMonitor.tsx @@ -1,15 +1,12 @@ import { memo, useEffect, useMemo, useRef, useState } from "react"; import { ArrowLeftRight, - Check, ChevronDown, ChevronUp, - CircleDashed, + History, LoaderCircle, - Minus, SkipForward, TriangleAlert, - X, } from "lucide-react"; import { buildAdvisorChecks, @@ -24,13 +21,17 @@ import { resolveAdvisorExchangeVisibility, resolveAdvisorLaneSegments, resolveAdvisorRemainingMs, - type AdvisorCheck, type AdvisorExchangeTone, } from "@/components/session/advisor-exchange.utils"; +import { AdvisorCheckIcon } from "@/components/session/AdvisorCheckIcon"; import { SESSION_INPUT_FLOATING_WRAPPER_CLASS_NAME } from "@/components/session/plan-viewer.utils"; import { useScopedTaskId } from "@/components/session/task-scope-context"; import { Button } from "@/components/ui/button"; import type { AdvisorExchangeSnapshot } from "@/lib/providers/advisor-activity"; +import { + advisorConsultLogEntryKey, + selectAdvisorConsultLog, +} from "@/lib/providers/advisor-consult-log"; import { getProviderWaveToneClass } from "@/lib/providers/model-catalog"; import type { ProviderId } from "@/lib/providers/provider.types"; import { UI_ELEVATION_CLASS } from "@/lib/ui-layers"; @@ -119,21 +120,6 @@ function ParticipantChip(props: { ); } -function CheckIcon(props: { status: AdvisorCheck["status"] }) { - if (props.status === "pass") { - return ; - } - if (props.status === "fail") { - return ; - } - if (props.status === "pending") { - return ( - - ); - } - return ; -} - /** Exported so the Lens harness can render the real card from real snapshots. */ export function AdvisorExchangeCard(props: { snapshot: AdvisorExchangeSnapshot; @@ -143,6 +129,12 @@ export function AdvisorExchangeCard(props: { onSkip: () => void; onDismiss: () => void; canSkip: boolean; + /** + * Opens the session consult log. The card shows one consult and clears on a + * linger timer, so this is the only way back to the ones it replaced. + */ + onOpenLog?: () => void; + consultLogCount?: number; }) { const { snapshot } = props; const tone = resolveAdvisorExchangeTone(snapshot); @@ -303,7 +295,7 @@ export function AdvisorExchangeCard(props: {
    {checks.map((check) => (
  • - +

    {!props.canSkip && snapshot.outcome !== "pending" ? ( -

    +
    + {props.onOpenLog && (props.consultLogCount ?? 0) > 0 ? ( + + ) : null} @@ -421,6 +425,14 @@ export function AdvisorExchangeMonitor() { const dismissAdvisorExchange = useAppStore( (state) => state.dismissAdvisorExchange, ); + const openAdvisorConsultLog = useAppStore( + (state) => state.openAdvisorConsultLog, + ); + // A primitive, not the entry array: the card re-renders on a 200ms clock and + // must not also re-render whenever an unrelated consult is archived. + const consultLogCount = useAppStore( + (state) => selectAdvisorConsultLog(state.advisorConsultLogByTask, taskId).length, + ); const [expanded, setExpanded] = useState(false); const [hovered, setHovered] = useState(false); @@ -482,6 +494,15 @@ export function AdvisorExchangeMonitor() { onDismiss={() => { dismissAdvisorExchange({ taskId }); }} + consultLogCount={consultLogCount} + onOpenLog={() => { + // Opens focused on the consult the card is showing, so "view all" + // never loses the one the user was already reading. + openAdvisorConsultLog({ + taskId, + entryKey: advisorConsultLogEntryKey(snapshot), + }); + }} />
    ); diff --git a/src/components/session/ChatArea.tsx b/src/components/session/ChatArea.tsx index 7ccb43db..6c7eee5a 100644 --- a/src/components/session/ChatArea.tsx +++ b/src/components/session/ChatArea.tsx @@ -7,6 +7,7 @@ import { type LucideIcon, } from "lucide-react"; import { memo, useCallback, useEffect, useRef, type MouseEvent } from "react"; +import { AdvisorConsultLogHost } from "@/components/session/AdvisorConsultLogDialog"; import { AdvisorExchangeMonitor } from "@/components/session/AdvisorExchangeMonitor"; import { ChatInput } from "@/components/session/ChatInput"; import { ChatPanel } from "@/components/session/ChatPanel"; @@ -363,6 +364,11 @@ function ChatAreaImpl(props: ChatAreaProps) {
    + {/* Outside the pointer-events-none overlay, and outside both of its + triggers: the exchange card clears on a linger timer and the + activity shelf is keyed per turn, so a dialog owned by either + would disappear while it was being read. */} +
    diff --git a/src/components/session/TurnActivity.tsx b/src/components/session/TurnActivity.tsx index 5818ab41..c1168483 100644 --- a/src/components/session/TurnActivity.tsx +++ b/src/components/session/TurnActivity.tsx @@ -44,9 +44,11 @@ import { type WorkGraphControlRequest, } from "@/components/session/WorkGraphTree"; import type { AdvisorExchangeSnapshot } from "@/lib/providers/advisor-activity"; +import { selectAdvisorConsultLog } from "@/lib/providers/advisor-consult-log"; import { buildTurnActivityItems, countTurnActivityItems, + resolveTurnActivityRowActivation, describeRetainedTurnHeadline, formatTurnActivityCountsLabel, promoteFirstPendingTodoForActiveTurn, @@ -176,6 +178,7 @@ export function TurnActivity(props: { host?: TurnActivityPlacement }) { activity, retainedActivity, advisorExchange, + hasAdvisorConsultLog, expandedByDefault, verification, rateLimits, @@ -196,6 +199,9 @@ export function TurnActivity(props: { host?: TurnActivityPlacement }) { state.providerTurnActivityByTask[taskId] ?? null, state.retainedTurnActivityByTask[taskId] ?? null, state.advisorExchangeByTask[taskId] ?? null, + // A boolean, not the entries: the shelf re-renders on a per-second clock + // and must not also re-render whenever a consult is archived. + selectAdvisorConsultLog(state.advisorConsultLogByTask, taskId).length > 0, state.settings.turnActivityExpandedByDefault, state.turnVerificationByWorkspace[state.activeWorkspaceId] ?? null, state.rateLimitsSnapshot, @@ -218,6 +224,12 @@ export function TurnActivity(props: { host?: TurnActivityPlacement }) { }, [focusTranscriptTool, taskId], ); + const openAdvisorConsultLog = useAppStore( + (state) => state.openAdvisorConsultLog, + ); + const handleOpenAdvisorLog = useCallback(() => { + openAdvisorConsultLog({ taskId }); + }, [openAdvisorConsultLog, taskId]); const handlePlacementChange = useCallback( (next: TurnActivityPlacement) => { updateSettings({ patch: { turnActivityPlacement: next } }); @@ -512,6 +524,8 @@ export function TurnActivity(props: { host?: TurnActivityPlacement }) { hasPendingInteractionCard, executionSummary, onSelectTool: handleSelectTool, + hasAdvisorConsultLog, + onOpenAdvisorLog: handleOpenAdvisorLog, taskId, workspaceId: activeWorkspaceId, projectPath, @@ -526,8 +540,10 @@ export function TurnActivity(props: { host?: TurnActivityPlacement }) { currentActivity, expandedByDefault, executionSummary, + handleOpenAdvisorLog, handleSelectTool, handleWorkGraphControl, + hasAdvisorConsultLog, hasPendingInteractionCard, isPlanPreparing, projectPath, @@ -791,6 +807,14 @@ interface TurnActivitySurfaceProps { * stay inert, so this never turns a todo or a status row into a dead button. */ onSelectTool?: (toolUseId: string) => void; + /** + * The task has archived consults, so the advisor row opens the consult log. + * Gated on the log rather than on this turn, so a turn that armed the Advisor + * without consulting it still reaches earlier consults. + */ + hasAdvisorConsultLog?: boolean; + /** Opens the session consult log from the advisor row. */ + onOpenAdvisorLog?: () => void; /** Identity of the task this shelf belongs to, used by the child-task rows. */ taskId?: string; workspaceId?: string | null; @@ -894,6 +918,7 @@ export const TurnActivitySurface = memo(function TurnActivitySurface( workItems: props.workItems, turnStartedAt: activityStartedAt, advisor: props.advisorExchange ?? null, + hasAdvisorConsultLog: props.hasAdvisorConsultLog ?? false, hasPendingInteractionCard: props.hasPendingInteractionCard, }), [ @@ -905,6 +930,7 @@ export const TurnActivitySurface = memo(function TurnActivitySurface( hasActivity, isStalled, props.advisorExchange, + props.hasAdvisorConsultLog, props.hasPendingInteractionCard, props.isPlanPreparing, props.todos, @@ -1140,6 +1166,7 @@ export const TurnActivitySurface = memo(function TurnActivitySurface( key={item.id} item={item} onSelectTool={props.onSelectTool} + onOpenAdvisorLog={props.onOpenAdvisorLog} showStartOffset={variant === "panel"} /> ))} @@ -1234,10 +1261,12 @@ function TurnActivityPlacementControls(props: { const TurnActivityRow = memo(function TurnActivityRow({ item, onSelectTool, + onOpenAdvisorLog, showStartOffset, }: { item: TurnActivityItem; onSelectTool?: (toolUseId: string) => void; + onOpenAdvisorLog?: () => void; /** * Roomy placements also print where in the turn the row started. The docked * shelf is one composer-width line and cannot spare the column. @@ -1247,8 +1276,13 @@ const TurnActivityRow = memo(function TurnActivityRow({ const detail = item.detail && item.detail !== item.title ? item.detail : undefined; const isCompleted = item.status === "completed"; - const toolUseId = item.toolUseId; - const canReveal = Boolean(toolUseId && onSelectTool); + const activation = resolveTurnActivityRowActivation(item); + const handler = + activation?.kind === "tool" && onSelectTool + ? { onClick: () => onSelectTool(activation.toolUseId), reveal: true } + : activation?.kind === "advisor-log" && onOpenAdvisorLog + ? { onClick: onOpenAdvisorLog, reveal: false } + : null; const baseTitle = detail ? `${item.title} · ${detail}` : item.title; const startOffsetLabel = showStartOffset && item.startOffsetSeconds != null @@ -1305,7 +1339,7 @@ const TurnActivityRow = memo(function TurnActivityRow({ "motion-safe:animate-in motion-safe:fade-in motion-safe:duration-200", ); - if (!canReveal || !toolUseId || !onSelectTool) { + if (!handler) { return (
    onSelectTool(toolUseId)} + title={ + handler.reveal + ? `${baseTitle} — show in conversation` + : `${baseTitle} — view all consults` + } + onClick={handler.onClick} > {body} diff --git a/src/components/session/advisor-consult-log.utils.ts b/src/components/session/advisor-consult-log.utils.ts new file mode 100644 index 00000000..bcbfc239 --- /dev/null +++ b/src/components/session/advisor-consult-log.utils.ts @@ -0,0 +1,213 @@ +import type { AdvisorExchangeOutcome } from "@/lib/providers/advisor-activity"; +import type { + AdvisorConsultLogEntry, + AdvisorConsultVerdict, + AdvisorVerdictTally, +} from "@/lib/providers/advisor-consult-log"; +import type { + ProviderTurnActivitySnapshot, + ProviderTurnWorkItem, + RetainedTurnActivity, +} from "@/lib/providers/turn-status"; + +/** + * What an archived consult's row should say it ended as. + * + * `pending` is only honest while the turn it belongs to is still running. + * Once that turn is gone a still-pending consult did not settle — the turn + * ended, was aborted, or the runtime never reported an outcome — and saying + * "Running" about it would be a lie the list never corrects. + */ +export type AdvisorConsultLogStatus = AdvisorExchangeOutcome | "unresolved"; + +export function resolveAdvisorConsultLogStatus(args: { + entry: AdvisorConsultLogEntry; + activeTurnId: string | null; +}): AdvisorConsultLogStatus { + const { outcome, turnId } = args.entry.snapshot; + if (outcome !== "pending") { + return outcome; + } + return turnId === args.activeTurnId ? "pending" : "unresolved"; +} + +export function describeAdvisorConsultLogStatus( + status: AdvisorConsultLogStatus, +): string { + switch (status) { + case "armed": + return "Armed"; + case "pending": + return "Running"; + case "completed": + return "Completed"; + case "failed": + return "Failed"; + case "timeout": + return "Timed out"; + case "aborted": + return "Aborted"; + case "skipped": + return "Skipped"; + case "unresolved": + return "Unresolved"; + } +} + +/** + * How many post-consult tool calls the detail pane lists. The section answers + * "what happened next", not "everything the turn did". + */ +export const ADVISOR_POST_CONSULT_WORK_ITEM_LIMIT = 6; + +/** + * Select the in-memory work snapshot that belongs to the consult being read. + * + * The store keeps only the live turn and the last finished turn. A consult from + * an older turn must therefore receive no work items, rather than borrowing a + * newer turn's items merely because their timestamps happen to be later. + */ +export function resolveAdvisorConsultWorkItems(args: { + entry: AdvisorConsultLogEntry | null; + activity: ProviderTurnActivitySnapshot | null; + retained: RetainedTurnActivity | null; +}): ProviderTurnWorkItem[] { + const turnId = args.entry?.snapshot.turnId; + if (!turnId) { + return []; + } + const snapshot = + args.activity?.turnId === turnId + ? args.activity + : args.retained?.snapshot.turnId === turnId + ? args.retained.snapshot + : null; + if (!snapshot) { + return []; + } + return snapshot.orderedWorkItemIds.flatMap((id) => { + const item = snapshot.workItemsById[id]; + return item ? [item] : []; + }); +} + +/** + * Tool calls of the same turn that started after this consult settled. + * + * Sequence only. Advice returns as an MCP tool result and the primary may + * ignore it, so nothing here implies the consult caused any of it — the copy + * in the detail pane says so explicitly. + * + * Empty until the consult settles (an unsettled consult has no "after"), and + * empty whenever the turn's work items are no longer in memory, which is the + * common case for older consults: work items exist only for the live turn and + * the last finished turn per task. + */ +export function resolveAdvisorPostConsultWorkItems(args: { + entry: AdvisorConsultLogEntry; + workItems: readonly ProviderTurnWorkItem[]; + limit?: number; +}): ProviderTurnWorkItem[] { + const { snapshot } = args.entry; + if (snapshot.outcome === "pending" || snapshot.outcome === "armed") { + return []; + } + const settledAt = snapshot.outcomeAt ?? snapshot.startedAt; + return args.workItems + .filter((item) => item.startedAt > settledAt) + .sort((left, right) => left.startedAt - right.startedAt) + .slice(0, args.limit ?? ADVISOR_POST_CONSULT_WORK_ITEM_LIMIT); +} + +export interface AdvisorTurnSpend { + consults: number; + inputTokens: number; + outputTokens: number; + /** `null` when no consult of the turn reported a cost at all. */ + totalCostUsd: number | null; +} + +/** + * What this turn's consults cost, summed over the archived entries. + * + * Absolute numbers only, never a share of the turn. `ChatMessage.usage` is + * per message and carries no turn id, so any denominator the renderer could + * build would drift as messages page in and out. + */ +export function summarizeAdvisorTurnSpend(args: { + entries: readonly AdvisorConsultLogEntry[]; + turnId: string; +}): AdvisorTurnSpend { + let consults = 0; + let inputTokens = 0; + let outputTokens = 0; + let totalCostUsd: number | null = null; + for (const entry of args.entries) { + if (entry.snapshot.turnId !== args.turnId) { + continue; + } + consults += 1; + inputTokens += entry.snapshot.inputTokens ?? 0; + outputTokens += entry.snapshot.outputTokens ?? 0; + if (entry.snapshot.totalCostUsd !== undefined) { + totalCostUsd = (totalCostUsd ?? 0) + entry.snapshot.totalCostUsd; + } + } + return { consults, inputTokens, outputTokens, totalCostUsd }; +} + +export function formatAdvisorSpend(args: { + inputTokens: number; + outputTokens: number; + totalCostUsd: number | null; +}): string { + const cost = + args.totalCostUsd === null ? "" : ` · $${args.totalCostUsd.toFixed(4)}`; + return `${args.inputTokens} in · ${args.outputTokens} out${cost}`; +} + +export const ADVISOR_VERDICT_OPTIONS: Array<{ + value: AdvisorConsultVerdict; + label: string; +}> = [ + { value: "helpful", label: "Helpful" }, + { value: "not_helpful", label: "Not helpful" }, + { value: "ignored", label: "Ignored" }, +]; + +export function describeAdvisorVerdict(verdict: AdvisorConsultVerdict): string { + switch (verdict) { + case "helpful": + return "Helpful"; + case "not_helpful": + return "Not helpful"; + case "ignored": + return "Ignored"; + } +} + +/** + * The running record for one advisor model. + * + * Deliberately has no denominator: the tally outlives ring eviction, so + * "4 of 9" would drift the moment an entry falls out of the log while its + * verdict stays counted. + */ +export function describeAdvisorVerdictTally( + tally: AdvisorVerdictTally | undefined, +): string { + if (!tally || tally.helpful + tally.notHelpful + tally.ignored === 0) { + return "No verdicts recorded for this advisor yet."; + } + const parts: string[] = []; + if (tally.helpful > 0) { + parts.push(`${tally.helpful} helpful`); + } + if (tally.notHelpful > 0) { + parts.push(`${tally.notHelpful} not helpful`); + } + if (tally.ignored > 0) { + parts.push(`${tally.ignored} ignored`); + } + return `${parts.join(" · ")} this session.`; +} diff --git a/src/components/session/turn-activity.utils.ts b/src/components/session/turn-activity.utils.ts index 4a7ab2ae..d80970ac 100644 --- a/src/components/session/turn-activity.utils.ts +++ b/src/components/session/turn-activity.utils.ts @@ -59,9 +59,39 @@ export interface TurnActivityItem { * transcript can actually reveal, so it doubles as "this row is clickable". */ toolUseId?: string; + /** + * A detail surface this row opens instead of revealing a transcript entry. + * + * Separate from `toolUseId` on purpose: that field asserts "the transcript + * can reveal this call", and borrowing it for the advisor row would put a + * row in the transcript-reveal path that has nothing to reveal. + */ + detailSurface?: "advisor-consult-log"; iconKey: TurnActivityIconKey; } +/** + * What clicking a row should do, if anything. + * + * One place decides, so the row component branches on the result rather than + * re-deriving "is this clickable" from two fields that mean different things. + */ +export type TurnActivityRowActivation = + | { kind: "tool"; toolUseId: string } + | { kind: "advisor-log" }; + +export function resolveTurnActivityRowActivation( + item: TurnActivityItem, +): TurnActivityRowActivation | null { + if (item.toolUseId) { + return { kind: "tool", toolUseId: item.toolUseId }; + } + if (item.detailSurface === "advisor-consult-log") { + return { kind: "advisor-log" }; + } + return null; +} + export interface TurnActivitySummary { label: string; activeCount: number; @@ -238,6 +268,14 @@ function describeAdvisorIdentity(snapshot: AdvisorExchangeSnapshot) { */ export function describeAdvisorTurnActivityItem( snapshot: AdvisorExchangeSnapshot, + options?: { + /** + * The task has archived consults to open. Gated on log emptiness rather + * than on this turn's outcome, so a merely-armed turn whose task consulted + * earlier still offers a way back to those consults. + */ + hasConsultLog?: boolean; + }, ): TurnActivityItem { const identity = describeAdvisorIdentity(snapshot); const limit = snapshot.consultLimit; @@ -255,6 +293,9 @@ export function describeAdvisorTurnActivityItem( ? `${identity} · available if the primary asks` : "Available if the primary asks", ...(limit ? { badge: `0/${limit}` } : {}), + ...(options?.hasConsultLog + ? { detailSurface: "advisor-consult-log" as const } + : {}), iconKey: "advisor", }; } @@ -268,6 +309,9 @@ export function describeAdvisorTurnActivityItem( title: `Advisor consult ${countLabel}`, detail: snapshot.question ?? identity ?? "Waiting on the advisor", ...(limit ? { badge: countLabel } : {}), + ...(options?.hasConsultLog + ? { detailSurface: "advisor-consult-log" as const } + : {}), iconKey: "advisor", }; } @@ -293,6 +337,9 @@ export function describeAdvisorTurnActivityItem( }`, detail: identity ? `${outcomeDetail} · ${identity}` : outcomeDetail, ...(limit ? { badge: `${snapshot.settledConsults}/${limit}` } : {}), + ...(options?.hasConsultLog + ? { detailSurface: "advisor-consult-log" as const } + : {}), iconKey: "advisor", }; } @@ -366,6 +413,8 @@ export function buildTurnActivityItems(args: { * countable from the same shelf. */ advisor?: AdvisorExchangeSnapshot | null; + /** The task has archived consults, so the advisor row can open the log. */ + hasAdvisorConsultLog?: boolean; /** * A chat-level approval/user-input card is already on screen, so the shelf * skips its own row rather than saying the same thing twice. @@ -420,7 +469,11 @@ export function buildTurnActivityItems(args: { // Fixed slot ahead of provider work: the row appears when the turn is // armed and only changes text afterwards, so it never reorders the list // mid-turn the way an insertion at consult time would. - items.push(describeAdvisorTurnActivityItem(args.advisor)); + items.push( + describeAdvisorTurnActivityItem(args.advisor, { + hasConsultLog: args.hasAdvisorConsultLog ?? false, + }), + ); } for (const item of args.workItems) { const elapsedSeconds = resolveWorkItemElapsedSeconds(item); diff --git a/src/lib/providers/advisor-activity.ts b/src/lib/providers/advisor-activity.ts index 232f002d..b77c1397 100644 --- a/src/lib/providers/advisor-activity.ts +++ b/src/lib/providers/advisor-activity.ts @@ -3,6 +3,10 @@ import type { NormalizedProviderEvent, ProviderId, } from "@/lib/providers/provider.types"; +import { + upsertAdvisorConsultLogEntry, + type AdvisorConsultLogByTask, +} from "@/lib/providers/advisor-consult-log"; /** * Advisor consult lifecycle phases. @@ -269,82 +273,148 @@ function isNewExchange( } /** - * Folds the turn's advisor events into the task's exchange snapshot. + * Applies one event to the running snapshot. * - * Returns the original map when nothing changed so the Zustand slice keeps a - * stable reference and subscribed components do not re-render. + * Returns `args.snapshot` **by reference** to mean "this event changed + * nothing", which is what lets the caller archive exactly the steps that did + * change instead of diffing the map once per flush. + */ +export function foldAdvisorEvent(args: { + snapshot: AdvisorExchangeSnapshot | undefined; + event: AdvisorActivityEvent; + turnId: string; +}): AdvisorExchangeSnapshot | undefined { + const { snapshot, event } = args; + if (event.phase === "armed" && snapshot) { + // The grant is announced once per turn. A repeat (a recoverable provider + // retry re-entering the runtime) must not erase consults already folded. + return snapshot; + } + if (snapshot && isNewExchange(snapshot, event)) { + // A new consult replaces the card; how many already settled this turn is + // carried forward so "Consult n/limit" stays truthful across cards. + return startSnapshot({ + event, + turnId: args.turnId, + settledConsults: snapshot.settledConsults, + }); + } + if (!snapshot) { + // A non-`started` first event still produces a usable record; dropping it + // would lose the outcome when the replay window evicted `started`. + return event.phase === "started" || event.phase === "armed" + ? startSnapshot({ event, turnId: args.turnId, settledConsults: 0 }) + : reduceEvent({ + snapshot: startSnapshot({ + event: { ...event, phase: "started" }, + turnId: args.turnId, + settledConsults: 0, + }), + event, + }); + } + return reduceEvent({ snapshot, event }); +} + +/** + * Folds the turn's advisor events into the task's exchange snapshot and + * archives every consult it passes through into the session consult log. + * + * The archive happens inside the fold loop rather than by comparing the map + * before and after. Provider events are rAF-batched, and rAF is paused while + * the window is hidden or occluded, so a single flush routinely carries several + * *complete* consults; a before/after comparison would keep only the last one + * and silently lose the rest — which is the failure the log exists to fix. + * + * Returns the original maps when nothing changed so the Zustand slices keep + * stable references and subscribed components do not re-render. */ export function applyAdvisorActivityEvents(args: { exchangeByTask: AdvisorExchangeByTask; + logByTask: AdvisorConsultLogByTask; taskId: string; turnId: string; events: NormalizedProviderEvent[]; -}): AdvisorExchangeByTask { + now?: number; +}): { + exchangeByTask: AdvisorExchangeByTask; + logByTask: AdvisorConsultLogByTask; +} { const advisorEvents = args.events.filter( (event): event is AdvisorActivityEvent => event.type === "advisor_activity", ); if (advisorEvents.length === 0) { - return args.exchangeByTask; + return { exchangeByTask: args.exchangeByTask, logByTask: args.logByTask }; } const current = args.exchangeByTask[args.taskId]; let snapshot = current?.turnId === args.turnId ? current : undefined; + let logByTask = args.logByTask; for (const event of advisorEvents) { - if (event.phase === "armed" && snapshot) { - // The grant is announced once per turn. A repeat (a recoverable provider - // retry re-entering the runtime) must not erase consults already folded. + const next = foldAdvisorEvent({ + snapshot, + event, + turnId: args.turnId, + }); + if (next === snapshot) { continue; } - if (snapshot && isNewExchange(snapshot, event)) { - // A new consult replaces the card; how many already settled this turn is - // carried forward so "Consult n/limit" stays truthful across cards. - snapshot = startSnapshot({ - event, - turnId: args.turnId, - settledConsults: snapshot.settledConsults, + snapshot = next; + // The turn-level grant is not a consult, so it never earns a log row — a + // turn that armed the Advisor and never asked it anything has nothing to + // review. + if (snapshot && !isAdvisorArmedOnly(snapshot)) { + logByTask = upsertAdvisorConsultLogEntry({ + logByTask, + taskId: args.taskId, + snapshot, + ...(args.now === undefined ? {} : { now: args.now }), }); - continue; - } - if (!snapshot) { - // A non-`started` first event still produces a usable record; dropping it - // would lose the outcome when the replay window evicted `started`. - snapshot = - event.phase === "started" || event.phase === "armed" - ? startSnapshot({ event, turnId: args.turnId, settledConsults: 0 }) - : reduceEvent({ - snapshot: startSnapshot({ - event: { ...event, phase: "started" }, - turnId: args.turnId, - settledConsults: 0, - }), - event, - }); - continue; } - snapshot = reduceEvent({ snapshot, event }); } - if (!snapshot || snapshot === current) { - return args.exchangeByTask; - } - return { ...args.exchangeByTask, [args.taskId]: snapshot }; + return { + exchangeByTask: + !snapshot || snapshot === current + ? args.exchangeByTask + : { ...args.exchangeByTask, [args.taskId]: snapshot }, + logByTask, + }; } /** * Store-shaped wrapper: returns the partial state patch, or `null` when the * events changed nothing. Keeps the fold (and its "did anything change?" * comparison) out of the hot `app.store.ts` event loop. + * + * Unchanged keys are **omitted** rather than echoed back, so spreading the + * patch into `set()` never replaces an untouched map reference. */ export function buildAdvisorExchangePatch(args: { exchangeByTask: AdvisorExchangeByTask; + logByTask: AdvisorConsultLogByTask; taskId: string; turnId: string; events: NormalizedProviderEvent[]; -}): { advisorExchangeByTask: AdvisorExchangeByTask } | null { + now?: number; +}): { + advisorExchangeByTask?: AdvisorExchangeByTask; + advisorConsultLogByTask?: AdvisorConsultLogByTask; +} | null { const next = applyAdvisorActivityEvents(args); - return next === args.exchangeByTask ? null : { advisorExchangeByTask: next }; + const exchangeChanged = next.exchangeByTask !== args.exchangeByTask; + const logChanged = next.logByTask !== args.logByTask; + if (!exchangeChanged && !logChanged) { + return null; + } + return { + ...(exchangeChanged + ? { advisorExchangeByTask: next.exchangeByTask } + : {}), + ...(logChanged ? { advisorConsultLogByTask: next.logByTask } : {}), + }; } export function clearAdvisorExchange(args: { diff --git a/src/lib/providers/advisor-consult-log.ts b/src/lib/providers/advisor-consult-log.ts new file mode 100644 index 00000000..e289859d --- /dev/null +++ b/src/lib/providers/advisor-consult-log.ts @@ -0,0 +1,267 @@ +import type { AdvisorExchangeSnapshot } from "@/lib/providers/advisor-activity"; +import type { ProviderId } from "@/lib/providers/provider.types"; + +/** + * The user's own call on whether a consult was worth it. + * + * Deliberately hand-set. Advice comes back as an MCP tool result and the + * primary is free to ignore it, so nothing in the event stream can say whether + * a consult changed the turn — an auto-computed "influence score" would be + * fabricated causality. `ignored` exists because "the advice was fine and the + * model dropped it anyway" is the outcome worth counting separately. + */ +export type AdvisorConsultVerdict = "helpful" | "not_helpful" | "ignored"; + +export interface AdvisorConsultLogEntry { + /** `${turnId}::${exchangeId ?? startedAt}` — see `advisorConsultLogEntryKey`. */ + key: string; + /** + * Held by reference, never copied. The fold already produces a fresh snapshot + * per change, so copying here would only double the memory per consult. + */ + snapshot: AdvisorExchangeSnapshot; + updatedAt: number; + verdict?: AdvisorConsultVerdict; +} + +/** Newest consult first, per task. */ +export type AdvisorConsultLogByTask = Record< + string, + readonly AdvisorConsultLogEntry[] | undefined +>; + +export interface AdvisorVerdictTally { + providerId: ProviderId; + model?: string; + helpful: number; + notHelpful: number; + ignored: number; +} + +/** Keyed by `${providerId}:${model ?? "unspecified"}`, not by task. */ +export type AdvisorVerdictTallyByModel = Record< + string, + AdvisorVerdictTally | undefined +>; + +/** + * Consults retained per task. + * + * Must stay **>= `MAX_ADVISOR_CONSULT_LIMIT` (20)** from + * `src/lib/providers/advisor.ts`, or a turn that spends its whole consult + * budget evicts its own earlier consults — the exact loss this log exists to + * fix. 24 leaves headroom for the previous turn's tail. + */ +export const ADVISOR_CONSULT_LOG_LIMIT = 24; + +/** + * Tasks that keep a log at once. Matches `RETAINED_TURN_ACTIVITY_LIMIT` so the + * "what ran after this consult" section has a chance of finding work items for + * the same tasks the log still covers. + * + * Worst case 8 x 24 x ~20 KB (advice <= 12,000 chars, question <= 8,000) is + * roughly 4 MB, all in memory and shed with the task. + */ +export const ADVISOR_CONSULT_LOG_TASK_LIMIT = 8; + +/** + * Shared empty result so a task with no consults never hands a subscriber a + * fresh array (which would re-render on every unrelated store write). + */ +export const EMPTY_ADVISOR_CONSULT_LOG: readonly AdvisorConsultLogEntry[] = []; + +/** + * Identity of one consult inside the log. + * + * `consultIndex` is deliberately not used: a recoverable provider retry can + * repeat it, which would silently merge two distinct consults into one row. + * `startedAt` is the fallback for runtimes that report no exchange id. + */ +export function advisorConsultLogEntryKey( + snapshot: AdvisorExchangeSnapshot, +): string { + return `${snapshot.turnId}::${snapshot.exchangeId ?? snapshot.startedAt}`; +} + +/** + * Tally key for the advisor that answered. `null` when the target never + * resolved, because a verdict on "no advisor" cannot be attributed to anything. + */ +export function advisorVerdictKey(advisor: { + providerId?: ProviderId; + model?: string; +}): string | null { + if (!advisor.providerId) { + return null; + } + return `${advisor.providerId}:${advisor.model ?? "unspecified"}`; +} + +function pruneAdvisorConsultLogTasks( + logByTask: AdvisorConsultLogByTask, +): AdvisorConsultLogByTask { + const taskIds = Object.keys(logByTask); + if (taskIds.length <= ADVISOR_CONSULT_LOG_TASK_LIMIT) { + return logByTask; + } + // Recency is the newest entry in the task, not the head of the array: a + // terminal fold replaces an entry in place, so position does not track time. + const ranked = taskIds + .map((taskId) => { + const entries = logByTask[taskId]; + let newest = 0; + for (const entry of entries ?? []) { + if (entry.updatedAt > newest) { + newest = entry.updatedAt; + } + } + return { taskId, newest }; + }) + .sort((left, right) => right.newest - left.newest) + .slice(0, ADVISOR_CONSULT_LOG_TASK_LIMIT); + + const next: AdvisorConsultLogByTask = {}; + for (const { taskId } of ranked) { + next[taskId] = logByTask[taskId]; + } + return next; +} + +/** + * Archives one consult snapshot. + * + * Called once per *folded step* rather than once per flush: provider events are + * rAF-batched (and rAF is paused while the window is hidden), so a single flush + * routinely carries several complete consults. A before/after comparison of the + * exchange map would keep only the last one — the bug this log exists to fix. + * + * An existing key is replaced **in place**, keeping both its position and its + * verdict, so the terminal fold supersedes the pending row without demoting it + * or discarding a rating the user already gave. + */ +export function upsertAdvisorConsultLogEntry(args: { + logByTask: AdvisorConsultLogByTask; + taskId: string; + snapshot: AdvisorExchangeSnapshot; + now?: number; +}): AdvisorConsultLogByTask { + const entries = args.logByTask[args.taskId] ?? EMPTY_ADVISOR_CONSULT_LOG; + const key = advisorConsultLogEntryKey(args.snapshot); + const now = args.now ?? Date.now(); + const index = entries.findIndex((entry) => entry.key === key); + + if (index >= 0) { + const existing = entries[index]!; + if (existing.snapshot === args.snapshot) { + return args.logByTask; + } + const nextEntries = entries.slice(); + nextEntries[index] = { + ...existing, + snapshot: args.snapshot, + updatedAt: now, + }; + return { ...args.logByTask, [args.taskId]: nextEntries }; + } + + const nextEntries = [ + { key, snapshot: args.snapshot, updatedAt: now }, + ...entries, + ].slice(0, ADVISOR_CONSULT_LOG_LIMIT); + return pruneAdvisorConsultLogTasks({ + ...args.logByTask, + [args.taskId]: nextEntries, + }); +} + +/** + * Records the user's verdict and folds it into the per-advisor-model tally. + * + * Returns `null` for a missing entry or a repeat of the same verdict so the + * caller can skip `set()` entirely — the persist middleware serializes on every + * `set`, regardless of whether the updater changed anything. + * + * Set-only: there is no deselect. The tally is intentionally *not* keyed by + * task, so it survives ring eviction and reads as a session-wide record of how + * a given advisor model has been doing. + */ +export function setAdvisorConsultLogVerdict(args: { + logByTask: AdvisorConsultLogByTask; + tallyByModel: AdvisorVerdictTallyByModel; + taskId: string; + entryKey: string; + verdict: AdvisorConsultVerdict; +}): { + logByTask: AdvisorConsultLogByTask; + tallyByModel: AdvisorVerdictTallyByModel; +} | null { + const entries = args.logByTask[args.taskId]; + if (!entries) { + return null; + } + const index = entries.findIndex((entry) => entry.key === args.entryKey); + if (index < 0) { + return null; + } + const existing = entries[index]!; + if (existing.verdict === args.verdict) { + return null; + } + + const nextEntries = entries.slice(); + nextEntries[index] = { ...existing, verdict: args.verdict }; + const logByTask = { ...args.logByTask, [args.taskId]: nextEntries }; + + const modelKey = advisorVerdictKey({ + providerId: existing.snapshot.advisorProviderId, + model: existing.snapshot.advisorModel, + }); + if (!modelKey || !existing.snapshot.advisorProviderId) { + return { logByTask, tallyByModel: args.tallyByModel }; + } + + const current = args.tallyByModel[modelKey] ?? { + providerId: existing.snapshot.advisorProviderId, + ...(existing.snapshot.advisorModel + ? { model: existing.snapshot.advisorModel } + : {}), + helpful: 0, + notHelpful: 0, + ignored: 0, + }; + const next: AdvisorVerdictTally = { ...current }; + if (existing.verdict) { + // Switching a verdict moves the count rather than adding a second one. + next[VERDICT_FIELD[existing.verdict]] = Math.max( + 0, + next[VERDICT_FIELD[existing.verdict]] - 1, + ); + } + next[VERDICT_FIELD[args.verdict]] += 1; + + return { + logByTask, + tallyByModel: { ...args.tallyByModel, [modelKey]: next }, + }; +} + +const VERDICT_FIELD: Record< + AdvisorConsultVerdict, + "helpful" | "notHelpful" | "ignored" +> = { + helpful: "helpful", + not_helpful: "notHelpful", + ignored: "ignored", +}; + +/** + * The Zustand read boundary. Never allocates, so a component may subscribe to + * it directly. A durable (SQLite-backed) log would replace only this function + * plus a hydrate action, with no component changes. + */ +export function selectAdvisorConsultLog( + logByTask: AdvisorConsultLogByTask, + taskId: string, +): readonly AdvisorConsultLogEntry[] { + return logByTask[taskId] ?? EMPTY_ADVISOR_CONSULT_LOG; +} diff --git a/src/store/app-store-provider-interaction-actions.ts b/src/store/app-store-provider-interaction-actions.ts index b4f82125..59f28770 100644 --- a/src/store/app-store-provider-interaction-actions.ts +++ b/src/store/app-store-provider-interaction-actions.ts @@ -27,6 +27,10 @@ import { interruptPendingToolInteractionsInMessages, } from "@/store/provider-message.utils"; import { clearAdvisorExchange } from "@/lib/providers/advisor-activity"; +import { + selectAdvisorConsultLog, + setAdvisorConsultLogVerdict, +} from "@/lib/providers/advisor-consult-log"; import { getWorkspaceSessionForState } from "@/store/workspace-runtime-state"; import type { WorkspaceSessionState } from "@/store/workspace-session-state"; import type { @@ -40,6 +44,10 @@ type ProviderInteractionActionKey = | "abortTaskTurn" | "skipTaskAdvisor" | "dismissAdvisorExchange" + | "openAdvisorConsultLog" + | "selectAdvisorConsultLogEntry" + | "closeAdvisorConsultLog" + | "setAdvisorConsultVerdict" | "resolveApproval" | "resolveUserInput" | "syncChildTasksIntoTurnGraph"; @@ -111,6 +119,52 @@ export function createProviderInteractionActions(args: { : { advisorExchangeByTask }; }); }, + openAdvisorConsultLog: ({ taskId, entryKey }) => { + // Dismissing the floating card must not erase the log, so opening the + // dialog deliberately touches nothing but the view state. + const entries = selectAdvisorConsultLog( + get().advisorConsultLogByTask, + taskId, + ); + const resolvedKey = + entryKey && entries.some((entry) => entry.key === entryKey) + ? entryKey + : (entries[0]?.key ?? null); + set({ advisorConsultLogView: { taskId, entryKey: resolvedKey } }); + }, + selectAdvisorConsultLogEntry: ({ entryKey }) => { + const view = get().advisorConsultLogView; + if (!view || view.entryKey === entryKey) { + return; + } + set({ advisorConsultLogView: { ...view, entryKey } }); + }, + closeAdvisorConsultLog: () => { + if (!get().advisorConsultLogView) { + return; + } + set({ advisorConsultLogView: null }); + }, + setAdvisorConsultVerdict: ({ taskId, entryKey, verdict }) => { + // Computed before `set` rather than inside it: a repeat verdict must not + // reach the store at all, because the persist middleware serializes on + // every `set` even when the updater returns the same state. + const state = get(); + const next = setAdvisorConsultLogVerdict({ + logByTask: state.advisorConsultLogByTask, + tallyByModel: state.advisorVerdictTallyByModel, + taskId, + entryKey, + verdict, + }); + if (!next) { + return; + } + set({ + advisorConsultLogByTask: next.logByTask, + advisorVerdictTallyByModel: next.tallyByModel, + }); + }, syncChildTasksIntoTurnGraph: ({ taskId, children }) => { // Computed before `set` rather than inside it: returning the same state // from the updater suppresses the subscriber notification but not the diff --git a/src/store/app-store.types.ts b/src/store/app-store.types.ts index 81b906f7..a6bcc649 100644 --- a/src/store/app-store.types.ts +++ b/src/store/app-store.types.ts @@ -14,6 +14,11 @@ import type { } from "@/lib/providers/provider.types"; import type { UpdateModelRuntimePreferenceArgs } from "@/lib/providers/model-runtime-preferences"; import type { AdvisorExchangeByTask } from "@/lib/providers/advisor-activity"; +import type { + AdvisorConsultLogByTask, + AdvisorConsultVerdict, + AdvisorVerdictTallyByModel, +} from "@/lib/providers/advisor-consult-log"; import type { ProviderTurnActivitySnapshot, RetainedTurnActivityByTask, @@ -204,6 +209,27 @@ export interface AppState * depend on transcript rendering. */ advisorExchangeByTask: AdvisorExchangeByTask; + /** + * Every consult of the session, newest first per task, in memory only. + * + * The exchange map above holds one snapshot per task, so a second consult in + * the same turn overwrites the first and the floating card auto-hides a few + * seconds later. This is the only place the question and the advice survive + * long enough to be reviewed. + */ + advisorConsultLogByTask: AdvisorConsultLogByTask; + /** + * The user's verdicts rolled up per advisor model, not per task, so it + * outlives ring eviction and reads as "how has this advisor been doing". + */ + advisorVerdictTallyByModel: AdvisorVerdictTallyByModel; + /** + * Which task's consult log is open, and which consult is selected in it. + * Store-held so exactly one dialog is open across split-pane chat areas, and + * so the short-lived triggers (the linger-timed card, the per-turn shelf) can + * open a dialog that outlives them. + */ + advisorConsultLogView: { taskId: string; entryKey: string | null } | null; nativeSessionReadyByTask: Record; providerSessionByTask: Record; providerGoalByTask: Record; @@ -601,6 +627,16 @@ export interface AppState skipTaskAdvisor: (args: { taskId: string }) => void; /** Dismisses the task's Advisor exchange card without touching the turn. */ dismissAdvisorExchange: (args: { taskId: string }) => void; + /** Opens the session consult log, optionally focused on one consult. */ + openAdvisorConsultLog: (args: { taskId: string; entryKey?: string }) => void; + selectAdvisorConsultLogEntry: (args: { entryKey: string }) => void; + closeAdvisorConsultLog: () => void; + /** Records the user's own call on a consult. Set-only; there is no deselect. */ + setAdvisorConsultVerdict: (args: { + taskId: string; + entryKey: string; + verdict: AdvisorConsultVerdict; + }) => void; resolveApproval: (args: { taskId: string; messageId: string; diff --git a/src/store/app.store.ts b/src/store/app.store.ts index 9375f545..508a3b6a 100644 --- a/src/store/app.store.ts +++ b/src/store/app.store.ts @@ -1606,9 +1606,9 @@ export const useAppStore = create()( providerTurnActivityByTask: {}, retainedTurnActivityByTask: {}, advisorExchangeByTask: {}, - nativeSessionReadyByTask: {}, - providerSessionByTask: {}, - providerGoalByTask: {}, + advisorConsultLogByTask: {}, advisorVerdictTallyByModel: {}, + advisorConsultLogView: null, nativeSessionReadyByTask: {}, + providerSessionByTask: {}, providerGoalByTask: {}, turnVerificationByWorkspace: {}, turnIntentComplianceByWorkspace: {}, workspaceRuntimeCacheById: {}, @@ -2838,6 +2838,7 @@ export const useAppStore = create()( // turn ended, and dropping it would hide advisor aborts. const advisorPatch = buildAdvisorExchangePatch({ exchangeByTask: currentState.advisorExchangeByTask, + logByTask: currentState.advisorConsultLogByTask, taskId: resolvedTaskId, turnId, events: pendingEvents, diff --git a/src/store/host-task-turn-sync.ts b/src/store/host-task-turn-sync.ts index 6b33bc06..13809bfa 100644 --- a/src/store/host-task-turn-sync.ts +++ b/src/store/host-task-turn-sync.ts @@ -16,6 +16,7 @@ import { applyAdvisorActivityEvents, type AdvisorExchangeByTask, } from "@/lib/providers/advisor-activity"; +import type { AdvisorConsultLogByTask } from "@/lib/providers/advisor-consult-log"; import { createWorkspaceSessionStateFromAppState, type WorkspaceRuntimeStatePatch, @@ -42,6 +43,7 @@ type HostTaskTurnStoreState = Parameters< >; retainedTurnActivityByTask: RetainedTurnActivityByTask; advisorExchangeByTask: AdvisorExchangeByTask; + advisorConsultLogByTask: AdvisorConsultLogByTask; }; interface LoadedHostTaskTurn { @@ -203,14 +205,21 @@ export function applyHostTaskTurnSync(args: { next: providerTurnActivityByTask, taskId: args.update.taskId, }); - const advisorExchangeByTask = args.update.activityEvents?.length + // A host batch carries the same hazard as a renderer flush: several complete + // consults can arrive at once, so the archive happens inside the fold rather + // than by comparing the exchange map on either side of it. + const advisor = args.update.activityEvents?.length ? applyAdvisorActivityEvents({ exchangeByTask: args.state.advisorExchangeByTask, + logByTask: args.state.advisorConsultLogByTask, taskId: args.update.taskId, turnId: args.update.turnId, events: args.update.activityEvents, }) - : args.state.advisorExchangeByTask; + : { + exchangeByTask: args.state.advisorExchangeByTask, + logByTask: args.state.advisorConsultLogByTask, + }; const sharedPatch = { hostOwnedTurnIdsByTask: { ...args.state.hostOwnedTurnIdsByTask, @@ -218,7 +227,8 @@ export function applyHostTaskTurnSync(args: { }, providerTurnActivityByTask, retainedTurnActivityByTask, - advisorExchangeByTask, + advisorExchangeByTask: advisor.exchangeByTask, + advisorConsultLogByTask: advisor.logByTask, taskWorkspaceIdById: { ...args.state.taskWorkspaceIdById, [args.update.taskId]: args.update.workspaceId, @@ -252,6 +262,7 @@ export function applyHostTaskTurnSync(args: { >; retainedTurnActivityByTask: RetainedTurnActivityByTask; advisorExchangeByTask: AdvisorExchangeByTask; + advisorConsultLogByTask: AdvisorConsultLogByTask; }, syncedSession: merged.session, turnSettled: merged.turnSettled, diff --git a/src/store/task-turn-runtime-cleanup.ts b/src/store/task-turn-runtime-cleanup.ts index 3fd88287..550cfd33 100644 --- a/src/store/task-turn-runtime-cleanup.ts +++ b/src/store/task-turn-runtime-cleanup.ts @@ -1,4 +1,5 @@ import type { AdvisorExchangeByTask } from "@/lib/providers/advisor-activity"; +import type { AdvisorConsultLogByTask } from "@/lib/providers/advisor-consult-log"; import type { ProviderTurnActivitySnapshot, RetainedTurnActivityByTask, @@ -25,6 +26,12 @@ export interface TaskTurnRuntimeEntries { */ retainedTurnActivityByTask: RetainedTurnActivityByTask; advisorExchangeByTask: AdvisorExchangeByTask; + /** + * The task's archived consults. Shed with the task, unlike + * `advisorVerdictTallyByModel`, which is keyed by advisor model rather than + * by task and so has nothing to drop here. + */ + advisorConsultLogByTask: AdvisorConsultLogByTask; hostOwnedTurnIdsByTask: Record; } @@ -80,6 +87,13 @@ export function removeTaskTurnRuntimeEntries(args: { if (advisorExchangeByTask) { patch.advisorExchangeByTask = advisorExchangeByTask; } + const advisorConsultLogByTask = removeRecordEntries( + args.state.advisorConsultLogByTask, + args.taskIds, + ); + if (advisorConsultLogByTask) { + patch.advisorConsultLogByTask = advisorConsultLogByTask; + } const hostOwnedTurnIdsByTask = removeRecordEntries( args.state.hostOwnedTurnIdsByTask, args.taskIds, diff --git a/tests/advisor-consult-log-render.test.tsx b/tests/advisor-consult-log-render.test.tsx new file mode 100644 index 00000000..cdae2f75 --- /dev/null +++ b/tests/advisor-consult-log-render.test.tsx @@ -0,0 +1,225 @@ +import { describe, expect, test } from "bun:test"; +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; + +import { AdvisorConsultLogDialog } from "@/components/session/AdvisorConsultLogDialog"; +import type { AdvisorExchangeSnapshot } from "@/lib/providers/advisor-activity"; +import type { AdvisorConsultLogEntry } from "@/lib/providers/advisor-consult-log"; +import type { ProviderTurnWorkItem } from "@/lib/providers/turn-status"; + +const T0 = 1_700_000_000_000; + +function snapshot( + overrides: Partial = {}, +): AdvisorExchangeSnapshot { + return { + turnId: "turn-1", + exchangeId: "exchange-1", + consultIndex: 1, + consultLimit: 5, + question: "Is the cancellation path sound?", + primaryProviderId: "claude-code", + primaryModel: "claude-opus-4-6", + advisorProviderId: "codex", + advisorModel: "gpt-5.6-sol", + isolation: "codex-ephemeral-read-only", + startedAt: T0, + timeoutMs: 90_000, + outcome: "completed", + outcomeAt: T0 + 4_000, + durationMs: 4_000, + advice: "Cancel the preflight before the primary aborts.", + adviceChars: 45, + inputTokens: 900, + outputTokens: 120, + settledConsults: 1, + stages: [ + { phase: "started", at: T0 }, + { phase: "completed", at: T0 + 4_000 }, + ], + ...overrides, + }; +} + +function entry( + key: string, + overrides: Partial = {}, + verdict?: AdvisorConsultLogEntry["verdict"], +): AdvisorConsultLogEntry { + return { + key, + snapshot: snapshot(overrides), + updatedAt: T0, + ...(verdict ? { verdict } : {}), + }; +} + +function workItem( + id: string, + startedAt: number, + title: string, +): ProviderTurnWorkItem { + return { + id, + kind: "tool", + status: "completed", + title, + progressMessages: [], + startedAt, + updatedAt: startedAt, + }; +} + +function render( + props: Partial[0]> = {}, +) { + return renderToStaticMarkup( + createElement(AdvisorConsultLogDialog, { + open: true, + onOpenChange: () => {}, + entries: [entry("turn-1::exchange-1")], + selectedKey: "turn-1::exchange-1", + onSelectEntry: () => {}, + activeTurnId: "turn-1", + workItems: [], + tallyByModel: {}, + onSetVerdict: () => {}, + ...props, + }), + ); +} + +describe("AdvisorConsultLogDialog", () => { + test("renders one row per archived consult", () => { + const html = render({ + entries: [ + entry("turn-1::exchange-2", { + exchangeId: "exchange-2", + consultIndex: 2, + question: "Does the retry path double-count?", + }), + entry("turn-1::exchange-1"), + ], + }); + + expect(html).toContain("Consult 1/5"); + expect(html).toContain("Consult 2/5"); + expect( + html.match(/data-testid="advisor-consult-log-row"/g) ?? [], + ).toHaveLength(2); + }); + + test("renders the selected consult's question and advice in full", () => { + const html = render(); + + expect(html).toContain("Is the cancellation path sound?"); + expect(html).toContain("Cancel the preflight before the primary aborts."); + expect(html).toContain("Did the advisor system work?"); + }); + + test("states that the question was never reported rather than hiding it", () => { + const html = render({ + entries: [entry("turn-1::exchange-1", { question: undefined })], + }); + + expect(html).toContain("The runtime did not report the question."); + }); + + test("never implies the spend is a share of the turn", () => { + // Load-bearing: ChatMessage.usage is per message and carries no turn id, so + // a percentage denominator would silently drift as messages page in and + // out. Asserted verbatim because dropping it turns an absolute number into + // an implied one. + const html = render(); + + expect(html).toContain( + "Reported by the runtime for the advisor call only. Stave reports usage per message, not per turn, so this is not a share of the turn's total.", + ); + expect(html).toContain("900 in · 120 out"); + }); + + test("never implies the consult caused what ran after it", () => { + // Load-bearing: advice returns as a tool result the primary may ignore, so + // this section is sequence, not causality, and must say so. + const html = render({ + workItems: [ + workItem("before", T0 + 1_000, "Read src/before.ts"), + workItem("after", T0 + 9_000, "Edit src/after.ts"), + ], + }); + + expect(html).toContain( + "Tool calls in this turn that started after the consult settled, in order. Sequence only — Stave cannot tell whether the advice caused them.", + ); + expect(html).toContain("Edit src/after.ts"); + expect(html).not.toContain("Read src/before.ts"); + }); + + test("says why nothing ran after when the work items are gone", () => { + const html = render({ workItems: [] }); + + expect(html).toContain( + "No tool calls from this turn are still in memory, so Stave cannot say what ran after.", + ); + }); + + test("offers the verdict options and marks the recorded one", () => { + const html = render({ + entries: [entry("turn-1::exchange-1", {}, "not_helpful")], + }); + + expect(html).toContain( + "Your own judgement, recorded per consult. Stave does not infer this.", + ); + expect(html).toContain("Was this consult helpful?"); + expect(html).toContain("Helpful"); + expect(html).toContain("Not helpful"); + expect(html).toContain("Ignored"); + expect(html).toContain('aria-checked="true"'); + }); + + test("reports the advisor's running record without a denominator", () => { + // No "4 of 9": the tally outlives ring eviction, so a denominator drifts. + const rated = render({ + entries: [entry("turn-1::exchange-1", {}, "helpful")], + tallyByModel: { + "codex:gpt-5.6-sol": { + providerId: "codex", + model: "gpt-5.6-sol", + helpful: 3, + notHelpful: 1, + ignored: 0, + }, + }, + }); + expect(rated).toContain("3 helpful · 1 not helpful this session."); + + expect(render()).toContain("No verdicts recorded for this advisor yet."); + }); + + test("an unsettled consult of a finished turn reads as unresolved", () => { + const html = render({ + entries: [ + entry("turn-1::exchange-1", { + outcome: "pending", + outcomeAt: undefined, + durationMs: undefined, + advice: undefined, + stages: [{ phase: "started", at: T0 }], + }), + ], + activeTurnId: "turn-2", + workItems: [workItem("after", T0 + 9_000, "Edit src/after.ts")], + }); + + expect(html).toContain("Unresolved"); + // The checks read off `outcome`, which is still `pending`, so the detail + // pane has to say why rather than claiming the advisor is being waited on. + expect(html).toContain( + "Its turn ended before the runtime reported an outcome, so this consult has no result and no cost to show.", + ); + // Neither section is honest about a consult that never settled. + expect(html).not.toContain("What ran after this consult"); + expect(html).not.toContain("Was this consult helpful?"); + }); +}); diff --git a/tests/advisor-exchange.test.ts b/tests/advisor-exchange.test.ts index 1e30328f..d0e64477 100644 --- a/tests/advisor-exchange.test.ts +++ b/tests/advisor-exchange.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { applyAdvisorActivityEvents, + buildAdvisorExchangePatch, clearAdvisorExchange, formatAdvisorDuration, isAdvisorArmedOnly, @@ -8,8 +9,25 @@ import { isAdvisorExchangeTerminal, type AdvisorExchangeSnapshot, } from "../src/lib/providers/advisor-activity"; +import { + ADVISOR_CONSULT_LOG_LIMIT, + ADVISOR_CONSULT_LOG_TASK_LIMIT, + EMPTY_ADVISOR_CONSULT_LOG, + advisorConsultLogEntryKey, + selectAdvisorConsultLog, + setAdvisorConsultLogVerdict, + upsertAdvisorConsultLogEntry, + type AdvisorConsultLogEntry, +} from "../src/lib/providers/advisor-consult-log"; +import { + resolveAdvisorConsultLogStatus, + resolveAdvisorConsultWorkItems, + resolveAdvisorPostConsultWorkItems, + summarizeAdvisorTurnSpend, +} from "../src/components/session/advisor-consult-log.utils"; import type { NormalizedProviderEvent } from "../src/lib/providers/provider.types"; import { NormalizedProviderEventSchema } from "../src/lib/providers/schemas"; +import { createWorkGraph } from "../src/lib/work-graph/work-graph-reducer"; import { ADVISOR_EXCHANGE_ATTENTION_LINGER_MS, ADVISOR_EXCHANGE_SETTLED_LINGER_MS, @@ -46,10 +64,23 @@ function advisorEvent( function fold(events: NormalizedProviderEvent[], turnId = "turn-1") { return applyAdvisorActivityEvents({ exchangeByTask: {}, + logByTask: {}, + taskId: "task-1", + turnId, + events, + }).exchangeByTask["task-1"] as AdvisorExchangeSnapshot; +} + +/** The archived consults the same events produce, newest first. */ +function foldLog(events: NormalizedProviderEvent[], turnId = "turn-1") { + return applyAdvisorActivityEvents({ + exchangeByTask: {}, + logByTask: {}, taskId: "task-1", turnId, events, - })["task-1"] as AdvisorExchangeSnapshot; + now: T0, + }).logByTask["task-1"] as readonly AdvisorConsultLogEntry[]; } /** One on-demand consult: started by the primary, answered by the advisor. */ @@ -169,32 +200,39 @@ describe("advisor exchange reducer", () => { test("a new turn replaces the previous exchange instead of merging", () => { const first = applyAdvisorActivityEvents({ exchangeByTask: {}, + logByTask: {}, taskId: "task-1", turnId: "turn-1", events: SUCCESSFUL_EXCHANGE, }); const second = applyAdvisorActivityEvents({ - exchangeByTask: first, + exchangeByTask: first.exchangeByTask, + logByTask: first.logByTask, taskId: "task-1", turnId: "turn-2", events: [advisorEvent({ phase: "started", at: T0 + 60_000 })], }); - expect(second["task-1"]?.turnId).toBe("turn-2"); - expect(second["task-1"]?.outcome).toBe("pending"); - expect(second["task-1"]?.settledConsults).toBe(0); + expect(second.exchangeByTask["task-1"]?.turnId).toBe("turn-2"); + expect(second.exchangeByTask["task-1"]?.outcome).toBe("pending"); + expect(second.exchangeByTask["task-1"]?.settledConsults).toBe(0); + // The new turn replaces the card but must not erase the archive. + expect(second.logByTask["task-1"]).toHaveLength(2); }); test("keeps a stable reference when no advisor events are present", () => { const map = { "task-1": fold(SUCCESSFUL_EXCHANGE) }; + const log = {}; const next = applyAdvisorActivityEvents({ exchangeByTask: map, + logByTask: log, taskId: "task-1", turnId: "turn-1", events: [{ type: "done", stop_reason: "end_turn" }], }); - expect(next).toBe(map); + expect(next.exchangeByTask).toBe(map); + expect(next.logByTask).toBe(log); }); test("recovers an outcome even when the started phase was evicted", () => { @@ -631,3 +669,498 @@ describe("advisor event schema", () => { }); }); }); + +describe("advisor consult log", () => { + const ARMED_GRANT = advisorEvent({ + phase: "armed", + consultLimit: 5, + advisorProviderId: "codex", + advisorModel: "gpt-5.6-sol", + }); + + /** Two complete consults, folded from one batch — the rAF-batching case. */ + const TWO_COMPLETE_CONSULTS: NormalizedProviderEvent[] = [ + ...SUCCESSFUL_EXCHANGE, + advisorEvent({ + phase: "started", + exchangeId: "exchange-2", + consultIndex: 2, + consultLimit: 5, + question: "Does the retry path double-count?", + advisorProviderId: "codex", + advisorModel: "gpt-5.6-sol", + at: T0 + 10_000, + }), + advisorEvent({ + phase: "completed", + exchangeId: "exchange-2", + at: T0 + 14_000, + durationMs: 4_000, + advice: "It does not.", + inputTokens: 100, + outputTokens: 20, + totalCostUsd: 0.002, + }), + ]; + + test("two complete consults in one batch produce two entries", () => { + // The regression the log exists for: provider events are rAF-batched (and + // rAF pauses while the window is hidden), so one flush can carry several + // finished consults. Comparing the exchange map before and after would + // keep only the last. + const entries = foldLog(TWO_COMPLETE_CONSULTS); + + expect(entries).toHaveLength(2); + // Newest first. + expect(entries[0]?.snapshot.exchangeId).toBe("exchange-2"); + expect(entries[1]?.snapshot.exchangeId).toBe("exchange-1"); + expect(entries[0]?.snapshot.advice).toBe("It does not."); + expect(entries[1]?.snapshot.question).toBe( + "Is the cancellation path sound?", + ); + }); + + test("the terminal fold replaces the pending entry in place", () => { + const pendingOnly = foldLog([SUCCESSFUL_EXCHANGE[0]!]); + expect(pendingOnly).toHaveLength(1); + expect(pendingOnly[0]?.snapshot.outcome).toBe("pending"); + + const settled = foldLog(SUCCESSFUL_EXCHANGE); + expect(settled).toHaveLength(1); + expect(settled[0]?.snapshot.outcome).toBe("completed"); + expect(settled[0]?.key).toBe(pendingOnly[0]!.key); + }); + + test("an armed-only turn writes no entry", () => { + expect(foldLog([ARMED_GRANT])).toBeUndefined(); + }); + + test("replacing an entry preserves an existing verdict and its position", () => { + const started = applyAdvisorActivityEvents({ + exchangeByTask: {}, + logByTask: {}, + taskId: "task-1", + turnId: "turn-1", + events: [SUCCESSFUL_EXCHANGE[0]!], + now: T0, + }); + const key = started.logByTask["task-1"]![0]!.key; + const rated = setAdvisorConsultLogVerdict({ + logByTask: started.logByTask, + tallyByModel: {}, + taskId: "task-1", + entryKey: key, + verdict: "helpful", + })!; + const settled = applyAdvisorActivityEvents({ + exchangeByTask: started.exchangeByTask, + logByTask: rated.logByTask, + taskId: "task-1", + turnId: "turn-1", + events: [SUCCESSFUL_EXCHANGE[1]!], + now: T0 + 1, + }); + + const entries = settled.logByTask["task-1"]!; + expect(entries).toHaveLength(1); + expect(entries[0]?.snapshot.outcome).toBe("completed"); + expect(entries[0]?.verdict).toBe("helpful"); + }); + + test("re-archiving the same snapshot keeps the map reference", () => { + const snapshot = fold(SUCCESSFUL_EXCHANGE); + const first = upsertAdvisorConsultLogEntry({ + logByTask: {}, + taskId: "task-1", + snapshot, + now: T0, + }); + const second = upsertAdvisorConsultLogEntry({ + logByTask: first, + taskId: "task-1", + snapshot, + now: T0 + 5_000, + }); + + expect(second).toBe(first); + }); + + test("a maxed-out turn never truncates its own consults", () => { + // The per-task bound must stay at or above MAX_ADVISOR_CONSULT_LIMIT (20), + // or spending the whole budget would evict the turn's earliest consults. + expect(ADVISOR_CONSULT_LOG_LIMIT).toBeGreaterThanOrEqual(20); + let logByTask = {}; + for (let index = 1; index <= 20; index += 1) { + logByTask = upsertAdvisorConsultLogEntry({ + logByTask, + taskId: "task-1", + snapshot: fold([ + advisorEvent({ + phase: "started", + exchangeId: `exchange-${index}`, + consultIndex: index, + at: T0 + index, + }), + ]), + now: T0 + index, + }); + } + expect(selectAdvisorConsultLog(logByTask, "task-1")).toHaveLength(20); + }); + + test("the per-task ring evicts the oldest consult", () => { + let logByTask = {}; + for (let index = 0; index < ADVISOR_CONSULT_LOG_LIMIT + 3; index += 1) { + logByTask = upsertAdvisorConsultLogEntry({ + logByTask, + taskId: "task-1", + snapshot: fold([ + advisorEvent({ + phase: "started", + exchangeId: `exchange-${index}`, + at: T0 + index, + }), + ]), + now: T0 + index, + }); + } + const entries = selectAdvisorConsultLog(logByTask, "task-1"); + expect(entries).toHaveLength(ADVISOR_CONSULT_LOG_LIMIT); + expect(entries[0]?.snapshot.exchangeId).toBe( + `exchange-${ADVISOR_CONSULT_LOG_LIMIT + 2}`, + ); + expect( + entries.some((entry) => entry.snapshot.exchangeId === "exchange-0"), + ).toBe(false); + }); + + test("the task ring evicts the least recently updated task", () => { + let logByTask = {}; + for (let index = 0; index < ADVISOR_CONSULT_LOG_TASK_LIMIT + 1; index += 1) { + logByTask = upsertAdvisorConsultLogEntry({ + logByTask, + taskId: `task-${index}`, + snapshot: fold([ + advisorEvent({ phase: "started", exchangeId: "e", at: T0 }), + ]), + now: T0 + index, + }); + } + expect(Object.keys(logByTask)).toHaveLength( + ADVISOR_CONSULT_LOG_TASK_LIMIT, + ); + expect(selectAdvisorConsultLog(logByTask, "task-0")).toBe( + EMPTY_ADVISOR_CONSULT_LOG, + ); + expect( + selectAdvisorConsultLog( + logByTask, + `task-${ADVISOR_CONSULT_LOG_TASK_LIMIT}`, + ), + ).toHaveLength(1); + }); + + test("dismissing the exchange card leaves the log intact", () => { + const folded = applyAdvisorActivityEvents({ + exchangeByTask: {}, + logByTask: {}, + taskId: "task-1", + turnId: "turn-1", + events: SUCCESSFUL_EXCHANGE, + now: T0, + }); + const cleared = clearAdvisorExchange({ + exchangeByTask: folded.exchangeByTask, + taskId: "task-1", + }); + + expect(cleared["task-1"]).toBeUndefined(); + expect(folded.logByTask["task-1"]).toHaveLength(1); + }); + + test("the patch omits the key that did not change", () => { + const seeded = applyAdvisorActivityEvents({ + exchangeByTask: {}, + logByTask: {}, + taskId: "task-1", + turnId: "turn-1", + events: SUCCESSFUL_EXCHANGE, + now: T0, + }); + expect( + buildAdvisorExchangePatch({ + exchangeByTask: seeded.exchangeByTask, + logByTask: seeded.logByTask, + taskId: "task-1", + turnId: "turn-1", + events: [{ type: "done", stop_reason: "end_turn" }], + }), + ).toBeNull(); + + const patch = buildAdvisorExchangePatch({ + exchangeByTask: {}, + logByTask: {}, + taskId: "task-1", + turnId: "turn-1", + events: [ARMED_GRANT], + })!; + // An armed grant updates the card but archives nothing, so the log key must + // not appear and replace an untouched map reference. + expect(patch.advisorExchangeByTask).toBeDefined(); + expect("advisorConsultLogByTask" in patch).toBe(false); + }); + + describe("verdicts", () => { + function seed() { + const folded = applyAdvisorActivityEvents({ + exchangeByTask: {}, + logByTask: {}, + taskId: "task-1", + turnId: "turn-1", + events: SUCCESSFUL_EXCHANGE, + now: T0, + }); + return { + logByTask: folded.logByTask, + entryKey: folded.logByTask["task-1"]![0]!.key, + }; + } + + test("records, switches, and refuses a repeat", () => { + const { logByTask, entryKey } = seed(); + const first = setAdvisorConsultLogVerdict({ + logByTask, + tallyByModel: {}, + taskId: "task-1", + entryKey, + verdict: "helpful", + })!; + expect(first.tallyByModel["codex:gpt-5.6-sol"]).toEqual({ + providerId: "codex", + model: "gpt-5.6-sol", + helpful: 1, + notHelpful: 0, + ignored: 0, + }); + + // A repeat must not reach `set()` at all. + expect( + setAdvisorConsultLogVerdict({ + ...first, + taskId: "task-1", + entryKey, + verdict: "helpful", + }), + ).toBeNull(); + + const switched = setAdvisorConsultLogVerdict({ + ...first, + taskId: "task-1", + entryKey, + verdict: "ignored", + })!; + // Switching moves the count rather than adding a second one. + expect(switched.tallyByModel["codex:gpt-5.6-sol"]).toEqual({ + providerId: "codex", + model: "gpt-5.6-sol", + helpful: 0, + notHelpful: 0, + ignored: 1, + }); + }); + + test("returns null for an entry that is not in the log", () => { + const { logByTask } = seed(); + expect( + setAdvisorConsultLogVerdict({ + logByTask, + tallyByModel: {}, + taskId: "task-1", + entryKey: "turn-9::missing", + verdict: "helpful", + }), + ).toBeNull(); + expect( + setAdvisorConsultLogVerdict({ + logByTask, + tallyByModel: {}, + taskId: "task-absent", + entryKey: "turn-1::exchange-1", + verdict: "helpful", + }), + ).toBeNull(); + }); + + test("the tally survives evicting the entry it came from", () => { + const { logByTask, entryKey } = seed(); + const rated = setAdvisorConsultLogVerdict({ + logByTask, + tallyByModel: {}, + taskId: "task-1", + entryKey, + verdict: "not_helpful", + })!; + let evicted = rated.logByTask; + for (let index = 0; index < ADVISOR_CONSULT_LOG_LIMIT; index += 1) { + evicted = upsertAdvisorConsultLogEntry({ + logByTask: evicted, + taskId: "task-1", + snapshot: fold([ + advisorEvent({ + phase: "started", + exchangeId: `filler-${index}`, + at: T0 + 1_000 + index, + }), + ]), + now: T0 + 1_000 + index, + }); + } + expect( + selectAdvisorConsultLog(evicted, "task-1").some( + (entry) => entry.key === entryKey, + ), + ).toBe(false); + expect(rated.tallyByModel["codex:gpt-5.6-sol"]?.notHelpful).toBe(1); + }); + }); + + test("selectAdvisorConsultLog returns one shared empty reference", () => { + expect(selectAdvisorConsultLog({}, "task-1")).toBe( + EMPTY_ADVISOR_CONSULT_LOG, + ); + expect(selectAdvisorConsultLog({}, "task-2")).toBe( + selectAdvisorConsultLog({}, "task-1"), + ); + }); + + test("entry keys separate consults that repeat a consult index", () => { + // A recoverable provider retry can reuse `consultIndex`, so it must never + // be the identity. + const entries = foldLog([ + advisorEvent({ phase: "started", exchangeId: "a", consultIndex: 1 }), + advisorEvent({ + phase: "started", + exchangeId: "b", + consultIndex: 1, + at: T0 + 10, + }), + ]); + expect(entries).toHaveLength(2); + expect(new Set(entries.map((entry) => entry.key)).size).toBe(2); + }); + + describe("presentation", () => { + function entryFor(events: NormalizedProviderEvent[], turnId = "turn-1") { + return { + key: advisorConsultLogEntryKey(fold(events, turnId)), + snapshot: fold(events, turnId), + updatedAt: T0, + } satisfies AdvisorConsultLogEntry; + } + + test("a still-pending consult reads as unresolved once its turn is gone", () => { + const entry = entryFor([SUCCESSFUL_EXCHANGE[0]!]); + expect( + resolveAdvisorConsultLogStatus({ entry, activeTurnId: "turn-1" }), + ).toBe("pending"); + expect( + resolveAdvisorConsultLogStatus({ entry, activeTurnId: "turn-2" }), + ).toBe("unresolved"); + expect( + resolveAdvisorConsultLogStatus({ entry, activeTurnId: null }), + ).toBe("unresolved"); + }); + + test("post-consult work items are filtered, ordered and capped", () => { + const entry = entryFor(SUCCESSFUL_EXCHANGE); + const settledAt = entry.snapshot.outcomeAt!; + const workItems = [ + { id: "late", startedAt: settledAt + 2_000 }, + { id: "early", startedAt: settledAt - 1 }, + { id: "next", startedAt: settledAt + 1 }, + ].map((item) => ({ + ...item, + kind: "tool" as const, + status: "completed" as const, + title: item.id, + progressMessages: [], + updatedAt: item.startedAt, + })); + + const resolved = resolveAdvisorPostConsultWorkItems({ + entry, + workItems, + }); + expect(resolved.map((item) => item.id)).toEqual(["next", "late"]); + expect( + resolveAdvisorPostConsultWorkItems({ entry, workItems, limit: 1 }), + ).toHaveLength(1); + // An unsettled consult has no "after". + expect( + resolveAdvisorPostConsultWorkItems({ + entry: entryFor([SUCCESSFUL_EXCHANGE[0]!]), + workItems, + }), + ).toEqual([]); + }); + + test("does not lend a newer turn's work items to an older consult", () => { + const entry = entryFor(SUCCESSFUL_EXCHANGE, "turn-old"); + const newerActivity = { + turnId: "turn-new", + providerId: "codex" as const, + startedAt: T0 + 20_000, + lastEventAt: T0 + 21_000, + stalledAt: null, + pendingInteraction: null, + workItemsById: { + newer: { + id: "newer", + kind: "tool" as const, + status: "completed" as const, + title: "Newer turn work", + progressMessages: [], + startedAt: T0 + 20_500, + updatedAt: T0 + 21_000, + }, + }, + orderedWorkItemIds: ["newer"], + workGraph: createWorkGraph({ + turnId: "turn-new", + providerId: "codex", + startedAt: T0 + 20_000, + }), + }; + + expect( + resolveAdvisorConsultWorkItems({ + entry, + activity: newerActivity, + retained: null, + }), + ).toEqual([]); + }); + + test("turn spend sums only this turn's consults", () => { + const entries = [ + ...foldLog(TWO_COMPLETE_CONSULTS), + entryFor(SUCCESSFUL_EXCHANGE, "turn-2"), + ]; + const spend = summarizeAdvisorTurnSpend({ entries, turnId: "turn-1" }); + + expect(spend.consults).toBe(2); + expect(spend.inputTokens).toBe(1_000); + expect(spend.outputTokens).toBe(140); + expect(spend.totalCostUsd).toBeCloseTo(0.002, 6); + }); + + test("turn spend reports no cost rather than a fake zero", () => { + const spend = summarizeAdvisorTurnSpend({ + entries: foldLog(SUCCESSFUL_EXCHANGE), + turnId: "turn-1", + }); + expect(spend.consults).toBe(1); + expect(spend.totalCostUsd).toBeNull(); + }); + }); +}); diff --git a/tests/turn-activity-render.test.tsx b/tests/turn-activity-render.test.tsx index f3acc403..affd7f5c 100644 --- a/tests/turn-activity-render.test.tsx +++ b/tests/turn-activity-render.test.tsx @@ -581,6 +581,89 @@ describe("TurnActivity", () => { // where "armed but never asked" has to be legible. expect(html).toContain("Advisor armed · 0 consults"); expect(html).toContain("0/5"); + // No archived consults yet, so the row has nothing to open. + expect(html).not.toContain('data-turn-activity-opens'); + }); + + describe("the advisor row as a consult log entry point", () => { + const advisorTurn = { + activeTurnId: "turn-advisor", + activity: { + turnId: "turn-advisor", + providerId: "codex" as const, + startedAt: 1_000, + lastEventAt: 5_000, + stalledAt: null, + pendingInteraction: null, + workItemsById: { + "tool-1": { + id: "tool-1", + kind: "tool" as const, + status: "completed" as const, + title: "Read src/app.ts", + toolUseId: "toolu_1", + progressMessages: [], + startedAt: 2_000, + updatedAt: 3_000, + }, + }, + orderedWorkItemIds: ["tool-1"], + }, + isPlanPreparing: false, + todos: [], + advisorExchange: { + turnId: "turn-advisor", + primaryProviderId: "codex" as const, + advisorProviderId: "claude-code" as const, + advisorModel: "claude-fable-5", + consultLimit: 5, + consultIndex: 1, + startedAt: 1_000, + outcome: "completed" as const, + outcomeAt: 4_000, + durationMs: 3_000, + settledConsults: 1, + stages: [], + }, + }; + const workItems = [advisorTurn.activity.workItemsById["tool-1"]!]; + + test("opens the log without claiming the transcript can reveal it", () => { + const html = renderToStaticMarkup( + createElement(TurnActivitySurface, { + ...advisorTurn, + workItems, + hasAdvisorConsultLog: true, + onOpenAdvisorLog: () => {}, + onSelectTool: () => {}, + }), + ); + + expect(html).toContain('data-turn-activity-opens="advisor-consult-log"'); + // The advisor row must not be mistaken for a revealable tool call: it + // stands for a consult the transcript never rendered. + const advisorRow = html.slice( + html.indexOf('data-turn-activity-item-id="advisor"'), + ); + expect(advisorRow.slice(0, advisorRow.indexOf(""))).not.toContain( + "data-turn-activity-revealable", + ); + // The activation refactor must not have cost the tool rows their reveal. + expect(html).toContain('data-turn-activity-revealable="true"'); + }); + + test("stays inert when the task has no archived consults", () => { + const html = renderToStaticMarkup( + createElement(TurnActivitySurface, { + ...advisorTurn, + workItems, + onSelectTool: () => {}, + }), + ); + + expect(html).not.toContain("data-turn-activity-opens"); + expect(html).toContain('data-turn-activity-revealable="true"'); + }); }); describe("replaying a finished turn in the panel", () => {