fix(transcript): redact tool arguments, and make the connect repair a barrier (MUL-7227)

Three findings from review of 096c92d8b. Tool input was never redacted, anywhere, and the invariant the previous commit claimed — that a renderer never sees a raw record — was false because of it. Arguments are as much on screen as results: they are the row summary, and for an edit they are the diff body. `traceToolArgSummary` cut a command to 120 characters before the caller redacted, so a key on that boundary rendered its head; and the diff surface redacted line by line, which cannot match a rule spanning lines such as a PEM block, so key material reached the highlighter and the screen. `redactTimelineItem` now walks `input` as well, before anything splits or summarizes it, and `StepBody` redacts the record it renders rather than trusting its caller — it already did that for `content` and `output`. A group row is not bounded. It folds an unlimited number of calls behind one line showing a tool name and a count, and `redactTraceRow` was redacting all of them before anything asked whether the fold was even open: a thousand folded reads is one visible row and was ~19ms of scanning per flush, back in the range this branch exists to remove. Groups are left alone and their members are redacted against the slice that is actually revealed. Measured with 1000 folded 8KiB reads: ~0ms. The mixed case, now that arguments are redacted too, is 40.4ms per flush before this branch and 1.3ms after, at 3000 messages. The connect repair did not survive the timing it was written for. Invalidation does nothing to a fetch already in flight — and that is exactly the read it exists to distrust, the one that started before the socket was live. A transcript GET outstanding at the handshake kept its stale answer, and the observed cache went to `undefined` rather than refetching. It cancels first now, so the refetch cannot predate the connection. `task-messages` also had two owners: the reconnect path invalidated it and the connect handler invalidated it again, sequentially, so a reconnect issued the same unpaginated transcript request twice with nothing to dedupe them. `invalidateWorkspaceScopedQueries` drops it; the connect handler fires on reconnects too and is now the only owner. Six more regression tests, each verified to fail without its fix, including both first-handshake orders and reconnect reading once. Verified: pnpm typecheck, pnpm lint (0 errors), pnpm test — core 1726, views 5000, desktop 626, web 266, docs 15. Reported-by: Elon Co-authored-by: multica-agent <[email protected]>
2026-11-09 09:56:47 +08:00 · 2026-09-10 16:02:23 +08:00
parent 096c92d8bb
commit 1f9808dd00
9 changed files with 261 additions and 43 deletions
@@ -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 }) => <QueryClientProvider client={qc}>{children}</QueryClientProvider>;
+
+    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 }) => <QueryClientProvider client={qc}>{children}</QueryClientProvider>;
+
+    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();
+  });
 });
@@ -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", () => {
@@ -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]);

@@ -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 <pre>.
  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 (
        <ToolDetailSurface
          text={clipped}
@@ -5,6 +5,8 @@ import {
  buildSteps,
  groupSteps,
  laneSegmentPosition,
+  redactTraceRow,
+  redactTraceStep,
  rowCalls,
  timelineTicks,
  toolKindTotals,
@@ -303,3 +305,38 @@ describe("toolKindTotals", () => {
    });
  });
 });
+
+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);
+  });
+});
@@ -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) };
@@ -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<string, unknown>) : 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<string, unknown> = {};
+    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.
 *
@@ -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(<QueryClientProvider client={client}>
+      <InlineCommentRun run={{ task, commentId: "comment", hasReply: false }} />
+    </QueryClientProvider>);
+
+    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(<StepBody item={item([
+      { task_id: id, issue_id: "issue", seq: 1, type: "tool_use", tool: "Edit", input: {
+        file_path: "config.ts",
+        old_string: "const key = '';",
+        new_string: `const key = \`-----BEGIN PRIVATE KEY-----\n${material}\n-----END PRIVATE KEY-----\`;`,
+      } },
+    ])} />);
+
+    expect(view.container.textContent).not.toContain(material);
+  });
 });
@@ -257,7 +257,10 @@ function InlineStep({ row, live, formatText }: { row: TraceRow; live: boolean; f
      {grouped ? <>
        {row.steps.length > limit && <button type="button" className="py-1 text-muted-foreground" onClick={() => setLimit((value) => value + 12)}>
          {t(($) => $.inline_run.show_earlier, { count: row.steps.length - limit })}</button>}
-        {row.steps.slice(-limit).map((step) => <InlineStep key={step.seq} row={step} live={live} formatText={formatText} />)}
+        {/* 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) => <InlineStep key={step.seq} row={redactTraceStep(step)} live={live} formatText={formatText} />)}
      </> : call ? <>
        {row.call && <StepBody item={row.call} />}
        {row.result && <StepBody item={row.result} />}