Skip to content

fix(links): preserve wrapped configuration filenames - #735

Open
opticon454 wants to merge 5 commits into
Ark0N:masterfrom
opticon454:fix/wrapped-file-paths
Open

opticon454 wants to merge 5 commits into
Ark0N:masterfrom
opticon454:fix/wrapped-file-paths

Conversation

@opticon454

@opticon454 opticon454 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

A terminal message such as:

Prepared a corrected configuration (/mnt/data/Kobo/Kobo
  eReader.conf).

linked /mnt/data/Kobo/Kobo as a folder and failed with "Folder not found". The shared path detector did not recognise .conf or parenthesised filenames containing spaces.

Recognise .conf and reserve each complete parenthesised spaced path before fragment detectors run. This fixes single-line and soft-wrapped paths in the terminal and inline paths in the Response Viewer, retaining existing root, prose and traversal guards. Only parenthesised candidates reject an earlier word ending in a comma; own-line, list-marker and labelled paths keep commas in directory names and filenames.

For CLI-inserted hard breaks, preserve the existing concatenation without inserting spaces. The Kobo example is textually indistinguishable from a break inside an unspaced filename. When clicked, check the original filename through the existing guarded attachment registration route, with notify: false, and try the spaced alternative only after HTTP 404. If both exist, the original wins. Every original result other than HTTP 404 goes to the normal opener, preserving workspace previews and log viewing for types the attachment registry rejects. External files retain the normal opener’s guarded registration and errors. After an original 404, the spaced alternative opens only if registration succeeds; a second 404 hands the original path to the normal opener. Labels, list markers and leading indentation inside parentheses are excluded from the lookup key. Hover performs no lookup and asynchronous resolution retains the clicked session. Both rows retain their correct clickable cells.

Regression coverage includes Ark0N's two unspaced filename examples, a three-row chain, the Kobo fallback, refusal handling, session affinity, workspace SVG/Markdown/AVIF/BMP/ICO/ZIP openers after HTTP 400, accurate workspace lookup-error messages, both filenames missing, labelled and list-marked hard wraps, the three reported comma-containing path forms, shared chat linkification, separate parenthesised paths, comma-separated lists, blocked roots and traversal. Chromium uses real xterm mouse clicks on both rows and the real client registration helper, with fixture HTTP responses for missing and existing files. No live filesystem or CLI capture was used to infer a wrapping heuristic.

Removed BUGS.md, updated the shared detector documentation and architecture invariants, and included a patch changeset.

Validation at 3847e377d8a4e2f331b363827470852a99b199b9:

  • Full CI unit/integration suite: 13,548 passed, 12 skipped, across 665 passing files and one skipped file; no failures.
  • Focused link regressions: 501 passed across nine files.
  • xterm-zerolag-input package: 282 passed across ten files.
  • Chromium: 8 passed, covering soft wraps, hard wraps where the original filename is present or absent, workspace SVG/Markdown activation after HTTP 400, the error for an unregistered workspace SVG alternative, both filenames missing, and a labelled hard-wrap fallback.
  • Lockfile, typecheck, lint, frontend syntax, browser exclusions, formatting, public assets, CLI catalogue and installer syntax passed. Bash 3.2 installer checks also passed on the identical release-base installer.

Rebased onto release 1.47.0, upstream/master f1f7eec4d6a54c73822d118262712535eea5fd28. This PR does not carry src/web/public/whats-new.json, so regeneration was not needed.

The initial regression tests failed before the correction. The remaining browser, mobile and performance suites were not run. No deployment is included.

Fixes #733. Current implementation: 3847e377d8a4e2f331b363827470852a99b199b9. The filesystem-lookup policy remains subject to the maintainer’s decision.

@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thank you for this, and for the careful tests (the Chromium click test on both rows is a nice touch). The PR makes .conf files linkable, keeps a parenthesised path with spaces whole in the terminal and the Response Viewer, and restores the space when a CLI wraps such a filename onto the next row.

The single-line and soft-wrap cases work well. One change is needed before this can go in:

1. src/web/public/constants.js:2571 (pathContinuation): the new join-spaced branch regresses parenthesised paths without spaces. It runs before the existing unspaced join. So a path in parentheses that a CLI hard-breaks inside its filename now gets a space inserted. With these two rows:

Saved the screenshot (/home/me/proj/out/ghost1b_game
  play.png).

master links and opens /home/me/proj/out/ghost1b_gameplay.png from either row. This branch opens /home/me/proj/out/ghost1b_game play.png, which does not exist. The same happens with ⎿ Wrote (/mnt/build/neon/builds/captures/ghost1b_gamepl over ay.png) ok, and with 3+ row chains, since the accumulated text still holds the (. I checked this with your linksOn harness from test/file-path-link-forms.test.ts: both cases pass on master and fail here. Claude Code breaks long tokens at its wrap width, and on a phone that is routine, so this would hit unspaced paths, which are the common case. The ask:

  • An unspaced join that already reaches an extension must keep working as it does on master (docs/architecture-invariants.md line 412 documents it). Please add the two cases above as regression tests.
  • Your Kobo hard-wrap case ((…/Kobo over eReader.conf)) has the same shape on screen as a break inside a name, so join-spaced needs a signal that separates them. One idea to test: a word-wrapper moves a token whole when it fits the row, so a break inside a parenthesised token shorter than the broken row should be a break at a space. Whether that holds for Claude Code's tool-output wrapping needs a real capture.
  • If no signal separates them reliably, say so here rather than picking one in code. Which reading should win for an ambiguous hard wrap is an open question on my side, not decided yet.

Two smaller things:

2. BUGS.md: please drop it. It is unrelated to the fix, and the repository root is kept to the files tools require there (see the note at the top of CLAUDE.md).

3. Docs for the new rules: spacedFilePathOnLine()'s JSDoc (constants.js:2250) still says a spaced path is recognised in "exactly these shapes", and the parenthesised group is now a fourth one. findFilePathLinks()'s JSDoc and docs/architecture-invariants.md (line 414 for the spaced-path rules, line 412 for the wrap join) should describe the parenthesised reservation and the spaced continuation, including whatever trade-off the final rule accepts.

Optional, if you are in there anyway: Updated (/home/me/proj, config.json) now links as one path, because spacedFilePathOnLine() never refuses an earlier word ending in , (master already does this for the whole-line form). Refusing it like : would close that. In terminalPathLine(), the segment loop (line 2667) re-runs continues() only to learn whether to insert the space. Carrying the kind out of extend() would avoid computing it twice.

Once point 1 is fixed, BUGS.md is gone and the docs are updated, this is ready to merge. Thanks again for the thorough issue and repro in #733.

@opticon454
opticon454 force-pushed the fix/wrapped-file-paths branch from 0cbbb8e to 97d53f6 Compare October 28, 2026 13:56
@opticon454

Copy link
Copy Markdown
Contributor Author

Addressed the review in 97d53f66, after rebasing onto release 1.47.0 (f1f7eec4).

The two hard-break shapes do not supply a reliable text-only distinction. I removed join-spaced and retained the original unspaced link text and cell mapping. On activation, the existing guarded attachment route checks the concatenated filename first; only HTTP 404 permits a check of the spaced alternative. If both exist, the original wins. Permission, transport and server errors stop resolution. There is no filesystem lookup on hover, and the clicked session is retained across the requests. This uses filesystem results rather than an unverified CLI wrapping heuristic; I did not use a live CLI capture to justify the policy.

Added both reported unspaced examples and a three-row chain, plus Kobo fallback, refusal handling and session-affinity regressions. Real Chromium clicks exercise both rows with the actual registration helper and fixture HTTP responses. Also removed BUGS.md, documented parenthesised reservation and the hard-wrap policy in both JSDoc and the architecture invariants, and rejected the comma-separated list example. The segment loop no longer recomputes continues().

Validation at 97d53f6621f4c36ea8b53669bf4a542755506cf2:

  • Full CI unit/integration suite: 13,522 passed, 12 skipped, 665 passing files and one skipped file; no failures.
  • Focused link regressions: 475 passed across nine files.
  • xterm-zerolag-input package: 282 passed.
  • Chromium: 3 passed, including actual mouse clicks on both rows for soft and hard wraps, with the original path present or absent.
  • Lockfile, typecheck, lint, frontend syntax, browser exclusions, formatting, CLI catalogue, public assets, installer syntax and Bash 3.2 checks passed. Pre-push static checks passed.

The branch was force-pushed with an explicit lease on the previous head. This PR does not carry src/web/public/whats-new.json, so no regeneration was needed. The patch changeset is updated. Fresh GitHub CI is pending. Remaining browser, mobile and performance suites were not run; no deployment was performed.

@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thanks for the quick turnaround, opticon454. This round keeps unspaced hard-wrapped paths working, links a parenthesised path with spaces whole in the terminal and the Response Viewer, adds .conf, makes a three-row wrapped path clickable from every row, and resolves the ambiguous Kobo hard wrap with a lookup through the attachment route on click.

The detector side is in good shape. I ran findFilePathLinks() from master and from this branch over about 35 parenthetical shapes (tool-call lines such as Read(...), Bash(cat a b) and Bash(cd x && cat y), comma and semicolon lists, and/arrow prose, URLs in parentheses, :42 suffixes), and the only differences are the intended ones. BUGS.md is gone, the docs describe the new rules, and both optional points are in. Thank you.

One change is needed:

1. src/web/public/terminal-ui.js:2342: the click-time lookup now gates in-workspace files on the attachment registry's extension list. When a link carries pathAlternatives, every click first calls _registerExternalPreview(), also for a path inside the session's workspace. The attachment registry refuses svg, markdown, avif, bmp, ico and zip with 400 Unsupported attachment type, and master opens all six from the terminal (the first five in the file preview through the workspace routes, zip in the log viewer). With the session in /home/me/proj:

Saved the diagram (/home/me/proj/out/system_archi
  tecture.svg).

master opens the SVG; this branch shows "Cannot preview .svg from outside the session workspace" and opens nothing. I checked this with your linksOn harness plus the real _registerExternalPreview() (fetch answering what the registry answers), and confirmed the 400s against the real registerExternalAttachment(). .markdown hitting it also goes against the rule that a markdown path clicked in the terminal opens the rendered view (src/web/CLAUDE.md, File Viewer text view). The new tests use .png, .log and .conf, which the registry accepts, so they pass. The ask:

  • Make the lookup change the outcome only when the original is missing: if the original registers, or answers anything other than 404, hand the original to the normal opener exactly as master does (for an out-of-workspace path that opener runs the same guarded route again and shows the same refusal, so errors still stop resolution). Open the spaced path only when the original answered 404 and the spaced one registered. Restricting the lookup to _isExternalPreviewPath() paths would also fix it, but drops the spaced fallback inside the workspace.
  • Add a regression test: an in-workspace .svg or .markdown path hard-wrapped in parentheses, registration mocked to answer 400, asserting it opens as on master.

Smaller things, fine to fold into the same push:

  • src/web/public/constants.js:2651: the walk-back no longer stops at the first row that cannot continue, so every hover runs extend() from each of the up to 12 rows above. On a screen of ordinary output that doubles the per-row cost in my measurement (110 to 220 microseconds, 131 to 246 row reads per terminalLinkLine() call). Skipping extend(first) when the trailing token of first.text holds no / gives the same result for less, since pathContinuation() refuses the first join from such a row anyway. The three-row fix itself is right.
  • CLAUDE.md:229, the File-path links paragraph: one sentence for the click-time lookup (activation only, original first, spaced path only after a 404, never on hover), next to the existing note that out-of-workspace paths go through the attachment routes.
  • docs/architecture-invariants.md:419: two blank lines after the new paragraph.
  • Optional: after a successful lookup on an out-of-workspace path, openFilePreview() registers the same path a second time. Passing the attachmentId through would save that round trip.

One question is still open on my side. Last round I asked you to say so if no text signal separates a break inside a name from a break at a space, rather than pick a reading in code, and you did say so. Resolving it with a filesystem lookup on click, with the existing file winning when both exist, is itself a choice about ambiguous hard wraps, and whether Codeman should do that is a decision I have not made yet. That question does not change point 1: as long as the lookup is in the branch, it has to leave master's behaviour alone whenever the original exists.

Next step on your side: the fix for point 1 with its test, plus whichever of the smaller items you want to include.

@opticon454

Copy link
Copy Markdown
Contributor Author

Fixed point 1 in fd0e5249. The attachment lookup no longer decides whether a workspace file may open. A successful registration, any non-404 refusal, or a connection failure hands the original path to the normal opener. Only an original 404 permits the spaced lookup, and the spaced path opens only after successful registration. The clicked session is retained and hover performs no lookup.

The six workspace regressions (svg, markdown, avif, bmp, ico, zip) failed before the correction and now pass from either row after a mocked HTTP 400. SVG and Markdown are also covered by real Chromium clicks using the actual registration helper and HTTP fixture responses. The tests additionally cover non-404 and connection-error hand-off, plus refusal of an unregistered spaced alternative.

Also skipped extend(first) when its trailing token contains no slash, added the activation-only lookup rule beside the attachment-route note in CLAUDE.md, fixed the extra blank line, and updated the architecture documentation and changeset. In a 24-row synthetic ordinary-output check, row reads fell from 7,602 to 5,625 with identical text and row bounds; this is a row-read comparison, not a wall-clock benchmark.

Validation at fd0e52496d7937fe25940b77639db07fd5728cd4:

  • Full CI unit/integration suite: 13,534 passed, 12 skipped, across 665 passing files and one skipped file; no failures.
  • Focused regressions: 487 passed across nine files.
  • Chromium: 5 passed.
  • xterm-zerolag-input: 282 passed.
  • Lockfile, typecheck, lint, frontend syntax, browser exclusions, formatting, public assets, CLI catalogue and installer syntax passed. Pre-push checks passed.

I understand that whether to keep filesystem-based disambiguation is still your decision. This correction preserves the original opener while that policy is under review. I left the optional attachment-ID reuse out of this change; the normal opener still rechecks its guards. Broader browser, mobile and performance suites were not run, and nothing was deployed. Fresh GitHub CI is pending.

@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the third round, opticon454. This version keeps the concatenated filename for hard wraps, hands every non-404 answer to the normal opener so workspace files open exactly as on master, and covers the six registry-rejected extensions with unit tests (and real Chromium clicks for .svg and .markdown). Point 1 from last round is fixed, and the slash check in the walk-back is a good trim.

One change is needed:

1. src/web/public/constants.js:2313: the comma refusal reaches every spaced-path shape, not only the parenthesised one. It lives in spacedFilePathOnLine(), which also judges the own-line, list-marker and label forms, so names that contain a comma now split there. Each of these links whole on master; on this branch each becomes a folder link to a directory that does not exist plus a relative fragment, which is the "Folder not found" symptom from #733 (the Response Viewer shares the detector, so it splits too):

/mnt/c/Users/me/Smith, John/report.docx        -> /mnt/c/Users/me/Smith (folder) + John/report.docx
- /home/me/Clients/Acme, Inc/contract.pdf      -> /home/me/Clients/Acme (folder) + Inc/contract.pdf
Saved to: /home/me/proj/Invoice 12, final.pdf  -> /home/me/proj/Invoice (folder) + final.pdf

That comes from my optional suggestion last round, which was about the parenthesised list Updated (/home/me/proj, config.json). The ask:

  • Apply the , refusal only to the candidate taken from inside parentheses (the reserve(spacedFilePathOnLine(match[1]), ...) call at constants.js:2213), for example through an options argument, and leave the : rule as it is.
  • Add the three lines above as regression tests that link whole, next to the existing Updated (/home/me/proj, config.json) test.
  • Reword the comma sentence in docs/architecture-invariants.md:416 and the JSDoc at constants.js:2266 so they say the rule applies inside parentheses.

A smaller one, fine in the same push:

2. src/web/public/terminal-ui.js:2350: the spaced fallback tells the user an in-workspace file is outside the workspace. With the session in /home/me/proj, rows Saved the diagram (/home/me/proj/out/system and architecture.svg)., the original systemarchitecture.svg missing and system architecture.svg present, the second registration answers 400 (the registry does not take .svg) and the toast reads "Cannot preview .svg from outside the session workspace", although the file is inside it. .markdown, .avif, .bmp, .ico and .zip behave the same. Master does not open this file either, so it is not a regression, and opening the spaced path only after a successful registration stays as specified. The ask is the message: for an in-workspace spaced path (!self._isExternalPreviewPath(path, sessionId)), show a message that names the path and does not say it is outside the workspace, with a test.

Still open on my side: whether Codeman should resolve an ambiguous hard wrap with a filesystem lookup on click at all. That decision is not made, and this round does not depend on it: point 1 is in the detector and is needed either way.

Once point 1 and its tests are in, plus the message fix, the remaining item on this PR is that open question. Thanks again for the careful tests and the clear write-ups.

@opticon454

Copy link
Copy Markdown
Contributor Author

Addressed both findings in 168c15ae.

Comma refusal now uses an explicit parenthesised context, including when checking a hard-wrap alternative. Ordinary own-line, list-marker and labelled candidates retain their existing comma handling, and the colon rule is unchanged. Added your three exact lines beside the parenthesised-list regression; each now links as one full path, while Updated (/home/me/proj, config.json) still produces separate links. Updated the JSDoc and architecture wording to match.

The shared registration helper now checks the path’s workspace scope when explaining an unsupported type. An in-workspace error names the attempted path and the attachment lookup; it does not say the path is external. External-path wording is unchanged. Unit tests cover all six reported extensions, and a Chromium case clicks both rows of the original-404/spaced-SVG-400 flow and checks the displayed error. The alternative still opens only after successful registration.

Validation at 168c15ae087d962439067778f802c8c9bdb28fdb:

  • Full CI unit/integration suite: 13,544 passed, 12 skipped, across 665 passing files and one skipped file; no failures.
  • Focused regressions: 497 passed across nine files.
  • Chromium: 6 passed.
  • xterm-zerolag-input: 282 passed.
  • Lockfile, typecheck, lint, frontend syntax, browser exclusions, formatting, public assets, CLI catalogue, installer syntax and pre-push checks passed.

The three comma regressions and six workspace-message regressions failed before the correction. This leaves the filesystem-lookup policy question for your decision; this push does not settle it. Fresh GitHub CI is pending. Broader browser, mobile and performance suites were not run, and nothing was deployed.

@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the fourth round, opticon454. This PR makes .conf files and parenthesised paths with spaces link whole in the terminal and the Response Viewer, keeps a path that wraps over three or more rows clickable from every row, and resolves the ambiguous hard wrap from #733 with a lookup on click.

Both items from last round are in, and I checked them:

  • The comma refusal now applies only inside parentheses. /mnt/c/Users/me/Smith, John/report.docx, the Acme, Inc list item and Saved to: …/Invoice 12, final.pdf link whole again, as on master, and Updated (/home/me/proj, config.json) still splits.
  • The error for an in-workspace spaced path names the path and no longer says it is outside the workspace.

Typecheck, lint, frontend syntax, formatting, browser exclusions and public assets pass here, the 13 link test files pass, and the new Chromium test passes (6 of 6). I also ran findFilePathLinks() and terminalLinkLine() from master and from this branch over about 60 single-row and multi-row shapes, and every difference is an intended one.

Two small things in the lookup code. Both only matter if the lookup stays:

  1. src/web/public/terminal-ui.js:2351: when neither file exists (the original and the spaced alternative both answer 404), the click shows the server's raw "Attachment file not found" toast, which names no path, where master opens the normal opener for the clicked path. Handing the original to the normal opener when the alternative also answers 404 (return activateFilePath(target, false, sessionId)) makes a dead link behave exactly as on master, so the lookup changes the outcome only when it finds the spaced file. The toast stays right for a spaced file that exists but is refused (400 or 403).
  2. src/web/public/constants.js:2640: path: before + after keeps a label or list marker that opens the parenthesised text. For Saved the notes (Path: /home/me/proj/out/My over Notes.md). the stored path is Path: /home/me/proj/out/MyNotes.md, which never equals the link text, so the lookup does not run. before.slice(spaced.index) + after fixes it.

The remaining item is the question from my earlier comments: whether Codeman should resolve an ambiguous hard wrap with a filesystem lookup on click at all (the original wins when both exist, the spaced name is tried only after a 404). That decision is still open on my side. The detector changes in this PR do not depend on it. Nothing else is needed from you for the points raised so far.

@opticon454

Copy link
Copy Markdown
Contributor Author

Addressed both lookup edge cases in 3847e377.

  • When both registrations return 404, activation hands the original path to the normal opener. Refusals such as 400 or 403 for the spaced alternative still show the error and do not open it.
  • The lookup key excludes the label, list marker and indentation recognised by spacedFilePathOnLine(), using before.slice(spaced.index) + after. Added Path:, a list marker, a marker plus label, and an indented label as regressions, activating from both rows.

The five new regressions failed before the correction. Chromium now also checks a labelled fallback and the double-404 hand-off using real clicks on both rows. Updated the architecture documentation and patch changeset.

Validation at 3847e377d8a4e2f331b363827470852a99b199b9:

  • Full CI unit/integration suite: 13,548 passed, 12 skipped, across 665 passing files and one skipped file; no failures.
  • Focused link regressions: 501 passed across nine files.
  • Chromium: 8 passed.
  • xterm-zerolag-input: 282 passed.
  • Lockfile, typecheck, lint, frontend syntax, browser exclusions, formatting, public assets, CLI catalogue, installer syntax and pre-push checks passed.

The filesystem-lookup policy question remains open for your decision. Fresh GitHub CI is pending. Broader browser, mobile and performance suites were not run, and nothing was deployed.

@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the fifth round, opticon454. This PR makes .conf files and parenthesised paths with spaces link whole in the terminal and the Response Viewer, keeps a path wrapped over three or more rows clickable from every row, and resolves the ambiguous hard wrap from #733 with a lookup through the attachment route on click.

Both items from last round are in, and I checked them:

  • When the original and the spaced filename both answer 404, the click now hands the original to the normal opener, so a dead link behaves as on master. A 400 or 403 for the spaced file still shows the error and opens nothing.
  • The lookup key now drops a label, list marker or indent inside the parentheses (before.slice(spaced.index) + after), and the four new regressions (Path: , - , - Path: , indented Path: ) cover both rows.

Typecheck, lint, frontend syntax, formatting, browser exclusions and public assets pass here, the full test suite passes (13,548 tests), and the Chromium test passes 8 of 8. I also ran your new tests against master's sources: 38 of them fail there, so they cover the change.

The remaining item is the question from my earlier comments: whether Codeman should resolve an ambiguous hard wrap with a filesystem lookup on click at all (the original wins when both exist, and the spaced name is tried only after a 404). That decision is still open on my side. Nothing else is needed from you for the points raised so far. Thank you again for the careful tests and the clear write-ups in every round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File links truncate wrapped filenames containing spaces, including .conf files

2 participants