diff --git a/packages/core/realtime/use-realtime-sync-connect-backfill.test.tsx b/packages/core/realtime/use-realtime-sync-connect-backfill.test.tsx index c83e57cc0..c6e3efac0 100644 --- a/packages/core/realtime/use-realtime-sync-connect-backfill.test.tsx +++ b/packages/core/realtime/use-realtime-sync-connect-backfill.test.tsx @@ -5,7 +5,7 @@ import type { ReactNode } from "react"; import { afterEach, describe, expect, it, vi } from "vitest"; import { api } from "../api"; import { WSClient } from "../api/ws-client"; -import { useTaskMessages } from "../chat/queries"; +import { chatKeys, useTaskMessages } from "../chat/queries"; import type { TaskMessagePayload } from "../types/events"; import { useRealtimeSync, type RealtimeSyncStores } from "./use-realtime-sync"; @@ -66,4 +66,66 @@ describe("timeline repair on connect", () => { await waitFor(() => expect(result.current.data?.map((m) => m.seq)).toEqual([1, 2, 3])); unmount(); ws.disconnect(); qc.clear(); }); + + it("recovers when the transcript read is still in flight at the handshake", async () => { + // The other order, and the harder one: the GET started before the socket + // was live, so its answer cannot be trusted however late it arrives. + // Invalidating cannot express that — against a fetch already in flight it + // does nothing at all — so the read is cancelled and reissued. + vi.stubGlobal("WebSocket", FakeWebSocket); + const ws = new WSClient("ws://example.test/ws", { cookieAuth: true }); + ws.connect(); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false, refetchOnWindowFocus: false } } }); + const wrapper = ({ children }: { children: ReactNode }) => {children}; + + let resolveFirst!: (rows: TaskMessagePayload[]) => void; + vi.mocked(api.listTaskMessages) + .mockImplementationOnce(() => new Promise((done) => { resolveFirst = done; })) + .mockResolvedValue([msg(1), msg(2), msg(3)]); + + const { unmount } = renderHook( + () => { useRealtimeSync(ws, stores); return useTaskMessages(id, true); }, + { wrapper }, + ); + await waitFor(() => expect(api.listTaskMessages).toHaveBeenCalledTimes(1)); + + act(() => { FakeWebSocket.last.onopen?.(); }); + // The stale snapshot lands after the handshake; it must not be what is kept. + await act(async () => { resolveFirst([msg(1)]); await Promise.resolve(); }); + + await waitFor(() => expect( + (qc.getQueryData(chatKeys.taskMessages(id)) as TaskMessagePayload[] | undefined)?.map((m) => m.seq), + ).toEqual([1, 2, 3])); + unmount(); ws.disconnect(); qc.clear(); + }); + + it("reads once when an established connection comes back", async () => { + // Reconnect runs the workspace-wide recovery as well as this repair, so + // `task-messages` had two owners and issued the same unpaginated request + // twice, one after the other — sequential, so nothing deduped them. + vi.stubGlobal("WebSocket", FakeWebSocket); + const ws = new WSClient("ws://example.test/ws", { cookieAuth: true }); + ws.connect(); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false, refetchOnWindowFocus: false } } }); + const wrapper = ({ children }: { children: ReactNode }) => {children}; + + vi.mocked(api.listTaskMessages).mockResolvedValue([msg(1)]); + const { unmount } = renderHook( + () => { useRealtimeSync(ws, stores); return useTaskMessages(id, true); }, + { wrapper }, + ); + await waitFor(() => expect(api.listTaskMessages).toHaveBeenCalledTimes(1)); + + // First handshake, then a drop and a second one. + await act(async () => { FakeWebSocket.last.onopen?.(); await Promise.resolve(); }); + await waitFor(() => expect(api.listTaskMessages).toHaveBeenCalledTimes(2)); + vi.mocked(api.listTaskMessages).mockClear(); + + await act(async () => { FakeWebSocket.last.onclose?.(); await Promise.resolve(); }); + await act(async () => { FakeWebSocket.last.onopen?.(); await Promise.resolve(); }); + await new Promise((done) => setTimeout(done, 50)); + + expect(api.listTaskMessages).toHaveBeenCalledTimes(1); + unmount(); ws.disconnect(); qc.clear(); + }); }); diff --git a/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx b/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx index d9b291b30..c7d59a7f0 100644 --- a/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx +++ b/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx @@ -119,10 +119,11 @@ describe("useRealtimeSync — ws instance change", () => { // Should have called invalidateQueries for all workspace-scoped keys // (16 workspace-scoped [incl. property definitions] + 6 per-issue - // prefixes + the workspace working-agents projection + 5 per-chat + // prefixes + the workspace working-agents projection + 4 per-chat // prefixes + 1 workspaceKeys.list() + 1 cross-workspace inbox unread - // summary = 31 calls) - expect(invalidateSpy).toHaveBeenCalledTimes(31); + // summary = 30 calls). `task-messages` is not among them; the connect + // handler owns that one — see the per-chat-session test below. + expect(invalidateSpy).toHaveBeenCalledTimes(30); }); it("does not re-invalidate when rerendered with the same ws instance", () => { @@ -230,7 +231,14 @@ describe("useRealtimeSync — ws instance change", () => { expect(calls).toContainEqual(["chat", "messages"]); expect(calls).toContainEqual(["chat", "messages-page"]); expect(calls).toContainEqual(["chat", "pending-task"]); - expect(calls).toContainEqual(["task-messages"]); + // Not `task-messages`. Its repair has a single owner — the connect handler, + // which fires for the new instance's handshake and for every reconnect. + // Invalidating it here too made a reconnect issue the same unpaginated + // transcript request twice in a row (MUL-7227), and invalidation cannot do + // the job anyway when the read it distrusts is still in flight; the connect + // handler cancels first. Covered by + // use-realtime-sync-connect-backfill.test.tsx. + expect(calls).not.toContainEqual(["task-messages"]); }); it("invalidates per-chat-session caches after an established ws reconnects", () => { diff --git a/packages/core/realtime/use-realtime-sync.ts b/packages/core/realtime/use-realtime-sync.ts index 92d05af9c..b8771b3ef 100644 --- a/packages/core/realtime/use-realtime-sync.ts +++ b/packages/core/realtime/use-realtime-sync.ts @@ -687,7 +687,10 @@ function invalidateWorkspaceScopedQueries(qc: QueryClient): void { qc.invalidateQueries({ queryKey: chatKeys.messagesAll() }); qc.invalidateQueries({ queryKey: chatKeys.messagesPageAll() }); qc.invalidateQueries({ queryKey: chatKeys.pendingTaskAll() }); - qc.invalidateQueries({ queryKey: chatKeys.taskMessagesAll() }); + // `task-messages` is deliberately absent: the connect handler in + // `useRealtimeSync` owns that repair and runs on reconnects too, so + // repeating it here only issued the same unpaginated transcript request a + // second time (MUL-7227). // A chat:cancel_finalized broadcast missed while disconnected is exactly // what the durable draft-restore rows exist for (#5219) — re-pull them so // a mounted composer recovers the prompt without a remount. @@ -1746,20 +1749,37 @@ export function useRealtimeSync( }; }, [ws, qc, authStore, onToast]); - // Connect -> repair timelines fetched before this client was subscribed. + // Connect -> repair timelines read before this client was subscribed. // - // `taskMessagesOptions` holds `staleTime: Infinity` and is kept current by - // `task:message` frames, so a row persisted between the HTTP read and the - // socket coming up is in neither: the response was snapshotted before it - // existed, and its broadcast went out while nobody here was listening. - // Removing the focus refetch (MUL-7227) took away the accident that used to - // paper over this, so it is repaired where it happens instead — once per - // connection, and only for the caches whose sole live source is the socket, - // rather than on every window focus. + // `taskMessagesOptions` holds `staleTime: Infinity` and is otherwise kept + // current by `task:message` frames, so a row persisted between the HTTP read + // and the socket coming up is in neither: the response was snapshotted + // before it existed, and its broadcast went out while nobody here was + // listening. Removing the focus refetch (MUL-7227) took away the accident + // that used to paper over that, so it is repaired where it happens instead. + // + // Cancel first, and do not settle for invalidating. Invalidation is a no-op + // against a fetch that is already in flight — which is precisely the read + // this is here to distrust, since it started before the socket was live. + // Cancelling ends it and lets the refetch below start a read that cannot + // predate the connection. The request already on the wire is spent either + // way; what this decides is whether its answer is allowed to be the one we + // keep. + // + // This is the only owner of that repair. `onConnect` fires for reconnects + // too, so `invalidateWorkspaceScopedQueries` deliberately leaves + // `task-messages` alone — two owners meant a reconnect issued the same + // request twice, one behind the other, which React Query has no reason to + // dedupe. useEffect(() => { if (!ws) return; return ws.onConnect(() => { - qc.invalidateQueries({ queryKey: chatKeys.taskMessagesAll() }); + void qc + .cancelQueries({ queryKey: chatKeys.taskMessagesAll() }) + .then(() => qc.refetchQueries({ queryKey: chatKeys.taskMessagesAll() })) + .catch(() => { + // a repair that could not run leaves the next mount to fetch + }); }); }, [ws, qc]); diff --git a/packages/views/common/task-transcript/agent-transcript-dialog.tsx b/packages/views/common/task-transcript/agent-transcript-dialog.tsx index e3ab5f81e..0585e6511 100644 --- a/packages/views/common/task-transcript/agent-transcript-dialog.tsx +++ b/packages/views/common/task-transcript/agent-transcript-dialog.tsx @@ -62,7 +62,7 @@ import { FOLLOW_EDGE_THRESHOLD, LINE_SCROLL_PX, } from "./transcript-follow"; -import { isOutputTruncated, type TimelineItem } from "./build-timeline"; +import { isOutputTruncated, redactTimelineItem, type TimelineItem } from "./build-timeline"; import { buildLanes, buildSteps, @@ -1757,12 +1757,18 @@ const DISPLAY_CLIP_CHARS = 8000; export function StepBody({ item }: { item: TimelineItem }) { const { t } = useT("agents"); - const detail = useMemo(() => traceEventDetail(item), [item]); - const image = useMemo(() => readImageResult(item.output), [item.output]); + // Redacted here, not by the caller, so this component is correct on its own + // — it already did this for `content` and `output`, and a tool argument is + // no less on screen than a tool result. It has to happen before + // `traceEventDetail`, which splits an edit's arguments into diff lines: a + // rule spanning lines, like a PEM block, cannot be matched after that split. + const safe = useMemo(() => redactTimelineItem(item), [item]); + const detail = useMemo(() => traceEventDetail(safe), [safe]); + const image = useMemo(() => readImageResult(safe.output), [safe.output]); // Stated where the output actually ends, for a reader who has just reached // the bottom and is wondering whether that was all of it. A header badge said // the same thing louder, before anyone had asked the question. - const note = isOutputTruncated(item) ? t(($) => $.transcript.output_truncated_note) : undefined; + const note = isOutputTruncated(safe) ? t(($) => $.transcript.output_truncated_note) : undefined; // A screenshot is a picture, not a 200KB base64 string in a 
.
   if (image) {
@@ -1795,17 +1801,16 @@ export function StepBody({ item }: { item: TimelineItem }) {
       // — under a note that already reports the same loss. Tool input has no
       // server-side budget and keeps the clip at its existing length; nothing
       // about how long an input renders is this change's business.
-      const clip = item.type === "tool_result" ? null : DISPLAY_CLIP_CHARS;
-      // Redact the whole body, then clip it. The other order drops the tail a
-      // pattern needs to match, so a secret straddling the clip renders its
-      // head (MUL-7227 review) — and the clip length is a display choice that
-      // must not decide what stays hidden.
-      const safe = redactSecrets(text);
+      const clip = safe.type === "tool_result" ? null : DISPLAY_CLIP_CHARS;
+      // `text` is already redacted: it comes from `detail`, which was built
+      // from the redacted record above. That order is the point — clipping
+      // first would drop the tail a rule needs to match and render its head,
+      // letting a display budget decide what stays hidden (MUL-7227 review).
       const clipped =
-        clip !== null && safe.length > clip
-          ? `${safe.slice(0, clip)}\n${t(($) => $.transcript.display_clipped)}`
-          : safe;
-      const path = item.type === "tool_use" ? readPathFromInput(item.input) : undefined;
+        clip !== null && text.length > clip
+          ? `${text.slice(0, clip)}\n${t(($) => $.transcript.display_clipped)}`
+          : text;
+      const path = safe.type === "tool_use" ? readPathFromInput(safe.input) : undefined;
       return (
          {
     });
   });
 });
+
+describe("redacting what a row renders", () => {
+  const secret = "AKIA1234567890ABCDEF";
+  const call = (seq: number): TraceCallStep => ({
+    kind: "call", seq, tool: "Read",
+    call: { seq, type: "tool_use", tool: "Read", input: { file_path: `/src/${seq}.ts` } },
+    result: { seq: seq + 1, type: "tool_result", tool: "Read", output: `key = ${secret}` },
+  });
+
+  it("leaves a fold's members alone, so a thousand hidden calls cost nothing", () => {
+    // A group shows only its tool name and a count until it is opened, and it
+    // can hold any number of calls. Redacting members here would put the whole
+    // fold back on the live-run flush path — the bound this exists to keep.
+    const group = groupSteps(Array.from({ length: 500 }, (_, i) => call(i * 2)))[0]!;
+    expect(group.kind).toBe("group");
+
+    expect(redactTraceRow(group)).toBe(group);
+  });
+
+  it("redacts a member once it is revealed", () => {
+    const step = redactTraceStep(call(0)) as TraceCallStep;
+
+    expect(step.result?.output).not.toContain(secret);
+    expect(step.result?.output).toContain("[REDACTED AWS KEY]");
+  });
+
+  it("redacts a standalone call's arguments as well as its result", () => {
+    const raw = call(0);
+    raw.call!.input = { command: `deploy --key ${secret}` };
+
+    const step = redactTraceStep(raw) as TraceCallStep;
+
+    expect(JSON.stringify(step.call?.input)).not.toContain(secret);
+  });
+});
diff --git a/packages/views/common/task-transcript/build-steps.ts b/packages/views/common/task-transcript/build-steps.ts
index 47afd98b1..e99b3aa49 100644
--- a/packages/views/common/task-transcript/build-steps.ts
+++ b/packages/views/common/task-transcript/build-steps.ts
@@ -193,21 +193,28 @@ export function groupSteps(steps: TraceStep[]): TraceRow[] {
 }
 
 /**
- * Redact every body this row can put on screen.
+ * Redact every value this row can put on screen by itself.
  *
  * Applied to the rows actually being rendered rather than to the whole
  * transcript: scanning every message on each live flush is what made the
  * client stall (MUL-7227), and the rows on screen are a bounded slice of it.
  * What must not change is the order — a row is redacted whole, before its
- * summary is cut to 200 characters and before `StepBody` reaches its display
- * clip, because a secret straddling either cut cannot be matched afterwards.
+ * summary is cut to 120 characters and before `StepBody` reaches its display
+ * clip, because a value straddling either cut cannot be matched afterwards.
+ *
+ * A group is deliberately left alone. It folds an unbounded number of calls
+ * behind one line that shows only the tool name and a count, so redacting its
+ * members here would put the whole fold back on the hot path and undo the
+ * bound this function exists to keep — a thousand folded reads is one visible
+ * row. Members are redacted where they are revealed, against the slice that is
+ * actually rendered.
  */
 export function redactTraceRow(row: TraceRow): TraceRow {
-  if (isGroupRow(row)) return { ...row, steps: row.steps.map(redactCallStep) };
+  if (isGroupRow(row)) return row;
   return redactTraceStep(row);
 }
 
-/** `redactTraceRow` for a single step. */
+/** `redactTraceRow` for a single step — including a revealed group member. */
 export function redactTraceStep(step: TraceStep): TraceStep {
   if (step.kind === "call") return redactCallStep(step);
   return { ...step, item: redactTimelineItem(step.item) };
diff --git a/packages/views/common/task-transcript/build-timeline.ts b/packages/views/common/task-transcript/build-timeline.ts
index f05bee208..ced9fe4ca 100644
--- a/packages/views/common/task-transcript/build-timeline.ts
+++ b/packages/views/common/task-transcript/build-timeline.ts
@@ -62,23 +62,63 @@ export function redactTimelineItems(items: TimelineItem[]): TimelineItem[] {
 }
 
 /**
- * Redact one record's bodies.
+ * Redact everything one record can put on screen.
  *
- * Whole bodies, before anything downstream clips or summarizes them. Every
- * consumer of a body cuts first and redacts second — `StepBody` at its display
- * clip, the step summaries at 200 characters — so a pattern that spans the cut
- * loses the tail it needs to match and the head renders. Redacting the record
- * first is what made that ordering harmless, and it stays harmless as long as
- * a raw record never reaches a renderer.
+ * Whole values, before anything downstream clips, summarizes, splits or
+ * highlights them. Every consumer cuts first and redacts second — `StepBody`
+ * at its display clip, the argument summary at 120 characters, the diff
+ * surface line by line — and a pattern only matches while both of its ends are
+ * present. Redacting the record first is what makes all of that harmless, and
+ * it only stays harmless while no raw record reaches a renderer.
+ *
+ * `input` is included, and it is not decoration: a tool's arguments are
+ * displayed as prominently as its output — as the row summary, and as the diff
+ * body for an edit, which is built by splitting `old_string` / `new_string`
+ * into lines. Redacting after that split can never match a rule spanning lines,
+ * such as a PEM block; redacting the source before it is split does.
  */
 export function redactTimelineItem(item: TimelineItem): TimelineItem {
   return {
     ...item,
     content: item.content ? redactSecrets(item.content) : item.content,
     output: item.output ? redactSecrets(item.output) : item.output,
+    input: item.input ? (redactUnknown(item.input) as Record) : item.input,
   };
 }
 
+/**
+ * Redact the strings inside an arbitrary tool-argument value.
+ *
+ * Tool inputs are whatever JSON the agent sent, so the shape is not ours to
+ * assume: `patch_apply` nests per-file bodies, `Edit` keeps them flat. Only
+ * strings are rewritten, and objects are rebuilt only when something in them
+ * changed, so an unaffected input keeps its identity and the memos downstream
+ * keep their hits.
+ */
+function redactUnknown(value: unknown): unknown {
+  if (typeof value === "string") return redactSecrets(value);
+  if (Array.isArray(value)) {
+    let changed = false;
+    const next = value.map((entry) => {
+      const redacted = redactUnknown(entry);
+      if (redacted !== entry) changed = true;
+      return redacted;
+    });
+    return changed ? next : value;
+  }
+  if (value && typeof value === "object") {
+    let changed = false;
+    const next: Record = {};
+    for (const [key, entry] of Object.entries(value)) {
+      const redacted = redactUnknown(entry);
+      if (redacted !== entry) changed = true;
+      next[key] = redacted;
+    }
+    return changed ? next : value;
+  }
+  return value;
+}
+
 /**
  * Whether this record's stored output is known to have dropped bytes.
  *
diff --git a/packages/views/common/task-transcript/redaction-ordering.test.tsx b/packages/views/common/task-transcript/redaction-ordering.test.tsx
index 43e2649a8..348e6682f 100644
--- a/packages/views/common/task-transcript/redaction-ordering.test.tsx
+++ b/packages/views/common/task-transcript/redaction-ordering.test.tsx
@@ -97,4 +97,40 @@ describe("redaction runs before every clip", () => {
     expect(view.container.querySelector(".hljs")).not.toBeNull();
     expect(view.container.textContent).not.toContain(fakeKey);
   });
+
+  it("hides a secret in a tool argument past the 120-character summary cut", async () => {
+    // Tool input reaches the screen as the row summary and, for an edit, as the
+    // diff body. Nothing upstream redacts it, so the record itself has to.
+    const client = new QueryClient({ defaultOptions: { queries: { retry: false } } });
+    vi.mocked(api.listTaskMessages).mockResolvedValue([
+      { task_id: id, issue_id: "issue", seq: 1, type: "tool_use", tool: "exec_command",
+        // Positioned so the 120-character cut lands inside the key: clipping
+        // first leaves its head on screen with nothing left to match against.
+        input: { command: "deploy --key " + "x".repeat(96) + " " + fakeKey } },
+    ]);
+    const view = renderWithI18n(
+      
+    );
+
+    await screen.findByText(/deploy --key/);
+    expect(view.container.textContent).not.toContain("AKIA123456");
+    expect(view.container.querySelector("[title]")?.getAttribute("title")).not.toContain("AKIA123456");
+    client.clear();
+  });
+
+  it("hides a PEM spanning several lines of an edit diff", () => {
+    // The PEM rule needs BEGIN and END together, so redacting the diff line by
+    // line can never match it. Redacting the argument before it is split into
+    // lines can.
+    const material = "QUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVo=";
+    const view = renderWithI18n();
+
+    expect(view.container.textContent).not.toContain(material);
+  });
 });
diff --git a/packages/views/issues/components/inline-comment-run.tsx b/packages/views/issues/components/inline-comment-run.tsx
index 7eb1c5a1c..39451a423 100644
--- a/packages/views/issues/components/inline-comment-run.tsx
+++ b/packages/views/issues/components/inline-comment-run.tsx
@@ -257,7 +257,10 @@ function InlineStep({ row, live, formatText }: { row: TraceRow; live: boolean; f
       {grouped ? <>
         {row.steps.length > limit && }
-        {row.steps.slice(-limit).map((step) => )}
+        {/* Redacted here rather than with the group row: a fold can hold any
+            number of calls and shows none of their bodies until it is opened,
+            so this slice is the first bounded set (MUL-7227). */}
+        {row.steps.slice(-limit).map((step) => )}
        : call ? <>
         {row.call && }
         {row.result && }