From a2f4294a5d2a8a02460cc64dc9c11ba9764aa252 Mon Sep 17 00:00:00 2001 From: probe Date: Fri, 23 Oct 2026 16:21:22 +0800 Subject: [PATCH 1/2] fix(webui): withhold the body of a diff entry outside the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A turn's diff can name a file outside the workspace. The runtime's capture layer records an out-of-workspace write as an absolute path, so the entry reaches the client with that file's content attached — and the client rendered it: the transcript's diff card, the workspace turn-review panel and the workspace review page each put the patch text on screen. The fix that stopped the collection at the source was made upstream in `packages/local-runtime/src/turns/file-changes.ts` and reverted in PR #240, because it changed a package this fork may not change. The boundary is therefore drawn on the client side, which is the last point before the content reaches a screen. A diff's file entries are now projected through that boundary before they reach a surface. An entry whose path resolves outside the workspace keeps its name, its status and its line counts, and loses its body: the diff text, the patch hunks, the file's content. The surface says what is withheld instead of showing it. The entry itself stays in the list — "which file was touched" is the part a person can still act on. The comparison is lexical, on posix-style strings, the same normalization the message file-reference reader uses. An absolute path is compared with the root segment by segment, so a sibling directory that shares the root's name is outside; a relative path is refused when it climbs out with `..`. Two limits are stated rather than papered over, and ADR 0024 and the tests say so out loud: * A symlink inside the workspace that points outside it stays contained here. The entry's path is a relative one and the browser has no filesystem to resolve it against; telling the two apart needs `realpath` on the runtime side. * An unknown workspace root is not a boundary. The entries are shown as they are, because a check that hid everything it could not place would hide ordinary content too. The upstream half of this is still open: nothing here stops the runtime from sending an out-of-workspace file's patch to the browser, only from the browser showing it. The client stops the content from being displayed; only the upstream change stops it from being collected. --- packages/webui/docs/README.md | 5 +- ...-the-workspace-boundary-for-a-diff-body.md | 76 ++++++ .../webui/docs/architecture/module-map.md | 2 +- packages/webui/docs/known-gaps.md | 26 +++ .../review/ui/ReviewSettingsPage.public.tsx | 23 +- .../features/transcript/ui/AssistantBody.tsx | 1 + .../features/transcript/ui/DiffCard.tsx | 21 +- .../features/transcript/ui/MessageItem.tsx | 3 +- .../features/workspace/ui/WorkspacePanels.tsx | 28 ++- .../common/diff-workspace-boundary.ts | 169 ++++++++++++++ .../gates/fixtures/path-metadata-f00.json | 9 + .../unit/diff-workspace-boundary.test.tsx | 219 ++++++++++++++++++ .../unit/webui-round3-acceptance.test.tsx | 5 +- release/public-source.json | 3 + test/vitest-suites.json | 1 + 15 files changed, 571 insertions(+), 20 deletions(-) create mode 100644 packages/webui/docs/adr/0024-the-client-draws-the-workspace-boundary-for-a-diff-body.md create mode 100644 packages/webui/src/client/projection/common/diff-workspace-boundary.ts create mode 100644 packages/webui/test/unit/diff-workspace-boundary.test.tsx diff --git a/packages/webui/docs/README.md b/packages/webui/docs/README.md index 68f15dcb3..8088a97ed 100644 --- a/packages/webui/docs/README.md +++ b/packages/webui/docs/README.md @@ -87,7 +87,10 @@ a browser tab strip and the two selections it separates, and amends ADR 0019 wit them they are the decision record for that feature. ADR 0020 records the accepted Mini App view, the runtime seams it needs, the desktop capabilities it deliberately drops, and the contract-baseline question it leaves open; it is the sole decision record for Mini App hosting. ADR 0021 records the model picker as one popup -with two states, and the cascade it replaces. A new decision takes the next free number. +with two states, and the cascade it replaces. ADR 0024 records the client-side workspace boundary +for a diff body: a diff entry outside the workspace keeps its name and its counts and loses its +content, the boundary is lexical, and a diff whose workspace root the client does not know is not +withheld. A new decision takes the next free number. | Retired | Was | Why it is not in the log | Where the live facts are | | --- | --- | --- | --- | diff --git a/packages/webui/docs/adr/0024-the-client-draws-the-workspace-boundary-for-a-diff-body.md b/packages/webui/docs/adr/0024-the-client-draws-the-workspace-boundary-for-a-diff-body.md new file mode 100644 index 000000000..ebaaea829 --- /dev/null +++ b/packages/webui/docs/adr/0024-the-client-draws-the-workspace-boundary-for-a-diff-body.md @@ -0,0 +1,76 @@ +# The client draws the workspace boundary for a diff body + +**Status:** Accepted and implemented. +**Date:** 2026-10-23. + +A turn's diff lists the files a turn touched, and one of those files can be **outside the +workspace**. The runtime's capture layer records an out-of-workspace write as an absolute path, so +the entry reaches the client with that file's content attached, and the client used to render it: +the diff card's own review panel and the workspace review panel both put the patch text on screen. + +The **WebUI client** now draws that boundary. A diff entry whose path resolves outside the +workspace root keeps its name, its status and its added/deleted line counts, and loses its body — +the diff text, the patch hunks, the file's content. The entry itself stays in the list, and the +surface says what is withheld instead of showing it. + +## Why the boundary is drawn here + +The fix that stopped the collection at the source was made upstream in +`packages/local-runtime/src/turns/file-changes.ts`, and it was **reverted in PR #240** because it +changed a package this fork may not change: the sixteen upstream packages are outside the boundary +of this branch, and a fix that reaches into one of them cannot be merged here. The upstream fix was +therefore not wrong — it was in the wrong layer for this repository. + +So the boundary is drawn on the client side, which is where the content becomes visible. The client +is the last point before the content reaches a screen, and it is the only layer this branch may +change. + +This is a **mitigation with a stated scope**, not a replacement for the upstream fix. The upstream +fix also stopped the content from being *collected*; the client-side boundary stops it from being +*shown*. The content still travels between the runtime and the browser for an out-of-workspace +file. Closing that gap properly needs the upstream change. + +## The rule is lexical + +The browser has no `path` module and no filesystem, so the comparison is made on posix-style +strings, the same normalization the message file-reference reader uses +(`client/features/transcript/projection/message-file-reference.ts`), extended to resolve `..` and +to compare absolute paths segment by segment. + +- A **relative** path is under the workspace root by construction, and is refused when it climbs + out of it (`../outside.txt`, `src/../../outside.txt`). +- An **absolute** path is compared with the root segment by segment, so a sibling directory whose + name starts with the root's name (`/work/project-backup` against the root `/work/project`) is + outside. A string prefix would have called it inside. +- The comparison is **case-sensitive except for a Windows drive letter**, and it accepts both + separator spellings, because the runtime writes the path in the host's spelling. + +## An unknown root is not a boundary + +A diff entry is only withheld when the client **knows** the workspace root. When the root is +missing — a session with no workspace, a surface that has none to give — the entries are shown as +they are. + +This is deliberate. A check that hid everything it could not place would hide ordinary content as +well, and the failure mode of "the diff shows nothing" is worse than the one it prevents: it is +silent, it looks like a bug, and it applies to the files a person actually worked on. The check is +therefore allowed to say "inside" only when it can compare, and the surfaces that have no root keep +the previous behaviour. + +The same rule is why the boundary is applied by a **projection** and not by the components: the +projection is handed the root, decides once, and hands the entries on. A component that renders +the entries it is given cannot leak a body it was never given. + +## What this decision does not settle + +- **A symlink inside the workspace that points outside it.** The entry's path is a relative one, so + it stays contained here, and the browser cannot see what it resolves to. Telling the two apart + needs `realpath` on every target, with careful handling of files that do not exist yet, on the + runtime side — the same limit the upstream fix states. The test suite says this out loud rather + than leaving it to be discovered. +- **The content still travels.** Nothing here stops the runtime from sending an out-of-workspace + file's patch to the browser; it stops the browser from showing it. Only the upstream fix stops + the collection. +- **The session-diff operation.** The client can ask the runtime for a whole session's diff + (`getSessionDiff`), and no surface consumes that answer today. When one does, it goes through the + same projection. diff --git a/packages/webui/docs/architecture/module-map.md b/packages/webui/docs/architecture/module-map.md index d5f674c1d..8b34c8015 100644 --- a/packages/webui/docs/architecture/module-map.md +++ b/packages/webui/docs/architecture/module-map.md @@ -14,7 +14,7 @@ where code lives. Locations, not command ownership. The businesses are the direc | `client/conversation/` | The conversation substrate's mechanisms: `stream-lease.ts`, `stream-loop.ts`, `request-freshness.ts`, `session-scope.ts`, `transcript-history.ts`, `transcript-request-ownership.ts`, `stream-instrumentation.ts` | Called by application owners, not by components. | | `client/platform/` | Browser IO: `transport.ts`, `event-channel.ts`, `storage.ts`, `wire.ts`, `connection-health.ts`, `browser-dom.ts`, `team-mode.ts`, `no-project.ts`, `session-import.ts`, `session-transfer-{target,download}.ts` | Adapters only; they decide no business rule. | | `client/bindings/` | React subscriptions and browser effects: `application-context.tsx`, `browser-capabilities.tsx`, `browser-effects.ts`, `composer-store.ts`, `event-channel-adapter.ts`, `navigation.ts`, `port-slices-context.tsx`, `use-connection-health.ts`, `use-query-state.tsx`, `use-session-state.ts`, `version-cells.ts` | Components read snapshots and invoke named commands here; they receive neither a store writer nor a sink factory. | -| `client/projection/common/` | Pure derivations no single business owns: `router.ts`, `event-kinds.ts`, `event-parsers.ts`, `context-menu.ts`, `flyout-position.ts`, `overlay-geometry.ts`, `outside-close.ts`, `paths.ts`, `unread-badge.ts`, `goal-message.ts` | No React, no transport, no browser global. Business-owned projections live in `client/features//projection/`. | +| `client/projection/common/` | Pure derivations no single business owns: `router.ts`, `event-kinds.ts`, `event-parsers.ts`, `context-menu.ts`, `diff-workspace-boundary.ts`, `flyout-position.ts`, `overlay-geometry.ts`, `outside-close.ts`, `paths.ts`, `unread-badge.ts`, `goal-message.ts` | No React, no transport, no browser global. Business-owned projections live in `client/features//projection/`. | | `client/shell/` | Page composition and the shell frame: `WebuiClientFoundationApp.tsx`, `ArchonPage.tsx`, `ArchonShell.tsx`, `ConversationSurface.tsx`, `ConnectionStatus.tsx`, `StartupFallback.tsx`, `WebuiErrorBoundary.tsx`, `WebuiZoneBoundary.tsx`, `NotFound.tsx`, `icons.tsx`, and the `use-*` shell hooks | Compose and present; create no owners, write no application state. | | `client/ui/` | Presentation primitives shared across businesses: `ContextMenu.tsx`, `ToggleSwitch.tsx`, `activity-phrase.ts`, `settings-glyphs.tsx`, `settings-layout.tsx` (the settings page skeleton: page, section, card, row, divider) | Reaches no business at all. | | `client/styles/` | `index.css` (the single compiled entry), `tokens.css`, `shell.css` | See `../design/design-language.md`. | diff --git a/packages/webui/docs/known-gaps.md b/packages/webui/docs/known-gaps.md index a92811591..8cf04fd46 100644 --- a/packages/webui/docs/known-gaps.md +++ b/packages/webui/docs/known-gaps.md @@ -134,6 +134,32 @@ cells, while the Mini App gate compares the raw kind string inside its own servi change to this client's row can reach. The accepted shape moves the Mini App service into the policy as a wiring cell this client's row carries. +## 10. A symlink out of the workspace is not detectable by the diff boundary + +**Implemented, with a stated limit.** A turn's diff can name a file outside the workspace, and the +client now withholds that file's body: the entry keeps its name, its status and its line counts, +and the surface says the content is not shown. The decision and its reasoning are in +[ADR 0024](adr/0024-the-client-draws-the-workspace-boundary-for-a-diff-body.md). + +What the boundary cannot do is tell a **symlink inside the workspace that points outside it** from +an ordinary file. The entry's path is a relative one, so it stays contained, and the browser has no +filesystem to resolve it against. Closing that needs `realpath` on every target, with careful +handling of files that do not exist yet, on the runtime side — which is also where the reverted +upstream fix would have had to live. The unit test says so out loud rather than leaving it to be +discovered. + +Two further limits are stated rather than hidden, and both are deliberate: + +- The comparison is **lexical**. It reads the path as written; it does not resolve it. +- A diff entry is only withheld when the client **knows** the workspace root. Without one the + entries are shown as they are, because a check that hid everything it could not place would hide + ordinary content too. + +The upstream half of this is still open: the fix that stopped the runtime from *collecting* an +out-of-workspace file's content was reverted in PR #240 because it changed a package this fork may +not change, so the content still travels between the runtime and the browser. The client stops it +from being shown; only the upstream change stops it from being collected. + ## Reported as missing, and actually correct - **The thirteen runtime-owner cells are not read by this package.** That is expected: they are diff --git a/packages/webui/src/client/features/review/ui/ReviewSettingsPage.public.tsx b/packages/webui/src/client/features/review/ui/ReviewSettingsPage.public.tsx index 118bcfdfc..6ea4f4edd 100644 --- a/packages/webui/src/client/features/review/ui/ReviewSettingsPage.public.tsx +++ b/packages/webui/src/client/features/review/ui/ReviewSettingsPage.public.tsx @@ -29,6 +29,10 @@ import { type WebuiWorktreeSourceSession, type WebuiWorktreeWorkspace, } from "../../sessions/projection/worktree-state.public.js"; +import { + isWebuiDiffPathWithinWorkspace, + WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE, +} from "../../../projection/common/diff-workspace-boundary.js"; import { selectWebuiCatalogFlatPage } from "../../sessions/application/session-catalog.public.js"; import { useWebuiSessionStoreContext } from "../../../bindings/application-context.js"; import { @@ -164,6 +168,7 @@ export function ReviewSettingsPage({ workspaceDir, onOpenFileLine }: { state={state} visible={visible} filtering={filtering} + workspaceDir={effectiveWorkspaceDir} onQueryChange={(query) => { dispatch({ type: "query-changed", query }); // Live search: the owner is asked for every keystroke, and the reducer @@ -174,10 +179,11 @@ export function ReviewSettingsPage({ workspaceDir, onOpenFileLine }: { onClearFilters={() => dispatch({ type: "clear-filters" })} onToggleFile={(fileId, willExpand, file) => { dispatch({ type: "toggle-file", fileId }); - if (willExpand && !state.diffs[fileId] && !state.diffErrors[fileId] && state.reviewSnapshotId) { + /* A file outside the workspace is not asked for its patch at all: the + * content is not going to be shown, so it does not have to travel. */ + if (willExpand && !state.diffs[fileId] && !state.diffErrors[fileId] && state.reviewSnapshotId && isWebuiDiffPathWithinWorkspace(effectiveWorkspaceDir, file.path)) { void loadDiffs([fileId], state.reviewSnapshotId); } - void file; }} onOpenFileLine={onOpenFileLine} />; @@ -278,10 +284,14 @@ export function WebuiReviewPanel({ state, visible, filtering, loading, note, emp
    {rows.map((file) => { const expanded = state.expandedFileIds.includes(file.fileId); - const diff = state.diffs[file.fileId]; - const diffError = state.diffErrors[file.fileId]; - const pending = state.loadingFileIds.includes(file.fileId); - return
  • + /* The workspace boundary decides whether the file's patch is shown. + * The entry stays in the list either way — the name and the counts + * are what a person can still act on. */ + const withinWorkspace = isWebuiDiffPathWithinWorkspace(workspaceDir, file.path); + const diff = withinWorkspace ? state.diffs[file.fileId] : undefined; + const diffError = withinWorkspace ? state.diffErrors[file.fileId] : undefined; + const pending = withinWorkspace && state.loadingFileIds.includes(file.fileId); + return
  • {expanded ?
    + {!withinWorkspace ?

    {WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE}

    : null} {pending ?

    正在读取这个文件的补丁…

    : null} {diffError ?

    {diffError}

    : null} {diff ?
      diff --git a/packages/webui/src/client/features/transcript/ui/AssistantBody.tsx b/packages/webui/src/client/features/transcript/ui/AssistantBody.tsx index 492376247..d848281d8 100644 --- a/packages/webui/src/client/features/transcript/ui/AssistantBody.tsx +++ b/packages/webui/src/client/features/transcript/ui/AssistantBody.tsx @@ -351,6 +351,7 @@ export function WebuiAssistantBody({ * edited-files summary sits at the end of the message. */} sum + file.additions, 0); const totalDeleted = files.reduce((sum, file) => sum + file.deletions, 0); @@ -127,9 +138,9 @@ export function WebuiDiffCard({
      {shown.map((file) => ( -
    • +
    • - + {`+${file.additions}`} {file.deletions > 0 ? {`-${file.deletions}`} : null} @@ -147,7 +158,9 @@ export function WebuiDiffCard({ {files.map((file) => (
      {file.file} - {file.diff ?
      {file.diff}
      : file.patch ?
      {JSON.stringify(file.patch, null, 2)}
      :

      当前运行时没有提供该文件的 patch 预览。

      } + {file.withinWorkspace + ? file.diff ?
      {file.diff}
      : file.patch ?
      {JSON.stringify(file.patch, null, 2)}
      :

      当前运行时没有提供该文件的 patch 预览。

      + :

      {WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE}

      }
      ))} diff --git a/packages/webui/src/client/features/transcript/ui/MessageItem.tsx b/packages/webui/src/client/features/transcript/ui/MessageItem.tsx index 58bd363c6..4cd0d30a3 100644 --- a/packages/webui/src/client/features/transcript/ui/MessageItem.tsx +++ b/packages/webui/src/client/features/transcript/ui/MessageItem.tsx @@ -19,6 +19,7 @@ import type { } from "../../../../shared/contracts/session.js"; import type { WebuiTranscriptCommands } from "../contracts/session-read-port.js"; import type { WebuiMessageFileReference } from "../projection/message-file-reference.js"; +import { projectWebuiDiffFiles } from "../../../projection/common/diff-workspace-boundary.js"; import type { WorkspacePanelCommand } from "../../workspace/contracts/panel-command.js"; import type { WebuiHistoricalTurnView, @@ -261,7 +262,7 @@ export function MessageItem({ onOpenFile={(reference) => { if (sessionId && workspaceDir) onOpenFile?.({ sessionId, workspaceDir, reference }); }} onOpenTurnReview={(review, selectedPath) => { if (!sessionId || !workspaceDir) return; - onOpenTurnReview?.({ type: "open-turn-review", sessionId, workspaceDir, messageId, assistantMessageId: review.sourceMessageId ?? assistantMessageId ?? messageId, ...(turnId ? { turnId } : {}), ...(review.changeSetId ? { changeSetId: review.changeSetId } : {}), files: review.fileChanges, ...(selectedPath ? { selectedPath } : {}) }); + onOpenTurnReview?.({ type: "open-turn-review", sessionId, workspaceDir, messageId, assistantMessageId: review.sourceMessageId ?? assistantMessageId ?? messageId, ...(turnId ? { turnId } : {}), ...(review.changeSetId ? { changeSetId: review.changeSetId } : {}), files: projectWebuiDiffFiles(review.fileChanges, workspaceDir), ...(selectedPath ? { selectedPath } : {}) }); }} assistantMessageId={assistantMessageId ?? initialDiff?.sourceMessageId ?? messageId} turnId={turnId} diff --git a/packages/webui/src/client/features/workspace/ui/WorkspacePanels.tsx b/packages/webui/src/client/features/workspace/ui/WorkspacePanels.tsx index 5f29ba3b0..0ff9b16f1 100644 --- a/packages/webui/src/client/features/workspace/ui/WorkspacePanels.tsx +++ b/packages/webui/src/client/features/workspace/ui/WorkspacePanels.tsx @@ -30,7 +30,6 @@ import { WebuiMarkdown } from "../../transcript/ui/markdown.public.js"; // port. The strip subscribes to `watchBrowserWebTabs` and, for the active // web tab, this file mounts `BrowserPanelHost` with the tab's target. import { BrowserPanelHost } from "../../browser/ui/BrowserPanelHost.public.js"; -import type { WebuiFileDiffInfoView } from "../../../../shared/contracts/session.js"; import type { WebuiWorkspaceFile, WebuiWorkspaceFileContent, @@ -75,6 +74,12 @@ import { import { useWebuiBrowserCapabilities } from "../../../bindings/browser-capabilities.js"; import { WebuiWorkspaceLauncher, WebuiWorkspaceTabStrip } from "./WebuiWorkspaceTabStrip.public.js"; import { webuiFileLanguage, projectWebuiUnifiedDiffLines, isWorkspaceArchivePath, isWorkspaceHtmlPath } from "../projection/panel-projection.js"; +import { + projectWebuiDiffFile, + projectWebuiDiffFiles, + WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE, + type WebuiDiffFileEntry, +} from "../../../projection/common/diff-workspace-boundary.js"; // `mergeWorkspaceFileChildren`, `chunkWebuiWorkspaceReviewFileIds` and // `WEBUI_WORKSPACE_REVIEW_DIFF_BATCH_SIZE` are re-exported below: the panel // tests import them from this module, which stays the panel's public surface. @@ -124,8 +129,8 @@ function highlightFileLine(line: string, language: string | undefined): string | } } -export function WebuiDiffFileSection({ file, selected, loading = false, error, onSelect }: { readonly file: WebuiFileDiffInfoView; readonly selected: boolean; readonly loading?: boolean; readonly error?: string; readonly onSelect?: () => void }): ReactElement { - const diff = typeof file.diff === "string" ? file.diff : ""; +export function WebuiDiffFileSection({ file, selected, loading = false, error, onSelect }: { readonly file: WebuiDiffFileEntry; readonly selected: boolean; readonly loading?: boolean; readonly error?: string; readonly onSelect?: () => void }): ReactElement { + const diff = file.withinWorkspace && typeof file.diff === "string" ? file.diff : ""; const lines = projectWebuiUnifiedDiffLines(diff); const language = webuiFileLanguage(file.file); return
      @@ -136,7 +141,12 @@ export function WebuiDiffFileSection({ file, selected, loading = false, error, o +{file.additions} {file.deletions > 0 ? -{file.deletions} : null} - {loading ?

      正在加载差异…

      : error ?

      {error}

      : lines.length ?
      {lines.map((line, index) => {
      +    {!file.withinWorkspace
      +      /* The file is outside the workspace, so its content is not shown. The
      +       * entry stays in the list: the file name and the counts are the part a
      +       * person can still act on. */
      +      ? 

      {WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE}

      + : loading ?

      正在加载差异…

      : error ?

      {error}

      : lines.length ?
      {lines.map((line, index) => {
             const highlighted = line.kind === "hunk" || line.kind === "noNewline" ? undefined : highlightFileLine(line.content, language);
             return 
               {line.kind === "hunk" ? {line.content} : line.kind === "noNewline" ? line.content : <>
      @@ -362,8 +372,14 @@ export function WebuiWorkspacePanel({ state, dispatch, sessionId, workspaceDir,
           // review to the workspace's review has to load the summary it now needs.
         }, [activeTab?.id, activeTab?.content, activeTab?.content.kind === "review" && activeTab.content.source === "workspace" ? activeTab.content.workspaceDir : undefined, queries, queryState.reviewRevision]);
         const workspaceReviewSelectedPath = activeTab?.content.kind === "review" && activeTab.content.source === "workspace" ? activeTab.content.selectedPath : undefined;
      +  const workspaceReviewWorkspaceDir = activeTab?.content.kind === "review" && activeTab.content.source === "workspace" ? activeTab.content.workspaceDir : undefined;
         const workspaceReviewFiles = reviewSummary !== undefined && reviewSummary.tabId === activeTab?.id ? reviewSummary.summary?.files ?? [] : [];
      -  const turnReviewFiles = activeTab?.content.kind === "review" && activeTab.content.source === "turn" ? activeTab.content.files ?? [] : [];
      +  // The turn's files are projected through the workspace boundary before they
      +  // are rendered: a file outside the workspace keeps its name and its counts,
      +  // and loses the body that would have shown its content.
      +  const turnReviewFiles = activeTab?.content.kind === "review" && activeTab.content.source === "turn"
      +    ? projectWebuiDiffFiles(activeTab.content.files, activeTab.content.workspaceDir)
      +    : [];
         const selectedReviewPath = activeTab?.content.kind === "review" && activeTab.content.source === "workspace"
           ? workspaceReviewFiles.some((file) => file.path === workspaceReviewSelectedPath) ? workspaceReviewSelectedPath : workspaceReviewFiles[0]?.path
           : activeTab?.content.kind === "review" && activeTab.content.source === "turn" ? activeTab.content.selectedPath ?? turnReviewFiles[0]?.file : undefined;
      @@ -616,7 +632,7 @@ export function WebuiWorkspacePanel({ state, dispatch, sessionId, workspaceDir,
                 
      {reviewSummary.summary.files.map((file) => { const result = reviewDiffs?.tabId === activeTab.id && reviewDiffs.snapshotId === activeReview?.reviewSnapshotId ? reviewDiffs.diffs[file.fileId] : undefined; const loading = !result && (reviewDiffs?.tabId !== activeTab.id || reviewDiffs.snapshotId !== activeReview?.reviewSnapshotId || reviewDiffs.loading); - return dispatch({ type: "select-review-file", tabId: activeTab.id, path: file.path })} />; + return dispatch({ type: "select-review-file", tabId: activeTab.id, path: file.path })} />; })}
      :

      当前没有变更。

      } : null} diff --git a/packages/webui/src/client/projection/common/diff-workspace-boundary.ts b/packages/webui/src/client/projection/common/diff-workspace-boundary.ts new file mode 100644 index 000000000..e661d5e1f --- /dev/null +++ b/packages/webui/src/client/projection/common/diff-workspace-boundary.ts @@ -0,0 +1,169 @@ +// The workspace boundary for a diff entry. +// +// A turn's diff lists the files a turn touched, and an entry in it can name a +// file outside the workspace: the runtime's capture layer records an +// out-of-workspace write as an absolute path, so the entry reaches the client +// with the outside file's content attached. The upstream fix that stopped the +// collection was reverted (PR #240) because it changed a package this fork may +// not change, so the boundary is drawn here instead — the client is the last +// place before the content reaches a screen. +// +// The rule is lexical. The browser has no `path` module and no filesystem, so +// the comparison is made on posix-style strings, the same normalization the +// message file-reference reader uses +// (`features/transcript/projection/message-file-reference.ts`), extended to +// resolve `..` and to compare absolute paths segment by segment. +// +// Two boundaries this does not close, stated rather than papered over: +// +// * A symlink inside the workspace that points outside it resolves outside, +// and the browser cannot see that: the entry's path is a relative one and +// stays contained here. Telling the two apart needs `realpath` on every +// target, which only the runtime side can do. +// * A path whose workspace root the client does not know is not a boundary. +// Without a root there is nothing to compare against, so the entries are +// shown as they are. A client that hid everything it could not place would +// hide ordinary content as well, which is the worse failure. + +import type { WebuiFileDiffInfoView } from "../../../shared/contracts/session.js"; + +/** + * What replaces the body of a file the boundary keeps out. + * + * The wording follows the other refusals in the diff surfaces: the file is + * named, and what is withheld is said plainly rather than left as an absence. + */ +export const WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE = + "该文件在工作区之外,内容未展示。"; + +/** + * One file's diff entry, with the boundary's verdict attached. + * + * The verdict travels with the entry so that the surface which renders the + * body and the projection which withholds it cannot disagree: both read the + * same flag, and there is no second path that decides on its own. + */ +export interface WebuiDiffFileEntry extends WebuiFileDiffInfoView { + /** + * Whether the entry's path stays inside the workspace root. + * + * `true` when the root is unknown: without a root there is no boundary to + * judge against, and an entry that cannot be placed must not be hidden for + * it. + */ + readonly withinWorkspace: boolean; +} + +/** A path spelled with posix separators, whatever the host wrote. */ +function toPosixPath(value: string): string { + return value.replace(/\\/gu, "/"); +} + +/** + * A Windows drive letter, lowercased so `C:` and `c:` name one drive. + */ +function driveLetter(segment: string): string | undefined { + return /^[a-z]:$/iu.test(segment) ? segment.toLowerCase() : undefined; +} + +/** + * The path's segments, with `.` dropped and `..` applied. + * + * `..` is applied against the segments collected so far, so `a/../b` is `b` + * and `../b` is nothing at all — the second cannot be placed under any root, + * which is what makes it out of bounds. The root's own segments are not part + * of the stack: climbing one level above the root is the case this module + * exists for, and a path that does it is refused rather than clamped back + * inside. + */ +function resolveSegments(value: string): string[] | undefined { + const segments: string[] = []; + const parts = toPosixPath(value).split("/"); + for (const [index, part] of parts.entries()) { + if (!part || part === ".") continue; + if (part === "..") { + if (segments.length === 0) return undefined; + segments.pop(); + continue; + } + segments.push(index === 0 ? (driveLetter(part) ?? part) : part); + } + return segments; +} + +/** Whether a path is spelled as an absolute path. */ +function isAbsolutePath(value: string): boolean { + const posix = toPosixPath(value); + return posix.startsWith("/") || /^[a-z]:\//iu.test(posix); +} + +/** + * Whether a path stays inside a workspace directory. + * + * The comparison is lexical and segment-wise. A relative path is resolved + * against the root and refused when it climbs out of it; an absolute path is + * compared with the root segment by segment, so a sibling directory whose + * name starts with the root's name (`/ws-backup` against the root `/ws`) is + * outside — a string prefix would have called it inside. + * + * An unknown root (`undefined`, blank, or a path that cannot be resolved) is + * not a boundary: the entry is allowed through, because a check that cannot + * place a file has no reason to withhold it. + */ +export function isWebuiDiffPathWithinWorkspace( + workspaceDir: string | undefined, + filePath: string, +): boolean { + if (!workspaceDir?.trim()) return true; + // A path that names no file cannot be placed inside the workspace. + if (!filePath.trim()) return false; + const root = resolveSegments(workspaceDir); + if (!root) return true; + const file = resolveSegments(filePath); + if (!file) return false; + if (!isAbsolutePath(filePath)) { + // A relative path is under the root by construction: `resolveSegments` + // already refused the ones that climb out of it. + return true; + } + if (file.length < root.length) return false; + return root.every((segment, index) => segment === file[index]); +} + +/** + * Project one diff entry for display. + * + * The entry keeps everything that traces the change — the file, its status, + * the added and deleted line counts — and loses only the body when the + * boundary says the file is outside the workspace. The entry itself stays in + * the list: "which file was touched" is the part a person can still act on. + */ +export function projectWebuiDiffFile( + file: WebuiFileDiffInfoView, + workspaceDir: string | undefined, +): WebuiDiffFileEntry { + if (isWebuiDiffPathWithinWorkspace(workspaceDir, file.file)) { + return { ...file, withinWorkspace: true }; + } + return { + file: file.file, + additions: file.additions, + deletions: file.deletions, + ...(file.status !== undefined ? { status: file.status } : {}), + withinWorkspace: false, + }; +} + +/** + * Project a diff's file entries for display. + * + * This is the one place the decision is made, so every surface that shows a + * diff body shows the same one: the entries handed to a renderer carry no + * body for a file outside the workspace, and the renderer only has to say so. + */ +export function projectWebuiDiffFiles( + files: readonly WebuiFileDiffInfoView[] | undefined, + workspaceDir: string | undefined, +): readonly WebuiDiffFileEntry[] { + return (files ?? []).map((file) => projectWebuiDiffFile(file, workspaceDir)); +} diff --git a/packages/webui/test/gates/fixtures/path-metadata-f00.json b/packages/webui/test/gates/fixtures/path-metadata-f00.json index 48730b4bc..dc40202c7 100644 --- a/packages/webui/test/gates/fixtures/path-metadata-f00.json +++ b/packages/webui/test/gates/fixtures/path-metadata-f00.json @@ -3099,6 +3099,15 @@ "business": null, "home": "current" }, + { + "path": "client/projection/common/diff-workspace-boundary.ts", + "layers": [ + "view" + ], + "role": "projection-common", + "business": null, + "home": "current" + }, { "path": "client/projection/common/dropdown-placement.ts", "layers": [ diff --git a/packages/webui/test/unit/diff-workspace-boundary.test.tsx b/packages/webui/test/unit/diff-workspace-boundary.test.tsx new file mode 100644 index 000000000..fe7ea2e56 --- /dev/null +++ b/packages/webui/test/unit/diff-workspace-boundary.test.tsx @@ -0,0 +1,219 @@ +// The workspace boundary for a diff entry. +// +// A turn's diff can name a file outside the workspace: the runtime's capture +// layer records an out-of-workspace write as an absolute path, and the entry +// reaches the client with that file's content attached. The upstream fix that +// stopped the collection was reverted (PR #240) because it changed a package +// this fork may not change, so the boundary is drawn on the client side. +// +// This file pins down the two halves of that boundary: +// +// * the projection, which removes the body before a surface can render it — +// the body is blocked in the data, not merely left out of the markup; +// * the surfaces, which say what is withheld instead of showing the content. +// +// The two are tested separately on purpose. A test that only reads the markup +// passes just as well when the component forgets to ask and the projection +// forgets to answer; a test that only reads the projection passes just as well +// when the component renders the body anyway. Together they say that neither +// one is carrying the other. +// +// Known boundary, and it is a boundary rather than a bug: a symlink inside the +// workspace that points outside it resolves outside, and the browser cannot see +// that. The entry's path is a relative one, so it stays contained here. The +// test below says so out loud rather than leaving it to be discovered. + +import { describe, expect, it } from "vitest"; +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; + +import { + isWebuiDiffPathWithinWorkspace, + projectWebuiDiffFile, + projectWebuiDiffFiles, + WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE, +} from "../../src/client/projection/common/diff-workspace-boundary.js"; +import { WebuiDiffCard } from "../../src/client/features/transcript/ui/DiffCard.js"; +import { WebuiDiffFileSection } from "../../src/client/features/workspace/ui/WorkspacePanels.js"; +import type { WebuiFileDiffInfoView } from "../../src/shared/contracts/session.js"; + +const WORKSPACE = "/work/project"; + +/** A body that must never reach a screen for a file outside the workspace. + * The marker is deliberately unlike anything in the file name, so a test that + * greps the markup cannot pass on the name alone. */ +const BODY_MARKER = "TOKEN_VALUE_9f3a"; +const BODY = `@@ -1,1 +1,1 @@\n-const token = "${BODY_MARKER}";\n+const token = "other";`; + +const insideFile: WebuiFileDiffInfoView = { + file: "src/app.ts", + additions: 1, + deletions: 1, + status: "modified", + diff: BODY, +}; + +const outsideFile: WebuiFileDiffInfoView = { + file: "/etc/outside.conf", + additions: 1, + deletions: 1, + status: "modified", + diff: BODY, +}; + +describe("the projection removes the body of a file outside the workspace", () => { + it("keeps the body of a file whose path is relative to the workspace", () => { + const entry = projectWebuiDiffFile(insideFile, WORKSPACE); + + expect(entry.withinWorkspace).toBe(true); + expect(entry.diff).toBe(BODY); + }); + + it("removes the body of an absolute path outside the workspace", () => { + const entry = projectWebuiDiffFile(outsideFile, WORKSPACE); + + expect(entry.withinWorkspace).toBe(false); + expect(entry.diff).toBeUndefined(); + expect(entry.patch).toBeUndefined(); + }); + + it("removes the patch object as well as the diff text", () => { + const entry = projectWebuiDiffFile( + { file: "/etc/secret.conf", additions: 0, deletions: 0, patch: { hunks: ["secret"] } }, + WORKSPACE, + ); + + expect(entry.withinWorkspace).toBe(false); + expect(entry.patch).toBeUndefined(); + }); + + it("removes the body of a path that walks out of the workspace with `..`", () => { + for (const file of ["../outside.txt", "src/../../outside.txt", ".."]) { + const entry = projectWebuiDiffFile({ file, additions: 1, deletions: 0, diff: BODY }, WORKSPACE); + + expect(entry.withinWorkspace).toBe(false); + expect(entry.diff).toBeUndefined(); + } + }); + + it("keeps the entry, its name and its counts when the body is removed", () => { + const entry = projectWebuiDiffFile(outsideFile, WORKSPACE); + + // The file name and the counts are the part a person can still act on: + // "which file was touched" survives even when its content does not. + expect(entry.file).toBe("/etc/outside.conf"); + expect(entry.additions).toBe(1); + expect(entry.deletions).toBe(1); + expect(entry.status).toBe("modified"); + }); + + it("keeps the body when the workspace root is unknown", () => { + // Without a root there is no boundary to judge against. A check that hid + // everything it could not place would hide ordinary content too. + const entry = projectWebuiDiffFile(outsideFile, undefined); + + expect(entry.withinWorkspace).toBe(true); + expect(entry.diff).toBe(BODY); + }); + + it("calls a sibling directory that shares the root's name outside", () => { + // A string prefix would have called "/work/project-backup" inside + // "/work/project"; the comparison is on segments. + expect(isWebuiDiffPathWithinWorkspace(WORKSPACE, "/work/project-backup/x.ts")).toBe(false); + expect(isWebuiDiffPathWithinWorkspace(WORKSPACE, "/work/project/x.ts")).toBe(true); + }); + + it("does not detect a symlink inside the workspace that points outside it", () => { + // The known boundary: the path looks contained and the browser has no way + // to know what it resolves to. Telling the two apart needs `realpath` on + // the runtime side. + expect(isWebuiDiffPathWithinWorkspace(WORKSPACE, "link/out.txt")).toBe(true); + }); + + it("keeps every entry of the list, in order", () => { + const entries = projectWebuiDiffFiles([insideFile, outsideFile], WORKSPACE); + + expect(entries).toHaveLength(2); + expect(entries.map((entry) => entry.file)).toEqual(["src/app.ts", "/etc/outside.conf"]); + expect(entries.map((entry) => entry.withinWorkspace)).toEqual([true, false]); + }); +}); + +describe("the diff card shows what is withheld instead of the content", () => { + const renderCard = (file: WebuiFileDiffInfoView, workspaceDir?: string): string => + renderToStaticMarkup( + createElement(WebuiDiffCard, { + workspaceDir, + initialView: { + status: "active", + changeSetId: "changes-1", + fileChanges: [file], + }, + // The card's own review panel is the surface that renders the body. + initialState: { unsupported: false, busy: false, expanded: false, reviewing: true }, + }), + ); + + it("shows the body of a file inside the workspace", () => { + const markup = renderCard(insideFile, WORKSPACE); + + expect(markup).toContain(BODY_MARKER); + expect(markup).not.toContain(WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE); + }); + + it("does not render the body of a file outside the workspace", () => { + const markup = renderCard(outsideFile, WORKSPACE); + + expect(markup).not.toContain(BODY_MARKER); + expect(markup).not.toContain("@@ -1,1 +1,1 @@"); + expect(markup).toContain(WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE); + }); + + it("keeps the file in the list with its counts", () => { + const markup = renderCard(outsideFile, WORKSPACE); + + expect(markup).toContain('data-file-path="/etc/outside.conf"'); + expect(markup).toContain("+1"); + expect(markup).toContain("-1"); + }); + + it("shows the body when the workspace root is unknown", () => { + const markup = renderCard(outsideFile); + + expect(markup).toContain(BODY_MARKER); + expect(markup).not.toContain(WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE); + }); +}); + +describe("the workspace review panel shows what is withheld instead of the content", () => { + const renderSection = (file: WebuiFileDiffInfoView, workspaceDir?: string): string => + renderToStaticMarkup( + createElement(WebuiDiffFileSection, { + file: projectWebuiDiffFile(file, workspaceDir), + selected: true, + }), + ); + + it("shows the body of a file inside the workspace", () => { + const markup = renderSection(insideFile, WORKSPACE); + + expect(markup).toContain(BODY_MARKER); + expect(markup).toContain("webui-turn-review-line--addition"); + }); + + it("does not render the body of a file outside the workspace", () => { + const markup = renderSection(outsideFile, WORKSPACE); + + expect(markup).not.toContain(BODY_MARKER); + expect(markup).not.toContain("webui-turn-review-line--addition"); + expect(markup).toContain(WEBUI_DIFF_OUTSIDE_WORKSPACE_NOTICE); + }); + + it("keeps the file's name and its counts in the section heading", () => { + const markup = renderSection(outsideFile, WORKSPACE); + + expect(markup).toContain("/etc/outside.conf"); + expect(markup).toContain("+1"); + expect(markup).toContain("-1"); + }); +}); diff --git a/packages/webui/test/unit/webui-round3-acceptance.test.tsx b/packages/webui/test/unit/webui-round3-acceptance.test.tsx index 1cbba0b08..a5960adc9 100644 --- a/packages/webui/test/unit/webui-round3-acceptance.test.tsx +++ b/packages/webui/test/unit/webui-round3-acceptance.test.tsx @@ -12,6 +12,7 @@ import { WebuiDiffCard } from "../../src/client/features/transcript/ui/DiffCard. import { WebuiGoalBanner } from "../../src/client/features/goal/ui/GoalBanner.public.js"; import { WebuiInteractionPanel } from "../../src/client/features/interactions/ui/InteractionPanel.public.js"; import { chunkWebuiWorkspaceReviewFileIds, WebuiDiffFileSection } from "../../src/client/features/workspace/ui/WorkspacePanels.js"; +import { projectWebuiDiffFile } from "../../src/client/projection/common/diff-workspace-boundary.js"; import { projectWebuiUnifiedDiffLines } from "../../src/client/features/workspace/projection/panel-projection.js"; import { WebuiFeedbackActions, @@ -212,7 +213,9 @@ describe("turn review unified diff projection", () => { it("renders workspace review additions and deletions using the shared highlighted diff rows", () => { const markup = renderToStaticMarkup(createElement(WebuiDiffFileSection, { - file: { file: "src/example.ts", additions: 1, deletions: 1, diff: "@@ -1 +1 @@\n-return false;\n+return true;" }, + // The section renders what the boundary projection decided, so the entry + // reaches it already judged. + file: projectWebuiDiffFile({ file: "src/example.ts", additions: 1, deletions: 1, diff: "@@ -1 +1 @@\n-return false;\n+return true;" }, "/work/project"), selected: true, })); diff --git a/release/public-source.json b/release/public-source.json index 70a83c790..af402166e 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3570,6 +3570,7 @@ "packages/webui/docs/adr/0021-webui-model-picker-is-one-popup-with-two-states.md", "packages/webui/docs/adr/0022-webui-workspace-panel-is-a-browser-tab-strip.md", "packages/webui/docs/adr/0023-webui-test-coverage-follows-user-workflows-and-evidence-levels.md", + "packages/webui/docs/adr/0024-the-client-draws-the-workspace-boundary-for-a-diff-body.md", "packages/webui/docs/adr/observation-architecture.md", "packages/webui/docs/agents/domain.md", "packages/webui/docs/agents/issue-tracker.md", @@ -4018,6 +4019,7 @@ "packages/webui/src/client/platform/transport.ts", "packages/webui/src/client/platform/wire.ts", "packages/webui/src/client/projection/common/context-menu.ts", + "packages/webui/src/client/projection/common/diff-workspace-boundary.ts", "packages/webui/src/client/projection/common/dropdown-placement.ts", "packages/webui/src/client/projection/common/event-kinds.ts", "packages/webui/src/client/projection/common/event-parsers.ts", @@ -4312,6 +4314,7 @@ "packages/webui/test/unit/conversation-usage-notice.test.ts", "packages/webui/test/unit/delivery-disposers.test.ts", "packages/webui/test/unit/diff-mutation-error.test.tsx", + "packages/webui/test/unit/diff-workspace-boundary.test.tsx", "packages/webui/test/unit/dropdown-placement.test.ts", "packages/webui/test/unit/event-kinds.test.ts", "packages/webui/test/unit/first-party-signature.test.ts", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 91abfa375..f75003d21 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -284,6 +284,7 @@ "packages/webui/test/unit/conversation-usage-notice.test.ts", "packages/webui/test/unit/delivery-disposers.test.ts", "packages/webui/test/unit/diff-mutation-error.test.tsx", + "packages/webui/test/unit/diff-workspace-boundary.test.tsx", "packages/webui/test/unit/dropdown-placement.test.ts", "packages/webui/test/unit/event-kinds.test.ts", "packages/webui/test/unit/first-party-signature.test.ts", From cd1e643207a306148c10f64b6cbce4e8dfad3ba8 Mon Sep 17 00:00:00 2001 From: probe Date: Fri, 23 Oct 2026 22:12:06 +0800 Subject: [PATCH 2/2] test(webui): stop the agent transcript case from racing three writes it reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The case reads three things that are written by someone else — the runtime's active turn, the client's captured request frames, and the runtime's event stream — and it read all three once, at a moment that is not the moment the write lands. The turn before the cancellation step is over when its answer is on screen, but the runtime retires the turn a moment later, and the case sent the next message into that gap. A session whose turn has not been retired does not start a turn: the message is queued instead, so it never reaches the model and the case waits for an event that will not come. The case now waits for the turn to be retired, the way it already does before its other sends. The plan-entry request was read out of the frame capture once, right after the answer that answers it had rendered. The capture is now polled, the way the persisted history next to it is already polled. The runtime's event watch was torn down the moment the turn went inactive — which is exactly when the runtime reports the failure — so an event still in flight when the watch was aborted was lost, and the comparison that follows had nothing on the runtime side to compare the client's event against. The watch now stays open until both sides carry the error. The turn retries the failing model call, so the runtime reports the failure more than once and the first report is not necessarily the one the client saw; the pairing now looks at every error each side reported, and the two views still have to name the same turn. How it was verified: the case was run repeatedly on the same machine. Before the change it failed on the cancellation gate in about two runs out of five, and the model server never received the message at all — the runtime never started a turn for it. The same case fails the same way on the trunk without this branch, so the flake is not this branch's; the change removes the races the case was reading through and leaves the assertions as they were. --- .../test/integration/agent-transcript.test.ts | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/packages/webui/test/integration/agent-transcript.test.ts b/packages/webui/test/integration/agent-transcript.test.ts index 773464e6e..b76a84751 100644 --- a/packages/webui/test/integration/agent-transcript.test.ts +++ b/packages/webui/test/integration/agent-transcript.test.ts @@ -983,6 +983,11 @@ it("a real agent turn resumes after refresh and drains a queued message in order await composer.fill("/plan Plan this synthetic task."); await composer.press("Enter"); await browserExpect(page.getByText("Synthetic plan-entry turn completed.")).toBeVisible({ timeout: 30_000 }); + // The captured frame and the rendered answer are two different writes, so + // the capture is polled the way the history below is polled rather than + // read once and raced. + await expect.poll(() => requests.some((request) => request.operation === "sendMessage" && + request.body?.content === "Plan this synthetic task."), { timeout: 20_000 }).toBe(true); const planEntry = requests.find((request) => request.operation === "sendMessage" && request.body?.content === "Plan this synthetic task."); expect(planEntry?.body).toMatchObject({ clientIntent: "plan-entry" }); @@ -1189,6 +1194,14 @@ it("a real agent turn resumes after refresh and drains a queued message in order const forkEditResponse = received.find((response) => response.requestId === forkEditRequest?.requestId); expect(forkEditResponse?.body).toMatchObject({ rewound: true, userMessageId: expect.any(String) }); await browserExpect(page.getByText("Edited fork message response.")).toBeVisible({ timeout: 30_000 }); + // The rendered answer and the retired turn are two different moments: the + // answer shows up when the model has answered, the turn is only over once + // the runtime says so. Sending into a session whose turn has not been + // retired yet does not start a turn — the message is queued instead, and + // it never reaches the model, so the case waits for the turn to be over + // the way the rest of this case does before it sends the next message. + await browserExpect.poll(() => port.getActiveTurn({ id: String(messageForkSessionId) }), { timeout: 20_000 }) + .toBeUndefined(); const editedForkHistory = await port.getMessages({ id: String(messageForkSessionId) }); const forkUserMessages = (editedForkHistory.messages ?? []).filter((message) => message.role === "user"); expect(forkUserMessages.some((message) => message.msgContent === "Fork isolation probe edited.")).toBe(true); @@ -1253,18 +1266,40 @@ it("a real agent turn resumes after refresh and drains a queued message in order await browserExpect.poll(() => port.getActiveTurn({ id: String(messageForkSessionId) }), { timeout: 30_000, }).toBeUndefined(); - retryLifecycleAbort.abort(); - await retryLifecycleWatch; const observedLifecycleEvents = () => received.flatMap((frame) => { if (frame.kind !== "event" || !frame.body || typeof frame.body !== "object" || Array.isArray(frame.body)) return []; const event = frame.body as { type?: unknown; payload?: unknown }; return typeof event.type === "string" && event.type.startsWith("session.") ? [event] : []; }); - await browserExpect.poll(() => observedLifecycleEvents().some((event) => + const errorTurnId = (event: { type?: unknown; payload?: unknown }) => + (event.payload as { turnId?: unknown } | undefined)?.turnId; + // The runtime's event stream and the client's are two deliveries of the + // same event, and the runtime's watch used to be torn down the moment the + // turn went inactive — which is exactly when the runtime reports the + // failure. An event still in flight when the watch is aborted is lost, and + // the comparison then has nothing on the runtime side to compare the + // client's event against. The watch therefore stays open until both sides + // carry the error. + await browserExpect.poll(() => { + const observed = observedLifecycleEvents().filter((event) => event.type === "session.error"); + const runtime = retryLifecycleEvents.filter((event) => event.type === "session.error"); + return { bothCarried: observed.length > 0 && runtime.length > 0, observed, runtime }; + }, { timeout: 20_000 }).toMatchObject({ bothCarried: true }); + retryLifecycleAbort.abort(); + await retryLifecycleWatch; + // The turn retries the failing model call, so the runtime reports the + // failure more than once and the first report is not necessarily the one + // the client saw. The two views have to name the same turn, and the + // pairing looks at every error each side reported. + expect(observedLifecycleEvents().some((event) => event.type === "session.error" && - (event.payload as { turnId?: unknown } | undefined)?.turnId === - (retryLifecycleEvents.find((event) => event.type === "session.error")?.payload as { turnId?: unknown } | undefined)?.turnId, - )).toBe(true); + retryLifecycleEvents.some((runtimeEvent) => + runtimeEvent.type === "session.error" && errorTurnId(runtimeEvent) === errorTurnId(event), + ), + ), JSON.stringify({ + observed: observedLifecycleEvents(), + runtime: retryLifecycleEvents, + })).toBe(true); const retryFailureVisible = await retryError.isVisible(); expect(retryFailureVisible, JSON.stringify({ retryModelAttempts,