diff --git a/.changeset/Repair-FilePathLinks.md b/.changeset/Repair-FilePathLinks.md index c9cafa3b..3418dccc 100644 --- a/.changeset/Repair-FilePathLinks.md +++ b/.changeset/Repair-FilePathLinks.md @@ -3,3 +3,5 @@ --- Keep parenthesised filenames with spaces clickable as one complete file path, including `.conf` configuration files. For an ambiguous hard wrap, check the original concatenated filename through the existing guarded attachment route when clicked, and try the spaced filename only after a not-found response. Every original response other than not-found retains the normal opener, including workspace files whose extensions the attachment registry rejects. A spaced alternative opens only after successful registration. Preserve clickable row positions and share the spaced-path detector between the terminal and Response Viewer. + +Restrict comma-separated-list detection to parenthesised candidates so existing own-line, list-marker and labelled paths with commas remain whole. A rejected workspace path is named in the attachment-lookup error without describing it as outside the workspace. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 31e792f1..cf823d05 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -413,7 +413,7 @@ A file path an agent prints is a link on both surfaces it can appear on, and cli 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 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. An earlier word holding a known extension anywhere (`a.ts#L12 foo.ts`) ends the path at that word, 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 run of `./` or `../` is not a segment (`Saved to: ../My Docs/plan.md` links whole; an interior `..` is still refused). 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. A path label vouches that a path follows, not for the words before it: a relative candidate whose only `/` word is its last is prose after the label (`Wrote: 120 lines to src/foo.ts`, `Path: see src/foo.ts`) and defers to the ordinary detectors. The cost is a labelled relative path whose only space is in its top directory, `Saved to: My Docs/plan.md` (and `Saved to: 03 Attachments/plan.pptx`), which splits as on master; `03 Attachments/Build SIPOC Design/Build-SIPOC.pptx`, `notes/a b/c.md`, `~/`, `./` and `../` forms and absolute paths still link whole. An earlier word that is a lowercase connective or verb (`SPACED_PATH_PROSE_WORD`: `and`, `or`, `to`, `in`, `the`, `is`, `has`, `contains`, `then`, `as` and a few more) or a bare `=` makes the candidate a sentence, in every shape: `/home/me/proj contains a.md`, `- /tmp/out has the logs in run.log`, `Path: /home/me/proj/out contains a.md`, `Files: src/components and utils/x.ts`, `Wrote: 3 files to docs/design notes/overview.md`. The cost is a name that uses one of those words (`Pros and Cons.md`, `How to deploy.md`, `Documents and Settings`), which splits as on master; `at` (`Screenshot 2026-10-24 at 10.00.00.png`) and `of` (`Copy of report.docx`) are left out of the list on purpose. What remains ambiguous is an own-line absolute path followed by words that hold none of them (`/home/me/proj/out report.pdf` reads like `/home/me/docs/Read Me.txt`), which links as one path. A command line is refused too: a first word that is an executable under `bin/`, `.bin/`, `scripts/` or `tools/` (`/opt/homebrew/bin/node server.js`, `/home/me/proj/scripts/deploy prod.yml`) or any `-x`/`--x` flag word (`/home/me/proj/node_modules/.bin/tsc -p tsconfig.json`); a bare `-` (`Docs - Copy`, `OneDrive - Contoso`) is not a flag. So is a shell prompt that prints an absolute working directory (WSL's `/mnt/c/...` is outside `$HOME`, so every earlier command in a shell's scrollback has one): a first word ending in `$` (`/home/me/proj$ cat notes.md`) and any earlier word with no letter or digit other than a bare `-` (`/mnt/c/proj ❯ code notes.md`, `/home/me/proj $ cat notes.md`, `/home/me/proj/out ✓ report.pdf`) refuse the candidate, while `Program Files (x86)` and `Docs - Copy` still link whole. The response viewer carries any leading `` label in a paragraph or list item, or one starting a later line of a paragraph (after a `
`, since chat renders with `breaks: true`), into detection without changing the label or displayed text, and `spacedFilePathOnLine()` judges it as it judges the same line in the terminal (`**Note:** /home/me/proj/out report.pdf` splits). A text node that follows any other inline element starts mid-line, so a placeholder word stands in for what precedes it (``Use `--config` /home/me/proj/cfg data.json`` splits as in the terminal), while a path inside inline code still links whole; paths split across 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. -Parentheses also bound an inline spaced path, such as `Updated (/mnt/c/My Docs/report.conf)`. The whole candidate is reserved before fragment detectors run; existing root, prose and traversal guards still apply. A comma at the end of an earlier word marks a list, not one filename. +Parentheses also bound an inline spaced path, such as `Updated (/mnt/c/My Docs/report.conf)`. The whole candidate is reserved before fragment detectors run; existing root, prose and traversal guards still apply. Inside parentheses only, a comma at the end of an earlier word marks a list, not one filename. Own-line, list-marker and labelled paths retain commas in filenames and directory names. Hard-wrapped filenames keep the existing concatenation without inserted spaces. A parenthesised final filename broken between words also carries a spaced alternative without changing its cell mapping. On activation only, the terminal checks the original path through the existing guarded attachment registration route (`notify: false`), tries the alternative only after HTTP 404, and retains the clicked session throughout. If both exist, the original wins. Every original result other than HTTP 404 goes to the normal opener, preserving workspace previews and log viewing even for extensions the attachment registry rejects; for an external path that opener retains the same attachment guards and errors. After an original 404, a spaced alternative opens only if it registers successfully. Hover performs no lookup. Text alone cannot distinguish the two filenames, so this uses the filesystem result rather than a spacing guess. diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 36a1c7d1..8a645941 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -2210,7 +2210,7 @@ function findFilePathLinks(text) { // Parentheses bound an inline path just as a whole line bounds a standalone path. const parenthesised = /\(([^()\r\n]{1,1000})\)/g; while ((match = parenthesised.exec(line)) !== null) { - reserve(spacedFilePathOnLine(match[1]), lineStart + match.index + 1); + reserve(spacedFilePathOnLine(match[1], { parenthesised: true }), lineStart + match.index + 1); } lineStart += line.length + 1; } @@ -2263,7 +2263,7 @@ const SPACED_FILE_PATH_LABEL_PATTERN = * it ends on a known extension, a rooted one has a directory before its first * space, no earlier word holds an extension (`a.md,`, `a.ts#L12`) or ends in `/`, * 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 or has + * `/`, `~/`, `./` or `../` path, no earlier word ends in `:` (or `,` inside parentheses), is an arrow or has * no letter or digit (a prompt glyph such as `❯` or `$`; a bare `-` is kept), and * a run of spaces (a column gap) is refused, so two paths on a line are never * merged. A relative candidate whose only `/` word is its last is prose after the @@ -2285,9 +2285,10 @@ const SPACED_FILE_PATH_LABEL_PATTERN = * Linear: one bounded prefix match, then string scans. * * @param {string} line One line, no newline. + * @param {{parenthesised?: boolean}} [options] Refuse comma-separated lists inside parentheses only. * @returns {{path: string, index: number, link: boolean} | null} */ -function spacedFilePathOnLine(line) { +function spacedFilePathOnLine(line, options = {}) { const text = String(line || ''); const prefix = /^[ \t]*((?:\d{1,3}[.)]|[-*•])[ \t]+)?((?:[A-Za-z][\w-]{0,30}(?: [\w-]{1,30}){0,3}):[ \t]+)?/.exec(text); const index = prefix[0].length; @@ -2310,7 +2311,8 @@ function spacedFilePathOnLine(line) { return null; } for (const word of words.slice(0, -1)) { - if (word.endsWith('/') || /[,:]$/.test(word) || /^(?:—|–|→|=>|->|<-|=)$/.test(word)) return null; + if (word.endsWith('/') || word.endsWith(':') || /^(?:—|–|→|=>|->|<-|=)$/.test(word)) return null; + if (options.parenthesised && word.endsWith(',')) return null; // A shell prompt that prints its working directory in full (`/home/me/proj$ cat // notes.md`; WSL's `/mnt/c/...` is outside `$HOME`) is a command line too, and so is // one whose directory has a space in it (`/mnt/c/My Docs$ ls notes.md`): the `$` @@ -2633,7 +2635,7 @@ function terminalPathLine(buffer, row, cols, maxRows) { if (opening >= 0 && closing > 0) { const before = text.slice(opening + 1); const after = next.text.slice(0, closing).trimStart(); - const spaced = spacedFilePathOnLine(`${before} ${after}`); + const spaced = spacedFilePathOnLine(`${before} ${after}`, { parenthesised: true }); if (!before.includes(')') && !/[/-]$/.test(before) && !after.includes('/') && spaced?.link) { alternatives.push({ path: before + after, spacedPath: spaced.path }); } diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 5dc971e6..e46baa4a 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -4195,7 +4195,7 @@ Object.assign(CodemanApp.prototype, { }, /** - * Register an out-of-workspace path as a live external attachment and return + * Register a file path as a live external attachment and return * its id, so the preview can render it through the by-id attachment routes. * * `notify: false` keeps this quiet: the caller is already opening the file in @@ -4220,12 +4220,15 @@ Object.assign(CodemanApp.prototype, { const reason = result?.error || `Cannot open this file (HTTP ${res.status})`; // The registry's type answer is a policy term, not an explanation, and the // user just clicked a file they can see on disk. Say what IS previewable - // from outside the workspace instead. + // through this lookup instead, using the path's actual workspace scope. if (/unsupported/i.test(reason)) { const ext = (filePath.split('.').pop() || '').toLowerCase(); + const target = this._isExternalPreviewPath(filePath, sessionId) + ? `.${ext} from outside the session workspace` + : `"${filePath}" through the attachment lookup`; return { status: res.status, - error: `Cannot preview .${ext} from outside the session workspace (images, video, audio, PDF, Office documents and text files only).`, + error: `Cannot preview ${target} (images, video, audio, PDF, Office documents and text files only).`, }; } return { error: reason, status: res.status }; diff --git a/test/folder-grid-ui.test.ts b/test/folder-grid-ui.test.ts index cbfdc8e0..131bcdf1 100644 --- a/test/folder-grid-ui.test.ts +++ b/test/folder-grid-ui.test.ts @@ -213,6 +213,38 @@ function loadApp(fetchImpl: (url: string, init?: { body?: string }) => unknown) } describe('guarded attachment registration results', () => { + it.each(['svg', 'markdown', 'avif', 'bmp', 'ico', 'zip'])( + 'names a rejected workspace .%s path without describing it as external', + async (extension) => { + const { app } = loadApp(async () => ({ + ok: false, + status: 400, + json: async () => ({ error: 'Unsupported attachment type' }), + })); + const path = `/home/joe/wiki/out/system architecture.${extension}`; + const result = await Object.getPrototypeOf(app)._registerExternalPreview.call(app, path, 's1'); + expect(result.status).toBe(400); + expect(result.error).toContain(path); + expect(result.error).not.toContain('outside the session workspace'); + } + ); + + it('retains the external-file explanation for a rejected external type', async () => { + const { app } = loadApp(async () => ({ + ok: false, + status: 400, + json: async () => ({ error: 'Unsupported attachment type' }), + })); + const result = await Object.getPrototypeOf(app)._registerExternalPreview.call( + app, + '/mnt/data/system architecture.svg', + 's1' + ); + expect(result.error).toBe( + 'Cannot preview .svg from outside the session workspace (images, video, audio, PDF, Office documents and text files only).' + ); + }); + it.each([404, 403, 500, 503])('preserves HTTP %s for the activation decision', async (status) => { const { app, fetch } = loadApp(async () => ({ ok: false, diff --git a/test/spaced-file-path-links.test.ts b/test/spaced-file-path-links.test.ts index a768bb58..062c58bc 100644 --- a/test/spaced-file-path-links.test.ts +++ b/test/spaced-file-path-links.test.ts @@ -46,6 +46,14 @@ describe('spaced file paths', () => { expect(paths('Updated (/home/me/proj, config.json)')).toEqual(['/home/me/proj', 'config.json']); }); + it.each([ + ['/mnt/c/Users/me/Smith, John/report.docx', '/mnt/c/Users/me/Smith, John/report.docx'], + ['- /home/me/Clients/Acme, Inc/contract.pdf', '/home/me/Clients/Acme, Inc/contract.pdf'], + ['Saved to: /home/me/proj/Invoice 12, final.pdf', '/home/me/proj/Invoice 12, final.pdf'], + ])('keeps commas in a non-parenthesised spaced path: %s', (line, path) => { + expect(findFilePathLinks(line)).toEqual([{ path, index: line.indexOf(path) }]); + }); + it('links an own-line absolute path with spaces whole', () => { expect(paths(LONG)).toEqual([LONG]); expect(paths('/mnt/c/proj/My Big Docs/report.docx')).toEqual(['/mnt/c/proj/My Big Docs/report.docx']); diff --git a/test/wrapped-file-paths.browser.test.ts b/test/wrapped-file-paths.browser.test.ts index 3890bae7..14dddf4f 100644 --- a/test/wrapped-file-paths.browser.test.ts +++ b/test/wrapped-file-paths.browser.test.ts @@ -1,4 +1,4 @@ -/** Real xterm clicks must open the complete spaced filename from either wrapped row. */ +/** Real xterm clicks resolve the complete filename or report its lookup error from either wrapped row. */ import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -22,12 +22,14 @@ describe('wrapped configuration file links (Chromium)', () => { { wrap: 'hard', originalExists: true, extension: 'conf', registrationStatus: 200 }, { wrap: 'hard', originalExists: true, extension: 'svg', registrationStatus: 400 }, { wrap: 'hard', originalExists: true, extension: 'markdown', registrationStatus: 400 }, + { wrap: 'hard', originalExists: false, extension: 'svg', registrationStatus: 400 }, ])( - 'opens the existing .$extension file from both rows: $wrap, original exists: $originalExists', + 'handles .$extension links on both rows: $wrap, original exists: $originalExists', async ({ wrap, originalExists, extension, registrationStatus }) => { const path = basePath.replace('.conf', `.${extension}`); const original = path.replace('Kobo eReader', 'KoboeReader'); const expected = wrap === 'hard' && originalExists ? original : path; + const lookupFails = registrationStatus === 400 && !originalExists; const lookups: string[] = []; const page = await browser.newPage({ viewport: { width: 1920, height: 600 } }); try { @@ -106,7 +108,16 @@ describe('wrapped configuration file links (Chromium)', () => { await page.waitForFunction(() => (window as any).app._linkHovered === true); await page.mouse.click(point.x, point.y); await page.waitForFunction((count) => (window as any).opened.length === count, row + 1); - expect(await page.evaluate(() => (window as any).opened)).toEqual(Array(row + 1).fill(expected)); + const opened = await page.evaluate(() => (window as any).opened as string[]); + if (lookupFails) { + for (const message of opened) { + expect(message).toMatch(/^error:/); + expect(message).toContain(path); + expect(message).not.toContain('outside the session workspace'); + } + } else { + expect(opened).toEqual(Array(row + 1).fill(expected)); + } await page.mouse.move(0, 0); } expect(lookups).toEqual(