From f12b5ac88b05fdd373d93ff723c6e9941dfb5c04 Mon Sep 17 00:00:00 2001 From: JD Date: Fri, 9 Oct 2026 14:10:46 -0400 Subject: [PATCH 1/2] feat(uploads): write prompt uploads to a hidden, self-ignoring .codeman-uploads/ folder A pasted image landed in /.claude-images/, a name that belongs to another tool, in a folder nothing ignored, so it showed up in git status of every case that ever received a paste. Uploads now go to a flat /.codeman-uploads/ that carries a .gitignore of `*` (written once, never over a file already there): in the workspace because that is the only path identical for a local agent and a container, flat because a nested .codeman/ is the data dir itself when the workspace is the home directory. The directory names live in paste-image-gc.ts alone. The old folder receives nothing but stays readable for one release: the hourly sweep and the delete cleanup go through uploadDirs(), the image watcher's ignore filter reads the names. uploadDirs() lists only real directories, none that is, contains or sits inside the data dir, and nothing for a remote session, since both consumers delete. The route refuses a remote (SSH) session before touching disk: the file would land on this host under the remote path, where the agent cannot read it. test/paste-image-dir-shared.test.ts binds port 0 and leaves the port guard's legacy list. Decided in #553. --- .gitignore | 4 +- CLAUDE.md | 4 +- docs/api-reference.md | 15 ++ docs/architecture-invariants.md | 6 +- src/image-watcher.ts | 8 +- src/web/paste-image-gc.ts | 114 +++++++++++---- src/web/public/terminal-ui.js | 2 +- src/web/routes/reboot-restore-routes.ts | 2 +- src/web/routes/session-routes.ts | 93 ++++++++---- src/web/server.ts | 19 +-- test/image-watcher.test.ts | 9 ++ test/link-provider-regex.test.ts | 4 +- test/paste-image-dir-shared.test.ts | 53 +++++-- test/paste-image-gc.test.ts | 132 ++++++++++++++++++ .../reboot-restore-rebuild-failure.test.ts | 2 +- test/routes/session-routes.test.ts | 91 +++++++++++- test/test-ports-guard.test.ts | 1 - 17 files changed, 459 insertions(+), 100 deletions(-) create mode 100644 test/paste-image-gc.test.ts diff --git a/.gitignore b/.gitignore index 5113ac0a..93003107 100644 --- a/.gitignore +++ b/.gitignore @@ -107,7 +107,9 @@ todo.md @fix_plan.md readme-preview.mjs -# Uploaded images land here under each session working dir (runtime artifact) +# Prompt uploads land here under each session working dir (runtime artifact); +# .claude-images/ is where they landed before the move. +.codeman-uploads/ .claude-images/ # Local-LLM harness smoke-test config (real IPs/keys) — see the .example.json diff --git a/CLAUDE.md b/CLAUDE.md index d122f877..fc3cacef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -295,6 +295,8 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Attachments** (live external document references; all wiring in `file-routes.ts`): a **registry** maps a stable `attachmentId` to a realpath-resolved, extension-allowlisted absolute path, so browser requests never carry arbitrary absolute paths. ⚠️ The **magic-link scanner** (`codeman://attach?...` in terminal output) is **prompt-injectable**, so its scan path is force-confined to the session workspace; a hostile prompt could otherwise exfiltrate arbitrary host files over SSE. The security gate is an extension **allowlist**, not a blocklist. `document-conversion-limiter.ts` caps converter spawns globally: without it, N large docs detected at once fork N multi-minute processes, which is a resource-exhaustion vector. → [architecture-invariants#attachments](docs/architecture-invariants.md#attachments) +**Prompt uploads** (`POST /api/sessions/:id/paste-image`; client wiring in `image-input.js` and `keyboard-accessory.js`): a pasted file is written to `/.codeman-uploads/` and its path lands on the prompt as unsent text. The folder is IN the workspace because that is the only path identical for a local and a container agent, FLAT because `/.codeman/` would BE the data dir when the workspace is the home directory, and self-ignoring through a `.gitignore` of `*` written ONCE with `wx` (a file already there is the user's and is never overwritten; the user's own ignore file is never touched). ⚠️ The directory names live ONLY in `paste-image-gc.ts` (`UPLOAD_DIR_NAMES`): the pre-move `.claude-images/` receives nothing but stays readable for one release, so the GC sweep and the delete cleanup (through `uploadDirs()`) and the image watcher's ignore filter (reading the names) cover BOTH; retire the legacy entry there, never by hand elsewhere. ⚠️ `uploadDirs()` returns only REAL directories (lstat, so a planted symlink is never one), none that is, contains or sits inside `getDataDir()`, and none for a remote session, since both consumers delete; the check is made at list time, and the same-user race after it is accepted. A remote (SSH) session is refused (400) before any disk touch: its `workingDir` is the remote path, so the file would land on THIS host where the agent cannot read it. → [architecture-invariants#buffers-uploads-and-terminal-history](docs/architecture-invariants.md#buffers-uploads-and-terminal-history) + **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`). 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`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server: `spreadsheet-preview.js` fetches the raw route with `?preview=true` (413 above `MAX_XLSX_BROWSER_PREVIEW_BYTES`, 10 MB) and hands the bytes to `spreadsheet-preview-worker.js`, the only place the pinned `exceljs`/`fflate` vendor bundles load (never on page load). `admitXlsx()` caps the ZIP before ExcelJS runs, counting what ExcelJS will EXPAND as well as what it reads (a merge costs its area, a `` past 16384 is refused, validations are never parsed via `ignoreNodes`, and defined names are never expanded: the worker stubs `_definedNames.model`). ⚠️ The INDEX a row or sheet claims is bounded too, since ExcelJS allocates and walks up to it: a `` outside 1-1048576 and an `xl/workbook.xml` `` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ The COLUMN index costs the same way (a row's cells live at `_cells[col - 1]`, so one `XFD` cell makes ExcelJS's `eachRow`/`eachCell`/`hasValues` visit 16,384 slots per row): `worksheetMetadata` builds each sheet's row and cell index from the keys that exist (`Object.keys` of `_rows` and `_cells`, skipping falsy and `Null`-type cells as `eachCell({ includeEmpty: false })` does), reads row heights in that pass and merges from `sheet._merges`, and `sendTile` reads cells from that index (`populatedRowsById`); never call ExcelJS's dense `eachRow`/`eachCell` there. `parseThemePalette` returns the default palette for a theme above 64 KB (64 * 1024 characters), since its patterns are quadratic on unclosed tags. ⚠️ Merges are capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) AND workbook-wide (`LIMITS.maxMerges`, 10,000), refused as `merge-limit` before ExcelJS loads, since ExcelJS's `_mergeCellsInternal` checks each merge against every earlier one on its sheet (quadratic). Every `` in `xl/styles.xml` is read with `readTagAttributes()` and refused (`number-format`) when its decoded `formatCode` is over 255 characters or has a `[` after its last `]`: ExcelJS's `isDateFmt` rescans to the end of the code per unclosed `[` once per numeric cell, and the code is echoed into the notice bar. ⚠️ `formatCellValue` caps every cell's display text at `LIMITS.maxCellTextChars` (1,000, ellipsis, never splitting a surrogate pair) for every value shape (rich text, hyperlink text, formula source, errors), since structured clone copies each tile cell's whole string to the page and the load timeout no longer covers tiles. `richTextPrefix` also visits at most `maxCellTextChars + 1` runs, not only keeps that much text: an empty `` adds nothing, so a shared string of a million empty runs was walked whole per cell, per tile (56 s a tile). ⚠️ Every part ExcelJS parses (all but the pure-bytes `xl/media/.`) is also charged per START TAG against `LIMITS.maxElements` (2,000,000, refused as `element-limit`): ExcelJS builds an object per element in sharedStrings, styles, comments, drawings, VML and tables too, and empty stored blocks pad a deflate stream past the ratio cap, so 8.3M empty `` runs in a 375 KB file cost 570 MB of heap. ⚠️ Admission keys every entry on the name ExcelJS will SEE (`excelJsEntryName()`: JSZip's `.`/`..`/empty-segment resolution, then one leading `/` stripped), refuses two entries that land on one name, and treats anything matching ExcelJS's UNANCHORED `xl/worksheets/sheet.xml` as a worksheet, so `/xl/...` or `xl/./...` cannot skip a counter, and ExcelJS parses only a STORE-only archive rebuilt from the entries admission inflated (`buildAdmittedArchive()`), never the fetched bytes; cell text goes through `textContent`, formulas are never evaluated. Bumping either package or editing the worker/core changes `SPREADSHEET_ASSET_VERSION`, which `npm run check:public-assets` pins. xls/ods stay download-only. → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) **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) @@ -493,7 +495,7 @@ curl -sk https://localhost:3000/api/subagents | jq # Background agents cat ~/.codeman/state.json | jq # Persisted state ``` -Legacy screenshots (deprecated): `GET/POST /api/screenshots` still read and write `~/.codeman/screenshots/`, but the upload page that fed them is gone, they log a one-time deprecation warning, and they are removed in a later MAJOR, after at least one MINOR release that carries the warning (`docs/versioning-policy.md`). To hand a file to an agent use `POST /api/sessions/:id/paste-image`. +Legacy screenshots (deprecated): `GET/POST /api/screenshots` still read and write `~/.codeman/screenshots/`, but the upload page that fed them is gone, they log a one-time deprecation warning, and they are removed in a later MAJOR, after at least one MINOR release that carries the warning (`docs/versioning-policy.md`). To hand a file to an agent use `POST /api/sessions/:id/paste-image` (see Prompt uploads). ## Performance & Limits diff --git a/docs/api-reference.md b/docs/api-reference.md index 47c22e0b..caea5234 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -466,6 +466,21 @@ geometry was read. The capture runs synchronous tmux calls on the server; the | `tail=` | Keep the newest `` of the result (`truncationReason: 'tail'` when it cut). | | `lines=` | With `full=1` only: read at most `` lines of tmux history above the visible frame. An integer of at least 1, clamped to the configured history limit; absent or malformed, the whole limit (100,000 lines by default), as before. `truncated` and `truncationReason` describe byte cuts only, not this bound. Without it a full capture reads all of that history before `tail` cuts it, so a client that keeps a fixed number of lines (the tile grid sends its xterm's scrollback plus its rows) should send it. | +## Prompt uploads (`POST /api/v1/sessions/:id/paste-image`) + +A `multipart/form-data` body with one `image` part. The file is written into the +session's workspace as `/.codeman-uploads/paste--.`, and +`data` carries `path` and `filename` for the client to type the path into the +prompt. The folder is Codeman's own: hidden, created on first use with a +`.gitignore` containing `*` (written once, never over a file already there), and +cleaned up the way pasted images always were: `paste-*` files older than 7 days +go in an hourly sweep, and the folder goes when the last session of that +workspace is killed. Uploads made before this release sit in `.claude-images/`; +that folder receives nothing new, and is swept and removed the same way for one +release. A remote (SSH) session answers 400, since the file would land on the +Codeman host under a path the remote agent cannot read; Docker sessions are +fine, their workspace is bind-mounted at the same absolute path. + ## Session lineage (`parentSessionId`) A create request may name the session that spawned it, which the web UI draws as a diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index fa4fb87c..06627c80 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -254,7 +254,7 @@ Further detail, closing: ⚠️ **Closing has the mirror-image race and one owne - **No start, attach or relaunch may be in flight** (`Session.paneLifecycleInFlight`, raised for the whole of `_setupOrAttachMuxSession()` and `restartCli()`). The dead-pane respawn revives an exited pane on purpose, and the pane reads as dead until `clearPaneExitForNewPane()` runs after its startup delay. - **The pane must have been up for `CLEAN_EXIT_MIN_PANE_LIFETIME_MS` (10 s)** since the last start, attach or relaunch finished (`Session.paneStartedAt`). A CLI that prints a startup error and exits 0 would otherwise lose its tab, and the error with it, seconds after launch; its row stays as `exited (0)` instead. An attach to an already running pane stamps it too, so an `/exit` within seconds of a server restart leaves a row to close by hand. -Scoping needs no check of its own here: `setPaneExit()` already forces `paneExit` to UNKNOWN for direct-PTY, remote, docker and discovered sessions. There is no setting, by the maintainer's decision on #446. ⚠️ Do not flip local panes to `remain-on-exit failed` to get the same effect: a destroyed pane ends the tmux session, the PTY exit nulls the pid, and the browser's `selectSession()` then launches a fresh CLI. `cleanupSession()` keeps `{workingDir}/.claude-images` while another session still uses that directory (`pasteImageDirInUseByOtherSession()`, `paste-image-gc.ts`), since the sweep would otherwise routinely delete a live sibling's pasted images. Paths are compared by `realpath`, and a detached session counts through its persisted record, because `killMux=false` removes it from the map while its pane keeps running; only a session being KILLED is exempt. Each exit gets ONE close attempt (keyed by session id and `at`), and a session being closed refuses `startInteractive()`/`startShell()` (`Session.markClosing()`), so a start that races the close cannot orphan a tmux session. `planRebootRestore()` refuses a record whose persisted `paneExit` is clean (`agent-exited`), which covers an agent that exited just before the power went, before the sweep reached it. Tests: `test/pane-exit-sweep.test.ts`, `test/paste-image-dir-shared.test.ts`, `test/reboot-restore.test.ts`, `test/tmux-manager.test.ts`. +Scoping needs no check of its own here: `setPaneExit()` already forces `paneExit` to UNKNOWN for direct-PTY, remote, docker and discovered sessions. There is no setting, by the maintainer's decision on #446. ⚠️ Do not flip local panes to `remain-on-exit failed` to get the same effect: a destroyed pane ends the tmux session, the PTY exit nulls the pid, and the browser's `selectSession()` then launches a fresh CLI. `cleanupSession()` keeps `{workingDir}/.codeman-uploads` (and the pre-move `.claude-images`) while another session still uses that directory (`pasteImageDirInUseByOtherSession()`, `paste-image-gc.ts`), since the sweep would otherwise routinely delete a live sibling's pasted images. Paths are compared by `realpath`, and a detached session counts through its persisted record, because `killMux=false` removes it from the map while its pane keeps running; only a session being KILLED is exempt. Each exit gets ONE close attempt (keyed by session id and `at`), and a session being closed refuses `startInteractive()`/`startShell()` (`Session.markClosing()`), so a start that races the close cannot orphan a tmux session. `planRebootRestore()` refuses a record whose persisted `paneExit` is clean (`agent-exited`), which covers an agent that exited just before the power went, before the sweep reached it. Tests: `test/pane-exit-sweep.test.ts`, `test/paste-image-dir-shared.test.ts`, `test/reboot-restore.test.ts`, `test/tmux-manager.test.ts`. ### Dead-pane respawn: the resume pin @@ -351,7 +351,7 @@ The escape hatch is the synced `workspaceHooksEnabled` setting (App Settings → ⚠️ Claude-mode only (others carry their conversation id in their own config object), and remote/docker sessions are never offered (`remote-or-docker`), because both need another host or container to be up. -⚠️ A failed rebuild is undone with `discardPartiallyBuiltSession()`, deliberately NOT `cleanupSession()`: the delete path would count the session's tokens into the lifetime totals, demote a pinned record to `stopped` (which this pass reads as an intentional kill, making the session permanently unrestorable) and recursively remove the WORKSPACE's `.claude-images`. +⚠️ A failed rebuild is undone with `discardPartiallyBuiltSession()`, deliberately NOT `cleanupSession()`: the delete path would count the session's tokens into the lifetime totals, demote a pinned record to `stopped` (which this pass reads as an intentional kill, making the session permanently unrestorable) and recursively remove the WORKSPACE's upload dirs (`.codeman-uploads`, and the pre-move `.claude-images`). ⚠️ `os.uptime()` reports the HOST's uptime, which a container shares, and that cuts both ways: after a genuine host reboot a containerized Codeman does see a short uptime and the banner works, but a container-only restart is invisible to it, which is the case where this would help most. Tests: `test/reboot-restore.test.ts`, `test/routes/reboot-restore-routes.test.ts`, `test/routes/reboot-restore-rebuild-failure.test.ts`, `test/discard-partially-built-session.test.ts`. @@ -1063,6 +1063,8 @@ Tests: `test/mobile-prompt-composer.test.ts` (in the CI gate, deliberately not u Target: 20 sessions, 50 agent windows at 60fps. Limits in `src/config/`: terminal 32MB (see below), text 1MB, messages 1000, max agents 500, max sessions 50, max SSE clients 100. **Terminal history** (`src/config/terminal-history.ts`, COD-80): tmux history-limit 100k lines, PTY buffer 32MB max / 24MB trim (env `CODEMAN_MAX_TERMINAL_BUFFER`/`CODEMAN_TRIM_TERMINAL_TO`; the env-derived trim is clamped ≤75% of max — trim ≥ max would disable `BufferAccumulator` trimming entirely = unbounded memory); browser xterm scrollback stays a separate hardcoded 50k (`DEFAULT_SCROLLBACK` in constants.js — 100k/tab is a mobile-memory hazard). tmux <3.7 allocates history at pane creation, so `createSession()` sets the global default in the same command queue immediately before `new-session`; tmux 3.7+ instead creates the session and targets only that pane, because changing the global option can resize and trim unrelated live panes. A settings change resizes tracked panes only on 3.7+ and otherwise affects future panes; no version can recover lines already evicted. Settings keys `terminalScrollbackLines`/`terminalBufferMaxBytes`/`terminalBufferTrimBytes` are schema-validated but inert (only `tmuxHistoryLimit` is wired); `buffer-limits.ts` re-exports the defaults. Text/message limits are env-overridable too (`CODEMAN_MAX_TEXT_OUTPUT`/`CODEMAN_TRIM_TEXT_TO`/`CODEMAN_MAX_MESSAGES`). **Image upload** (`image-input.js` / `config/buffer-limits.ts`): up to `_maxBatchImages` 20 images/batch (bounded concurrency 3), per-file `MAX_PASTE_IMAGE_BYTES` 50MB (env `CODEMAN_MAX_PASTE_IMAGE_BYTES`); the mobile camera-roll picker auto-downscales to fit before upload. **HEIC paste uploads** (#151): converted server-side to JPEG in a `worker_threads` worker (`web/heic-jpeg-worker.ts`, resourceLimits + 30s timeout) gated by `runWithConversionLimit()`; detection is magic-byte based (covers Android/MIUI HEIFs mislabeled as JPEG); headers declaring > 64MP are rejected 415 BEFORE decode (decompression-bomb guard). Deps: `heic-decode` + `jpeg-js`. Use `LRUMap` for bounded caches, `StaleExpirationMap` for TTL cleanup. Anti-flicker pipeline: `docs/terminal-anti-flicker.md`. +**Prompt uploads live in `/.codeman-uploads/`** (`POST /api/sessions/:id/paste-image`; the names live ONLY in `src/web/paste-image-gc.ts`): IN the workspace because that is the only path that resolves identically for a local agent and a container (only the workspace is bind-mounted, at the same absolute path; `~/.codeman` is not); hidden so it stays out of `git status` and the agent's view of the repository; FLAT because a nested `/.codeman/` IS the data dir (`join(homedir(), '.codeman')`, `src/config/instance.ts`) when the workspace is the home directory, which nothing refuses; and self-ignoring through a `.gitignore` of `*` written once with `wx`, so a file already there is the user's and stays. The pre-move `.claude-images/` receives nothing but stays readable for one release: `UPLOAD_DIR_NAMES` lists both, the hourly sweep and the delete cleanup in `cleanupSession()` go through `uploadDirs()`, and the image watcher's ignore filter reads the names. ⚠️ `uploadDirs()` returns only REAL directories (lstat at the leaf, so a planted `.codeman-uploads -> /other-case/.codeman-uploads` is never listed: `readdir` follows a link to a directory, and the sweep would otherwise age out the other case's uploads through it), none that is, contains or sits inside `getDataDir()` (contrived, `CODEMAN_INSTANCE=uploads` makes `~/.codeman-uploads` the data dir of a home workspace and `CODEMAN_DATA_DIR` can point inside one, but it is one check next to a recursive delete), and nothing for a remote (SSH) session, whose `workingDir` is the remote path and would otherwise name a same-named LOCAL directory for the sweep and the delete. The check is made when the directories are listed: a same-user process that swaps a listed directory for a link during the sweep's awaits is accepted (Node has no `openat()`, and that actor already writes anywhere this process can). The route refuses a remote (SSH) session with 400 before any disk touch: its `workingDir` is the remote path, so the file would land on THIS host where the agent cannot read it. Tests: `test/paste-image-gc.test.ts`, `test/paste-image-dir-shared.test.ts`, the paste-image block of `test/routes/session-routes.test.ts`. + ### Process-tree walks are bounded **Process-tree walks are bounded** (`proc-tree.ts`, pure + unit tested): `collectDescendants(pid, byParent)` is the ONE descendant traversal, fed by a single cached `ps -eo pid=,ppid=` snapshot (`refreshProcSnapshot()` in tmux-manager.ts: in-flight-shared, async because `execSync`'s timeout cannot return at all while spawnSync waits on an unkillable child, and ANY error discards the result rather than caching a truncated `ps`, which would make whole subtrees invisible to the kill path). diff --git a/src/image-watcher.ts b/src/image-watcher.ts index e0f8c7ae..8f8dc560 100644 --- a/src/image-watcher.ts +++ b/src/image-watcher.ts @@ -14,6 +14,7 @@ import { basename, extname, relative } from 'node:path'; import { statSync } from 'node:fs'; import type { AttachmentDetectedEvent, AttachmentDetectedType, ImageDetectedEvent } from './types.js'; import { KeyedDebouncer } from './utils/index.js'; +import { UPLOAD_DIR_NAMES } from './web/paste-image-gc.js'; // ========== Types ========== @@ -157,12 +158,15 @@ export class ImageWatcher extends EventEmitter { // Watch all subdirectories (images may be saved in src/, assets/, etc.) // Ignore common heavy directories for performance ignored: (path: string) => { - // Skip node_modules, .git, and other heavy directories + // Skip node_modules, .git, and other heavy directories, and Codeman's + // own upload folders: a pdf the user handed to the agent is not a file + // the agent produced. if ( path.includes('/node_modules/') || path.includes('/.git/') || path.includes('/dist/') || - path.includes('/.next/') + path.includes('/.next/') || + UPLOAD_DIR_NAMES.some((name) => path.includes(`/${name}/`)) ) { return true; } diff --git a/src/web/paste-image-gc.ts b/src/web/paste-image-gc.ts index 8cee31ce..c99099ac 100644 --- a/src/web/paste-image-gc.ts +++ b/src/web/paste-image-gc.ts @@ -1,25 +1,76 @@ /** - * @fileoverview Periodic GC for paste-image files. + * @fileoverview Periodic GC for prompt-upload files, and the one place that + * names the directories they live in. * * Without cleanup, /api/sessions/:id/paste-image accumulates files indefinitely - * under {workingDir}/.claude-images/. The route only triggers cleanup on + * under {workingDir}/.codeman-uploads/. The route only triggers cleanup on * killMux=true session deletion, so long-lived sessions can fill disk under * heavy pasting. This sweeper bounds disk use by deleting `paste-*` files - * older than MAX_AGE_MS from each live session's image dir on an interval. + * older than MAX_AGE_MS from each live session's upload dirs on an interval. * * Conservative defaults — only files matching the `paste-` prefix are * considered, and we lstat (not stat) so a planted symlink cannot escape the - * image dir. + * upload dir. */ import fs from 'node:fs/promises'; -import { realpathSync } from 'node:fs'; -import { join, resolve } from 'node:path'; +import { lstatSync, realpathSync } from 'node:fs'; +import { join, resolve, sep } from 'node:path'; +import { getDataDir } from '../config/instance.js'; import type { SessionPort } from './ports/index.js'; const MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; // 7 days const SWEEP_INTERVAL_MS = 60 * 60 * 1000; // 1 hour const INITIAL_DELAY_MS = 30 * 1000; // 30s after startup +/** + * Where a prompt upload is written, relative to the session's working + * directory. IN the workspace, because that is the only path that resolves + * identically for a local agent and a container (only the workspace is + * bind-mounted, at the same absolute path); hidden, so it stays out of + * `git status` and the agent's view of the repository; FLAT, because a nested + * `/.codeman/` is Codeman's own data dir when the workspace is the + * home directory; and self-ignoring, through a `.gitignore` of `*` the route + * writes once. + */ +export const UPLOADS_DIR = '.codeman-uploads'; +/** Where uploads landed before the move: written to by nothing, readable for one release. */ +export const LEGACY_UPLOADS_DIR = '.claude-images'; +/** Every upload dir name, current first. Retiring the legacy one here retires it for every reader. */ +export const UPLOAD_DIR_NAMES = [UPLOADS_DIR, LEGACY_UPLOADS_DIR]; + +/** + * Every directory a session's uploads sit in, current first. Both consumers + * act on what this returns, the hourly sweep and the recursive delete in + * cleanupSession(), so it lists only REAL directories (a link planted by a + * workspace script, `.codeman-uploads -> /other-case/.codeman-uploads`, is + * not one; readdir follows a link to a directory), none that is, contains or + * sits inside this instance's data dir (a home workspace reaches it under a + * contrived instance name, `CODEMAN_INSTANCE=uploads`, and `CODEMAN_DATA_DIR` + * can point inside one), and nothing for a remote (SSH) session, whose + * workingDir is the remote path and would name a same-named LOCAL directory + * here. The check is made when listing: a same-user process that swaps a + * listed directory for a link afterwards is accepted, since it already writes + * anywhere this process can. + */ +export function uploadDirs(session: { workingDir: string; remote?: unknown }): string[] { + if (session.remote) return []; + const dataDir = canonicalDir(getDataDir()); + return UPLOAD_DIR_NAMES.map((name) => join(session.workingDir, name)).filter((dir) => { + if (!isRealDir(dir)) return false; + const real = canonicalDir(dir); + return real !== dataDir && !real.startsWith(dataDir + sep) && !dataDir.startsWith(real + sep); + }); +} + +/** lstat, so a symlink is not a directory, whatever it points at. */ +function isRealDir(p: string): boolean { + try { + return lstatSync(p).isDirectory(); + } catch { + return false; + } +} + export async function sweepPasteImagesOnce( ctx: Pick, now: number = Date.now() @@ -28,26 +79,27 @@ export async function sweepPasteImagesOnce( let scanned = 0; let deleted = 0; for (const session of ctx.sessions.values()) { - const dir = join(session.workingDir, '.claude-images'); - let entries: string[]; - try { - entries = await fs.readdir(dir); - } catch { - continue; // dir absent — nothing to do - } - for (const name of entries) { - if (!name.startsWith('paste-')) continue; - const p = join(dir, name); - scanned += 1; + for (const dir of uploadDirs(session)) { + let entries: string[]; try { - const st = await fs.lstat(p); - if (!st.isFile()) continue; - if (st.mtimeMs < cutoff) { - await fs.unlink(p); - deleted += 1; - } + entries = await fs.readdir(dir); } catch { - // best-effort: skip permission/race errors silently + continue; // gone since listed — nothing to do + } + for (const name of entries) { + if (!name.startsWith('paste-')) continue; + const p = join(dir, name); + scanned += 1; + try { + const st = await fs.lstat(p); + if (!st.isFile()) continue; + if (st.mtimeMs < cutoff) { + await fs.unlink(p); + deleted += 1; + } + } catch { + // best-effort: skip permission/race errors silently + } } } } @@ -55,7 +107,7 @@ export async function sweepPasteImagesOnce( } /** - * The path two sessions must share to share a paste-image dir: the canonical + * The path two sessions must share to share an upload dir: the canonical * path when it can be resolved, so a sibling that reaches the same directory * through a symlink matches, and the normalised path otherwise (a directory * that no longer exists has nothing left to protect). @@ -68,7 +120,7 @@ function canonicalDir(dir: string): string { } } -/** One session the paste-image guard weighs: its id, directory and, for a persisted record, its status. */ +/** One session the upload-dir guard weighs: its id, directory and, for a persisted record, its status. */ export interface PasteImageDirUser { id: string; workingDir: string; @@ -76,11 +128,11 @@ export interface PasteImageDirUser { } /** - * Does another live session still use this working directory's paste-image - * dir? Deleting a session removes `{workingDir}/.claude-images` recursively, - * and several sessions routinely share one case directory, so without this - * check closing one session deletes the pasted images a sibling in the same - * case still refers to. + * Does another live session still use this working directory's upload dirs? + * Deleting a session removes them (`uploadDirs()`) recursively, and several + * sessions routinely share one case directory, so without this check closing + * one session deletes the pasted images a sibling in the same case still + * refers to. * * Two kinds of sibling count as live: * diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index d95960b2..bb44d3bc 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -2004,7 +2004,7 @@ Object.assign(CodemanApp.prototype, { const cmdPattern = /\b(tail|cat|head|less|grep|watch|vim|nano)\s+(?:[^\s\/]+\s+){0,4}(\/[^\s"'<>|;&\n\x00-\x1f]+)/g; // Pattern 2: Paths with common extensions. Image/PDF/media extensions are - // included so pasted-attachment paths (`.claude-images/paste-*.png`) and + // included so pasted-attachment paths (`.codeman-uploads/paste-*.png`) and // screenshots an agent just wrote are clickable; those open the file // preview rather than the log viewer (see addLink). // diff --git a/src/web/routes/reboot-restore-routes.ts b/src/web/routes/reboot-restore-routes.ts index f060c539..e6a50f5d 100644 --- a/src/web/routes/reboot-restore-routes.ts +++ b/src/web/routes/reboot-restore-routes.ts @@ -275,7 +275,7 @@ export function registerRebootRestoreRoutes(app: FastifyInstance, ctx: RebootRes // count this session's historical tokens into the lifetime totals, demote // a pinned record to `stopped` (which this pass reads as an intentional // kill, making the session permanently unrestorable) and delete the - // workspace's `.claude-images`. This undoes only the construction. + // workspace's `.codeman-uploads`. This undoes only the construction. await ctx .discardPartiallyBuiltSession(entry.sessionId) .catch((discardErr: unknown) => diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 8affbeb0..508044ea 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -188,6 +188,7 @@ import { } from '../response-viewer-transcript.js'; import { readDeepSeekLastResponse } from '../../deepseek-transcript.js'; import { appendClaudeCustomTitle } from '../../claude-session-title.js'; +import { UPLOADS_DIR } from '../paste-image-gc.js'; // Path to linked-cases registry (same file used by case-routes resolveCasePath) const LINKED_CASES_FILE = dataPath('linked-cases.json'); @@ -361,6 +362,50 @@ export function imageMagicMatchesExt(data: Buffer, ext: string): boolean { } } +/** + * Create or re-verify `{workingDir}/.codeman-uploads` for a prompt upload, or + * null when something other than a regular directory sits there. An agent or + * postinstall script could plant `.codeman-uploads -> ~/.ssh/` and redirect + * future writes outside workingDir: lstat (not stat) sees the symlink itself, + * and mkdir without `recursive` does not follow one for the leaf either; + * O_EXCL|O_NOFOLLOW on the file open makes the write itself symlink-safe. + * The folder ignores itself: a `.gitignore` of `*`, written once with O_EXCL, + * so the user's repository never sees uploads, their own ignore file is never + * touched, and a file already there is theirs and stays as it is. + */ +async function ensureUploadDir(workingDir: string): Promise { + // workingDir is guaranteed to exist (live session). + const uploadDir = join(workingDir, UPLOADS_DIR); + try { + const dirStat = await fs.lstat(uploadDir); + if (dirStat.isSymbolicLink() || !dirStat.isDirectory()) return null; + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + try { + await fs.mkdir(uploadDir); + } catch (mkErr: unknown) { + // Concurrent uploads (a batch of photos) race to create the dir — the + // losers get EEXIST. Treat an already-present REAL directory as success, + // but re-verify it isn't a symlink a racing actor planted. + if ((mkErr as NodeJS.ErrnoException).code !== 'EEXIST') throw mkErr; + const raceStat = await fs.lstat(uploadDir); + if (raceStat.isSymbolicLink() || !raceStat.isDirectory()) return null; + } + } + const ignoreFile = join(uploadDir, '.gitignore'); + try { + // 'wx' = O_CREAT|O_EXCL, which also fails on a symlink at the path. + await fs.writeFile(ignoreFile, '*\n', { flag: 'wx' }); + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === 'EEXIST') return uploadDir; + // The create can succeed before the write fails (ENOSPC): an empty ignore + // file would read as the user's on the next upload, so take it back. + await fs.rm(ignoreFile, { force: true }); + throw err; + } + return uploadDir; +} + // Per-(IP, sessionId) token bucket for paste-image. 30 requests/minute. // Bucket map entries are pruned when they drift > 1h stale to bound memory // against a flood of unique IP keys. @@ -5198,6 +5243,18 @@ export function registerSessionRoutes( const session = findSessionOrFail(ctx, id, req); + // The file lands on THIS host under the session's working directory, which + // for a remote (SSH) session is the remote path: the agent there could never + // read it, and the write would land in a same-named local directory or fail. + // Docker is fine, its workspace is bind-mounted at the same absolute path. + if (session.remote) { + reply.code(400); + return createErrorResponse( + ApiErrorCode.INVALID_INPUT, + 'Prompt uploads are not supported for remote (SSH) sessions' + ); + } + if (!req.isMultipart()) { reply.code(400); return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Expected multipart/form-data'); @@ -5294,37 +5351,11 @@ export function registerSessionRoutes( return createErrorResponse(ApiErrorCode.INVALID_INPUT, `Image bytes do not match declared type ${ext}`); } - // Save to {workingDir}/.claude-images/ - // Refuse symlinks at imageDir — an agent or postinstall script could plant - // `.claude-images -> ~/.ssh/` and redirect future writes outside workingDir. - // We lstat (not stat) so we see the symlink itself. Use mkdir without - // `recursive` so the leaf creation does not follow a symlink either, and - // O_EXCL|O_NOFOLLOW on the file open so the write itself is symlink-safe. - const imageDir = join(session.workingDir, '.claude-images'); - try { - const dirStat = await fs.lstat(imageDir); - if (dirStat.isSymbolicLink() || !dirStat.isDirectory()) { - reply.code(403); - return createErrorResponse(ApiErrorCode.INVALID_INPUT, '.claude-images is not a regular directory'); - } - } catch (err: unknown) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; - // Non-recursive mkdir: does not follow symlinks for the leaf. - // session.workingDir is guaranteed to exist (live session). - try { - await fs.mkdir(imageDir); - } catch (mkErr: unknown) { - // Concurrent uploads (a batch of photos) race to create .claude-images — - // the losers get EEXIST. Treat an already-present REAL directory as - // success, but re-verify it isn't a symlink a racing actor planted - // (preserve the symlink-safety guarantee above). - if ((mkErr as NodeJS.ErrnoException).code !== 'EEXIST') throw mkErr; - const raceStat = await fs.lstat(imageDir); - if (raceStat.isSymbolicLink() || !raceStat.isDirectory()) { - reply.code(403); - return createErrorResponse(ApiErrorCode.INVALID_INPUT, '.claude-images is not a regular directory'); - } - } + // {workingDir}/.codeman-uploads/, see ensureUploadDir. + const imageDir = await ensureUploadDir(session.workingDir); + if (!imageDir) { + reply.code(403); + return createErrorResponse(ApiErrorCode.INVALID_INPUT, `${UPLOADS_DIR} is not a regular directory`); } // Date.now() collides on same-ms uploads from two tabs (last-write wins // silently). Append 8 hex chars so concurrent pastes get distinct names. diff --git a/src/web/server.ts b/src/web/server.ts index 01f51dcc..3f2e1e56 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -33,7 +33,7 @@ import fastifyCookie from '@fastify/cookie'; import fastifyStatic from '@fastify/static'; import fastifyWebsocket from '@fastify/websocket'; import fastifyMultipart from '@fastify/multipart'; -import { pasteImageDirInUseByOtherSession, startPasteImageGc } from './paste-image-gc.js'; +import { pasteImageDirInUseByOtherSession, startPasteImageGc, uploadDirs } from './paste-image-gc.js'; import { CLEAN_EXIT_CLOSE_REASON, shouldCloseCleanlyExitedSession } from '../pane-exit-sweep.js'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -1546,11 +1546,14 @@ export class WebServer extends EventEmitter { killing: this.killingSessions, }) ) { - const pasteImageDir = join(session.workingDir, '.claude-images'); - try { - rmSync(pasteImageDir, { recursive: true, force: true }); - } catch { - // Best-effort cleanup + // Both upload dirs, the pre-move one too; uploadDirs() lists only real + // directories clear of the data dir, and none for a remote session. + for (const uploadDir of uploadDirs(session)) { + try { + rmSync(uploadDir, { recursive: true, force: true }); + } catch { + // Best-effort cleanup + } } } // Drop the agent skill's preamble cache for this session (seeded at create). @@ -2911,7 +2914,7 @@ export class WebServer extends EventEmitter { } // Bound disk use under heavy paste-image traffic: delete `paste-*` files - // older than 7 days from each live session's .claude-images/ hourly. + // older than 7 days from each live session's upload dirs hourly. if (!this.testMode) { this._pasteImageGcStop = startPasteImageGc({ sessions: this.sessions }); // Surface event-loop stalls (e.g. a slow synchronous tmux/ps call) so the @@ -3363,7 +3366,7 @@ export class WebServer extends EventEmitter { * path adds the session's token totals to the lifetime figures, demotes a * pinned record to `stopped` (the durable marker of an intentional kill, which * would make the session permanently ineligible for a reboot restore), drops - * the persisted Ralph state, and recursively removes `.claude-images` from the + * the persisted Ralph state, and recursively removes the upload dirs from the * WORKING DIRECTORY, which belongs to the workspace rather than to this session * and may hold another live session's pasted images. * diff --git a/test/image-watcher.test.ts b/test/image-watcher.test.ts index c7e97955..267157c6 100644 --- a/test/image-watcher.test.ts +++ b/test/image-watcher.test.ts @@ -31,6 +31,7 @@ vi.mock('node:fs', async (importOriginal) => { }); import { ImageWatcher } from '../src/image-watcher.js'; +import { watch } from 'chokidar'; import { statSync } from 'node:fs'; describe('ImageWatcher', () => { @@ -92,6 +93,14 @@ describe('ImageWatcher', () => { expect(watcher.getWatchedSessions()).toHaveLength(1); }); + it("ignores Codeman's own upload folders, so a pdf the user handed over is not a detected artifact", () => { + watcher.watchSession('session-1', '/home/user/project'); + const [, opts] = vi.mocked(watch).mock.calls.at(-1) as unknown as [string, { ignored: (p: string) => boolean }]; + expect(opts.ignored('/home/user/project/.codeman-uploads/paste-1-ab.pdf')).toBe(true); + expect(opts.ignored('/home/user/project/.claude-images/paste-1-ab.png')).toBe(true); + expect(opts.ignored('/home/user/project/docs/report.pdf')).toBe(false); + }); + it('should replace watcher when working directory changes', () => { watcher.watchSession('session-1', '/home/user/project-a'); watcher.watchSession('session-1', '/home/user/project-b'); diff --git a/test/link-provider-regex.test.ts b/test/link-provider-regex.test.ts index 5aa9f97a..b295d703 100644 --- a/test/link-provider-regex.test.ts +++ b/test/link-provider-regex.test.ts @@ -124,11 +124,11 @@ describe('terminal link-provider regexes (shipped source)', () => { }); it('the file-path pattern links pasted image/PDF/media attachment paths', () => { - // `.claude-images/paste-*.png` is what Codeman writes for a pasted screenshot; + // `.codeman-uploads/paste-*.png` is what Codeman writes for a pasted screenshot; // without image extensions the path rendered as plain, unclickable text. const ext = shippedPattern('FILE_PATH_LINK_PATTERN'); const cases = [ - '/home/arkon/default/claudeman/.claude-images/paste-1785164958410-d11eb7d0.png', + '/home/arkon/default/claudeman/.codeman-uploads/paste-1785164958410-d11eb7d0.png', '/tmp/shot.jpeg', '/opt/app/report.pdf', '/home/a/diagram.svg', diff --git a/test/paste-image-dir-shared.test.ts b/test/paste-image-dir-shared.test.ts index 1adc5a53..6a77b7e1 100644 --- a/test/paste-image-dir-shared.test.ts +++ b/test/paste-image-dir-shared.test.ts @@ -1,8 +1,9 @@ /** - * @fileoverview Deleting a session keeps `.claude-images` while a sibling in the + * @fileoverview Deleting a session keeps the upload dirs while a sibling in the * same working directory is still live (Ark0N/Codeman#446). * - * `cleanupSession()` removes `{workingDir}/.claude-images` recursively. That + * `cleanupSession()` removes `{workingDir}/.codeman-uploads` (and the pre-move + * `.claude-images`) recursively. That * dir belongs to the working directory, not to the session, and several * sessions routinely share one case directory, so closing one used to delete * the pasted images a live sibling still referred to. The exited-agent sweep @@ -11,15 +12,13 @@ * * Port: 3188 */ -import { existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { existsSync, lstatSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { WebServer } from '../src/web/server.js'; import { pasteImageDirInUseByOtherSession } from '../src/web/paste-image-gc.js'; -const PORT = 3188; - describe('pasteImageDirInUseByOtherSession', () => { const none = new Set(); const check = ( @@ -108,12 +107,13 @@ describe('pasteImageDirInUseByOtherSession', () => { describe('deleting a session that shares its working directory', () => { let server: WebServer; let workingDir: string; - const base = `http://localhost:${PORT}`; + let base = ''; beforeAll(async () => { workingDir = mkdtempSync(join(tmpdir(), 'codeman-paste-shared-')); - server = new WebServer(PORT, false, true); + server = new WebServer(0, false, true); await server.start(); + base = `http://localhost:${server.boundPort}`; }); afterAll(async () => { @@ -133,18 +133,43 @@ describe('deleting a session that shares its working directory', () => { const remove = (id: string) => fetch(`${base}/api/sessions/${id}`, { method: 'DELETE' }); - it('keeps the images while a sibling is live, and removes them with the last session', async () => { + it('keeps the uploads while a sibling is live, and removes them with the last session', async () => { const first = await create(); const second = await create(); - const imageDir = join(workingDir, '.claude-images'); - mkdirSync(imageDir, { recursive: true }); - writeFileSync(join(imageDir, 'paste-1.png'), 'x'); + // Both the current dir and the pre-move one, which a case in use back then still carries. + const uploadDir = join(workingDir, '.codeman-uploads'); + const legacyDir = join(workingDir, '.claude-images'); + mkdirSync(uploadDir, { recursive: true }); + mkdirSync(legacyDir, { recursive: true }); + writeFileSync(join(uploadDir, 'paste-1.png'), 'x'); + writeFileSync(join(legacyDir, 'paste-0.png'), 'x'); expect((await remove(first)).status).toBe(200); - expect(existsSync(join(imageDir, 'paste-1.png'))).toBe(true); + expect(existsSync(join(uploadDir, 'paste-1.png'))).toBe(true); + expect(existsSync(join(legacyDir, 'paste-0.png'))).toBe(true); expect((await remove(second)).status).toBe(200); - expect(existsSync(imageDir)).toBe(false); + expect(existsSync(uploadDir)).toBe(false); + expect(existsSync(legacyDir)).toBe(false); + }); + + it('does not follow a planted symlink at the upload dir into another case when the last session closes', async () => { + const other = mkdtempSync(join(tmpdir(), 'codeman-paste-other-')); + const otherUploads = join(other, '.codeman-uploads'); + mkdirSync(otherUploads); + writeFileSync(join(otherUploads, 'paste-7.png'), 'x'); + // Planted by a workspace script. uploadDirs() never lists a link, so the delete + // removes nothing here, and the link itself stays: it is not Codeman's to remove. + symlinkSync(otherUploads, join(workingDir, '.codeman-uploads')); + try { + const only = await create(); + expect((await remove(only)).status).toBe(200); + expect(existsSync(join(otherUploads, 'paste-7.png'))).toBe(true); + expect(lstatSync(join(workingDir, '.codeman-uploads')).isSymbolicLink()).toBe(true); + } finally { + rmSync(join(workingDir, '.codeman-uploads'), { force: true }); + rmSync(other, { recursive: true, force: true }); + } }); it('keeps the images while a sibling is only detached, since it still runs in tmux', async () => { @@ -154,7 +179,7 @@ describe('deleting a session that shares its working directory', () => { // write, so wait for the record a long-running session would already have. const store = (server as unknown as { store: { getSession: (id: string) => unknown } }).store; await vi.waitFor(() => expect(store.getSession(detached)).toBeTruthy(), { timeout: 10_000 }); - const imageDir = join(workingDir, '.claude-images'); + const imageDir = join(workingDir, '.codeman-uploads'); mkdirSync(imageDir, { recursive: true }); writeFileSync(join(imageDir, 'paste-2.png'), 'x'); diff --git a/test/paste-image-gc.test.ts b/test/paste-image-gc.test.ts new file mode 100644 index 00000000..c49b27f0 --- /dev/null +++ b/test/paste-image-gc.test.ts @@ -0,0 +1,132 @@ +/** + * @fileoverview The hourly upload sweep reads BOTH upload dirs of a live session: + * `.codeman-uploads` and the `.claude-images` a case in use before the move still + * carries. Only `paste-*` regular files past the age cap go. `uploadDirs()` never + * lists a planted symlink, a directory that is, contains or sits inside the data + * dir, or anything for a remote session, since the sweep and the delete cleanup + * both act on what it returns. + */ +import { existsSync, lstatSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, utimesSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { LEGACY_UPLOADS_DIR, UPLOADS_DIR, sweepPasteImagesOnce, uploadDirs } from '../src/web/paste-image-gc.js'; + +const DAY_MS = 24 * 60 * 60 * 1000; +const sessionsOf = (workingDir: string, remote?: unknown) => ({ + sessions: new Map([['s1', { workingDir, remote } as never]]) as never, +}); + +describe('sweepPasteImagesOnce', () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + it('ages paste-* files out of both the current and the legacy upload dir', async () => { + const workingDir = mkdtempSync(join(tmpdir(), 'codeman-gc-')); + dirs.push(workingDir); + const now = Date.now(); + const current = join(workingDir, UPLOADS_DIR); + const legacy = join(workingDir, LEGACY_UPLOADS_DIR); + mkdirSync(current); + mkdirSync(legacy); + expect(uploadDirs({ workingDir })).toEqual([current, legacy]); + const old = (now - 8 * DAY_MS) / 1000; + const fresh = (now - 1 * DAY_MS) / 1000; + const plant = (dir: string, name: string, mtime: number) => { + writeFileSync(join(dir, name), 'x'); + utimesSync(join(dir, name), mtime, mtime); + }; + plant(current, 'paste-1-aa.pdf', old); + plant(current, 'paste-2-bb.png', fresh); + plant(current, 'keep.png', old); // not an upload of ours + plant(legacy, 'paste-0-cc.png', old); + symlinkSync(join(legacy, 'paste-0-cc.png'), join(current, 'paste-3-dd.png')); + utimesSync(join(current, 'paste-3-dd.png'), old, old); + + const result = await sweepPasteImagesOnce(sessionsOf(workingDir), now); + + expect(result.deleted).toBe(2); + expect(existsSync(join(current, 'paste-1-aa.pdf'))).toBe(false); + expect(existsSync(join(legacy, 'paste-0-cc.png'))).toBe(false); + expect(existsSync(join(current, 'paste-2-bb.png'))).toBe(true); + expect(existsSync(join(current, 'keep.png'))).toBe(true); + // A planted symlink is skipped (lstat, never followed), so the sweep leaves the link itself. + expect(lstatSync(join(current, 'paste-3-dd.png')).isSymbolicLink()).toBe(true); + }); + + it('never reads through a planted symlink at the upload dir', async () => { + const workingDir = mkdtempSync(join(tmpdir(), 'codeman-gc-')); + const other = mkdtempSync(join(tmpdir(), 'codeman-gc-other-')); + dirs.push(workingDir, other); + const now = Date.now(); + const old = (now - 8 * DAY_MS) / 1000; + mkdirSync(join(other, UPLOADS_DIR)); + writeFileSync(join(other, UPLOADS_DIR, 'paste-9-zz.png'), 'x'); + utimesSync(join(other, UPLOADS_DIR, 'paste-9-zz.png'), old, old); + // readdir follows a link to a directory, so a listed link would let the sweep + // age out the other case's uploads through it. + symlinkSync(join(other, UPLOADS_DIR), join(workingDir, UPLOADS_DIR)); + + expect(uploadDirs({ workingDir })).toEqual([]); + expect(await sweepPasteImagesOnce(sessionsOf(workingDir), now)).toEqual({ scanned: 0, deleted: 0 }); + expect(existsSync(join(other, UPLOADS_DIR, 'paste-9-zz.png'))).toBe(true); + }); +}); + +describe('uploadDirs', () => { + const dirs: string[] = []; + const savedDataDir = process.env.CODEMAN_DATA_DIR; + afterEach(() => { + if (savedDataDir === undefined) delete process.env.CODEMAN_DATA_DIR; + else process.env.CODEMAN_DATA_DIR = savedDataDir; + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + it('lists the upload dir of a home workspace, which sits beside the data dir, not inside it', () => { + // The home-as-workspace case the flat name exists for: `/.codeman-uploads` + // is a sibling of `/.codeman`, so a prefix test without the separator would + // wrongly refuse it. + const home = mkdtempSync(join(tmpdir(), 'codeman-gc-home-')); + dirs.push(home); + process.env.CODEMAN_DATA_DIR = join(home, '.codeman'); + const dir = join(home, UPLOADS_DIR); + mkdirSync(dir); + expect(uploadDirs({ workingDir: home })).toEqual([dir]); + }); + + it('refuses an upload dir that is the data dir, contains it, or sits inside it', () => { + const root = mkdtempSync(join(tmpdir(), 'codeman-gc-data-')); + const parent = mkdtempSync(join(tmpdir(), 'codeman-gc-link-')); + dirs.push(root, parent); + const uploads = join(root, UPLOADS_DIR); + mkdirSync(join(uploads, 'state'), { recursive: true }); + // `CODEMAN_INSTANCE=uploads` on a home workspace: the upload dir IS the data dir. + process.env.CODEMAN_DATA_DIR = uploads; + expect(uploadDirs({ workingDir: root })).toEqual([]); + // A data dir pointed inside the upload dir: the recursive delete would take it. + process.env.CODEMAN_DATA_DIR = join(uploads, 'state'); + expect(uploadDirs({ workingDir: root })).toEqual([]); + // An upload dir inside the data dir, directly and through a symlinked working + // directory (the upload dir itself is real there). + process.env.CODEMAN_DATA_DIR = root; + expect(uploadDirs({ workingDir: root })).toEqual([]); + symlinkSync(root, join(parent, 'ws')); + expect(uploadDirs({ workingDir: join(parent, 'ws') })).toEqual([]); + }); + + it('lists nothing for a remote session, whose workingDir is the remote path', async () => { + // The same path on THIS host belongs to whoever has a local case there. + const workingDir = mkdtempSync(join(tmpdir(), 'codeman-gc-remote-')); + dirs.push(workingDir); + mkdirSync(join(workingDir, UPLOADS_DIR)); + writeFileSync(join(workingDir, UPLOADS_DIR, 'paste-1-aa.png'), 'x'); + const old = (Date.now() - 8 * DAY_MS) / 1000; + utimesSync(join(workingDir, UPLOADS_DIR, 'paste-1-aa.png'), old, old); + const remote = { hostId: 'gpu-box' }; + expect(uploadDirs({ workingDir, remote })).toEqual([]); + expect(await sweepPasteImagesOnce(sessionsOf(workingDir, remote))).toEqual({ scanned: 0, deleted: 0 }); + expect(existsSync(join(workingDir, UPLOADS_DIR, 'paste-1-aa.png'))).toBe(true); + }); +}); diff --git a/test/routes/reboot-restore-rebuild-failure.test.ts b/test/routes/reboot-restore-rebuild-failure.test.ts index 98cf7a17..9602831d 100644 --- a/test/routes/reboot-restore-rebuild-failure.test.ts +++ b/test/routes/reboot-restore-rebuild-failure.test.ts @@ -144,7 +144,7 @@ describe('a rebuild that fails after the session is registered', () => { expect(ctx.sessions.has('a')).toBe(false); // NOT the user-initiated delete: that would bank this session's historical // tokens into the lifetime totals, demote a pinned record to `stopped`, and - // delete the workspace's .claude-images. + // delete the workspace's .codeman-uploads. expect(ctx.cleanupSession).not.toHaveBeenCalled(); await app.close(); }); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index d982a815..7dc6c4cf 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -18,9 +18,9 @@ import Fastify, { type FastifyInstance } from 'fastify'; import fastifyCookie from '@fastify/cookie'; import fastifyMultipart from '@fastify/multipart'; import { dirname, join } from 'node:path'; -import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { registryFilePath, reloadCliRegistry } from '../../src/config/cli-registry/registry.js'; -import { mkdtemp, rm, mkdir, writeFile } from 'node:fs/promises'; +import fs, { mkdtemp, rm, mkdir, writeFile, readFile, readdir, symlink } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js'; import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; @@ -285,7 +285,7 @@ describe('session-routes', () => { expect(res.statusCode).toBe(200); const body = JSON.parse(res.body); expect(body.success).toBe(true); - expect(body.data.path).toMatch(/\/\.claude-images\/paste-\d+-[a-f0-9]{8}\.jpg$/); + expect(body.data.path).toMatch(/\/\.codeman-uploads\/paste-\d+-[a-f0-9]{8}\.jpg$/); expect(heicConvert).toHaveBeenCalledWith(heic); }); @@ -314,10 +314,93 @@ describe('session-routes', () => { expect(res.statusCode).toBe(200); const body = JSON.parse(res.body); expect(body.success).toBe(true); - expect(body.data.path).toMatch(/\/\.claude-images\/paste-\d+-[a-f0-9]{8}\.jpg$/); + expect(body.data.path).toMatch(/\/\.codeman-uploads\/paste-\d+-[a-f0-9]{8}\.jpg$/); expect(heicConvert).toHaveBeenCalledWith(heic); }); + const jpeg = Buffer.from('ffd8ffe000104a46494600010100', 'hex'); + const upload = (name: string, mime: string, bytes: Buffer) => + harness.app.inject({ + method: 'POST', + url: `/api/sessions/${harness.ctx._sessionId}/paste-image`, + headers: { + host: 'codeman.test', + origin: 'http://codeman.test', + 'content-type': 'multipart/form-data; boundary=codeman-test-boundary', + }, + payload: imageUploadBody('codeman-test-boundary', name, mime, bytes), + }); + + it('writes into .codeman-uploads with a self-ignoring .gitignore, written once and never over a file already there', async () => { + const workDir = await mkdtemp(join(tmpdir(), 'codeman-uploads-')); + harness.ctx._session.workingDir = workDir; + try { + const first = await upload('shot.jpg', 'image/jpeg', jpeg); + expect(first.statusCode).toBe(200); + expect(JSON.parse(first.body).data.path).toMatch(/\/\.codeman-uploads\/paste-\d+-[a-f0-9]{8}\.jpg$/); + expect(await readdir(workDir)).toEqual(['.codeman-uploads']); + expect(await readFile(join(workDir, '.codeman-uploads', '.gitignore'), 'utf8')).toBe('*\n'); + + await writeFile(join(workDir, '.codeman-uploads', '.gitignore'), 'theirs\n'); + expect((await upload('shot.jpg', 'image/jpeg', jpeg)).statusCode).toBe(200); + expect(await readFile(join(workDir, '.codeman-uploads', '.gitignore'), 'utf8')).toBe('theirs\n'); + } finally { + await rm(workDir, { recursive: true }); + } + }); + + it('takes back an ignore file whose write failed, so the next upload writes a real one', async () => { + const workDir = await mkdtemp(join(tmpdir(), 'codeman-uploads-')); + harness.ctx._session.workingDir = workDir; + const ignoreFile = join(workDir, '.codeman-uploads', '.gitignore'); + // The exclusive create succeeds and the two-byte write fails, as on a full disk. + const spy = vi.spyOn(fs, 'writeFile').mockImplementationOnce(async (path) => { + await writeFile(path as string, ''); + throw Object.assign(new Error('no space left on device'), { code: 'ENOSPC' }); + }); + try { + expect((await upload('shot.jpg', 'image/jpeg', jpeg)).statusCode).toBe(500); + expect(existsSync(ignoreFile)).toBe(false); + expect((await upload('shot.jpg', 'image/jpeg', jpeg)).statusCode).toBe(200); + expect(await readFile(ignoreFile, 'utf8')).toBe('*\n'); + } finally { + spy.mockRestore(); + await rm(workDir, { recursive: true }); + } + }); + + it('refuses a planted symlink at .codeman-uploads and writes nothing through it', async () => { + const workDir = await mkdtemp(join(tmpdir(), 'codeman-uploads-')); + const elsewhere = await mkdtemp(join(tmpdir(), 'codeman-elsewhere-')); + harness.ctx._session.workingDir = workDir; + await symlink(elsewhere, join(workDir, '.codeman-uploads')); + try { + const res = await upload('shot.jpg', 'image/jpeg', jpeg); + expect(res.statusCode).toBe(403); + expect(JSON.parse(res.body).error).toBe('.codeman-uploads is not a regular directory'); + expect(await readdir(elsewhere)).toEqual([]); + } finally { + await rm(workDir, { recursive: true }); + await rm(elsewhere, { recursive: true }); + } + }); + + it('refuses an upload for a remote session before touching disk', async () => { + const workDir = await mkdtemp(join(tmpdir(), 'codeman-uploads-')); + harness.ctx._session.workingDir = workDir; + // A remote session's workingDir is the REMOTE path: a file written here is unreadable there. + (harness.ctx._session as unknown as { remote: unknown }).remote = { hostId: 'gpu-box' }; + try { + const res = await upload('shot.jpg', 'image/jpeg', jpeg); + expect(res.statusCode).toBe(400); + expect(JSON.parse(res.body).error).toMatch(/remote/); + expect(await readdir(workDir)).toEqual([]); + } finally { + (harness.ctx._session as unknown as { remote: unknown }).remote = undefined; + await rm(workDir, { recursive: true }); + } + }); + it('returns 415 with the error envelope when HEIC conversion fails', async () => { heicConvert.mockClear(); heicConvert.mockRejectedValueOnce(new Error('HEIC dimensions 30000x30000 exceed the 64MP decode limit')); diff --git a/test/test-ports-guard.test.ts b/test/test-ports-guard.test.ts index 20c7d8b8..77607d88 100644 --- a/test/test-ports-guard.test.ts +++ b/test/test-ports-guard.test.ts @@ -52,7 +52,6 @@ const LEGACY_FIXED_PORT_FILES = new Set( 'operation-lightspeed.test.ts', 'ownership-scoping.test.ts', 'pane-exit-sweep.test.ts', - 'paste-image-dir-shared.test.ts', 'perf-browser.test.ts', 'quick-start.test.ts', 'ralph-integration.test.ts', From ba36cc36d44a1b8307a22b9b2f6d683616bb6044 Mon Sep 17 00:00:00 2001 From: JD Date: Fri, 9 Oct 2026 20:05:21 -0400 Subject: [PATCH 2/2] fix(uploads): list upload dirs bounded and async, and keep uploads inside the data dir collected The hourly sweep called lstatSync and realpathSync on every live session's working directory, so a linked case on a mount that stopped answering blocked the event loop 30 s after boot and then every hour; the old sweep was fully async. uploadDirs() now probes the workspace with probePathKind() first and skips an unknown path without touching it, then uses fs.promises for the lstat and realpath. The sweep keeps the probe's stall cap; cleanupSession(), acting on one path at the user's request, passes pastCap and removes the directories with fs.rm. Refusing an upload dir that sits strictly inside the data dir protected nothing (it only ever holds uploads, and a link is already excluded by lstat) while the route kept writing there, so uploads under a workspace like the ~/.codeman/app that install.sh clones were never swept and never removed. Only the two cases that matter stay refused: the upload dir being the data dir, or containing it. Also: the ignore file's take-back after a failed write no longer replaces the error that caused it with its own, and the shared-dir test's header no longer names the fixed port it stopped using. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- src/web/paste-image-gc.ts | 59 ++++++++++++++++++++--------- src/web/routes/session-routes.ts | 5 ++- src/web/server.ts | 10 +++-- test/paste-image-dir-shared.test.ts | 13 ++++++- test/paste-image-gc.test.ts | 57 ++++++++++++++++++++-------- 7 files changed, 105 insertions(+), 43 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index fc3cacef..ede43fd8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -295,7 +295,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Attachments** (live external document references; all wiring in `file-routes.ts`): a **registry** maps a stable `attachmentId` to a realpath-resolved, extension-allowlisted absolute path, so browser requests never carry arbitrary absolute paths. ⚠️ The **magic-link scanner** (`codeman://attach?...` in terminal output) is **prompt-injectable**, so its scan path is force-confined to the session workspace; a hostile prompt could otherwise exfiltrate arbitrary host files over SSE. The security gate is an extension **allowlist**, not a blocklist. `document-conversion-limiter.ts` caps converter spawns globally: without it, N large docs detected at once fork N multi-minute processes, which is a resource-exhaustion vector. → [architecture-invariants#attachments](docs/architecture-invariants.md#attachments) -**Prompt uploads** (`POST /api/sessions/:id/paste-image`; client wiring in `image-input.js` and `keyboard-accessory.js`): a pasted file is written to `/.codeman-uploads/` and its path lands on the prompt as unsent text. The folder is IN the workspace because that is the only path identical for a local and a container agent, FLAT because `/.codeman/` would BE the data dir when the workspace is the home directory, and self-ignoring through a `.gitignore` of `*` written ONCE with `wx` (a file already there is the user's and is never overwritten; the user's own ignore file is never touched). ⚠️ The directory names live ONLY in `paste-image-gc.ts` (`UPLOAD_DIR_NAMES`): the pre-move `.claude-images/` receives nothing but stays readable for one release, so the GC sweep and the delete cleanup (through `uploadDirs()`) and the image watcher's ignore filter (reading the names) cover BOTH; retire the legacy entry there, never by hand elsewhere. ⚠️ `uploadDirs()` returns only REAL directories (lstat, so a planted symlink is never one), none that is, contains or sits inside `getDataDir()`, and none for a remote session, since both consumers delete; the check is made at list time, and the same-user race after it is accepted. A remote (SSH) session is refused (400) before any disk touch: its `workingDir` is the remote path, so the file would land on THIS host where the agent cannot read it. → [architecture-invariants#buffers-uploads-and-terminal-history](docs/architecture-invariants.md#buffers-uploads-and-terminal-history) +**Prompt uploads** (`POST /api/sessions/:id/paste-image`; client wiring in `image-input.js` and `keyboard-accessory.js`): a pasted file is written to `/.codeman-uploads/` and its path lands on the prompt as unsent text. The folder is IN the workspace because that is the only path identical for a local and a container agent, FLAT because `/.codeman/` would BE the data dir when the workspace is the home directory, and self-ignoring through a `.gitignore` of `*` written ONCE with `wx` (a file already there is the user's and is never overwritten; the user's own ignore file is never touched). ⚠️ The directory names live ONLY in `paste-image-gc.ts` (`UPLOAD_DIR_NAMES`): the pre-move `.claude-images/` receives nothing but stays readable for one release, so the GC sweep and the delete cleanup (through `uploadDirs()`) and the image watcher's ignore filter (reading the names) cover BOTH; retire the legacy entry there, never by hand elsewhere. ⚠️ `uploadDirs()` is async and probes the workspace BOUNDED first (`probePathKind()`, the #516 rule: an `unknown` path is skipped, never touched; the sweep keeps the stall cap and the delete passes `pastCap`), then returns only REAL directories (lstat, so a planted symlink is never one), none that is or contains `getDataDir()` (one strictly inside it only holds uploads and is listed), and none for a remote session, since both consumers delete; the check is made at list time, and the same-user race after it is accepted. A remote (SSH) session is refused (400) before any disk touch: its `workingDir` is the remote path, so the file would land on THIS host where the agent cannot read it. → [architecture-invariants#buffers-uploads-and-terminal-history](docs/architecture-invariants.md#buffers-uploads-and-terminal-history) **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`). 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`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server: `spreadsheet-preview.js` fetches the raw route with `?preview=true` (413 above `MAX_XLSX_BROWSER_PREVIEW_BYTES`, 10 MB) and hands the bytes to `spreadsheet-preview-worker.js`, the only place the pinned `exceljs`/`fflate` vendor bundles load (never on page load). `admitXlsx()` caps the ZIP before ExcelJS runs, counting what ExcelJS will EXPAND as well as what it reads (a merge costs its area, a `` past 16384 is refused, validations are never parsed via `ignoreNodes`, and defined names are never expanded: the worker stubs `_definedNames.model`). ⚠️ The INDEX a row or sheet claims is bounded too, since ExcelJS allocates and walks up to it: a `` outside 1-1048576 and an `xl/workbook.xml` `` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ The COLUMN index costs the same way (a row's cells live at `_cells[col - 1]`, so one `XFD` cell makes ExcelJS's `eachRow`/`eachCell`/`hasValues` visit 16,384 slots per row): `worksheetMetadata` builds each sheet's row and cell index from the keys that exist (`Object.keys` of `_rows` and `_cells`, skipping falsy and `Null`-type cells as `eachCell({ includeEmpty: false })` does), reads row heights in that pass and merges from `sheet._merges`, and `sendTile` reads cells from that index (`populatedRowsById`); never call ExcelJS's dense `eachRow`/`eachCell` there. `parseThemePalette` returns the default palette for a theme above 64 KB (64 * 1024 characters), since its patterns are quadratic on unclosed tags. ⚠️ Merges are capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) AND workbook-wide (`LIMITS.maxMerges`, 10,000), refused as `merge-limit` before ExcelJS loads, since ExcelJS's `_mergeCellsInternal` checks each merge against every earlier one on its sheet (quadratic). Every `` in `xl/styles.xml` is read with `readTagAttributes()` and refused (`number-format`) when its decoded `formatCode` is over 255 characters or has a `[` after its last `]`: ExcelJS's `isDateFmt` rescans to the end of the code per unclosed `[` once per numeric cell, and the code is echoed into the notice bar. ⚠️ `formatCellValue` caps every cell's display text at `LIMITS.maxCellTextChars` (1,000, ellipsis, never splitting a surrogate pair) for every value shape (rich text, hyperlink text, formula source, errors), since structured clone copies each tile cell's whole string to the page and the load timeout no longer covers tiles. `richTextPrefix` also visits at most `maxCellTextChars + 1` runs, not only keeps that much text: an empty `` adds nothing, so a shared string of a million empty runs was walked whole per cell, per tile (56 s a tile). ⚠️ Every part ExcelJS parses (all but the pure-bytes `xl/media/.`) is also charged per START TAG against `LIMITS.maxElements` (2,000,000, refused as `element-limit`): ExcelJS builds an object per element in sharedStrings, styles, comments, drawings, VML and tables too, and empty stored blocks pad a deflate stream past the ratio cap, so 8.3M empty `` runs in a 375 KB file cost 570 MB of heap. ⚠️ Admission keys every entry on the name ExcelJS will SEE (`excelJsEntryName()`: JSZip's `.`/`..`/empty-segment resolution, then one leading `/` stripped), refuses two entries that land on one name, and treats anything matching ExcelJS's UNANCHORED `xl/worksheets/sheet.xml` as a worksheet, so `/xl/...` or `xl/./...` cannot skip a counter, and ExcelJS parses only a STORE-only archive rebuilt from the entries admission inflated (`buildAdmittedArchive()`), never the fetched bytes; cell text goes through `textContent`, formulas are never evaluated. Bumping either package or editing the worker/core changes `SPREADSHEET_ASSET_VERSION`, which `npm run check:public-assets` pins. xls/ods stay download-only. → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 06627c80..2a7fe091 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -1063,7 +1063,7 @@ Tests: `test/mobile-prompt-composer.test.ts` (in the CI gate, deliberately not u Target: 20 sessions, 50 agent windows at 60fps. Limits in `src/config/`: terminal 32MB (see below), text 1MB, messages 1000, max agents 500, max sessions 50, max SSE clients 100. **Terminal history** (`src/config/terminal-history.ts`, COD-80): tmux history-limit 100k lines, PTY buffer 32MB max / 24MB trim (env `CODEMAN_MAX_TERMINAL_BUFFER`/`CODEMAN_TRIM_TERMINAL_TO`; the env-derived trim is clamped ≤75% of max — trim ≥ max would disable `BufferAccumulator` trimming entirely = unbounded memory); browser xterm scrollback stays a separate hardcoded 50k (`DEFAULT_SCROLLBACK` in constants.js — 100k/tab is a mobile-memory hazard). tmux <3.7 allocates history at pane creation, so `createSession()` sets the global default in the same command queue immediately before `new-session`; tmux 3.7+ instead creates the session and targets only that pane, because changing the global option can resize and trim unrelated live panes. A settings change resizes tracked panes only on 3.7+ and otherwise affects future panes; no version can recover lines already evicted. Settings keys `terminalScrollbackLines`/`terminalBufferMaxBytes`/`terminalBufferTrimBytes` are schema-validated but inert (only `tmuxHistoryLimit` is wired); `buffer-limits.ts` re-exports the defaults. Text/message limits are env-overridable too (`CODEMAN_MAX_TEXT_OUTPUT`/`CODEMAN_TRIM_TEXT_TO`/`CODEMAN_MAX_MESSAGES`). **Image upload** (`image-input.js` / `config/buffer-limits.ts`): up to `_maxBatchImages` 20 images/batch (bounded concurrency 3), per-file `MAX_PASTE_IMAGE_BYTES` 50MB (env `CODEMAN_MAX_PASTE_IMAGE_BYTES`); the mobile camera-roll picker auto-downscales to fit before upload. **HEIC paste uploads** (#151): converted server-side to JPEG in a `worker_threads` worker (`web/heic-jpeg-worker.ts`, resourceLimits + 30s timeout) gated by `runWithConversionLimit()`; detection is magic-byte based (covers Android/MIUI HEIFs mislabeled as JPEG); headers declaring > 64MP are rejected 415 BEFORE decode (decompression-bomb guard). Deps: `heic-decode` + `jpeg-js`. Use `LRUMap` for bounded caches, `StaleExpirationMap` for TTL cleanup. Anti-flicker pipeline: `docs/terminal-anti-flicker.md`. -**Prompt uploads live in `/.codeman-uploads/`** (`POST /api/sessions/:id/paste-image`; the names live ONLY in `src/web/paste-image-gc.ts`): IN the workspace because that is the only path that resolves identically for a local agent and a container (only the workspace is bind-mounted, at the same absolute path; `~/.codeman` is not); hidden so it stays out of `git status` and the agent's view of the repository; FLAT because a nested `/.codeman/` IS the data dir (`join(homedir(), '.codeman')`, `src/config/instance.ts`) when the workspace is the home directory, which nothing refuses; and self-ignoring through a `.gitignore` of `*` written once with `wx`, so a file already there is the user's and stays. The pre-move `.claude-images/` receives nothing but stays readable for one release: `UPLOAD_DIR_NAMES` lists both, the hourly sweep and the delete cleanup in `cleanupSession()` go through `uploadDirs()`, and the image watcher's ignore filter reads the names. ⚠️ `uploadDirs()` returns only REAL directories (lstat at the leaf, so a planted `.codeman-uploads -> /other-case/.codeman-uploads` is never listed: `readdir` follows a link to a directory, and the sweep would otherwise age out the other case's uploads through it), none that is, contains or sits inside `getDataDir()` (contrived, `CODEMAN_INSTANCE=uploads` makes `~/.codeman-uploads` the data dir of a home workspace and `CODEMAN_DATA_DIR` can point inside one, but it is one check next to a recursive delete), and nothing for a remote (SSH) session, whose `workingDir` is the remote path and would otherwise name a same-named LOCAL directory for the sweep and the delete. The check is made when the directories are listed: a same-user process that swaps a listed directory for a link during the sweep's awaits is accepted (Node has no `openat()`, and that actor already writes anywhere this process can). The route refuses a remote (SSH) session with 400 before any disk touch: its `workingDir` is the remote path, so the file would land on THIS host where the agent cannot read it. Tests: `test/paste-image-gc.test.ts`, `test/paste-image-dir-shared.test.ts`, the paste-image block of `test/routes/session-routes.test.ts`. +**Prompt uploads live in `/.codeman-uploads/`** (`POST /api/sessions/:id/paste-image`; the names live ONLY in `src/web/paste-image-gc.ts`): IN the workspace because that is the only path that resolves identically for a local agent and a container (only the workspace is bind-mounted, at the same absolute path; `~/.codeman` is not); hidden so it stays out of `git status` and the agent's view of the repository; FLAT because a nested `/.codeman/` IS the data dir (`join(homedir(), '.codeman')`, `src/config/instance.ts`) when the workspace is the home directory, which nothing refuses; and self-ignoring through a `.gitignore` of `*` written once with `wx`, so a file already there is the user's and stays. The pre-move `.claude-images/` receives nothing but stays readable for one release: `UPLOAD_DIR_NAMES` lists both, the hourly sweep and the delete cleanup in `cleanupSession()` go through `uploadDirs()`, and the image watcher's ignore filter reads the names. ⚠️ `uploadDirs()` is async: the working directory is a user-chosen path, so it is probed BOUNDED first (`probePathKind()`, the #516 rule; `unknown` is skipped and never touched, the hourly sweep keeps the stall cap and `cleanupSession()` passes `pastCap`, since it acts on one path at the user's request), and the lstat and realpath after it are `fs.promises` calls, never sync, because the sweep runs 30 s after boot and hourly over every live session and a linked case on a dead mount would otherwise freeze the whole server. It returns only REAL directories (lstat at the leaf, so a planted `.codeman-uploads -> /other-case/.codeman-uploads` is never listed: `readdir` follows a link to a directory, and the sweep would otherwise age out the other case's uploads through it), none that is or contains `getDataDir()` (contrived, `CODEMAN_INSTANCE=uploads` makes `~/.codeman-uploads` the data dir of a home workspace and `CODEMAN_DATA_DIR` can point inside one, but it is one check next to a recursive delete; one strictly INSIDE the data dir is listed, since it only ever holds uploads and a workspace like the `~/.codeman/app` that `install.sh` clones would otherwise never have its uploads collected), and nothing for a remote (SSH) session, whose `workingDir` is the remote path and would otherwise name a same-named LOCAL directory for the sweep and the delete. The check is made when the directories are listed: a same-user process that swaps a listed directory for a link during the sweep's awaits is accepted (Node has no `openat()`, and that actor already writes anywhere this process can). The route refuses a remote (SSH) session with 400 before any disk touch: its `workingDir` is the remote path, so the file would land on THIS host where the agent cannot read it. Tests: `test/paste-image-gc.test.ts`, `test/paste-image-dir-shared.test.ts`, the paste-image block of `test/routes/session-routes.test.ts`. ### Process-tree walks are bounded diff --git a/src/web/paste-image-gc.ts b/src/web/paste-image-gc.ts index c99099ac..462fce35 100644 --- a/src/web/paste-image-gc.ts +++ b/src/web/paste-image-gc.ts @@ -13,9 +13,10 @@ * upload dir. */ import fs from 'node:fs/promises'; -import { lstatSync, realpathSync } from 'node:fs'; +import { realpathSync } from 'node:fs'; import { join, resolve, sep } from 'node:path'; import { getDataDir } from '../config/instance.js'; +import { probePathKind, type PathProbeOptions } from '../utils/bounded-path-probe.js'; import type { SessionPort } from './ports/index.js'; const MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; // 7 days @@ -43,34 +44,56 @@ export const UPLOAD_DIR_NAMES = [UPLOADS_DIR, LEGACY_UPLOADS_DIR]; * act on what this returns, the hourly sweep and the recursive delete in * cleanupSession(), so it lists only REAL directories (a link planted by a * workspace script, `.codeman-uploads -> /other-case/.codeman-uploads`, is - * not one; readdir follows a link to a directory), none that is, contains or - * sits inside this instance's data dir (a home workspace reaches it under a - * contrived instance name, `CODEMAN_INSTANCE=uploads`, and `CODEMAN_DATA_DIR` - * can point inside one), and nothing for a remote (SSH) session, whose + * not one; readdir follows a link to a directory), none that is or contains + * this instance's data dir (a home workspace reaches it under a contrived + * instance name, `CODEMAN_INSTANCE=uploads`, and `CODEMAN_DATA_DIR` can point + * inside one; a directory strictly below the data dir only ever holds uploads + * and is listed, or the uploads of a workspace like `~/.codeman/app` would + * never be collected), and nothing for a remote (SSH) session, whose * workingDir is the remote path and would name a same-named LOCAL directory - * here. The check is made when listing: a same-user process that swaps a - * listed directory for a link afterwards is accepted, since it already writes - * anywhere this process can. + * here. The working directory is a user-chosen path, so it is probed BOUNDED + * first (#516): one on a mount that stopped answering reads `unknown` and is + * skipped, never touched. The sweep keeps the probe's stall cap; the delete, + * acting on one path at the user's request, passes `pastCap`. The check is + * made when listing: a same-user process that swaps a listed directory for a + * link afterwards is accepted, since it already writes anywhere this process + * can. */ -export function uploadDirs(session: { workingDir: string; remote?: unknown }): string[] { +export async function uploadDirs( + session: { workingDir: string; remote?: unknown }, + probe: PathProbeOptions = {} +): Promise { if (session.remote) return []; - const dataDir = canonicalDir(getDataDir()); - return UPLOAD_DIR_NAMES.map((name) => join(session.workingDir, name)).filter((dir) => { - if (!isRealDir(dir)) return false; - const real = canonicalDir(dir); - return real !== dataDir && !real.startsWith(dataDir + sep) && !dataDir.startsWith(real + sep); - }); + if ((await probePathKind(session.workingDir, probe)) !== 'directory') return []; + const dataDir = await realDir(getDataDir()); + const dirs: string[] = []; + for (const dir of UPLOAD_DIR_NAMES.map((name) => join(session.workingDir, name))) { + if (!(await isRealDir(dir))) continue; + const real = await realDir(dir); + if (real === dataDir || dataDir.startsWith(real + sep)) continue; + dirs.push(dir); + } + return dirs; } /** lstat, so a symlink is not a directory, whatever it points at. */ -function isRealDir(p: string): boolean { +async function isRealDir(p: string): Promise { try { - return lstatSync(p).isDirectory(); + return (await fs.lstat(p)).isDirectory(); } catch { return false; } } +/** `canonicalDir()` for the listing, which must not block the event loop on a user path. */ +async function realDir(dir: string): Promise { + try { + return await fs.realpath(dir); + } catch { + return resolve(dir); + } +} + export async function sweepPasteImagesOnce( ctx: Pick, now: number = Date.now() @@ -79,7 +102,7 @@ export async function sweepPasteImagesOnce( let scanned = 0; let deleted = 0; for (const session of ctx.sessions.values()) { - for (const dir of uploadDirs(session)) { + for (const dir of await uploadDirs(session)) { let entries: string[]; try { entries = await fs.readdir(dir); diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 508044ea..9e7f54c7 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -399,8 +399,9 @@ async function ensureUploadDir(workingDir: string): Promise { } catch (err: unknown) { if ((err as NodeJS.ErrnoException).code === 'EEXIST') return uploadDir; // The create can succeed before the write fails (ENOSPC): an empty ignore - // file would read as the user's on the next upload, so take it back. - await fs.rm(ignoreFile, { force: true }); + // file would read as the user's on the next upload, so take it back; its own + // failure must not replace the cause. + await fs.rm(ignoreFile, { force: true }).catch(() => {}); throw err; } return uploadDir; diff --git a/src/web/server.ts b/src/web/server.ts index 3f2e1e56..bb159e16 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -37,7 +37,7 @@ import { pasteImageDirInUseByOtherSession, startPasteImageGc, uploadDirs } from import { CLEAN_EXIT_CLOSE_REASON, shouldCloseCleanlyExitedSession } from '../pane-exit-sweep.js'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { existsSync, mkdirSync, readFileSync, chmodSync, rmSync, statSync } from 'node:fs'; +import { existsSync, mkdirSync, readFileSync, chmodSync, statSync } from 'node:fs'; import fs from 'node:fs/promises'; import { execSync } from 'node:child_process'; import { hostname as getHostname, uptime as osUptime } from 'node:os'; @@ -1547,10 +1547,12 @@ export class WebServer extends EventEmitter { }) ) { // Both upload dirs, the pre-move one too; uploadDirs() lists only real - // directories clear of the data dir, and none for a remote session. - for (const uploadDir of uploadDirs(session)) { + // directories that are not the data dir and do not contain it, none for a + // remote session, and nothing on a workspace whose bounded probe did not + // answer (pastCap: this acts on one path at the user's request). + for (const uploadDir of await uploadDirs(session, { pastCap: true })) { try { - rmSync(uploadDir, { recursive: true, force: true }); + await fs.rm(uploadDir, { recursive: true, force: true }); } catch { // Best-effort cleanup } diff --git a/test/paste-image-dir-shared.test.ts b/test/paste-image-dir-shared.test.ts index 6a77b7e1..7af4ec54 100644 --- a/test/paste-image-dir-shared.test.ts +++ b/test/paste-image-dir-shared.test.ts @@ -10,7 +10,7 @@ * closes sessions unattended, which turns that from an occasional loss into a * routine one. * - * Port: 3188 + * Port: 0 (ephemeral, read from `server.boundPort`) */ import { existsSync, lstatSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -18,6 +18,12 @@ import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { WebServer } from '../src/web/server.js'; import { pasteImageDirInUseByOtherSession } from '../src/web/paste-image-gc.js'; +import { probePathKind } from '../src/utils/bounded-path-probe.js'; + +vi.mock('../src/utils/bounded-path-probe.js', async (importOriginal) => { + const real = await importOriginal(); + return { ...real, probePathKind: vi.fn(real.probePathKind) }; +}); describe('pasteImageDirInUseByOtherSession', () => { const none = new Set(); @@ -148,9 +154,14 @@ describe('deleting a session that shares its working directory', () => { expect(existsSync(join(uploadDir, 'paste-1.png'))).toBe(true); expect(existsSync(join(legacyDir, 'paste-0.png'))).toBe(true); + // The delete acts on one path at the user's request, so its probe goes past + // the bulk stall cap (#516); the hourly sweep keeps the cap. Cleared first: + // the create path probes the same workspace past the cap too. + vi.mocked(probePathKind).mockClear(); expect((await remove(second)).status).toBe(200); expect(existsSync(uploadDir)).toBe(false); expect(existsSync(legacyDir)).toBe(false); + expect(probePathKind).toHaveBeenCalledWith(workingDir, { pastCap: true }); }); it('does not follow a planted symlink at the upload dir into another case when the last session closes', async () => { diff --git a/test/paste-image-gc.test.ts b/test/paste-image-gc.test.ts index c49b27f0..37241d07 100644 --- a/test/paste-image-gc.test.ts +++ b/test/paste-image-gc.test.ts @@ -2,15 +2,22 @@ * @fileoverview The hourly upload sweep reads BOTH upload dirs of a live session: * `.codeman-uploads` and the `.claude-images` a case in use before the move still * carries. Only `paste-*` regular files past the age cap go. `uploadDirs()` never - * lists a planted symlink, a directory that is, contains or sits inside the data - * dir, or anything for a remote session, since the sweep and the delete cleanup - * both act on what it returns. + * lists a planted symlink, a directory that is or contains the data dir, anything + * under a workspace whose bounded probe did not answer, or anything for a remote + * session, since the sweep and the delete cleanup both act on what it returns. */ import { existsSync, lstatSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, utimesSync, writeFileSync } from 'node:fs'; +import fsp from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { LEGACY_UPLOADS_DIR, UPLOADS_DIR, sweepPasteImagesOnce, uploadDirs } from '../src/web/paste-image-gc.js'; +import { probePathKind } from '../src/utils/bounded-path-probe.js'; + +vi.mock('../src/utils/bounded-path-probe.js', async (importOriginal) => { + const real = await importOriginal(); + return { ...real, probePathKind: vi.fn(real.probePathKind) }; +}); const DAY_MS = 24 * 60 * 60 * 1000; const sessionsOf = (workingDir: string, remote?: unknown) => ({ @@ -31,7 +38,7 @@ describe('sweepPasteImagesOnce', () => { const legacy = join(workingDir, LEGACY_UPLOADS_DIR); mkdirSync(current); mkdirSync(legacy); - expect(uploadDirs({ workingDir })).toEqual([current, legacy]); + expect(await uploadDirs({ workingDir })).toEqual([current, legacy]); const old = (now - 8 * DAY_MS) / 1000; const fresh = (now - 1 * DAY_MS) / 1000; const plant = (dir: string, name: string, mtime: number) => { @@ -69,7 +76,7 @@ describe('sweepPasteImagesOnce', () => { // age out the other case's uploads through it. symlinkSync(join(other, UPLOADS_DIR), join(workingDir, UPLOADS_DIR)); - expect(uploadDirs({ workingDir })).toEqual([]); + expect(await uploadDirs({ workingDir })).toEqual([]); expect(await sweepPasteImagesOnce(sessionsOf(workingDir), now)).toEqual({ scanned: 0, deleted: 0 }); expect(existsSync(join(other, UPLOADS_DIR, 'paste-9-zz.png'))).toBe(true); }); @@ -84,7 +91,7 @@ describe('uploadDirs', () => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); - it('lists the upload dir of a home workspace, which sits beside the data dir, not inside it', () => { + it('lists the upload dir of a home workspace, which sits beside the data dir, not inside it', async () => { // The home-as-workspace case the flat name exists for: `/.codeman-uploads` // is a sibling of `/.codeman`, so a prefix test without the separator would // wrongly refuse it. @@ -93,10 +100,10 @@ describe('uploadDirs', () => { process.env.CODEMAN_DATA_DIR = join(home, '.codeman'); const dir = join(home, UPLOADS_DIR); mkdirSync(dir); - expect(uploadDirs({ workingDir: home })).toEqual([dir]); + expect(await uploadDirs({ workingDir: home })).toEqual([dir]); }); - it('refuses an upload dir that is the data dir, contains it, or sits inside it', () => { + it('refuses an upload dir that is the data dir or contains it, and lists one inside it', async () => { const root = mkdtempSync(join(tmpdir(), 'codeman-gc-data-')); const parent = mkdtempSync(join(tmpdir(), 'codeman-gc-link-')); dirs.push(root, parent); @@ -104,16 +111,34 @@ describe('uploadDirs', () => { mkdirSync(join(uploads, 'state'), { recursive: true }); // `CODEMAN_INSTANCE=uploads` on a home workspace: the upload dir IS the data dir. process.env.CODEMAN_DATA_DIR = uploads; - expect(uploadDirs({ workingDir: root })).toEqual([]); + expect(await uploadDirs({ workingDir: root })).toEqual([]); // A data dir pointed inside the upload dir: the recursive delete would take it. process.env.CODEMAN_DATA_DIR = join(uploads, 'state'); - expect(uploadDirs({ workingDir: root })).toEqual([]); - // An upload dir inside the data dir, directly and through a symlinked working - // directory (the upload dir itself is real there). + expect(await uploadDirs({ workingDir: root })).toEqual([]); + // An upload dir strictly inside the data dir only ever holds uploads (install.sh + // clones the app to ~/.codeman/app), so it is listed, directly and through a + // symlinked working directory. process.env.CODEMAN_DATA_DIR = root; - expect(uploadDirs({ workingDir: root })).toEqual([]); + expect(await uploadDirs({ workingDir: root })).toEqual([uploads]); symlinkSync(root, join(parent, 'ws')); - expect(uploadDirs({ workingDir: join(parent, 'ws') })).toEqual([]); + expect(await uploadDirs({ workingDir: join(parent, 'ws') })).toEqual([join(parent, 'ws', UPLOADS_DIR)]); + }); + + it('skips a workspace whose bounded probe does not answer, without touching it', async () => { + // A linked case on a mount that stopped answering reads `unknown` (#516): the + // sweep must not lstat under it, which would hold a threadpool worker. + const workingDir = mkdtempSync(join(tmpdir(), 'codeman-gc-stalled-')); + dirs.push(workingDir); + mkdirSync(join(workingDir, UPLOADS_DIR)); + vi.mocked(probePathKind).mockResolvedValueOnce('unknown'); + const lstat = vi.spyOn(fsp, 'lstat'); + try { + expect(await uploadDirs({ workingDir })).toEqual([]); + expect(lstat).not.toHaveBeenCalled(); + } finally { + lstat.mockRestore(); + } + expect(probePathKind).toHaveBeenCalledWith(workingDir, {}); }); it('lists nothing for a remote session, whose workingDir is the remote path', async () => { @@ -125,7 +150,7 @@ describe('uploadDirs', () => { const old = (Date.now() - 8 * DAY_MS) / 1000; utimesSync(join(workingDir, UPLOADS_DIR, 'paste-1-aa.png'), old, old); const remote = { hostId: 'gpu-box' }; - expect(uploadDirs({ workingDir, remote })).toEqual([]); + expect(await uploadDirs({ workingDir, remote })).toEqual([]); expect(await sweepPasteImagesOnce(sessionsOf(workingDir, remote))).toEqual({ scanned: 0, deleted: 0 }); expect(existsSync(join(workingDir, UPLOADS_DIR, 'paste-1-aa.png'))).toBe(true); });