diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index b0acb7cc..83a6d304 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 reserves spaced candidates first (including unlinkable spans), then detects rooted matches, 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`. -Spaced paths are considered first by `findFilePathLinks()`: accepted candidates reserve their entire span even when unlinkable, so later detectors cannot link fragments. Relative and `~/` spaced paths require an explicit path label (`Path:`, `File:`, `Files:`, `Saved to:`, `Saved as:`, `Wrote:`, `Written to:`, `Output:`, `Created:`, `Location:`, `Attachment:`), optionally preceded by a marker. Generic labels and bare bullets defer to ordinary detection, including slash-containing prose such as `CI/CD` or `45/45`. A later `/`, `~/`, `./` or `../` word starts a separate path; an earlier word ending in `/` is a separate folder. Absolute paths retain own-line and marker forms, including the residual ambiguity `/home/me/proj contains a.md`. The response viewer uses a leading `` path label in a paragraph or list item as detection context without changing the label or displayed text; paths split across other inline elements remain detected per text node. Tests feed vendored marked output into the jsdom linkifier, so consumed bullets and bold labels are covered. Trailing whitespace uses linear `trimEnd()` before the length cap. +Spaced paths are considered first by `findFilePathLinks()`: accepted candidates reserve their entire span even when unlinkable, so later detectors cannot link fragments. Relative and `~/` spaced paths require an explicit path label (`Path:`, `File:`, `Files:`, `Saved to:`, `Saved as:`, `Wrote:`, `Written to:`, `Output:`, `Created:`, `Location:`, `Attachment:`), optionally preceded by a marker. Generic labels and bare bullets defer to ordinary detection, including slash-containing prose such as `CI/CD` or `45/45`. A later `/`, `~/`, `./` or `../` word starts a separate path; an earlier word ending in `/` is a separate folder. Absolute paths keep the own-line and marker forms, including the residual ambiguity `/home/me/proj contains a.md`, plus the same path labels; any other label (`Error:`, `Note:`, `Saved the report to:`) defers an absolute path too. A rooted candidate needs a directory before its first space, so `/review the changes in src/a.ts` is prose. A `:` followed by a digit inside an earlier word is a line suffix (`a.ts:42:See b.md`, grep and compiler output) and ends the path there, a run of spaces is a column gap rather than a name, and a leading `./` is not a segment. An unlinkable candidate reserves its span only while its last word holds a `/`, so `/usr/bin/env node build.js` keeps its ordinary links while `/etc/secret files/passwords.txt` links nothing. The response viewer uses a leading `` path label in a paragraph or list item, or one starting a later line of a paragraph (after a `
`, since chat renders with `breaks: true`), as detection context without changing the label or displayed text; paths split across other inline elements remain detected per text node. Tests feed vendored marked output into the jsdom linkifier, so consumed bullets and bold labels are covered. Trailing whitespace uses linear `trimEnd()` before the length cap. **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/app.js b/src/web/public/app.js index 7aee5636..131f2837 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -3015,18 +3015,23 @@ class CodemanApp { // invalidates the traversal. const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT); const targets = []; + // Same promise as the guard above: a stale constants.js without the label + // pattern only loses the carried label, it never throws. + const labelPattern = typeof SPACED_FILE_PATH_LABEL_PATTERN !== 'undefined' ? SPACED_FILE_PATH_LABEL_PATTERN : null; for (let node = walker.nextNode(); node; node = walker.nextNode()) { if (node.parentElement?.closest('a')) continue; // Markdown puts **Saved to:** in a separate node. Carry only - // an explicit leading path label into detection; never arbitrary prose - // or a label from another block. The label itself stays untouched. + // an explicit path label that starts its line into detection; never + // arbitrary prose or a label from another block. Chat renders with + // `breaks: true`, so a later line of a paragraph starts after a
. + // The label itself stays untouched. const label = node.previousSibling; - const beforeLabel = label?.previousSibling; + let beforeLabel = label?.previousSibling; + if (beforeLabel?.nodeType === 3 && !beforeLabel.nodeValue.trim()) beforeLabel = beforeLabel.previousSibling; const labelText = label?.nodeName === 'STRONG' ? label.textContent : ''; const leadingLabel = - /^(?:P|LI)$/.test(node.parentElement?.tagName || '') && - (!beforeLabel || (beforeLabel.nodeType === 3 && !beforeLabel.nodeValue.trim() && !beforeLabel.previousSibling)); - const prefix = leadingLabel && SPACED_FILE_PATH_LABEL_PATTERN.test(labelText) ? `${labelText} ` : ''; + /^(?:P|LI)$/.test(node.parentElement?.tagName || '') && (!beforeLabel || beforeLabel.nodeName === 'BR'); + const prefix = leadingLabel && labelPattern?.test(labelText) ? `${labelText} ` : ''; const found = findFilePathLinks(prefix + (node.nodeValue || '')) .filter((hit) => hit.index >= prefix.length) .map((hit) => ({ ...hit, index: hit.index - prefix.length })); diff --git a/src/web/public/constants.js b/src/web/public/constants.js index db3006b4..d8b87884 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -2234,17 +2234,24 @@ const SPACED_FILE_PATH_LABEL_PATTERN = * - /mnt/c/My Docs/report.docx after a list marker (`-`, `*`, `•`, `1.`, `2)`) * Saved to: 03 Attachments/Build SIPOC Design/plan.pptx after a short label (one to four words + `:`) * - * An absolute path may stand alone or follow a marker. Relative and `~/` - * paths require an explicit path label (SPACED_FILE_PATH_LABEL_PATTERN), with - * an optional marker before it: generic labels and bullets also introduce prose. One trailing - * sentence mark is left out. The candidate must read as ONE path: it ends on a - * known extension, and no earlier word ends on one or `/`; later words cannot - * start a new `/`, `~/`, `./` or `../` path. No earlier word ends - * in `:` or is an arrow, so two paths on a line are never merged. + * An absolute path may stand alone or follow a marker. Any label, before an + * absolute, relative or `~/` path, must be an explicit path label + * (SPACED_FILE_PATH_LABEL_PATTERN), with an optional marker before it: generic + * labels (`Error:`, `Note:`) and bare bullets also introduce prose, so a relative + * path needs a path label to be taken spaced. One trailing sentence mark is left + * out, and a leading `./` is not a segment. The candidate must read as ONE path: + * it ends on a known extension, a rooted one has a directory before its first + * space, no earlier word ends on an extension or `/`, and none holds a `:` and a + * digit (`a.ts:42`, a line suffix ends the path). Later words cannot start a new + * `/`, `~/`, `./` or `../` path, no earlier word ends in `:` or is an arrow, and + * a run of spaces (a column gap) is refused, so two paths on a line are never + * merged. * * A candidate that is one path but not a linkable one (an unknown root such as - * `/etc`, a `.`/`..`/empty segment) comes back with `link: false`: its span links - * nothing, rather than the fragment after its last space. + * `/etc`, a `.`/`..`/empty segment) comes back with `link: false` when its last + * word still holds a `/`: its span links nothing, rather than the fragment after + * its last space. Otherwise (`/usr/bin/env node build.js`) the line is left to + * the ordinary detectors. * * Linear: one bounded prefix match, then string scans. * @@ -2257,18 +2264,29 @@ function spacedFilePathOnLine(line) { const index = prefix[0].length; const path = text.slice(index).trimEnd().replace(/[.,;:!?]$/, ''); if (!path.includes(' ') || !path.includes('/') || path.length > 1000) return null; - if (!path.startsWith('/') && !SPACED_FILE_PATH_LABEL_PATTERN.test(prefix[2] || '')) return null; + // After a label, only a path label introduces a path; with no label, only an + // absolute path may stand alone. `Error: /a/b contains c.md` is prose. + if (prefix[2] ? !SPACED_FILE_PATH_LABEL_PATTERN.test(prefix[2]) : !path.startsWith('/')) return null; if (!PATH_TOKEN_ENDS_WITH_EXTENSION.test(path)) return null; - if (/[\t"'`<>|;&\x00-\x1f\\]/.test(path) || path.includes('://')) return null; + // A run of spaces is a column gap (`git status`, `ls -l`), not a name. + if (/[\t"'`<>|;&\x00-\x1f\\]/.test(path) || path.includes('://') || path.includes(' ')) return null; const words = path.split(' '); + // A rooted candidate must have a directory before its first space: `/usr/bin/env node a.js` + // or `/review the changes in src/a.ts` is a command, not a spaced path. + if (path.startsWith('/') && !/^\/[^/]+\//.test(words[0])) return null; for (const word of words.slice(0, -1)) { if (word.endsWith('/') || word.endsWith(':') || /^(?:—|–|→|=>|->|<-)$/.test(word)) return null; + // `a.ts:42` (grep -n, compiler output): the path already ended at the line suffix. + if (/:\d/.test(word)) return null; if (PATH_TOKEN_ENDS_WITH_EXTENSION.test(word.replace(/[.,;:!?)\]}]+$/, ''))) return null; } if (words.slice(1).some((word) => /^(?:\/|~\/|\.{1,2}\/)/.test(word))) return null; const rooted = SPACED_PATH_ROOT.test(path) || path.startsWith('~/'); - const segments = path.replace(/^~?\//, '').split('/'); + const segments = path.replace(/^(?:~?\/|\.\/)/, '').split('/'); const link = (rooted || !path.startsWith('/')) && segments.every((seg) => seg && seg !== '.' && seg !== '..'); + // Reserve an unlinkable span only when it still reads as a path to its end; + // otherwise leave the line to the ordinary detectors. + if (!link && !words[words.length - 1].includes('/')) return null; return { path, index, link }; } diff --git a/test/response-viewer-file-links.test.ts b/test/response-viewer-file-links.test.ts index fa08cca2..36f454d2 100644 --- a/test/response-viewer-file-links.test.ts +++ b/test/response-viewer-file-links.test.ts @@ -58,7 +58,9 @@ vm.runInContext( readFileSync(resolve(import.meta.dirname, '../src/web/public/vendor/marked.min.js'), 'utf8'), markedContext ); -const marked = markedContext.marked as { parse(text: string): string }; +const marked = markedContext.marked as { parse(text: string, options?: object): string }; +/** The options `_renderMarkdown` passes, so the tests see chat's real markup (a newline becomes `
`). */ +const renderChat = (markdown: string) => marked.parse(markdown, { breaks: true, gfm: true }); const CodemanApp = loadCodemanAppClass(); const APP_SOURCE = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); @@ -89,8 +91,16 @@ describe('response viewer file-path linkifier', () => { ['**Saved to:** /mnt/c/My Docs/a.docx', ['/mnt/c/My Docs/a.docx']], ['**Tests:** 45/45 pass in test/foo.test.ts', ['test/foo.test.ts']], ['**Error:** I/O failure reading data.json', ['data.json']], + [ + 'Done.\n**Saved to:** 03 Attachments/Build SIPOC Design/Build-SIPOC.pptx', + ['03 Attachments/Build SIPOC Design/Build-SIPOC.pptx'], + ], + [ + '**Size:** 2 MB\n**Saved to:** 03 Attachments/Build SIPOC Design/Build-SIPOC.pptx', + ['03 Attachments/Build SIPOC Design/Build-SIPOC.pptx'], + ], ])('links rendered Markdown with consumed markers and labels: %s', (markdown, expected) => { - const html = marked.parse(markdown); + const html = renderChat(markdown); const original = document.createElement('div'); original.innerHTML = html; const root = linkify(html); @@ -103,8 +113,9 @@ describe('response viewer file-path linkifier', () => { for (const markdown of [ 'See **Saved to:** 03 Attachments/Build SIPOC Design/Build-SIPOC.pptx', '**Saved to:**\n\n03 Attachments/Build SIPOC Design/Build-SIPOC.pptx', + 'Done.\nSee **Saved to:** 03 Attachments/Build SIPOC Design/Build-SIPOC.pptx', ]) { - expect(paths(linkify(marked.parse(markdown))).map((a) => a.getAttribute('data-path'))).toEqual([ + expect(paths(linkify(renderChat(markdown))).map((a) => a.getAttribute('data-path'))).toEqual([ 'Design/Build-SIPOC.pptx', ]); } diff --git a/test/spaced-file-path-links.test.ts b/test/spaced-file-path-links.test.ts index 25675c5b..a5aca00d 100644 --- a/test/spaced-file-path-links.test.ts +++ b/test/spaced-file-path-links.test.ts @@ -67,12 +67,15 @@ describe('spaced file paths', () => { expect(paths(`${label} ${p}`)).toEqual([p]); }); - it.each(['- ', '* ', '• ', '1. ', '2) '])('links a spaced path after the list marker %j', (marker) => { - const rel = '03 Attachments/Build SIPOC Design/Build-SIPOC.pptx'; - expect(paths(`${marker}${rel}`)).toEqual(['Design/Build-SIPOC.pptx']); - expect(paths(`${marker}Path: ${rel}`)).toEqual([rel]); - expect(paths(` ${marker}${LONG}`)).toEqual([LONG]); - }); + it.each(['- ', '* ', '• ', '1. ', '2) '])( + 'defers a relative spaced path after a bare list marker %j, links it after a path label', + (marker) => { + const rel = '03 Attachments/Build SIPOC Design/Build-SIPOC.pptx'; + expect(paths(`${marker}${rel}`)).toEqual(['Design/Build-SIPOC.pptx']); + expect(paths(`${marker}Path: ${rel}`)).toEqual([rel]); + expect(paths(` ${marker}${LONG}`)).toEqual([LONG]); + } + ); it('works line by line in code-block text, where list markers remain literal', () => { const text = `Wrote two files:\n- ${LONG}\n- Path: notes/a b/c.md\n`; @@ -165,6 +168,64 @@ describe('spaced file paths', () => { }); }); +/** The links master returned for `line`, in order: `[path, folder?]` pairs, each at its first occurrence. */ +const masterLinks = (line: string, ...links: Array<[string, true?]>) => + links.map(([path, folder]) => ({ path, index: line.indexOf(path), ...(folder ? { folder } : {}) })); + +describe('spaced file paths: shapes that keep master links', () => { + // grep -n, rg -n and compiler output: a `:` plus a digit inside an earlier + // word is a line suffix, so the path already ended there. + it.each([ + ['/home/me/proj/README.md:12:See docs/setup.md', '/home/me/proj/README.md', 'docs/setup.md'], + [ + ' /home/me/proj/CLAUDE.md:42:Deep detail lives in docs/architecture-invariants.md', + '/home/me/proj/CLAUDE.md', + 'docs/architecture-invariants.md', + ], + ['/home/me/proj/src/a.ts:42:5 - error in b.ts', '/home/me/proj/src/a.ts', 'b.ts'], + ['/home/me/proj/src/index.ts:10 updated config.json', '/home/me/proj/src/index.ts', 'config.json'], + ])('ends the path at a line suffix: %s', (line, first, second) => { + expect(findFilePathLinks(line)).toEqual(masterLinks(line, [first], [second])); + }); + + // A generic label introduces prose for an absolute path as much as a relative one. + it.each([ + ['Error: /home/me/proj contains a.md', '/home/me/proj', 'a.md'], + ['Warning: /home/me/proj is not tracked, see notes.md', '/home/me/proj', 'notes.md'], + ['Working directory: /home/me/proj, see README.md', '/home/me/proj', 'README.md'], + ['- Note: /tmp/out has the logs in run.log', '/tmp/out', 'run.log'], + ])('defers an absolute path after a generic label: %s', (line, folder, file) => { + expect(findFilePathLinks(line)).toEqual(masterLinks(line, [folder, true], [file])); + }); + + it('documented trade-off: an absolute spaced path after a non-path label splits as on master', () => { + const line = 'Saved the report to: /mnt/c/My Docs/a.docx'; + expect(findFilePathLinks(line)).toEqual(masterLinks(line, ['/mnt/c/My', true], ['Docs/a.docx'])); + }); + + it.each([ + ['/usr/bin/python3 script.py', 'script.py'], + ['/usr/bin/env node build.js', 'build.js'], + ['/review the changes in src/foo.ts', 'src/foo.ts'], + ])('does not reserve a command line that only looks like one path: %s', (line, file) => { + expect(findFilePathLinks(line)).toEqual(masterLinks(line, [file])); + }); + + it('still refuses an unlinkable spaced path that reads as one path to its end', () => { + expect(paths('/etc/secret files/passwords.txt')).toEqual([]); + }); + + it('does not merge a column gap into one path', () => { + const line = '/home/me/proj modified notes.md'; + expect(findFilePathLinks(line)).toEqual(masterLinks(line, ['/home/me/proj', true], ['notes.md'])); + }); + + it('reads a leading ./ as the workspace, not a segment', () => { + expect(paths('Saved to: ./My Docs/plan.md')).toEqual(['./My Docs/plan.md']); + expect(paths('Saved to: ./notes.md')).toEqual(['./notes.md']); + }); +}); + describe('terminal link provider with spaced paths', () => { type Link = { text: string; activate: (event: unknown, text: string) => void }; const TERMINAL_UI = readFileSync(resolve(import.meta.dirname, '../src/web/public/terminal-ui.js'), 'utf8');