From 2a90fb8118a8f603c52c8d8a77c4077bedbc342e Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sat, 24 Oct 2026 22:28:29 -0400 Subject: [PATCH] fix(terminal): keep the next table record out of a last-column URL A URL in the last column of a space-aligned table ended on a slash, so the first cell of the next record (billing-api, CHANGELOG.md, ...) passed every continuation test and was joined onto it. The alignment check that refused this only ran when other columns followed the URL. A continuation row must now start inside the URL's own cell: a gap of two or more spaces between its indent and where the URL starts on the first row means it opens an earlier column. An indented token that is a whole email address is also refused. Adds the six table shapes and the email case as negative tests at detector and provider level, and documents the rule. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- src/web/public/constants.js | 15 ++++- test/terminal-url-continuation.test.ts | 82 ++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f528681b..de020024 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -224,7 +224,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **File-path links (terminal + chat)**: a path an agent prints is clickable on BOTH surfaces and opens the file-preview overlay. ⚠️ ONE pattern (`FILE_PATH_LINK_PATTERN` / `absoluteFilePathPattern()` in constants.js) feeds the xterm link provider AND `_linkifyFilePaths()`, a fresh instance per call (`lastIndex`). Both call `findFilePathLinks()`, which adds `~/`, relative and folder paths without overlapping a rooted match or a URL; a relative path resolves against `workingDir` (`resolveLinkedFilePath`), `~/` only server-side on the session's host (attachment route), and `terminalPathLine()` joins a path a program hard-wrapped short of the edge. ⚠️ A folder link opens the **folder grid**: `POST /api/sessions/:id/folders` guards + GRANTS the folder (`folder-listing.ts`, own registry, never the 200-capped attachment registry), files come by bare name via `…/folders/:folderId/files/:name/*` through `resolveServableAttachmentPath` plus the same extension/regular-file gate on the symlink's resolved target, so no browser request carries a path; `~/` is refused in a Docker case (the container's home is not this host's). The chat linkifier walks TEXT NODES with DOM APIs, never rebuilds sanitized markup as a string. ⚠️ An out-of-workspace path goes through the ATTACHMENT routes (`POST /api/sessions/:id/attachments` with `notify: false`), never by widening `file-content`/`file-raw` or `file-stream-manager`'s `tail -f` allowlist. ⚠️ `TEXT_ATTACHMENT_EXTENSIONS` IS `EDITABLE_EXTENSIONS` (never a second list), and widening READ must never widen RUN: `html`/`htm`/`svg` stay download-only, other text is inert `text/plain`+`nosniff`. ⚠️ The ONE place HTML renders is `/html-view//...` (`html-view-routes.ts`, `html-view-capabilities.ts`): `POST /api/sessions/:id/html-view` mints a capability for the file's own directory, every response carries `Content-Security-Policy: sandbox` WITHOUT `allow-same-origin` (opaque origin even as a top-level tab, so the page can never read Codeman or drive its API) and with NO fetch directive (open network, owner decision 2026-10-13: never restrict it or add `allow-same-origin` without the owner), and only non-dot, asset-allowlisted files under that directory are served (GET/HEAD only; capabilities revoked with the web-tab ones on logout). That tree is served recursively, so minting is refused for a hidden directory or a broad root (`isBroadHtmlViewRoot`); the preview renders by default and the **Page** pill (or a refused mint) falls through to the source view and its Edit button. Never serve HTML same-origin anywhere else. Tests: `test/routes/html-view-routes.test.ts`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server; the admission limits that keep a hostile workbook from freezing the worker live in `src/web/CLAUDE.md` (Spreadsheet preview). → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) -The terminal provider scans `terminalLinkLine()`, which extends `terminalPathLine()` for URL continuations in indented paragraphs and table cells. List markers, bare numbers and version tokens never continue a URL. `terminalCellWrappedPaths()` follows each table cell independently, gated by `pathContinuation()` and the per-cell row-length rule; accepted paths displace other links on their cells. Full-width divider rows stop `terminalLogicalLine()` stitching for both links and touch selection. OSC 8 links confirm the real target URI before the shared opener runs. +The terminal provider scans `terminalLinkLine()`, which extends `terminalPathLine()` for URL continuations in indented paragraphs and table cells. List markers, bare numbers, version tokens and whole email addresses never continue a URL. A URL in the last column of a space-aligned table never takes the next row's first cell: a continuation row must start inside the URL's own cell, so a gap of two or more spaces between its indent and where the URL starts on the row above means it is the next record. `terminalCellWrappedPaths()` follows each table cell independently, gated by `pathContinuation()` and the per-cell row-length rule; accepted paths displace other links on their cells. Full-width divider rows stop `terminalLogicalLine()` stitching for both links and touch selection. OSC 8 links confirm the real target URI before the shared opener runs. **Filesystem path picker** (Link Existing "Browse" + the mobile keyboard's `📁 Path` key): lazy one-directory browsing via `GET /api/filesystem/browse`, with `GET /api/filesystem/preview` for the tapped file. Inserts the path **without** Enter, so the prompt is never submitted; the sibling `⌫ All` key clears only the unsent prompt and must never send the agent's `/clear`. ⚠️ This is a **second file-serving surface and inherits neither the attachment confinement nor its ownership scoping** — it allowlists Home, `CASES_DIR`, `/mnt/d` and `CODEMAN_FILE_PICKER_ROOTS`, blocks sensitive trees, and rejects symlink escapes **after** `realpath`. ⚠️ The optional `sessionId` is an ownership boundary that must be `canAccessOwned`-checked by hand (it does not go through `findSessionOrFail`), and in multi-user mode a non-admin gets only their own `userSpacePath` as a root: per-user spaces live INSIDE `homedir()`, so a `Home` root exposes every other user's workspace. Previews go through the same global conversion limiter, and Markdown/TXT/JSON are served as inert `text/plain`. → [architecture-invariants#filesystem-path-picker](docs/architecture-invariants.md#filesystem-path-picker) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 68559b28..1d0e124e 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -409,7 +409,7 @@ A file path an agent prints is a link on both surfaces it can appear on, and cli **Path forms beyond the rooted one.** `findFilePathLinks(text)` (constants.js) is the single entry point both consumers call; it returns the rooted matches first, then `~/` and relative paths (`RELATIVE_FILE_PATH_LINK_PATTERN`, same extension list, pinned equal by `test/link-provider-regex.test.ts`), then folder paths, each only where it overlaps neither an earlier match nor an `http(s)` URL. A relative match must start at a boundary (line start, whitespace, quote, bracket, `=`, `,`, `>`), so it can never begin inside a word, an absolute path or a URL; brackets, `=` and `,` end it (`(shot.png)`, `OUT=shots/a.png`). `resolveLinkedFilePath()` resolves a relative path against the session's `workingDir` (`.`/`..` collapsed, so `../x.png` lands outside and routes like any external path) and passes absolute and `~/` paths through. ⚠️ `~` is the home of the SESSION'S host, which only the server knows: `~/` paths always take the attachment route, where `registerExternalAttachment` expands them (`homedir()` locally, `$HOME` inside the remote probe script), and the guard runs on the expanded, realpath-resolved path as before. ⚠️ Programs hard-wrap their own output short of the terminal edge (Claude Code breaks tool output at 59-118 columns), so xterm's `isWrapped` never fires. `terminalPathLine()` (link provider only; touch selection keeps `terminalLogicalLine`) joins the next row when the previous row ends in a path token with a `/` and no extension and the join reaches an extension; a row that starts a new path is never joined: rooted or `~/`, `./`/`../`, or a relative one that opens on the same top directory the broken token did (the next entry of `git ls-files`/`find`/`ls -R`: `src/components` + `src/index.ts` are two paths, not `src/componentssrc/index.ts`), while a wrapped path continues INSIDE a name, so `…/captures/2` + `026-10-10/…` still joins; a 3+ row chain is kept only if a later row completes the path, and the broken row must be at least as long as the continuing one (keeps `cd /mnt/foo` + a longer next line apart). Tests: `test/file-path-link-forms.test.ts`. -The terminal provider scans `terminalLinkLine()`, which extends `terminalPathLine()` for URL continuations in indented paragraphs and table cells. List markers, bare numbers and version tokens never continue a URL. A token with only hyphens, underscores or dots must end its cell (row end or a gap of at least two spaces), so prose such as `self-hosted runners` is not appended. A row end counts as a cell end, so `⏺ Docs live at https://example.com/guide/` followed by ` self-hosted.` still links `.../guide/self-hosted`, and an indented `and/or the API reference covers it.` still links `.../guide/and/or`; both are pinned as known trade-offs. A continuation must be indented (a column-0 row is new output, which keeps `status=200`, `24/7` and `really?` out), and a shell prompt (`user@host:`), a home path (`~/`) or an HTTP status line never continues a URL. A divider row never continues the row above it, so a URL that reaches the right edge does not absorb a full-width `────` row below. A real plain slug followed by prose falls back to the original fragment link; `%`, `&`, `=`, `/` or `?` still qualify as stronger URL punctuation. `terminalCellWrappedPaths()` follows each table cell independently, gated by `pathContinuation()` and the per-cell row-length rule; accepted paths displace other links on their cells. Full-width divider rows stop `terminalLogicalLine()` stitching for both links and touch selection. OSC 8 links confirm the real target URI before the shared opener runs. +The terminal provider scans `terminalLinkLine()`, which extends `terminalPathLine()` for URL continuations in indented paragraphs and table cells. List markers, bare numbers and version tokens never continue a URL. A token with only hyphens, underscores or dots must end its cell (row end or a gap of at least two spaces), so prose such as `self-hosted runners` is not appended. A row end counts as a cell end, so `⏺ Docs live at https://example.com/guide/` followed by ` self-hosted.` still links `.../guide/self-hosted`, and an indented `and/or the API reference covers it.` still links `.../guide/and/or`; both are pinned as known trade-offs. A continuation must be indented (a column-0 row is new output, which keeps `status=200`, `24/7` and `really?` out), and a shell prompt (`user@host:`), a home path (`~/`), a whole email address or an HTTP status line never continues a URL. A URL in the last column of a space-aligned table never takes the next row's first cell: a continuation row must start inside the URL's own cell, so a gap of two or more spaces between its indent and where the URL starts on the row above means it is the next record. A divider row never continues the row above it, so a URL that reaches the right edge does not absorb a full-width `────` row below. A real plain slug followed by prose falls back to the original fragment link; `%`, `&`, `=`, `/` or `?` still qualify as stronger URL punctuation. `terminalCellWrappedPaths()` follows each table cell independently, gated by `pathContinuation()` and the per-cell row-length rule; accepted paths displace other links on their cells. Full-width divider rows stop `terminalLogicalLine()` stitching for both links and touch selection. OSC 8 links confirm the real target URI before the shared opener runs. **Folder grid: a folder is granted, then served by bare file name.** A folder path (rooted or `~/` whose last segment has no extension, relative only with a trailing `/`; `FOLDER_PATH_LINK_PATTERN` + `folderLinkPath()`) opens `openFolderGrid()` (panels-ui.js), as does the preview's "Show folder" button. `POST /api/sessions/:id/folders {path}` (`src/folder-listing.ts`) resolves the folder (locally a bounded `probePathKind(…, { pastCap: true })` first, then async `fs.realpath`; remotely one ssh round trip via `remoteListFolder`, through `runWithRemoteSshLimit`), runs the attachment guard on it (blocklist, `confineToWorkspace`, and in multi-user mode `folderAllowedForUser`: a non-admin only inside their own `userSpacePath` or the session's `workingDir`, as the filesystem path picker), locally BEFORE reading it, lists non-hidden subfolders and files of a supported attachment type (each file also blocklist-checked, newest first, capped at `MAX_FOLDER_ENTRIES`), and GRANTS it an id. Files are then fetched as `GET …/folders/:folderId/files/:name/(raw|preview|thumbnail)`: `:name` must be a bare file name of a supported type (no `/`, `\`, NUL, no leading `.`, since the listing hides those), the local file is probed bounded (`unknown` → 503, never 404), and the joined path goes through the SAME `resolveServableAttachmentPath` as an attachment by id (symlink re-resolution, remote re-probe, guard), then the same gate registration applies to what a symlink RESOLVES to (a supported extension on the resolved path and a regular file: locally `fs.stat`, remotely the probe kind; else 400, so `shot.png -> ../secret.bin` is not served and a FIFO or device behind a supported name never reaches `open`), then `folderAllowedForUser` once more on the resolved file, before `serveServableRaw` (the attachment raw route's body, shared) streams it. So browser requests still never carry a path. ⚠️ Grants live in their own registry (`folderGrants`, 32 per session, cleared with the session's attachments), NOT the attachment registry: that one is capped at 200 per session, and paging a folder of 300 renders would evict the session's real attachments. ⚠️ A local folder is never touched unbounded (#516 rule): a folder on a network mount that stopped answering answers 503 with `describeUnknownPath()`, not 404 and not a frozen server. ⚠️ The remote listing script tells "missing" (`E|n`, 404) from "is a file" (`E|f`, 400 `Not a folder`), because the client falls back to the file preview on `Not a folder`; a remote extensionless file otherwise answered "Folder not found". ⚠️ `.folder-grid-overlay` sits at `z-index: 5090`, one step BELOW the preview (5100) and above the response viewer (5000): a tile's preview opens on top and closing it lands back on the grid. ⚠️ ←/→/Esc are handled in a document CAPTURE listener with `stopPropagation`, because the terminal keeps keyboard focus under the overlays and an arrow that reached xterm would be typed into the session; inputs other than xterm's helper textarea keep their keys. ⚠️ A route PARAMETER is capped by the router (Fastify's default is 100 characters, answered as 404 "Route not found"), and a file name goes up to 255, percent-encoded: the server sets `routerOptions.maxParamLength` to `MAX_ROUTE_PARAM_LENGTH` (route-helpers.ts), the route test harness uses the same constant, and a wiring guard pins it. ⚠️ `~/` in a Docker case is refused with 400 (attachment POST and `POST …/folders`): the session is local for these routes, but `~` inside the container is the container's own home, and expanding it on the host would open a different file than the one the agent printed. Tests: `test/routes/folder-grid-routes.test.ts` (incl. the listing script on a real `sh`), `test/folder-grid-ui.test.ts`. diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 3f8f5d23..deb5cf2d 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -2569,12 +2569,15 @@ function findTerminalUrls(text) { * - the URL fragment ends on a character a wrapper breaks after * (`/ ? & = - _ % # + ~`); a following table column may be excluded; * - the next row is indented, but no further right than the column the URL - * started in (a table cell or list item continuation, not unrelated text); + * started in (a table cell or list item continuation, not unrelated text), + * and no column gap lies between its indent and the URL on the first row + * (that would be the next record of a table); * - that row's first token looks like URL material (contains one of * `% & = / ? . - _`), is not a bare number/version or list marker, * and is not itself a new `scheme://`, a shell prompt (`user@host:`), - * a home path (`~/`) or an HTTP status line; without `% & = / ?`, the token - * must finish its cell (row end or a gap of at least two spaces). + * a home path (`~/`), an email address or an HTTP status line; without + * `% & = / ?`, the token must finish its cell (row end or a gap of at + * least two spaces). * The last rule is what stops prose such as "…/files/" + "then run it" from * being glued into one link. * @@ -2629,10 +2632,16 @@ function terminalUrlContinuationExtent(buffer, row, cols, maxRows) { / {2,}/.test(first.text.slice(indent, last.startIndex))) ) break; + // The next row must start inside the URL's own cell: a column gap between + // its indent and the URL on the first row means it opens an earlier + // column (the next record of a table), not the rest of the URL. + if (/ {2,}/.test(first.text.slice(indent, last.startIndex).trimStart())) break; if (/^[a-z][a-z0-9+.-]*:\/\//i.test(token) || !/^[A-Za-z0-9%_~]/.test(token)) break; // A shell prompt (`user@host:~/dir$`), a home path and an HTTP status line // are never the rest of a URL, however much URL punctuation they carry. if (/^[\w.-]+@[\w.-]+:|^~\/|^HTTP\/\d/i.test(token)) break; + // Nor is a token that is a whole email address. + if (/^[\w.+-]+@[\w-]+(?:\.[\w-]+)+$/.test(token)) break; if (!/[%&=/?.\-_]/.test(token) || /^v?\d+(?:\.\d+)*(?:[-+][\w.-]+)?[.,;:!?)]?$/i.test(token)) break; // Hyphens, underscores and dots also occur in prose. Without stronger // URL punctuation, the token must finish its cell, not introduce a sentence. diff --git a/test/terminal-url-continuation.test.ts b/test/terminal-url-continuation.test.ts index b03a5ccf..ad6fc98f 100644 --- a/test/terminal-url-continuation.test.ts +++ b/test/terminal-url-continuation.test.ts @@ -120,6 +120,54 @@ const NOT_CONTINUATION_ROWS: [string, string, string, string | null][] = [ ['Status page: https://status.example.com/', '24/7 monitoring is on.', 'https://status.example.com/', null], ['Did you mean https://example.com/search?q=', 'really? no.', 'https://example.com/search?q=', null], ]; +// The last column of a space-aligned table: the next row opens the next record +// in an earlier column, so its first cell is never the rest of the URL. Each +// row is indented two spaces. The URL on each row, then any path link expected +// on the second row. +const TABLE_LAST_COLUMN_ROWS: [string, string, string, string, string | null][] = [ + [ + ' auth-api https://auth.example.com/', + ' billing-api https://billing.example.com/', + 'https://auth.example.com/', + 'https://billing.example.com/', + null, + ], + [ + ' docs.example.com https://docs.example.com/', + ' api.example.com https://api.example.com/', + 'https://docs.example.com/', + 'https://api.example.com/', + null, + ], + [ + ' Homepage: https://example.com/', + ' Bug-reports: https://github.com/org/repo/issues/', + 'https://example.com/', + 'https://github.com/org/repo/issues/', + null, + ], + [ + ' user_service https://users.example.com/v1/', + ' order_service https://orders.example.com/v1/', + 'https://users.example.com/v1/', + 'https://orders.example.com/v1/', + null, + ], + [ + ' README.md https://example.com/docs/', + ' CHANGELOG.md https://example.com/changes/', + 'https://example.com/docs/', + 'https://example.com/changes/', + 'CHANGELOG.md', + ], + [ + ' my-service- https://svc.example.com/api/', + ' long-name https://svc2.example.com/', + 'https://svc.example.com/api/', + 'https://svc2.example.com/', + null, + ], +]; // Known trade-offs: a row end counts as a cell end, so a short indented word // with URL punctuation still continues the URL. Pinned so a change is deliberate. const TRADE_OFF_ROWS: [string, string, string][] = [ @@ -238,6 +286,16 @@ describe('terminalUrlContinuationExtent (wrap joining)', () => { } ); + it.each(TABLE_LAST_COLUMN_ROWS)('does not join the next table record after %s', (first, next) => { + const buffer = fakeBuffer([{ text: first }, { text: next }], 100); + for (const row of [0, 1]) expect(terminalUrlContinuationExtent(buffer, row, 100, 12)).toBeNull(); + }); + + it('does not join an indented email address', () => { + const buffer = fakeBuffer([{ text: ' https://example.com/' }, { text: ' user@example.com' }], 100); + for (const row of [0, 1]) expect(terminalUrlContinuationExtent(buffer, row, 100, 12)).toBeNull(); + }); + it.each(TRADE_OFF_ROWS)('still joins the known trade-off after %s', (first, next) => { const buffer = fakeBuffer([{ text: first }, { text: next }], 100); for (const row of [0, 1]) @@ -413,6 +471,30 @@ describe('terminal link provider with wrapped table URLs', () => { if (pathOnNext) expect(second.map((link) => link.text)).toContain(pathOnNext); }); + it.each(TABLE_LAST_COLUMN_ROWS)( + 'keeps each last-column URL on its own row: %s', + (first, next, url, nextUrl, pathOnNext) => { + const { provide, openSpy } = harness([{ text: first }, { text: next }], 100); + const links = provide(1).filter((link) => /^https?:/.test(link.text)); + expect(links.map((link) => link.text)).toEqual([url]); + links[0].activate(null, links[0].text); + expect(openSpy).toHaveBeenCalledExactlyOnceWith(url, '_blank', 'noopener,noreferrer'); + const second = provide(2); + expect(second.filter((link) => /^https?:/.test(link.text)).map((link) => link.text)).toEqual([nextUrl]); + if (pathOnNext) expect(second.map((link) => link.text)).toContain(pathOnNext); + } + ); + + it('keeps an indented email address out of the URL above it', () => { + const { provide } = harness([{ text: ' https://example.com/' }, { text: ' user@example.com' }], 100); + expect( + provide(1) + .filter((link) => /^https?:/.test(link.text)) + .map((link) => link.text) + ).toEqual(['https://example.com/']); + expect(provide(2).filter((link) => /^https?:/.test(link.text))).toEqual([]); + }); + it.each(TRADE_OFF_ROWS)('pins the known trade-off link after %s', (first, next, joined) => { const { provide } = harness([{ text: first }, { text: next }], 100); for (const line of [1, 2]) {