From 66c8fef97f3ce9676f97a8898b06f9215abd60a4 Mon Sep 17 00:00:00 2001 From: JD Date: Thu, 8 Oct 2026 00:24:55 -0400 Subject: [PATCH] chore(uploads): remove the broken upload page and deprecate /api/screenshots The tunnel Upload URL page (upload.html) has been broken since the response envelope landed in 458fb81c: it reads j.filename and j.files while the server answers { success, data: { filename } } and { success, data: { files } }, so every upload reported "Saved: undefined" and the recent list stayed empty. Nothing else reads ~/.codeman/screenshots/, and handing a file to an agent goes through POST /api/sessions/:id/paste-image into the session's own workspace, so the page, its Settings row, the suffix branch of the tunnel row helper (now folded into its one caller) and the Upload URL i18n key go. The three /api/screenshots routes keep working unchanged and log one deprecation warning per process on first use, naming paste-image as the replacement. Per docs/versioning-policy.md they are removed in a later MAJOR, after at least one MINOR release that carries the warning; the docs, CLAUDE.md and the multi-user plan say so. Static caching: with upload.html gone no HTML is served by @fastify/static any more (every page has its own no-cache route; on a built tree only the precompressed index.html.gz artifact is reachable, as application/gzip, and nothing requests it). The .html branch of setHeaders was therefore dead and goes with the test that fetched upload.html to reach it; a comment now says a new static HTML page needs its own route. The index.html no-cache assertion on the route stays. --- .prettierignore | 1 - CLAUDE.md | 4 +- docs/api-reference.md | 7 ++ docs/multi-user-plan.md | 4 +- docs/wiki/Settings-Reference.md | 2 +- src/web/public/i18n.js | 1 - src/web/public/index.html | 4 - src/web/public/settings-ui.js | 18 ++-- src/web/public/upload.html | 155 ------------------------------ src/web/routes/system-routes.ts | 14 +++ src/web/server.ts | 13 +-- test/routes/system-routes.test.ts | 18 ++++ test/static-cache-headers.test.ts | 15 +-- 13 files changed, 57 insertions(+), 199 deletions(-) delete mode 100644 src/web/public/upload.html diff --git a/.prettierignore b/.prettierignore index cdfaf71e..25f5ba46 100644 --- a/.prettierignore +++ b/.prettierignore @@ -24,7 +24,6 @@ src/web/public/settings-ui.js src/web/public/sw.js src/web/public/terminal-ui.js src/web/public/voice-input.js -src/web/public/upload.html scripts/remotion/ # Hand-maintained; Prettier escapes underscores in glob paths and corrupts paragraphs. diff --git a/CLAUDE.md b/CLAUDE.md index a665ae61..5fc892f6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -127,7 +127,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Code style**: Prettier (`singleQuote: true`, `printWidth: 120`, `trailingComma: "es5"`) — config lives in the **`"prettier"` key of `package.json`**, not a `.prettierrc` (keeps the repo root short; editors read it natively). `.prettierignore` stays at the root because Prettier resolves it relative to cwd. ESLint flat config (`config/eslint.config.js`) allows `no-console`, warns on `@typescript-eslint/no-explicit-any`. Ignores: `app.js`, `scripts/**/*.mjs`, `src/web/public/vendor/**`, `scripts/remotion/**`. -**Prettier scope is deliberately narrow.** `npm run format` globs only `src/**/*.ts` and `src/web/public/**` (`lint` only `src/**/*.ts`), and `.prettierignore` then exempts most of `src/web/public/*.js` (app.js, styles.css, **mobile.css**, index.html, upload.html, and 15 hand-formatted modules) plus `CLAUDE.md`. Those files are hand-formatted by design; `npm run check:public-assets` and `check:frontend-syntax` are what guard them (NUL bytes + JS syntax), not Prettier. Do not "fix" a file by adding it back to Prettier's scope. +**Prettier scope is deliberately narrow.** `npm run format` globs only `src/**/*.ts` and `src/web/public/**` (`lint` only `src/**/*.ts`), and `.prettierignore` then exempts most of `src/web/public/*.js` (app.js, styles.css, **mobile.css**, index.html, and 15 hand-formatted modules) plus `CLAUDE.md`. Those files are hand-formatted by design; `npm run check:public-assets` and `check:frontend-syntax` are what guard them (NUL bytes + JS syntax), not Prettier. Do not "fix" a file by adding it back to Prettier's scope. ## Common Gotchas @@ -487,7 +487,7 @@ curl -sk https://localhost:3000/api/subagents | jq # Background agents cat ~/.codeman/state.json | jq # Persisted state ``` -Mobile screenshots: `~/.codeman/screenshots/`, accessed via `GET/POST /api/screenshots`. +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`. ## Performance & Limits diff --git a/docs/api-reference.md b/docs/api-reference.md index 89a873b7..e41549c6 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -45,6 +45,13 @@ payload return `{ "success": true, "data": {} }`. > `GET /api/sessions/:id/tail-file` (SSE), `GET /api/download`, > `GET /api/screenshots/:name`, `GET /q/:code` (QR redirect), and the > `GET /ws/sessions/:id/terminal` WebSocket upgrade. +> +> **Deprecated:** `POST /api/screenshots`, `GET /api/screenshots` and +> `GET /api/screenshots/:name` keep working but log a one-time warning on first +> use. They are removed in a later MAJOR, after at least one MINOR release that +> carries this warning (see `docs/versioning-policy.md`). To hand +> a file to an agent, use `POST /api/sessions/:id/paste-image`, which saves it into +> that session's workspace. > The [agent wait endpoints](#long-polling-agent-wait) use the normal envelope but > are the only JSON endpoints that deliberately **hold the connection open**, for up diff --git a/docs/multi-user-plan.md b/docs/multi-user-plan.md index a3438971..44055467 100644 --- a/docs/multi-user-plan.md +++ b/docs/multi-user-plan.md @@ -158,7 +158,7 @@ Codeman now ships a global **Startup Mode** picker (App Settings, Claude CLI tab | `GET /api/away-digest` | Aggregate only owned sessions/events | | `GET /api/subagents`, workflow runs | Filter by owning session (`claudeSessionId -> session -> owner`); agents not attributable to any session: admin-only | | Push (`push-routes.ts`) | Subscription records currently carry NO identity (keyed by endpoint only): `subscribe` stamps `username`. All 8 `PUSH_EVENT_MAP` events are session-scoped, so routing = resolve owner from `data.sessionId`, deliver to that owner's (plus admins') subscriptions. Legacy identity-less subscriptions: admin-only delivery | -| Screenshots `/api/screenshots` | Per-user subdir `~/.codeman/screenshots//` in multi-user mode. Note: `GET /:name` deliberately rejects `/` in names as traversal, so derive the subdir server-side from `req.authUser` and keep client-visible names flat | +| Screenshots `/api/screenshots` (deprecated) | Deprecated: removed in a later MAJOR, so no per-user subdir is planned; the replacement `POST /api/sessions/:id/paste-image` is already session-scoped. Former plan: per-user subdir `~/.codeman/screenshots//` in multi-user mode. Note: `GET /:name` deliberately rejects `/` in names as traversal, so derive the subdir server-side from `req.authUser` and keep client-visible names flat | | Attachments | Already session-scoped; inherits the session owner check. `attachmentConfineToWorkspace` is a global, default-OFF setting today: in multi-user mode it is FORCED ON for non-admins regardless of the setting (their attachments must resolve inside their own space); the setting keeps meaning what it means for admins | | File routes (browse/preview) | Path allowlist adds: non-admin paths must resolve (realpath) inside their own space or their own sessions' workingDirs | | Settings (`settings.json`) | Global, admin-only writes in multi-user mode; reads allowed (per-device display keys stay in localStorage as today). Per-user server settings: out of scope v1 | @@ -228,7 +228,7 @@ These operate directly on `users.json` via `user-store.ts` (no server needed), h | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Default (no flag) | No behavior change. No new file reads on the hot path. All new fields optional in state | | State round-trip | `SessionState.owner`, `MuxSession.owner`, `CronJob.owner`, registry `owner` fields are optional; old state loads clean; new state loaded by an old build ignores unknown fields (existing tolerant parsing) | -| Instance isolation | `users.json`, audit log, screenshots subdirs all via `dataPath()`; user spaces dir is shared across instances like `~/codeman-cases` is today (documented) | +| Instance isolation | `users.json` and the audit log via `dataPath()`; user spaces dir is shared across instances like `~/codeman-cases` is today (documented) | | API versioning | HTTP API is internal per `docs/versioning-policy.md`; still, all changes are additive. Ship as a **minor** version | | Hooks | Unchanged (instance-level hook secret; owner resolved from the session) | diff --git a/docs/wiki/Settings-Reference.md b/docs/wiki/Settings-Reference.md index 63335691..ee6dfcb1 100644 --- a/docs/wiki/Settings-Reference.md +++ b/docs/wiki/Settings-Reference.md @@ -152,7 +152,7 @@ Rebinding for the shortcut registry. See [Keyboard Shortcuts](Keyboard-Shortcuts ### System `CLAUDE.md` template for new cases, default working directory, the image watcher, and -Cloudflare tunnel controls including the tunnel and upload URLs. The **Diagnostics** group runs +Cloudflare tunnel controls including the tunnel URL. The **Diagnostics** group runs `codeman doctor` on the server and lists the agent CLIs, tmux, Node and the optional office tools with their versions and install hints (admin only in multi-user mode). In multi-user mode, the **Users** administration entry is injected here. diff --git a/src/web/public/i18n.js b/src/web/public/i18n.js index 538534b2..7f98bba7 100644 --- a/src/web/public/i18n.js +++ b/src/web/public/i18n.js @@ -351,7 +351,6 @@ 'Remote Access': '远程访问', 'Cloudflare Tunnel': 'Cloudflare 隧道', 'Tunnel URL': '隧道地址', - 'Upload URL': '上传地址', Updates: '更新', 'Current Version': '当前版本', 'Check for Updates': '检查更新', diff --git a/src/web/public/index.html b/src/web/public/index.html index 958371f6..aa773488 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -2933,10 +2933,6 @@ - diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index e8fed034..ac78d598 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -1708,17 +1708,16 @@ Object.assign(CodemanApp.prototype, { } }, - _updateTunnelUrlRow(rowId, displayId, url, suffix = '') { - const row = document.getElementById(rowId); - const display = document.getElementById(displayId); + _updateTunnelUrlDisplay(url) { + const row = document.getElementById('tunnelUrlRow'); + const display = document.getElementById('tunnelUrlDisplay'); if (!row || !display) return; if (url) { - const fullUrl = url + suffix; row.style.display = ''; - display.textContent = fullUrl; + display.textContent = url; display.onclick = () => { - navigator.clipboard.writeText(fullUrl).then(() => { - this.showToast(`${suffix ? 'Upload' : 'Tunnel'} URL copied`, 'success'); + navigator.clipboard.writeText(url).then(() => { + this.showToast('Tunnel URL copied', 'success'); }); }; } else { @@ -1728,11 +1727,6 @@ Object.assign(CodemanApp.prototype, { } }, - _updateTunnelUrlDisplay(url) { - this._updateTunnelUrlRow('tunnelUrlRow', 'tunnelUrlDisplay', url); - this._updateTunnelUrlRow('tunnelUploadUrlRow', 'tunnelUploadUrlDisplay', url, '/upload.html'); - }, - showTunnelQR() { // Close existing popup if open this.closeTunnelQR(); diff --git a/src/web/public/upload.html b/src/web/public/upload.html deleted file mode 100644 index cec405cb..00000000 --- a/src/web/public/upload.html +++ /dev/null @@ -1,155 +0,0 @@ - - - - -Upload Screenshot - Codeman - - -
- ← Back to Codeman -

Upload Screenshot

-
-

Tap to select image

- PNG, JPG, WebP — up to 10 MB -
- -
- Preview -
-
- -
-
-
- - diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index 53275f5e..e2251c24 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -1295,8 +1295,20 @@ export function registerSystemRoutes( // ═══════════════════════════════════════════════════════════════ // ========== Screenshots ========== + // Deprecated (the upload page is gone): removed in a later MAJOR per + // docs/versioning-policy.md. Warns once per process on first use. + let screenshotsDeprecationWarned = false; + const warnScreenshotsDeprecated = (): void => { + if (screenshotsDeprecationWarned) return; + screenshotsDeprecationWarned = true; + console.warn( + '[deprecated] /api/screenshots is deprecated and will be removed in a future major release. ' + + 'Use POST /api/sessions/:id/paste-image to hand a file to a session.' + ); + }; app.post('/api/screenshots', async (req, reply) => { + warnScreenshotsDeprecated(); const contentType = req.headers['content-type'] ?? ''; if (!contentType.includes('multipart/form-data')) { return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Expected multipart/form-data'); @@ -1383,6 +1395,7 @@ export function registerSystemRoutes( }); app.get('/api/screenshots', async () => { + warnScreenshotsDeprecated(); if (!existsSync(SCREENSHOTS_DIR)) { return { files: [] }; } @@ -1396,6 +1409,7 @@ export function registerSystemRoutes( }); app.get('/api/screenshots/:name', async (req, reply) => { + warnScreenshotsDeprecated(); const { name } = req.params as { name: string }; // Prevent path traversal if (name.includes('/') || name.includes('\\') || name.includes('..')) { diff --git a/src/web/server.ts b/src/web/server.ts index 4442e544..04546a57 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -920,7 +920,9 @@ export class WebServer extends EventEmitter { }); // Serve static files — content-hashed assets (e.g. app.a3f8c2e1.js) are immutable, cache aggressively. - // HTML must revalidate every time so browsers pick up new hashed filenames after deploys. + // HTML must revalidate every time so browsers pick up new hashed filenames after deploys, and every + // HTML page has its own route that says so (/, /index.html, /session/:id). ⚠️ A new static .html + // needs such a route too: this plugin would hand it a year of `immutable`. // cacheControl disabled so setHeaders owns Cache-Control for plain static assets. // preCompressed: serve pre-built .br/.gz files (from build step) to avoid per-request CPU compression await this.app.register(fastifyStatic, { @@ -932,7 +934,7 @@ export class WebServer extends EventEmitter { // `ServerResponse` to a `FastifyReply`, so it is `reply.header()` here and // NOT `res.setHeader()`. A v9-style body throws TypeError on every static // request, which is every page load. See the v10.0.0 release notes. - setHeaders: (reply, path) => { + setHeaders: (reply) => { // ⚠️ That same change ALSO flipped precedence, and silently. Under v9 this // callback wrote to the raw response and Fastify's staged reply headers then // overwrote it, so a route that set its own Cache-Control before .sendFile() @@ -941,12 +943,7 @@ export class WebServer extends EventEmitter { // no-store` its route asks for — a service worker that can never update. // So: a route that already decided keeps its answer. if (reply.getHeader('Cache-Control') !== undefined) return; - // Use .includes() not .endsWith() — preCompressed serves .html.br/.html.gz - if (path.includes('.html')) { - reply.header('Cache-Control', 'no-cache'); - } else { - reply.header('Cache-Control', 'public, max-age=31536000, immutable'); - } + reply.header('Cache-Control', 'public, max-age=31536000, immutable'); }, }); diff --git a/test/routes/system-routes.test.ts b/test/routes/system-routes.test.ts index aa1fae13..6087e7c4 100644 --- a/test/routes/system-routes.test.ts +++ b/test/routes/system-routes.test.ts @@ -680,6 +680,24 @@ describe('system-routes', () => { }); }); + describe('/api/screenshots deprecation', () => { + it('warns once across requests and leaves the response unchanged', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + mockedExistsSync.mockReturnValue(false); + const first = await harness.app.inject({ method: 'GET', url: '/api/screenshots' }); + const second = await harness.app.inject({ method: 'GET', url: '/api/screenshots' }); + expect(JSON.parse(first.body)).toEqual({ files: [] }); + expect(JSON.parse(second.body)).toEqual({ files: [] }); + const deprecations = warn.mock.calls.filter((c) => String(c[0]).includes('/api/screenshots')); + expect(deprecations).toHaveLength(1); + expect(String(deprecations[0][0])).toContain('POST /api/sessions/:id/paste-image'); + } finally { + warn.mockRestore(); + } + }); + }); + // ========== GET /api/screenshots/:name ========== describe('GET /api/screenshots/:name', () => { diff --git a/test/static-cache-headers.test.ts b/test/static-cache-headers.test.ts index 9bad8785..99ca6f6d 100644 --- a/test/static-cache-headers.test.ts +++ b/test/static-cache-headers.test.ts @@ -11,7 +11,8 @@ * * That contract is load-bearing: assets are served `immutable` for a year, and * `index.html` must revalidate every time or a deploy leaves browsers on stale - * markup (see `cacheBustAssets` in server.ts). + * markup (see `cacheBustAssets` in server.ts). No HTML is left behind the static + * plugin (every page has its own route), so the HTML half is asserted on the route. * * These tests drive a REAL WebServer on purpose. Asserting against an inline * re-registration of the plugin would keep passing after a revert in server.ts. @@ -61,18 +62,6 @@ describe('static asset Cache-Control headers', () => { expect(res.headers.get('cache-control')).toBe('public, max-age=31536000, immutable'); }); - it('makes static HTML revalidate so deploys are picked up', async () => { - // ⚠️ upload.html, NOT index.html. `/index.html` has its own explicit route that - // answers from renderIndexHtml() and never reaches @fastify/static, so asserting - // on it passes even with setHeaders fully broken (verified: reverting server.ts - // to the v9 form fails the two /app.js tests and leaves an index.html assertion - // green). upload.html has no route of its own, so it is the only HTML that - // actually exercises the `.html` branch of setHeaders. - const res = await get(`${baseUrl}/upload.html`); - expect(res.status).toBe(200); - expect(res.headers.get('cache-control')).toBe('no-cache'); - }); - it('lets a route keep the Cache-Control it set, so sw.js stays uncached', async () => { // Regression guard for the OTHER half of the v10 change. setHeaders runs for // sendFile() too, and v10 moved it from the raw response onto the reply, which