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