Skip to content

refactor(native-chat): the chat visual reader checks one file by its path - #28189

Closed
brennanb2025 wants to merge 4 commits into
brennanb2025/cvs-s0-chat-scratch-folderfrom
brennanb2025/cvs-s2-visual-reader-path
Closed

brennanb2025 wants to merge 4 commits into
brennanb2025/cvs-s0-chat-scratch-folderfrom
brennanb2025/cvs-s2-visual-reader-path

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​236 $\color{#cf222e}{\Huge{\mathbf{−}}}$​34 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​202
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​78 $\color{#cf222e}{\Huge{\mathbf{−}}}$​19 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​59

ELI5

When a chat draws a picture (an HTML "visual"), Orca keeps that file in a private Orca folder on the computer that runs the chat, and reads it back through a careful, locked-down reader. Today that reader is asked for "chat folder X, file Y". This PR changes it to be asked for one full file path instead, and it checks that the path really points at a visual inside Orca's own chat scratch folder before reading. Nothing looks different to users yet.

What Changed

Problem / what is missing. The next PRs in this stack let the desktop and paired or SSH hosts read a chat visual as an ordinary file, by its full path on the host. The only code allowed to read visuals automatically (without the user clicking anything) is the protected visual reader, and it could not be handed a full path: it only took a folder plus a file name. Without this change, the later "automatic visual read" would have had to re-implement the safety checks or bypass them.

User-facing change. None yet. Chats show visuals exactly as before, on every host. This prepares the reader that S4 (the restricted chat-visual read kind) will call.

Mechanism. src/main/native-chat/native-chat-visual-file-read.ts:

  • readNativeChatVisualPath(scratchRoot, filePath, knownRevision?) replaces readNativeChatVisualFile(folder, file, knownRevision?). It first checks the path's shape without touching the disk, then runs the same on-disk read as before.
  • nativeChatVisualPathParts(scratchRoot, filePath) is the shape check. A path is accepted only when it is exactly <scratch root>/<32 hex chat folder>/<visual file name>:
    • absolute and already normalized (no .., ., doubled or trailing separators);
    • its parent is a 32-character lowercase hex folder, which is how the host names each chat's folder, sitting directly under the root (not deeper, not in the root itself);
    • the file name passes the same validator the ::orca-visual{file=...} line uses.
      Path syntax follows the root: on a Windows host either separator works and the comparison ignores case; POSIX hosts stay case-sensitive.
  • Every other path is refused as outside_folder before any disk access. Every existing protection stays: the root and chat folder must be real directories (not symlinks), the file is opened without following a symlink, it must be a regular file of at most 512 KiB read with a bounded read, its identity is checked again after opening, and it must be valid UTF-8 with no NUL bytes. There is still no hard-link check, and the reader never writes.
  • agentSession.readVisual (the existing host call older clients use) now resolves the data directory (it may be relative), builds the full path itself from the session id and file name, and delegates. Its replies are unchanged, including revision, unchanged, and every error code.

Where this sits. S2 of 14 in the chat-visuals scratch-folder stack. Base: S0 (brennanb2025/cvs-s0-chat-scratch-folder, which renames each chat's folder to <data dir>/chat-scratch/<hash>); this PR reads from that new root through S0's nativeChatScratchRootFor / nativeChatScratchFolderFor helpers and never reads the old native-chat-visuals root. S4 (the restricted chat-visual read kind) depends on it.

Why

Doing the path check inside the existing protected reader keeps one place that decides what an automatic visual read may touch, rather than a second checker beside it. Checking the shape first and then rebuilding the folder path from the host's own root means the on-disk checks always walk the host's own spelling of the folder, never a client-supplied one. Refusing non-normalized paths (instead of cleaning them up) keeps the rule simple: if the path a caller sends is not exactly the visual's path, the answer is no.

Differences from the common pattern:

  • intended: the reader refuses non-normalized paths instead of normalizing them. Safe because every legitimate caller builds the path from the host's published folder plus a validated file name, so a normalized path is always available.
  • intended: Windows paths are matched case-insensitively against the root. Safe because the folder is rebuilt from the host's own root before any disk access, so case only affects acceptance, not which directory is read.

Linked Issue

Part of the chat-visuals scratch-folder stack (no separate issue).

Visual Proof

N/A: no visual or interaction change; the visual reader's replies are identical.

Testing

  • src/main/native-chat/native-chat-visual-file-read.test.ts (22 tests, plus a relative scratch root): existing reader tests adapted to the path input, plus: path outside the root (including a chat-scratch-evil lookalike sibling), a relative data directory, parent not 32-hex (word, uppercase, 31 and 33 chars), file directly in the root or nested deeper, .. traversal and non-normalized spellings, relative paths, invalid file names, and Windows path forms (backslash, forward slash, drive-letter case, other drive, drive-relative, \\?\ and UNC prefixes, :stream, device names, trailing dot/space/separator).

  • src/main/runtime/rpc/methods/structured-agent-session-visual.test.ts (15 tests, incl. a relative data directory): exact success reply shape, unchanged, missing folder and missing file as not_found, too_large, not_text, not_a_file, symlinked file as outside_folder, plus the existing session/location/params tests.

  • tsc -p config/tsconfig.node.json, full pnpm run typecheck, and the repo lint and code-quality checks pass locally (macOS). Windows path handling is covered by syntax tests that run on every platform.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Security: the reader still refuses anything outside one chat folder under this host's scratch root, before touching the disk.
  • Cross-platform: path syntax follows the host root (Windows or POSIX).
  • SSH / paired hosts and older clients: agentSession.readVisual is unchanged on the wire and in its replies.
  • Mobile: unaffected; it uses the same unchanged host call.
  • Performance: one extra string check per read.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…path

The protected visual reader now takes one absolute host path and accepts it only
when it names a valid visual file directly inside a host-minted 32-hex chat
folder under this host's visuals root. Every on-disk protection is unchanged.
agentSession.readVisual derives the path itself and delegates, with the same
responses.
…-folder' into brennanb2025/cvs-s2-visual-reader-path

# Conflicts:
#	src/main/native-chat/native-chat-visual-file-read.test.ts
#	src/main/native-chat/native-chat-visual-file-read.ts
#	src/main/runtime/rpc/methods/structured-agent-session-visual.test.ts
#	src/main/runtime/rpc/methods/structured-agent-session-visual.ts
@brennanb2025
brennanb2025 changed the base branch from main to brennanb2025/cvs-s0-chat-scratch-folder October 13, 2026 08:00
…-folder' into brennanb2025/cvs-s2-visual-reader-path
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Superseded by the rebuilt six-PR chat scratch stack (#28385 merged, #28389, #28397, #28437, #28394, #28479). This piece is replaced by #28397 (readVisual results carry the file path). Closing.

@brennanb2025
brennanb2025 deleted the brennanb2025/cvs-s2-visual-reader-path branch October 14, 2026 02:41
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.

1 participant